Remove re-implementation of sigstore proto types #94
Conversation
Signed-off-by: Samuel Giddins <[email protected]>
Signed-off-by: Samuel Giddins <[email protected]>
Signed-off-by: Samuel Giddins <[email protected]>
lib/sigstore/signer.rb
Outdated
| end | ||
|
|
||
| general_names = leaf.extension(Internal::X509::Extension::SubjectAlternativeName).general_names | ||
| expected_san = [[:uniformResourceIdentifier, @identity_token.identity]] |
There was a problem hiding this comment.
is this generally going to be true, even for other issuers?
There was a problem hiding this comment.
Nope, unfortunately -- the Google and GitHub (user, not actions) issuers will embed a SAN with an rfc822Name instead, corresponding to the underlying email subject.
IIRC the way we handle this on sigstore-python is to enforce that there's exactly one SAN and check the SAN's value, regardless of its specified type. This isn't ideal from a domain separation perspective but it's about as clean as it currently gets without a stronger notion of subject "type" on the OIDC side.
There was a problem hiding this comment.
hmm -- given the spec calls for the client to validate the returned SAN, it would be helpful if we could publish a simple mapping of ID providers to subject names somewhere in fulcio that clients can grab and substitute into?
There was a problem hiding this comment.
Yeah, this would be good to have -- I think the closest thing to this currently is https://github.com/sigstore/fulcio/blob/b2186c01da1ddf807bde3ea8c450226d8e001d88/pkg/config/config.go#L182-L201, but that's only in Fulcio's code and not documented anywhere separately AFAICT.
(IMO we could make this general in a follow-up, or weaken the check to not include the SAN type and only match on the SAN value. Your call!)
There was a problem hiding this comment.
| end | ||
|
|
||
| body = JSON.parse(resp.body) | ||
|
|
||
| # TODO: verify | ||
| # The signer MUST verify the log entry as in Spec: Transparency Service. |
There was a problem hiding this comment.
given this happens immediately below in verify, is verifying the log entry separately necessary?
There was a problem hiding this comment.
My interpretation of the spec is "no," although sigstore-python doesn't actually do "verify after sign" yet 😅. CC @loosebazooka @steiza for other client opinions.
There was a problem hiding this comment.
(maybe i'm reading this ruby code wrong but) Java does a verification here https://github.com/sigstore/sigstore-java/blob/main/sigstore-java/src/main/java/dev/sigstore/KeylessSigner.java#L338 immediately after obtaining the log entry. There's some presumed minor benefit that if the log goes rogue, you get to detect it a little early?
There was a problem hiding this comment.
sigstore-go does not verify the transparency log entry it receives during signing, but it will verify the bundle before returning it to the user, if a trusted root to verify with is provided.
| @@ -396,5 +392,91 @@ def extract_timestamp_from_verification_data(data) | |||
| resp.token_info.gen_time | |||
| end | |||
| end | |||
|
|
|||
| def find_rekor_entry(bundle, hashed_input, offline:) | |||
There was a problem hiding this comment.
moved from the model into the verifier
segiddins
changed the title
Signed-off-by: Samuel Giddins <[email protected]>
Signed-off-by: Samuel Giddins <[email protected]>
segiddins
deleted the
segiddins/begin-removing-wrappers-around-sigstore-proto-types
branch
* Begin removing wrappers around sigstore proto types Signed-off-by: Samuel Giddins <[email protected]> * Remove verification materials Signed-off-by: Samuel Giddins <[email protected]> * Implement more signing verification Signed-off-by: Samuel Giddins <[email protected]> * Address 2 comments Signed-off-by: Samuel Giddins <[email protected]> * Relax SAN check Signed-off-by: Samuel Giddins <[email protected]> --------- Signed-off-by: Samuel Giddins <[email protected]>
* Begin removing wrappers around sigstore proto types Signed-off-by: Samuel Giddins <[email protected]> * Remove verification materials Signed-off-by: Samuel Giddins <[email protected]> * Implement more signing verification Signed-off-by: Samuel Giddins <[email protected]> * Address 2 comments Signed-off-by: Samuel Giddins <[email protected]> * Relax SAN check Signed-off-by: Samuel Giddins <[email protected]> --------- Signed-off-by: Samuel Giddins <[email protected]>
* Sigstore signer Signed-off-by: Samuel Giddins <[email protected]> Revert "Add merkle inclusion proof tests" This reverts commit 7a4bef7f6f1f0f7a8bb76e9c9a214d6de5900fa7. * [DNM] Dump base64 of failed req for debugging Signed-off-by: Samuel Giddins <[email protected]> * Remove re-implementation of sigstore proto types (#94) * Begin removing wrappers around sigstore proto types Signed-off-by: Samuel Giddins <[email protected]> * Remove verification materials Signed-off-by: Samuel Giddins <[email protected]> * Implement more signing verification Signed-off-by: Samuel Giddins <[email protected]> * Address 2 comments Signed-off-by: Samuel Giddins <[email protected]> * Relax SAN check Signed-off-by: Samuel Giddins <[email protected]> --------- Signed-off-by: Samuel Giddins <[email protected]> * Use Internal::Key for sigstore keyrings too Signed-off-by: Samuel Giddins <[email protected]> * Address comments on signing pr Signed-off-by: Samuel Giddins <[email protected]> * Stop hanging keyrings off the rekor client Signed-off-by: Samuel Giddins <[email protected]> * Actually use proper tmpdir for home Signed-off-by: Samuel Giddins <[email protected]> --------- Signed-off-by: Samuel Giddins <[email protected]>
Targets #81
Signed-off-by: Samuel Giddins [email protected]