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

sigstore, test: honor PublicKeyDetails when loading Keyrings #953

Merged
merged 4 commits into from
Apr 3, 2024

Conversation

woodruffw
Copy link
Member

@woodruffw woodruffw commented Apr 2, 2024

See sigstore/sigstore-conformance#140 for some context -- we were previously ignoring PublicKeyDetails entirely.

This adds a Key abstraction that gets constructed from the PublicKey message, handling PublicKeyDetails in the process. It also cleans up some of our error types and old tests (like test_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.

Signed-off-by: William Woodruff <william@trailofbits.com>
@woodruffw woodruffw added the refactoring Refactoring tasks. label Apr 2, 2024
@woodruffw woodruffw added this to the 3.0 milestone Apr 2, 2024
@woodruffw woodruffw requested review from jku and haydentherapper April 2, 2024 22:29
@woodruffw woodruffw self-assigned this Apr 2, 2024
Comment on lines +138 to +147
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}")
Copy link
Member Author

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 🙂

Copy link
Contributor

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

Copy link
Member Author

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)

Signed-off-by: William Woodruff <william@trailofbits.com>
Copy link
Contributor

@haydentherapper haydentherapper left a 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?

Comment on lines +138 to +147
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}")
Copy link
Contributor

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(),
}

Copy link
Contributor

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?

Copy link
Member Author

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.

@woodruffw
Copy link
Member Author

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?

Pretty much -- I intend to fix the key ID calculation as well, but I didn't want to blow this diff up too much 🙂

Copy link
Member

@jku jku left a 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.

@woodruffw
Copy link
Member Author

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 😉)

@woodruffw woodruffw enabled auto-merge (squash) April 3, 2024 15:34
@woodruffw woodruffw merged commit 426120f into main Apr 3, 2024
25 checks passed
@woodruffw woodruffw deleted the ww/use-keydetails branch April 3, 2024 15:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactoring Refactoring tasks.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants