Token fixes #412
Token fixes #412
Conversation
jku
force-pushed
the
token-fixes
branch
2 times, most recently
from
ba0ed7b
to
73597db
Compare
Signed-off-by: Jussi Kukkonen <[email protected]>
* Make email optional when parsing JWT (e.g. GitHub actions does not use it) * Add IdentityToken.identity field: this is the identity claim that we believe Fulcio uses for this issuer * Fix the bundle signing so it uses the new identity field * Add test with a GitHub Actions token Note that signing with a Sub claim is still not supported but we're now a bit closer. Signed-off-by: Jussi Kukkonen <[email protected]>
Apparently Fulcio does not care about the CSR subject: just claim everything is an email. https://github.com/sigstore/fulcio/blob/main/fulcio.proto#L106 Signed-off-by: Jussi Kukkonen <[email protected]>
This comment was marked as outdated.
This comment was marked as outdated.
src/oauth/token.rs
Outdated
| let identity = match claims.iss.as_str() { | ||
| "https://accounts.google.com" | ||
| | "https://oauth2.sigstore.dev/auth" | ||
| | "https://oauth2.sigstage.dev/auth" => { | ||
| if let Some(email) = claims.email.as_ref() { | ||
| Identity::Email(email.clone()) | ||
| } else { | ||
| return Err(SigstoreError::IdentityTokenError( | ||
| "Email claim not found in JWT".into(), | ||
| )); | ||
| } | ||
| } | ||
| _ => { | ||
| if let Some(sub) = claims.sub.as_ref() { | ||
| Identity::Sub(sub.clone()) | ||
| } else { | ||
| return Err(SigstoreError::IdentityTokenError( | ||
| "Sub claim not found in JWT".into(), | ||
| )); | ||
| } | ||
| } | ||
| }; | ||
|
|
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Back to ready for review: the IdentityToken is fine, I just messed up GitHubs token-request-token handling in my test. Speaking of that, I can add add my test for signing-in-a-github-workflow in this PR too, I just figured it does not need to be here. Let me know if you have opinions |
We don't judge the claims that OIDC provider makes (apart from some compatibility checks): make this clear in the API and docs. Signed-off-by: Jussi Kukkonen <[email protected]>
|
Latest commit makes it clear that identity claim comes from the OIDC JWT. Let me know if you feel strongly about the solution here: I can make identity_claim a method in |
|
FYI in case you'd rather see a more complete picture:
|
| assert_eq!(identity_token.claims.email, None); | ||
| assert_eq!( | ||
| identity_token.claims.sub, | ||
| Some(String::from("repo:sigstore-conformance/extremely-dangerous-public-oidc-beacon:ref:refs/heads/main")) |
There was a problem hiding this comment.
I'm going to double check if this is correct: It is the token "sub" field... but it does not seem to be what "bundle verify" uses for --identity
There was a problem hiding this comment.
I guess that's just how it works? When we ask for the certificate we have no idea what the actual certificate subject is going to be as it's not the identity claim in the OIDC token... I really hope I'm just tired and that's not true
There was a problem hiding this comment.
yes, I found the identity handling bits in fulcio and can confirm: before we get the certificate we ultimately do not know who we are signing as... exposing the token identity here is still useful since it happens to match the signing identity in the interactive case but it's unfortunately not as useful as I hoped.
I'm not too confident with rust or this code base yet so advice is welcome: I can see there are many ways to implement something like the "signing identity" here but I don't think I have a feel for best practices yet...
Summary
Support more types of OIDC tokens -- specifically support the "Sub" field used as the signing identity by some issuers.
Note that signing with a "Sub" identity now works (the CSR will use the email OID for everything as before but apparently that works: sigstore-python does that too) in places like Github actions but there are no tests yet.
My plan after this is to:
... but these are currently not part of this PR.
Fixes #413