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
Add initial support for GQ signatures #3
Conversation
Using sign rather than prove as that's what we're really doing with GQ
ae59fb4
to
0bb7a71
Compare
Just working through your protocol description. One thing that confused me is that you use |
Yes, what I've written as the suggested value of |
Timing side challenges in RSA are real and something we should be concerned with and even if we were using bigint, we'd want to have unittests that confirmed that there operations are constant time. I don't think this should hold up merging the PR, instead I've created an issue so that we can do a full review later: #11 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did a light code review, deferring a full crypto review until later (see #11). Happy to merge if after math/rand
fix. Beyond that, it looks good
gq/gq_test.go
Outdated
"testing" | ||
"time" | ||
|
||
insecurerand "math/rand" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you use: "crypto/rand"
here instead or is there a dependency on the RSA library?
See example of crypto/rand usage:
https://github.com/bastionzero/openpubkey/blob/main/pktoken/signer.go#L186
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yeah, I'm using math/rand for this test because crypto/rand doesn't support seeding the rng (as you would hope) and I wanted the tests to be deterministic. I realize I've only half-implemented what I was planning though - to seed the rng with a random number which we also print out, so that if there's some failure in a test to do with the random input we can reproduce it easily.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh never mind, this doesn't actually work - I thought it was because I was looking at cached test output. The rsa.GenerateKey function has some code that randomly reads an extra byte from the rng to stop you relying on it being deterministic with a seeded rng 😅
I will remove the parameters I've added and just use crypto/rand.Reader everywhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apologizes, that's my mistake. I thought that was the RNG for gq, not the test, the file name gq_test.go
should have tipped me off.
I realize I've only half-implemented what I was planning though - to seed the rng with a random number which we also print out, so that if there's some failure in a test to do with the random input we can reproduce it easily.
That's an excellent idea. If you figure out a way to implement it with rsa.GenerateKey, we should do it. If not, don't worry about it.
Closes #8
Adds a
gq
package for GQ signature support. The package stands alone at the moment, it isn't actually used by any of the other code yet.Currently, an OpenPubkey token contains the original signature from the OIDC provider. This arguably means that it isn't safe to share the OpenPubkey token, as the token could be used to impersonate the subject to a service that doesn't check the audience claim, or to trick an OpenPubkey client to issue a token for the subject.
We can create a GQ signature to prove that we have the original OIDC provider's signature. This can be verified using the original OIDC signing payload (
header || '.' || payload
) and the OIDC provider's public key. This works because GQ signature private keys are equivalent to RSA signatures.The algorithms described here are from GQ1 in ISO/IEC 14888-2:2008.
Notation
If$n$ is a number, $|n|$ is the number of bits required to store $n$ .
Signing flow
Input
Algorithm
Verification flow
Input
Algorithm
Potential issues
Go's math/big package says:
I'm not sure if this is a problem at the moment.
We currently just pass the OIDC payload as the message to be signed. This could be something more useful. I think it's fine to basically sign the public key?