Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remote GPG key info #2401

Merged
merged 12 commits into from
Aug 20, 2021
Merged

Remote GPG key info #2401

merged 12 commits into from
Aug 20, 2021

Conversation

dbnicholson
Copy link
Member

This is split out from #2260. It adds an API, ostree_repo_remote_get_gpg_keys, and CLI (remote list-gpg-keys) for getting information about GPG keys associated with a remote (or the global keys when no remote is specified).

That's useful on its own, but the latter part of the PR adds an implementation to generate OpenPGP Web Key Directory URLs so that you can get a well defined place to download an updated key. In #2260 I was trying to go further and have ostree API to go fetch those and import them, but I think with the appropriate information available it would be easy to write a tool that does the fetching and importing itself. That part isn't strictly necessary for anything, but I found it convenient to have the implementation here so it could be usable by any OSTree consumer that relies on GPG verification.

There are a couple parts that I'm not really sure about. First, the key listing is basically a GVariant version of gpgme_key_t and friends. I'd kinda prefer to work with a real data structure, but I was unsure about committing to a type there.

Second, Web Key Directory (WKD) URLs use an obscure base32 encoding referred to as zbase32 that's supposed to be more usable for humans. I find the use dubious as what it's encoding is a SHA1 checksum, so the encoding is always 32 characters and I know I could never handle that in my head. Anyways, that's what's in the spec and I copied in the author's reference C implementation. If that's troublesome, I could also code up a naive implementation. It's just base32 with an alternate alphabet and this isn't in any fast path.

dbnicholson and others added 9 commits July 15, 2021 15:50
Currently the verifier only imports all the GPG keys when verifying
data, but it would also be useful for inspecting the trusted keys.
In order to use the GPG verifier, it needs to be seeded with GPG keys
after instantation. Currently this is only used for verifying data, but
it will also be used for getting a list of trusted GPG keys in a
subsequent commit.
Currently the verifier decides whether to include the global keyrings
based on whether the specified remote has its own keyring or not. Allow
callers to exclude the global keyrings even when that's not the case.
This will be used in a subsequent commit in order to get the GPG keys
only associated with a remote.
This function enumerates the trusted GPG keys for a remote and returns
an array of `GVariant`s describing them. This is useful to see which
keys are collected by ostree for a particular remote. The same
information can be gathered with `gpg`. However, since ostree allows
multiple keyring locations, that's only really useful if you have
knowledge of how ostree collects GPG keyrings.

The format of the variants is documented in
`OSTREE_GPG_KEY_GVARIANT_FORMAT`. This format is primarily a copy of
selected fields within `gpgme_key_t` and its subtypes. The fields are
placed within vardicts rather than using a more efficient tuple of
concrete types. This will allow flexibility if more components of
`gpgme_key_t` are desired in the future.
This provides a wrapper for the `ostree_repo_remote_get_gpg_keys`
function to show the GPG keys associated with a remote. This is
particularly useful for validating that GPG key updates have been
applied. Tests are added, which checks the
`ostree_repo_remote_get_gpg_keys` API by extension.
This will be used to implement the PGP Web Key Directory (WKD) URL
generation. This is a slightly cleaned up implementation[1] taken from
the zbase32 author's original implementation[2]. It provides a single
zbase32_encode API to convert a set of bytes to the zbase32 encoding.

I believe this should be acceptable for inclusion in ostree. The license
in the source files is BSD style while the original repo LICENSE file
claims the Creative Commons CC0 1.0 Universal license, which is public
domain.

1. https://github.com/dbnicholson/libbase32/tree/for-ostree
2. https://github.com/zooko/libbase32
Calculate the advanced and direct update URLs for the key discovery
portion[1] of the OpenPGP Web Key Directory specification, and include
the URLs in the key listing in ostree_repo_remote_get_gpg_keys(). These
URLs can be used to locate updated GPG keys for the remote.

