jwtauthccl: use RedactableString for detailed error

Also, only join the detailed error string if it's non-empty. Release note: None
cockroachdb · Jul 11, 2024 · 915d33f · 915d33f
1 parent 48fd69b
commit 915d33f
Show file tree

Hide file tree

Showing 4 changed files with 14 additions and 7 deletions.
diff --git a/pkg/ccl/jwtauthccl/BUILD.bazel b/pkg/ccl/jwtauthccl/BUILD.bazel
@@ -21,6 +21,7 @@ go_library(
         "//pkg/util/syncutil",
         "//pkg/util/uuid",
         "@com_github_cockroachdb_errors//:errors",
+        "@com_github_cockroachdb_redact//:redact",
         "@com_github_lestrrat_go_jwx//jwk",
         "@com_github_lestrrat_go_jwx//jwt",
     ],
@@ -55,6 +56,7 @@ go_test(
         "//pkg/util/timeutil",
         "@com_github_cockroachdb_errors//:errors",
         "@com_github_cockroachdb_errors//oserror",
+        "@com_github_cockroachdb_redact//:redact",
         "@com_github_lestrrat_go_jwx//jwa",
         "@com_github_lestrrat_go_jwx//jwk",
         "@com_github_lestrrat_go_jwx//jwt",

diff --git a/pkg/ccl/jwtauthccl/authentication_jwt.go b/pkg/ccl/jwtauthccl/authentication_jwt.go
@@ -26,6 +26,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/util/syncutil"
 	"github.com/cockroachdb/cockroach/pkg/util/uuid"
 	"github.com/cockroachdb/errors"
+	"github.com/cockroachdb/redact"
 	"github.com/lestrrat-go/jwx/jwk"
 	"github.com/lestrrat-go/jwx/jwt"
 )
@@ -145,7 +146,7 @@ func (authenticator *jwtAuthenticator) ValidateJWTLogin(
 	user username.SQLUsername,
 	tokenBytes []byte,
 	identMap *identmap.Conf,
-) (detailedErrorMsg string, authError error) {
+) (detailedErrorMsg redact.RedactableString, authError error) {
 	authenticator.mu.Lock()
 	defer authenticator.mu.Unlock()
 
@@ -185,7 +186,7 @@ func (authenticator *jwtAuthenticator) ValidateJWTLogin(
 	if authenticator.mu.conf.jwksAutoFetchEnabled {
 		jwkSet, err = authenticator.remoteFetchJWKS(ctx, issuerUrl)
 		if err != nil {
-			return fmt.Sprintf("unable to fetch jwks: %v", err),
+			return redact.Sprintf("unable to fetch jwks: %v", err),
 				errors.Newf("JWT authentication: unable to validate token")
 		}
 	} else {

diff --git a/pkg/ccl/jwtauthccl/authentication_jwt_test.go b/pkg/ccl/jwtauthccl/authentication_jwt_test.go
@@ -39,6 +39,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/util/timeutil"
 	"github.com/cockroachdb/errors"
 	"github.com/cockroachdb/errors/oserror"
+	"github.com/cockroachdb/redact"
 	"github.com/lestrrat-go/jwx/jwa"
 	"github.com/lestrrat-go/jwx/jwk"
 	"github.com/lestrrat-go/jwx/jwt"
@@ -184,7 +185,7 @@ func TestJWTSingleKey(t *testing.T) {
 	JWKSAutoFetchEnabled.Override(ctx, &s.ClusterSettings().SV, true)
 	detailedErrorMsg, err := verifier.ValidateJWTLogin(ctx, s.ClusterSettings(), username.MakeSQLUsernameFromPreNormalizedString(invalidUsername), token, identMap)
 	require.ErrorContains(t, err, "JWT authentication: unable to validate token")
-	require.EqualValues(t, "unable to fetch jwks: Get \"issuer1/.well-known/openid-configuration\": unsupported protocol scheme \"\"", detailedErrorMsg)
+	require.EqualValues(t, redact.RedactableString(`unable to fetch jwks: ‹Get "issuer1/.well-known/openid-configuration"›: ‹unsupported protocol scheme ""›`), detailedErrorMsg)
 
 	// Set the JWKS cluster setting.
 	JWKSAutoFetchEnabled.Override(ctx, &s.ClusterSettings().SV, false)

diff --git a/pkg/sql/pgwire/auth_methods.go b/pkg/sql/pgwire/auth_methods.go
@@ -706,7 +706,7 @@ type JWTVerifier interface {
 		_ username.SQLUsername,
 		_ []byte,
 		_ *identmap.Conf,
-	) (detailedErrorMsg string, authError error)
+	) (detailedErrorMsg redact.RedactableString, authError error)
 }
 
 var jwtVerifier JWTVerifier
@@ -715,7 +715,7 @@ type noJWTConfigured struct{}
 
 func (c *noJWTConfigured) ValidateJWTLogin(
 	_ context.Context, _ *cluster.Settings, _ username.SQLUsername, _ []byte, _ *identmap.Conf,
-) (detailedErrorMsg string, authError error) {
+) (detailedErrorMsg redact.RedactableString, authError error) {
 	return "", errors.New("JWT token authentication requires CCL features")
 }
 
@@ -782,8 +782,11 @@ func authJwtToken(
 			return security.NewErrPasswordUserAuthFailed(user)
 		}
 		if detailedErrors, authError := jwtVerifier.ValidateJWTLogin(ctx, execCfg.Settings, user, []byte(token), identMap); authError != nil {
-			c.LogAuthFailed(ctx, eventpb.AuthFailReason_CREDENTIALS_INVALID,
-				errors.Join(authError, errors.Newf("%s", detailedErrors)))
+			errForLog := authError
+			if detailedErrors != "" {
+				errForLog = errors.Join(errForLog, errors.Newf("%s", detailedErrors))
+			}
+			c.LogAuthFailed(ctx, eventpb.AuthFailReason_CREDENTIALS_INVALID, errForLog)
 			return authError
 		}
 		return nil