Skip to content
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

jwtauthccl: use RedactableString for detailed error #127016

Merged
merged 1 commit into from
Jul 12, 2024

Conversation

rafiss
Copy link
Collaborator

@rafiss rafiss commented Jul 11, 2024

Also, only join the detailed error string if it's non-empty.

Epic: None
Release note: None

@rafiss rafiss requested review from souravcrl and a team July 11, 2024 18:13
@rafiss rafiss requested review from a team as code owners July 11, 2024 18:13
Copy link

blathers-crl bot commented Jul 11, 2024

It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR?

🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@rafiss rafiss force-pushed the redactable-jwt-error branch 2 times, most recently from d9559ba to 7108d7a Compare July 11, 2024 19:12
Also, only join the detailed error string if it's non-empty.

Release note: None
@rafiss rafiss force-pushed the redactable-jwt-error branch from 7108d7a to 915d33f Compare July 11, 2024 20:36
Copy link
Contributor

@souravcrl souravcrl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 4 of 4 files at r1, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @rafiss)


pkg/ccl/jwtauthccl/authentication_jwt.go line 189 at r1 (raw file):

		jwkSet, err = authenticator.remoteFetchJWKS(ctx, issuerUrl)
		if err != nil {
			return redact.Sprintf("unable to fetch jwks: %v", err),

I found we can't redact this err targetedly. link: https://reviewable.io/reviews/cockroachdb/cockroach/126227#-O1I-UnF7W0DluMm7qoz


pkg/sql/pgwire/auth_methods.go line 786 at r1 (raw file):

		if detailedErrors, authError := jwtVerifier.ValidateJWTLogin(ctx, execCfg.Settings, user, []byte(token), identMap); authError != nil {
			errForLog := authError
			if detailedErrors != "" {

took note of this and updated my implementation https://reviewable.io/reviews/cockroachdb/cockroach/126227

Copy link
Collaborator Author

@rafiss rafiss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for the review!

bors r+

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @souravcrl)


pkg/ccl/jwtauthccl/authentication_jwt.go line 189 at r1 (raw file):

Previously, souravcrl wrote…

I found we can't redact this err targetedly. link: https://reviewable.io/reviews/cockroachdb/cockroach/126227#-O1I-UnF7W0DluMm7qoz

that's correct, we cannot retroactively apply selective redaction to an error after it's constructed. it's up to the creator of the error to do that, or to define a SafeFormatError function for that error type.

so for errors from 3rd party libraries, the entire error text will get redacted in logs.

@craig
Copy link
Contributor

craig bot commented Jul 12, 2024

Build failed (retrying...):

@craig craig bot merged commit 8e25e64 into cockroachdb:master Jul 12, 2024
22 checks passed
@rafiss rafiss deleted the redactable-jwt-error branch July 18, 2024 20:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants