-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Upgrade interned jwx (0.9.x) with github.com/lestrrat-go/jwx/v3 #7733
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
Conversation
✅ Deploy Preview for openpolicyagent ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
johanfylling
left a comment
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.
This is a great effort, thank you for working on this! 😃
There are a couple of failing yaml tests (TestRego); looks like they're all related to error message mismatches.
v1/bundle/keys.go
Outdated
| block, _ := pem.Decode([]byte(s.Key)) | ||
| if block != nil { | ||
| return sign.GetSigningKey(s.Key, jwa.SignatureAlgorithm(s.Algorithm)) | ||
| alg, ok := jwa.LookupSignatureAlgorithm(s.Algorithm) |
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.
This check looks like it could be less expensive than reading a file from disk, should we move it up top?
| return errors.New("no keys found in JWK set") | ||
| } | ||
| if jwk.GetKeyTypeFromKey(key) != keys.Keys[0].GetKeyType() { | ||
| return errors.New("JWK derived key type and keyType parameter do not match") |
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.
This is a funny check, but I assume it's not necessary anymore. Ensured to not happen by jwk.Parse()?
go.mod
Outdated
| github.com/gobwas/glob v0.2.3 | ||
| github.com/google/go-cmp v0.7.0 | ||
| github.com/google/uuid v1.6.0 | ||
| github.com/lestrrat-go/jwx/v3 v3.0.8 |
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.
I think vendoring won't impede reviewing.
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.
I was also actively updating the code, so... :) I will add the vendored code in my subsequent commits
|
I think I fixed most everything? I'm not sure why I'm getting these conflicts as I seem to be up-to-date... |
Signed-off-by: Daisuke Maki <[email protected]>
Signed-off-by: Daisuke Maki <[email protected]>
Signed-off-by: Daisuke Maki <[email protected]>
* use jws.Parse to remove duplicate code (jws handles it) * align error messages to new error messages Signed-off-by: Daisuke Maki <[email protected]>
Signed-off-by: Daisuke Maki <[email protected]>
Signed-off-by: Daisuke Maki <[email protected]>
Signed-off-by: Daisuke Maki <[email protected]>
Signed-off-by: Daisuke Maki <[email protected]>
Signed-off-by: Daisuke Maki <[email protected]>
Signed-off-by: Daisuke Maki <[email protected]>
Signed-off-by: Daisuke Maki <[email protected]>
Signed-off-by: Daisuke Maki <[email protected]>
Signed-off-by: Daisuke Maki <[email protected]>
Signed-off-by: Daisuke Maki <[email protected]>
Signed-off-by: Daisuke Maki <[email protected]>
Signed-off-by: Daisuke Maki <[email protected]>
Signed-off-by: Daisuke Maki <[email protected]>
Signed-off-by: Daisuke Maki <[email protected]>
Signed-off-by: Daisuke Maki <[email protected]>
Signed-off-by: Daisuke Maki <[email protected]>
Signed-off-by: Daisuke Maki <[email protected]>
Signed-off-by: Daisuke Maki <[email protected]>
Signed-off-by: Daisuke Maki <[email protected]>
Signed-off-by: Daisuke Maki <[email protected]>
Signed-off-by: Daisuke Maki <[email protected]>
|
added the signoffs. |
|
Thank you @lestrrat! I'll try to review your changes as soon as possible, but might take me until start of next week to clear out my backlog. |
johanfylling
left a comment
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.
Thank you! LGTM 👍
We should remove the draft status from this PR, so CI/CD can run all checks (unless there is some reason not to that I'm unaware of).
There are still some tests that haven't been touched up with the new error messages:
$ make test
...
--- FAIL: TestRego (0.50s)
--- FAIL: TestRego/v1/jwtencodesignheadererrors/Unknown_signature_algorithm (0.00s)
exported_test.go:36: expected topdown error text "invalid character" but got: "test-0.rego:4: eval_builtin_error: io.jwt.encode_sign_raw: missing or invalid 'alg' header: cannot parse JSON: cannot parse object: missing ':' after object key; unparsed tail: \"JWT\\\",\\r\\n \\\"alg\\\":\\\"HS256\\\"}\""
--- FAIL: TestRego/v1/jwtencodesignheadererrors/Empty_headers_input_error (0.00s)
exported_test.go:36: expected topdown error text "unexpected end of JSON input" but got: "test-0.rego:4: eval_builtin_error: io.jwt.encode_sign_raw: missing or invalid 'alg' header: cannot parse JSON: cannot parse empty string; unparsed tail: \"\""
--- FAIL: TestRego/v1/jwtencodesignheadererrors/Empty_JSON_header_Error (0.00s)
exported_test.go:36: expected topdown error text "unsupported signature algorithm" but got: "test-0.rego:4: eval_builtin_error: io.jwt.encode_sign_raw: unknown JWS algorithm: "
--- FAIL: TestRego/v1/jwtencodesignheadererrors/No_JSON_Error (0.00s)
exported_test.go:36: expected topdown error text "invalid character" but got: "test-0.rego:4: eval_builtin_error: io.jwt.encode_sign_raw: unknown JWS algorithm: "
--- FAIL: TestRego/v1/jwtencodesignheadererrors/unknown_signature_algorithm (0.00s)
exported_test.go:36: expected topdown error text "unknown signature algorithm" but got: "test-0.rego:4: eval_builtin_error: io.jwt.encode_sign_raw: unknown JWS algorithm: dummy"
--- FAIL: TestRego/v0/jwtencodesignheadererrors/unknown_signature_algorithm (0.00s)
exported_test.go:29: expected topdown error text "unknown signature algorithm" but got: "test-0.rego:4: eval_builtin_error: io.jwt.encode_sign_raw: unknown JWS algorithm: dummy"
--- FAIL: TestRego/v0/jwtencodesignheadererrors/Unknown_signature_algorithm (0.00s)
exported_test.go:29: expected topdown error text "invalid character" but got: "test-0.rego:4: eval_builtin_error: io.jwt.encode_sign_raw: missing or invalid 'alg' header: cannot parse JSON: cannot parse object: missing ':' after object key; unparsed tail: \"JWT\\\",\\r\\n \\\"alg\\\":\\\"HS256\\\"}\""
--- FAIL: TestRego/v0/jwtencodesignheadererrors/No_JSON_Error (0.00s)
exported_test.go:29: expected topdown error text "invalid character" but got: "test-0.rego:4: eval_builtin_error: io.jwt.encode_sign_raw: unknown JWS algorithm: "
--- FAIL: TestRego/v0/jwtencodesignheadererrors/Empty_headers_input_error (0.00s)
exported_test.go:29: expected topdown error text "unexpected end of JSON input" but got: "test-0.rego:4: eval_builtin_error: io.jwt.encode_sign_raw: missing or invalid 'alg' header: cannot parse JSON: cannot parse empty string; unparsed tail: \"\""
--- FAIL: TestRego/v0/jwtencodesignheadererrors/Empty_JSON_header_Error (0.00s)
exported_test.go:29: expected topdown error text "unsupported signature algorithm" but got: "test-0.rego:4: eval_builtin_error: io.jwt.encode_sign_raw: unknown JWS algorithm: "
--- FAIL: TestRegoWithNDBCache (0.50s)
--- FAIL: TestRegoWithNDBCache/v1/jwtencodesignheadererrors/unknown_signature_algorithm (0.00s)
exported_test.go:69: expected topdown error text "unknown signature algorithm" but got: "test-0.rego:4: eval_builtin_error: io.jwt.encode_sign_raw: unknown JWS algorithm: dummy"
--- FAIL: TestRegoWithNDBCache/v1/jwtencodesignheadererrors/Unknown_signature_algorithm (0.00s)
exported_test.go:69: expected topdown error text "invalid character" but got: "test-0.rego:4: eval_builtin_error: io.jwt.encode_sign_raw: missing or invalid 'alg' header: cannot parse JSON: cannot parse object: missing ':' after object key; unparsed tail: \"JWT\\\",\\r\\n \\\"alg\\\":\\\"HS256\\\"}\""
--- FAIL: TestRegoWithNDBCache/v1/jwtencodesignheadererrors/No_JSON_Error (0.00s)
exported_test.go:69: expected topdown error text "invalid character" but got: "test-0.rego:4: eval_builtin_error: io.jwt.encode_sign_raw: unknown JWS algorithm: "
--- FAIL: TestRegoWithNDBCache/v1/jwtencodesignheadererrors/Empty_headers_input_error (0.00s)
exported_test.go:69: expected topdown error text "unexpected end of JSON input" but got: "test-0.rego:4: eval_builtin_error: io.jwt.encode_sign_raw: missing or invalid 'alg' header: cannot parse JSON: cannot parse empty string; unparsed tail: \"\""
--- FAIL: TestRegoWithNDBCache/v1/jwtencodesignheadererrors/Empty_JSON_header_Error (0.00s)
exported_test.go:69: expected topdown error text "unsupported signature algorithm" but got: "test-0.rego:4: eval_builtin_error: io.jwt.encode_sign_raw: unknown JWS algorithm: "
--- FAIL: TestRegoWithNDBCache/v0/jwtencodesignheadererrors/unknown_signature_algorithm (0.00s)
exported_test.go:60: expected topdown error text "unknown signature algorithm" but got: "test-0.rego:4: eval_builtin_error: io.jwt.encode_sign_raw: unknown JWS algorithm: dummy"
--- FAIL: TestRegoWithNDBCache/v0/jwtencodesignheadererrors/Unknown_signature_algorithm (0.00s)
exported_test.go:60: expected topdown error text "invalid character" but got: "test-0.rego:4: eval_builtin_error: io.jwt.encode_sign_raw: missing or invalid 'alg' header: cannot parse JSON: cannot parse object: missing ':' after object key; unparsed tail: \"JWT\\\",\\r\\n \\\"alg\\\":\\\"HS256\\\"}\""
--- FAIL: TestRegoWithNDBCache/v0/jwtencodesignheadererrors/No_JSON_Error (0.00s)
exported_test.go:60: expected topdown error text "invalid character" but got: "test-0.rego:4: eval_builtin_error: io.jwt.encode_sign_raw: unknown JWS algorithm: "
--- FAIL: TestRegoWithNDBCache/v0/jwtencodesignheadererrors/Empty_headers_input_error (0.00s)
exported_test.go:60: expected topdown error text "unexpected end of JSON input" but got: "test-0.rego:4: eval_builtin_error: io.jwt.encode_sign_raw: missing or invalid 'alg' header: cannot parse JSON: cannot parse empty string; unparsed tail: \"\""
--- FAIL: TestRegoWithNDBCache/v0/jwtencodesignheadererrors/Empty_JSON_header_Error (0.00s)
exported_test.go:60: expected topdown error text "unsupported signature algorithm" but got: "test-0.rego:4: eval_builtin_error: io.jwt.encode_sign_raw: unknown JWS algorithm: "
FAILThese are the files with failing tests:
v0/test/cases/testdata/v1/jwtencodesignheadererrors/test-jwtencodesignheadererrors-0379.yamlv0/test/cases/testdata/v1/jwtencodesignheadererrors/test-jwtencodesignheadererrors-0380.yamlv0/test/cases/testdata/v1/jwtencodesignheadererrors/test-jwtencodesignheadererrors-0381.yamlv0/test/cases/testdata/v1/jwtencodesignheadererrors/test-jwtencodesignheadererrors-0382.yamlv0/test/cases/testdata/v1/jwtencodesignheadererrors/test-jwtencodesignheadererrors-0383.yamlv1/test/cases/testdata/v1/jwtencodesignheadererrors/test-jwtencodesignheadererrors-0379.yamlv1/test/cases/testdata/v1/jwtencodesignheadererrors/test-jwtencodesignheadererrors-0380.yamlv1/test/cases/testdata/v1/jwtencodesignheadererrors/test-jwtencodesignheadererrors-0381.yamlv1/test/cases/testdata/v1/jwtencodesignheadererrors/test-jwtencodesignheadererrors-0382.yamlv1/test/cases/testdata/v1/jwtencodesignheadererrors/test-jwtencodesignheadererrors-0383.yaml
And we should sync with main.
If you give me push access to your branch, I can help you wrap these things.
| // | ||
| // (lestrrat) Whoa, you're going to trust the payload before you | ||
| // verify the signature? Even if it's for backwrds compatibility, | ||
| // Is this OK? |
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.
It's not ideal, but given that the key must still exist outside the JWT, and the signature encompasses both the header and payload, this wouldn't impact the security concerns, right?
Unless I'm completely mistaken, if it wasn't for the key-id and algorithm meta being required as cleartext for encrypted tokens, the header doesn't add cryptographic value; it's more for separating meta about the token from its claims(?).
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.
I agree that it's not an everyday situation, but the attacker would be able to choose which key the data is to be verified with. So if hypothetically a particular key has already been compromised/leaked, it would be very easy for an attacker to forge a JWS with the leaked key, and force you to verify its authenticity with said key. Yes, I agree that all bets are already off when the key is leaked, but this would just be another opening that they could exploit further.
From my standpoint I'd have to say to stick with "don't ever trust the payload until it's properly verified".
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.
the attacker would be able to choose which key the data is to be verified with.
Isn't that also the case for when the key-ID is in the header, given that the token has been created by the attacker wholesale?
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.
I suppose in 99% of the cases what you just described is true; I admit I'm only the implementer and not the designer of the protocol, and thus my knowledge there is weak sauce and I'm willing to concede that I don't entirely know what I'm talking about. But I do, however, as someone who is at least partially responsible for other people's security, believe that I shouldn't just say it's okay to break protocol rules because something bigger is broken -- I believe every small bit counts.
Either way, I don't think it needs to be changed right now, as I'm sure there could be plenty of code out there which already relies on this behavior, so it's just my $.02. When/if the time comes for a major upgrade, I hope this is at least looked at with a fresh set of eyes :)
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.
Thank you for your input on this @lestrrat. I agree with you that it's often better to follow protocol, and not do anything special, as it can open up unforeseen attack vectors 👍. I'm not familiar with the background of this fallback behavior, nor how many users we have out there that rely on it, but we might be able to drop it in the future - or make it an opt-in through configuration.
|
@johanfylling apologies, I did notice activity, but I didn't immediately notice this was addressed to me. You should have received an invitation to my repository |
|
@lestrrat, you shouldn't need to give me full access to your repo, it should be possible to only give push access to the origin branch for this PR. |
johanfylling
left a comment
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.
Thank you for the great work! 😃
Would you mind amending your last commit to add a sign-off, to make the DCO check happy?
Signed-off-by: Daisuke Maki <[email protected]>
|
@johanfylling DCO problem fixed! |
Signed-off-by: Johan Fylling <[email protected]>
Signed-off-by: Johan Fylling <[email protected]>
Conflicts: go.mod go.sum vendor/golang.org/x/sys/cpu/cpu_other_ppc64x.go vendor/golang.org/x/sys/cpu/cpu_other_riscv64.go vendor/golang.org/x/sys/cpu/cpu_other_x86.go vendor/modules.txt
Signed-off-by: Johan Fylling <[email protected]>
johanfylling
left a comment
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.
@lestrrat, I've pushed a couple of commits to your branch to make it ready to merge.
While doing so, I encountered a minor usability issue in the API. Not a blocker, but if it was possible to easily differentiate between when a header attribute is missing and when the header is broken/invalid, it'd great.
v1/bundle/verify.go
Outdated
| // use to determine the key to use for verification. | ||
| hdr := jwsbb.HeaderParseCompact(hdrb64) | ||
| v, err := jwsbb.HeaderGetString(hdr, "kid") | ||
| if err != nil && err.Error() != `jwsbb: header "kid" not found` { |
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.
@lestrrat, here it'd be very useful if the headerNotFoundError error type was public, so we can do a type check instead of a string comparison. Alternatively, a helper function that does the type check, if there are reasons for keeping the type private.
Would it be possible to make this update?
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, my bad. I named the struct headerNotFound, but I made the public accessor to this error FieldNotFound, and ended up not testing against errors.Is. What a f'up. Fix coming...
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.
I tested this locally, but can you check on your side if this fixes it for you:
- Please get jwx @ lestrrat-go/jwx@c14c1d5
- Please apply the following diff to v1/bundle/verify.go
I'd appreciate it if you could verify that it works on your side before I make another release in haste :)
diff --git a/v1/bundle/verify.go b/v1/bundle/verify.go
index 8dfc18a38..2302a2ae3 100644
--- a/v1/bundle/verify.go
+++ b/v1/bundle/verify.go
@@ -130,12 +130,16 @@ func verifyJWTSignature(token string, bvc *VerificationConfig) (*DecodedSignatur
// use to determine the key to use for verification.
hdr := jwsbb.HeaderParseCompact(hdrb64)
v, err := jwsbb.HeaderGetString(hdr, "kid")
- if err != nil && err.Error() != `jwsbb: header "kid" not found` {
+ switch {
+ case err == nil:
+ // err == nils means we found the key ID in the header
+ keyID = v
+ case errors.Is(err, jwsbb.ErrHeaderNotFound()):
+ // no "kid" in the header. no op.
+ default:
+ // some other error occurred while trying to extract the key ID
return nil, fmt.Errorf("failed to extract key ID from headers: %w", err)
}
- // if kid is not present in the header, it will still return the empty
- // string, so it would be the same as if we had not set it at all.
- keyID = v
}
// Because we want to fallback to ds.KeyID when we can't find theThere 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.
This works splendidly on my end 👍
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.
Thanks! release and a commit coming up in a bit...
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.
@johanfylling done 666105c
This release fixes the problem with undetectable HeaderNotFound error Signed-off-by: Daisuke Maki <[email protected]>
|
What a terrific contribution! Thank you, @lestrrat 👏 |
|
Thanks @anderseknert and @johanfylling ! |
Why the changes in this PR are needed?
fixes #7638
What are the changes in this PR?
Replaces the interned github.com/lestrrat-go/jwx (probably v0.9.x!!!) with latest github.com/lestrrat-go/jwx/v3
Notes to assist PR review:
At the time of creation, this PR has these problems, which have been discussed in #7638 already
go mod vendorhas been run locally, but as of this PR submission it has not been included in this PR, as it probably clutters up the diff.Further comments: