Allow setting op signature type on pk token creation #63
Conversation
| @@ -36,9 +36,15 @@ type PKToken struct { | |||
| Cos *Signature // Cosigner Signature | |||
| } | |||
|
|
|||
| func New(idToken []byte, cicToken []byte) (*PKToken, error) { | |||
| func New(idToken []byte, cicToken []byte, isGQ bool) (*PKToken, error) { | |||
There was a problem hiding this comment.
What do you think about?
func New(idToken []byte, cicToken []byte, sigType SignatureType,) I like the property that this would document what is being flagged
pktoken.New(idToken.Bytes(), cicToken, pktoken.Gq)
vs a naked Boolean
pktoken.New(idToken.Bytes(), cicToken, false)
Just a idea, not a blocking request
There was a problem hiding this comment.
I have some small issues with the signature type and exposing it like that. Ideally, I would like to be able to rely on the arg value in the header to figure out whether or not it's a GQ signature and verify based on that instead of an unprotected value. So, for now I want to keep it a bool because that's what we have elsewhere and hopefully we won't need that in the future.
|
Thanks for taking a look at this and figuring how the bug was introduced. How tricky would it be to unit test this so that we catch regressions? |
Found a super easy way to do this. I just took our existing tests and had them check the provider's signature type instead of assuming it based on test data. d4fabf2 |
Before, we were assuming that the PK token was always being created with the oidp's ID token. That assumption changed in #48, but the code was still setting the signature type as
oidcinstead ofoidc_gq. Therefore, the validator thought the provider's signature was non-gq and went through that flow where it failed because the signature was gq.Relates to issue #62