release-23.2: CRDB-28040 : JWKS fetch from jwks_uri #119768
Conversation
|
This change is |
dhartunian
changed the title
|
Thanks for opening a backport. Please check the backport criteria before merging:
If your backport adds new functionality, please ensure that the following additional criteria are satisfied:
Also, please add a brief release justification to the body of your PR to justify this |
| @@ -240,6 +240,15 @@ func jwtRunTest(t *testing.T, insecure bool) { | |||
| t.Fatalf("wrong number of comma separated argumenets to jwt_cluster_setting ident_map: %d", len(a.Vals)) | |||
| } | |||
| pgwire.ConnIdentityMapConf.Override(context.Background(), sv, strings.Join(args, " ")) | |||
| case "jwks_auto_fetch.enabled": | |||
There was a problem hiding this comment.
I don't see this case added to any diffs in testdata. Am I missing something?
There was a problem hiding this comment.
The test cases related to this case has been added in pkg/ccl/jwtauthccl/authentication_jwt_test.go, not in the data driven. I added this case here for completeness and avoid missing definitions for data driven format.
| if authenticator.mu.conf.jwksAutoFetchEnabled { | ||
| jwkSet, err = remoteFetchJWKS(ctx, issuerUrl) | ||
| if err != nil { | ||
| return errors.Newf("JWT authentication: unable to validate token") |
There was a problem hiding this comment.
The original error is lost in these handlers. I know we want to avoid leaking sensitive information, but this will make debugging this feature more challenging. Is there a reason we didn't wrap the err here and elsewhere?
There was a problem hiding this comment.
This is the discussion that lead to removal of logging lines. I agree that in some cases it might make it difficult to debug the exact error, but it is inline with other error handlings in this file. I am open to consider an alternate solution and include if available. https://reviewable.io/reviews/cockroachdb/cockroach/117054#-Nne7jnh5oeb9srWuZw2
There was a problem hiding this comment.
in some other places (e.g. password checking), we use the AuthConn.LogAuthFailed() helper so the detailed failure can be logged internally, and we show a more generic message to the user who failed auth.
cockroach/pkg/sql/pgwire/auth.go
Line 141 in 128b396
i don't think we have access to that logger in this part of the code. refactoring the code is possible, but i'd say can be done separately from this backport
There was a problem hiding this comment.
Reviewable status:
complete! 1 of 0 LGTMs obtained (waiting on @BabuSrithar)
pkg/ccl/jwtauthccl/authentication_jwt.go line 173 at r1 (raw file):
Previously, BabuSrithar (BabuSrithar) wrote…
This is the discussion that lead to removal of logging lines. I agree that in some cases it might make it difficult to debug the exact error, but it is inline with other error handlings in this file. I am open to consider an alternate solution and include if available. https://reviewable.io/reviews/cockroachdb/cockroach/117054#-Nne7jnh5oeb9srWuZw2
Gotcha. Thanks.
pkg/ccl/testccl/authccl/auth_test.go line 243 at r1 (raw file):
Previously, BabuSrithar (BabuSrithar) wrote…
The test cases related to this case has been added in pkg/ccl/jwtauthccl/authentication_jwt_test.go, not in the data driven. I added this case here for completeness and avoid missing definitions for data driven format.
👍
| if authenticator.mu.conf.jwksAutoFetchEnabled { | ||
| jwkSet, err = remoteFetchJWKS(ctx, issuerUrl) | ||
| if err != nil { | ||
| return errors.Newf("JWT authentication: unable to validate token") |
There was a problem hiding this comment.
in some other places (e.g. password checking), we use the AuthConn.LogAuthFailed() helper so the detailed failure can be logged internally, and we show a more generic message to the user who failed auth.
cockroach/pkg/sql/pgwire/auth.go
Line 141 in 128b396
i don't think we have access to that logger in this part of the code. refactoring the code is possible, but i'd say can be done separately from this backport
|
Have built 23.2 locally with this branch and tested following scenarios.
|
This commit adds capability to fetch remote JWKS from issuer's jwks_uri endpoint. This will satisfy the requirement to have an ability to automatically fetch the new JWK when the existing JWK is rotated - without human intervention or custom scripts. Changes include 1. The existing order of token signature verification first and rest of claims next is modified to get issuer first and then the token signature verification. This change is requied to determine the issuer for which the jwks has to be fetched remotely. 2. Introduction of a new cluster setting called `server.jwt_authentication.jwks_auto_fetch.enabled` 3. Depending on the value of `server.jwt_authentication.jwks_auto_fetch.enabled` use JWKS configured through cluster setting or remotely fetch JWKS from jwks_uri of the issuer 4. Modification to exiting test cases to match the new order of verification steps. The change is backward compatible and not changes required in existing deployments and JWT Auth usage. (cherry picked from commit 900408e)
BabuSrithar
force-pushed
the
backport23.2-117054
branch
from
6818291
to
ac6ece7
Compare
|
Thanks for the review @dhartunian and @rafiss. The Extended CI is failing on unlrealted tests and they are different random ones every time I re-run. Can you check if it is ok to merge this change ? |
Backport 1/1 commits from #117054 on behalf of @BabuSrithar.
/cc @cockroachdb/release
Fixes CRDB-28040 ( JWKS fetch from jwks_uri )
This commit adds capability to fetch remote JWKS from issuer's jwks_uri endpoint. This will satisfy the requirement to have an ability to automatically fetch the new JWK when the existing JWK is rotated - without human intervention or custom scripts.
Changes include
The existing order of token signature verification first and rest of claims next is modified to get issuer first and then the token signature verification. This change is requied to determine the issuer for which the jwks has to be fetched remotely.
Introduction of a new cluster setting called
server.jwt_authentication.jwks_auto_fetch.enabledDepending on the value of
server.jwt_authentication.jwks_auto_fetch.enableduse JWKS configured through cluster setting or remotely fetch JWKS from jwks_uri of the issuerModification to exiting test cases to match the new order of verification steps.
The change is backward compatible and not changes required in existing deployments and JWT Auth usage.
(cherry picked from commit 900408e)
Release justification: high-priority need for functionality in 23.2