lib/repo: Search a list of paths in gpgkeypath for gpg keys

[rfairley]

This allows specifying gpgpath as a list of
paths that can point to a file or a directory. If a directory path
is given, paths to all regular files in the directory are added
to the remote as gpg ascii keys. If the path is not a directory,
the file is directly added (whether regular file, empty - errors
will be reported later when verifying gpg keys e.g. when pulling).

Adding the gpgkeypath property looks like:

ostree --repo=repo remote add --set=gpgpath="/path/key1.asc,/path/keys.d" R1 https://example.com/some/remote/ostree/repo

Closes #773

Still left to do

• Update documentation of gpgkeypath

Other considerations

• Checks before adding the keys in _ostree_gpg_verifier_add_key_ascii_file and if/how checks will be reported. Right now, entries in the list that are not directories are added automatically (whether regular, or don't exist, ...) (same as before). If the entry is a directory, all regular files in that directory are added, without traversing directories
• Regexp matching of list entries

[dustymabe]

ostree --repo=repo remote add --set=gpgpath="/path/key1.asc;/path/keys.d" R1 https://example.com/some/remote/ostree/repo

is that a semi-colon ; above ? is there a reason to use a semi-colon over a comma?

[rfairley]

@dustymabe Initially I was thinking the keyfile where gpgpath ends up would need semi-colons so that glibc could parse it (https://developer.gnome.org/glib/stable/glib-Key-value-file-parser.html#glib-Key-value-file-parser.description). But the separator for glibc to parse keys with multiple values can be changed to any character, so there is no real need to use semi-colons. Commas are also mentioned in that link. I'd be happy to go with commas, they do look nicer than semi-colons as well.

[rfairley]

Thanks @cgwalters for the review and helpful comments! I pushed a couple of fixup commits.

The first e75ac9d addresses the changes doing things the first way I had - not using the libglnx API.

The second, I used glnx_dirfd_iterator_init_at in ostree-gpg-verifier.c. With that came a bit of restructuring, but it seems to work well overall. One part where I found I still needed to use GKeyFile is 9c7ad70#diff-23d4c8887c7dda91613c9451bf3fe7c0R5112 to check if this is a directory. Am just having a look now into ot_dfd_iter_init_allow_noent() which you mentioned - sorry looked over this comment before. I think using this would avoid the need to use GKeyFile.

[rfairley]

Pushed another fixup which cleans up the loop in ostree-repo.c from the previous commit and uses ot_dfd_iter_init_allow_noent().

This changes behaviour when coming across a path that does not exist. Previously, the path to a file that did not exist would give a message No file or directory. This is because the filepath was added to the verifier if it did not exist.

Now, if the path does not exist, the path does not get added to the verifier. No error message indicates this, so this is silent. A test showing this change in behaviour is this https://github.com/ostreedev/ostree/pull/1773/files#diff-9a33529a3274e91605ccacb39789b065R220.

A benefit of having NOENT be silent is that if multiple gpgkeypaths are specified, and at least one of them exists and contains a valid key, the pull operation will not fail due to one of the other paths not existing.

The downside is there is no specific error message given letting the user know a path was imported that doesn't exist.

Either having NOENT be silent or enforcing that all the paths exist can be done. Will think about this. Any ideas which way to go, or if there is another way to report NOENT would be appreciated!

[rfairley]

^ Latest commits address the decl-and-inits, and I changed the nonexistent path handling to give No such file or directory if any of the paths given in the list don't exist. This behaviour is what _ostree_gpg_verifier_add_keyring_dir() follows, I think it makes sense to do the same for _ostree_gpg_verifier_add_keyfile_path().

Sorry for all the flip-flopping in the recent changes, will keep the changes settled now.

[rfairley]

Updated the manpage.

There are 2 last questions I have:

• I noticed the documentation in ostree.repo-config.xml mentions the config file follows the latest Freedesktop standards for Desktop Entry Files. While GNOME mentions commas being acceptable instead of semicolons to separate values, Freedesktop mentions only semicolons. Are we strictly following Freedesktop?
• Currently any file in a given directory path is imported. Adding a pattern matcher so that only *.asc files are imported would be simple to add in, should this be done? I thought about pattern matching from the paths given in the gpgkeypath list, to enable globbing, but it does complicate the logic. Maybe add as an enhancement if requested.

[jlebon]

I noticed the documentation in ostree.repo-config.xml mentions the config file follows the latest Freedesktop standards for Desktop Entry Files. While GNOME mentions commas being acceptable instead of semicolons to separate values, Freedesktop mentions only semicolons. Are we strictly following Freedesktop?

Hmm, it would be nice to support both I think, since traditionally keyfiles use semicolons for lists. So that's an easy gotcha to fall into. What do you think of something like:

• parse as a string first, if there are neither commas or semicolons, just return that
• otherwise, use g_key_file_set_list_separator depending on above and parse as a list

?

Currently any file in a given directory path is imported. Adding a pattern matcher so that only *.asc files are imported would be simple to add in, should this be done?

I think the original reason for this ask was to support just passing /etc/pki/rpm-gpg, which only holds keys. So I think doing this later when a use case shows up would be fine.

[rfairley]

Thanks @jlebon !

What do you think of something like:

parse as a string first, if there are neither commas or semicolons, just return that
otherwise, use g_key_file_set_list_separator depending on above and parse as a list

I like that idea, so g_key_file_set_list_separator can set the separator to , or ; depending on which one is present in the key being read (gpgkeypath here, but this logic can apply to other keys). If both ; and , are present in a string parsed for a single key, then we throw an error? Or perhaps default to separating by the ; in that case, but error might be more informative.

I think doing this later when a use case shows up would be fine.

+1

[jlebon]

If both ; and , are present in a string parsed for a single key, then we throw an error? Or perhaps default to separating by the ; in that case, but error might be more informative.

Sure, making it an error SGTM!

[rfairley]

⬆️ fixup to add the check for both ; and ,, returning an error if both are present.

The function ot_keyfile_get_string_as_list deals with this check, and returns a null-terminated array of strings (whether the single-string or list-of-strings case).

[jlebon]

You can use g_build_filename for this and drop the need for sep.

[cgwalters]

Yeah, though strictly speaking...since we're doing fd-relative here, a cleaner approach would be to open the file here, stashing it an another array, and then later using gpgme_data_new_from_fd() just like we do for paths.

Or even more strongly, open every file here and then change the verifier to just iterate over the fds.

(This is an optional followup)

[rfairley]

Ahh, the utility functions are handy. Added this in.

Storing the fds while reading the paths and later iterating over those once added to the verifier sounds clean, saves doing the intermediate work of storing the path. I added a TODO for this.

[jlebon]

This looks good to me. Nice work! 👍
Will let @cgwalters have a final lookover.

[cgwalters]

Very nice, thanks!

@rh-atomic-bot r+ a802199

[rh-atomic-bot]

📋 Looks like this PR is still in progress, ignoring approval

[cgwalters]

@rh-atomic-bot r+ a802199

[rh-atomic-bot]

⚡ Test exempted: merge already tested.

[rfairley]

Thanks for the review @cgwalters and @jlebon !

[dustymabe]

woot! Nice work @rfairley