chainguard issuer: use the same proof of possession cosign expects #1725
Conversation
Signed-off-by: Priya Wadhwa <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1725 +/- ##
==========================================
- Coverage 57.93% 49.61% -8.32%
==========================================
Files 50 71 +21
Lines 3119 4184 +1065
==========================================
+ Hits 1807 2076 +269
- Misses 1154 1879 +725
- Partials 158 229 +71 ☔ View full report in Codecov by Sentry. |
| return &workflowPrincipal{ | ||
| issuer: token.Issuer, | ||
| subject: token.Subject, | ||
| subject: subject, |
There was a problem hiding this comment.
We want our subject to be our subject. It's the challenge that's wrong.
There was a problem hiding this comment.
I don't know what else keys off of the principal.Name but I could see us changing that.
I think that Fulcio and cosign using different functions to extract the information being signed for PoP is incredibly fragile because in cases like this the different pieces of logic will disagree even though nothing is actually incorrect (they are just extracting things inconsistently).
There was a problem hiding this comment.
It looks like this is challenging to call directly the way the principal interface is set up, and the only call site of Name() is for the PoP so I can stage a change to do this with some test cases.
There was a problem hiding this comment.
I would create another field, popSubject, set equal to oauthflow.SubjectFromToken, and update Name() to return token.popSubject.
There was a problem hiding this comment.
I sent: #1726, which I think threads the needle.
I don't love this because Name() could start getting used elsewhere assuming it's subject and it would only break us, but anything more requires more significant surgery.
There was a problem hiding this comment.
@haydentherapper I just saw your comment, that's what my PR does essentially.
|
Closing in favor of the other |
this should allow the chainguard issuer to work when the email verified claim is set