Add GPGKey and use with GPGSigner #488
Conversation
There was a problem hiding this comment.
Looks pretty good to me: it's obviously a bit of a mess with going back and forth beween legacy gpg format and and shared one, but that's all hidden behind the signer API now...
I think requiring secretshandler to be none is wrong but that's the only complaint really.
Is there a test that ensures that the client usage (desdrialize + verify) works without gnupg being installed?
| exceptions.FormatError, | ||
| gpg_exceptions.KeyExpirationError, |
There was a problem hiding this comment.
UnsupportedLibraryError too maybe as this will run in the client?
| if secrets_handler is not None: | ||
| raise ValueError("GPGSigner does not support a secrets handler") | ||
|
|
There was a problem hiding this comment.
I think this is not useful -- code that uses from_priv_key_uri() is aiming to work with as many signers as it can, using the shared API. This seems to force me to write two code paths for no gain.
There was a problem hiding this comment.
@jku, do you think we should just ignore it? Maybe log a warning/info? With current implementation of securesystemslib.gpg we cannot pass any secrets to the gpg command.
There was a problem hiding this comment.
No need for warning as far as I am concerned: the way I imagine this being used (and the way I am using it) is that the signing code doesn't care which signers are used or what they do:
- online signing app likely does not have a secrets handler
- an offline signing tool does provide a secrets handler
- even the offline signing tool does not care whether the secrets handler is used or not: a gpg signing might not use it but a yubikey signing will
|
rebased without changes |
|
Thanks for the review.
Agreed. This is not nice but can be easily fixed, once in-toto also transitions to the signer API.
Will change.
Will (fix and) add tests before un-drafting this PR. |
|
Thanks for the comments, @jku! I've addressed your concerns and added plenty of test, which also revealed a serialization bug (fixed in 3d140d5). Beyond that, I removed the keyid field from GPGSigner, given that it also has a GPGKey attached, which has the same user-assigned keyid. I am still unsure how to best resolve the issue that this signer keyid may differ from the keyid of a signature generated by that signer. This is because gpg may use a subkey of the key identified by the passed keyid and we assign the subkey keyid to the signature, so that we know, at verification time, which key or subkey of the public key bundle we should use. I have a feeling that the signer API should just not support the subkey-masterkey relationship. It adds, for no benefit, a PKI hierarchy to tuf/in-toto, which already have their own PKI hierarchies. This solution entails the following changes
caveat: Explicitly identifying a public key before signing requires additional tooling to allow signing with the default key, which is currently possible in in-toto. An alternative solution, which allows both PKI hierarchies alongside each other, would be to accept public key bundles with subkeys, but always assign the master key keyid to the signature, regardless of what key/subkey was used. However, this would require a different way of matching the correct key in the public key bundle with a signature at verification time. |
|
One more minor thing I thought about is the For context:
|
I haven't had a chance to look through how this is used but would this change impact our notions of subkeys during verification? If only the subkey is returned, would in-toto (for example) have to look elsewhere when verifying that threshold wasn't met using subkeys? In in-toto, we use |
Yes, both of my proposals are not compatible with current in-toto usage of GPG keys and signatures. in-toto currently considers all keys in a public key bundle (masterkey + subkeys), both, to check if a key is authorized via layout to provide a signature, and to identify the exact key or subkey in the bundle to verify the signature. But this is not a concern, because the key and signature metadata formats of the GPGSigner API in securesystemslib are already incompatible with in-toto,. For in-toto-backwards- and securesystemslib-signer-compatibility, I proposed adding a custom _LegacyGPGSigner API to the in-toto code base (see my other comment above). |
|
Another thought about parallel PKI: We export gpg keys with Given that in-toto and TUF already have a mechanism to expire signing keys, that is via an expiration date in the delegating metadata, I wonder, if we should ignore expiration information of gpg keys, when verifying signatures, and leave it to the user to not add gpg keys to delegating metadata that would expire earlier than the metadata. |
I agree with this, and both of your proposed solutions seem solid (although I admit I'm not fully immersed in the problem space of PGP subkeys).
This is a reasonable point: increasing the complexity of the serialization format doesn't seem useful if the same practical security advantage can be had by making the key-import code just slightly more complex. |
lukpueh
force-pushed
the
add-GPGKey
branch
2 times, most recently
from
3ccdecc
to
1752316
Compare
GPGKey is a regular Key with additional GnuPG specific key fields, and verification method. It also has conversion helpers to translate from and to a non-in-toto/tuf-spec compliant key format, which is still used by the underlying securesystemslib.gpg subpackage. GPGSigner is updated to: - take a GPGKey as constructor argument, and implement - `from_priv_key_uri`, to load signer from `"gnupg:[<GnuPG homedir>][?id=<keyid>]"` - `import_`, to import a public key from a GnuPG keyring and return it along with a uri to create the signer. Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>
Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>
- add expected exception to verify method - warn on passed secrets handler, don't raise Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>
Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>
The keyid is redundant with the keyid of the attached public key instance. Same goes for the keyid parameter in the related private key uri, which can also be read from the public key instance passed to the from_priv_key_uri method. Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>
Include unrecognized fields in GPGKey.to_dict. Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>
Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>
New tests for: - `sign` and `import_` failure but successful verification, if 'gpg' is not available. - verification failure, if 'cryptography' is not available. - key de/serialization (also legacy format) and comparison (__eq__) - expected failures on `from_priv_key` and `verify_signature` Improvements: - remove obsolete `assertFalse` in `with self.assertRaises` block - condense tests in `test_gpg_functions` (use DDT instead of copy-paste) Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>
Supporting subkeys in a GPGKey and considering them for verification adds, for no benefit, a PKI hierarchy to tuf/in-toto, which already have their own PKI hierarchies. In addition it makes the verification (and delegation) code more complex and error prone. This commit drops subkey support, by the following two changes: - import_: require exact match between passed keyid, and one of the keys in the bundle returned by gpg, and return a GPGKey only for that key, w/o subkeys - sign: require exact match between keyid on attached public key and keyid on the signature returned by gpg Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>
GPGSigner does not support signing with a default key (unlike the lower level securesystemslib gpg signing function), thus the test is redundant with the non-default key test. Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>
- creation_time + validity_period: key validity should be determined by the metadata expiration time alone, and not by an additional signer-specific key expiration, which is prone to be out of sync (we don't use gpg for verification) - subkeys: dropped support in a previous commit - hashes: static for currently supported schemes. If other hash algorithms are needed, they should be encoded it in the scheme string. Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>
These methods are ugly no matter where they are implemented. I move them to the signer to keep them together with the legacy signature format conversion methods. Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>
Change warning type log statement about exported subkeys to debug. This is no longer relevant for GPGSigner.import_, which don't return key bundles to the user, but only a key or a subkey. Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>
|
|
||
| for key in raw_keys: | ||
| if key["keyid"] == keyid: | ||
| # TODO: Raise here if key is expired, revoked, incapable, ... |
There was a problem hiding this comment.
I'll create a ticket for this. We can also partially re-use #190)
| ("rsa", "pgp+rsa-pkcsv1.5"): GPGKey, | ||
| ("dsa", "pgp+dsa-fips-180-2"): GPGKey, | ||
| ("eddsa", "pgp+eddsa-ed25519"): GPGKey, |
There was a problem hiding this comment.
are these the same keytype/scheme combos that were used before, but the serialization format is now slightly different? Would it make sense sense to use e.g. a different keyname?
I guess in-toto is the only existing user... so do what makes sense there.
There was a problem hiding this comment.
The thing is, these keytype/scheme combos are completely new. We did use the same values as type/method combos, but I think that's okay.
The `gpg` subpackage provides a vaguely defined API (`gpg.functions`) to create signatures, export public keys, and verify signatures. This API and the used formats are incompatible with the securesystemslib signer API. For the sake of a consistent API, the `gpg` subpackage is marked as internal (renamed to `_gpg`) and the above mentioned functionality is exposed via the new signer API. Replacement methods are: - `GPGSigner.import_` (replaces `export_pubkey`) - `GPGSigner.sign` - `GPGKey.verify_signature` Note that public key and signature formats also change, in order to match `Key` and `Signature` interfaces. This means: - signature field `signature` is renamed to `sig` - public key fields `type`, `method` and `hashes` are replaced by `keytype` and `scheme` fields, and - public keys no longer include `subkeys` or key expiration infos. This means that the signature verification function no longer needs to decide, if a key is authorized or valid to verify a given signature. See discussion for context: secure-systems-lab#488 (comment) secure-systems-lab#488 (comment) Signed-off-by: Lukas Puehringer <lukas.puehringer@nyu.edu>
EDIT 2023/01/20: remove key id param in private key URI
EDIT 2023/03/06:
see #488 (comment) pp. and commit messages for more details
Closes #450
Kept as draft to,
Description of the changes being introduced by the pull request:
GPGKey is a regular Key with additional GnuPG specific key fields,
and verification method. It also has conversion helpers to translate
from and to a non-in-toto/tuf-spec compliant key format, which is
still used by the underlying securesystemslib.gpg subpackage.
GPGSigner is updated to:
from_priv_key_uri, to load signer fromgnupg:[<GnuPG homedir>]import_, to import a public key from a GnuPG keyring and return it along with a uri to create the signer.Please verify and check that the pull request fulfils the following requirements: