-
Notifications
You must be signed in to change notification settings - Fork 49
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
sigstore, test: honor PublicKeyDetails when loading Keyrings #953
Conversation
Signed-off-by: William Woodruff <william@trailofbits.com>
if public_key.key_details in self._RSA_SHA_256_DETAILS: | ||
hash_algorithm = hashes.SHA256() | ||
key = load_der_public_key(public_key.raw_bytes, types=(rsa.RSAPublicKey,)) | ||
elif public_key.key_details in self._EC_DETAILS_TO_HASH: | ||
hash_algorithm = self._EC_DETAILS_TO_HASH[public_key.key_details] | ||
key = load_der_public_key( | ||
public_key.raw_bytes, types=(ec.EllipticCurvePublicKey,) | ||
) | ||
else: | ||
raise InvalidKeyError(f"unsupported key type: {public_key.key_details}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NB: This brings us to parity with the previous behavior, i.e. allowing ECDSA and RSA. But it gives us the flexility to allow additional keytypes in the future 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 to a nice error message
test/unit/internal/test_ctfe.py
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NB: This file entirely tested the old keyring loading APIs, including APIs that were never used anyways (like Keyring.add
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Separately, this will also "fix" our handling of key IDs per sigstore/rekor#2062, in the sense that we'll go from failing closed on key ID mismatch to trying all keys in the keyring.
Only concern with this is it's inefficient to verify with all keys. Is the motivation to avoid dealing with the key ID calculation and continuing to support RSA?
if public_key.key_details in self._RSA_SHA_256_DETAILS: | ||
hash_algorithm = hashes.SHA256() | ||
key = load_der_public_key(public_key.raw_bytes, types=(rsa.RSAPublicKey,)) | ||
elif public_key.key_details in self._EC_DETAILS_TO_HASH: | ||
hash_algorithm = self._EC_DETAILS_TO_HASH[public_key.key_details] | ||
key = load_der_public_key( | ||
public_key.raw_bytes, types=(ec.EllipticCurvePublicKey,) | ||
) | ||
else: | ||
raise InvalidKeyError(f"unsupported key type: {public_key.key_details}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 to a nice error message
_PublicKeyDetails.PKIX_ECDSA_P384_SHA_384: hashes.SHA384(), | ||
_PublicKeyDetails.PKIX_ECDSA_P521_SHA_512: hashes.SHA512(), | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no need to do this here, but is it relatively straightforward to add ed25519 down the line?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep! It should just be another branch (below) in the future.
Pretty much -- I intend to fix the key ID calculation as well, but I didn't want to blow this diff up too much 🙂 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good, thanks.
test_trust_root.py is really begging to be simplified a bit... but this is fine.
Yeah, this is on my distant queue (unless you or @javanlacerda feel like taking a stab at it 😉) |
See sigstore/sigstore-conformance#140 for some context -- we were previously ignoring
PublicKeyDetails
entirely.This adds a
Key
abstraction that gets constructed from thePublicKey
message, handlingPublicKeyDetails
in the process. It also cleans up some of our error types and old tests (liketest_ctfe.py
, which is entirely obviated by the trustroot APIs and their tests now).Separately, this will also "fix" our handling of key IDs per sigstore/rekor#2062, in the sense that we'll go from failing closed on key ID mismatch to trying all keys in the keyring.