1. https://datatracker.ietf.org/doc/html/draft-koch-openpgp-webkey-service#section-3.1
If the key UID contains a valid email address, include the GPG WKD
update URLs in GVariant returned by ostree_repo_remote_get_gpg_keys().

return TRUE;
#else /* OSTREE_DISABLE_GPGME */
return glnx_throw (error, "GPG feature is disabled in a build time");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps disabled at build time?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right. I'm pretty sure I copied this from another optional GPG feature, but there's no reason to copy the slightly wrong grammar.

* - key: `fingerprint`, value: `s`, key fingerprint hexadecimal string
* - key: `created`, value: `x`, key creation timestamp (seconds since
* the Unix epoch in UTC, big-endian)
* - key: `expires`, value: `x`, key expiration timestamp (seconds since
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It sounds like this is really a mx perhaps?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Possibly. This is copying the gpgme approach where 0 is no expiration (as noted a little later in the comment). I could special case 0 to allow for a mx. According to the OpenPGP RFC either missing or 0 are interpreted as not expiring. I think treating 0 as never expires is a little simpler and consistent with gpgme.

}

char *
zbase32_encode(const unsigned char *data, size_t length)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like the existing code has a lot of possible integer overflows, though we are only ever using it with length == 20.
Do you think it would be reasonable to make this generic encoder static/private and only expose a small wrapper which enforces the expected input array size?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that makes sense. I think you could also add an assert that length is less than SIZE_MAX / 8. I don't see how the rest of the code could overflow that condition.

@@ -65,6 +65,9 @@ Boston, MA 02111-1307, USA.
<cmdsynopsis>
<command>ostree remote gpg-import</command> <arg choice="opt" rep="repeat">OPTIONS</arg> <arg choice="req">NAME</arg> <arg choice="opt" rep="repeat">KEY-ID</arg>
</cmdsynopsis>
<cmdsynopsis>
<command>ostree remote list-gpg-keys</command> <arg choice="req">NAME</arg>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To align with the command above, gpg-list-keys or gpg-list maybe?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. I like gpg-list-keys.

Copy link
Member

@cgwalters cgwalters left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only skimmed this so far but I think it makes sense.

Mind splitting out the prep patches as a separate PR?

Comment on lines +265 to +267
if (tmp_dir != NULL) {
ot_gpgme_kill_agent (tmp_dir);
(void) glnx_shutil_rm_rf_at (AT_FDCWD, tmp_dir, NULL, NULL);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can probably make these autocleanups too in the future.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think so. The gpgme code could use some sprucing up.

{
g_auto(GVariantDict) subkey_dict = OT_VARIANT_BUILDER_INITIALIZER;
g_variant_dict_init (&subkey_dict, NULL);
g_variant_dict_insert_value (&subkey_dict, "fingerprint",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd love to figure out a good way to have reliable documentation for these keys. It's a downside of the gvariant approach.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. You can do decently in the FORMAT documentation, but it could definitely be better.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could also just turn this into a few boxed structs if that would be better. I actually really prefer working with actual data structures rather than variants, but I chose a variant to allow more flexibility for future changes.

@dbnicholson
Copy link
Member Author

Mind splitting out the prep patches as a separate PR?

For the prep do you mean everything before lib/repo: Add ostree_repo_remote_get_gpg_keys()?

Copy link
Member

@cgwalters cgwalters left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, let's merge this - we can do further commits as followups.

@cgwalters cgwalters merged commit 98f3fe3 into ostreedev:main Aug 20, 2021
@dbnicholson dbnicholson deleted the gpg-key-info branch August 20, 2021 20:29
@dbnicholson
Copy link
Member Author

OK, let's merge this - we can do further commits as followups.

Are you planning a release? Most of the outstanding stuff is pretty minimal, but @lucab's suggestion of ostree remote gpg-list-keys instead of ostree remote list-gpg-keys should probably be done before a release goes out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants