Merge #153798

153798: sql/catalog: improve redaction of privilege validation errors r=spilchen a=spilchen

We had a sentry error come in for a privilege validation error. The error looked like this:
```
privilege.go:397: relation × (52): ×
```

This change improves things so not everything is redacted. It now looks something like this:
```
privilege.go:397: relation × (52): user root must have exactly [ALL] privileges on table ‹×›
```

Closes #152912
Epic: none
Release note: none

Co-authored-by: Matt Spilchen <[email protected]>

Author: craig[bot]

pkg/sql/catalog/catpb/BUILD.bazel

@@ -49,10 +49,13 @@ go_library(
     deps = [
         "//pkg/keys",
         "//pkg/security/username",
+        "//pkg/sql/pgwire/pgcode",
+        "//pkg/sql/pgwire/pgerror",
         "//pkg/sql/privilege",
         "//pkg/sql/sem/catconstants",
         "//pkg/sql/sem/catid",
         "@com_github_cockroachdb_errors//:errors",
+        "@com_github_cockroachdb_redact//:redact",
     ],
 )
 
@@ -68,6 +71,8 @@ go_test(
         "//pkg/sql/sem/catid",
         "//pkg/testutils",
         "//pkg/util/leaktest",
+        "@com_github_cockroachdb_redact//:redact",
+        "@com_github_stretchr_testify//require",
     ],
 )
 

pkg/sql/catalog/catpb/privilege.go

@@ -6,15 +6,17 @@
 package catpb
 
 import (
-	"fmt"
 	"sort"
 
 	"github.com/cockroachdb/cockroach/pkg/keys"
 	"github.com/cockroachdb/cockroach/pkg/security/username"
+	"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode"
+	"github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror"
 	"github.com/cockroachdb/cockroach/pkg/sql/privilege"
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/catconstants"
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/catid"
 	"github.com/cockroachdb/errors"
+	"github.com/cockroachdb/redact"
 )
 
 // PrivilegeDescVersion is a custom type for PrivilegeDescriptor versions.
@@ -364,21 +366,27 @@ func (p PrivilegeDescriptor) ValidateSuperuserPrivileges(
 		// We expect an "admin" role. Check that it has desired superuser permissions.
 		username.AdminRoleName(),
 	} {
+		// In case we hit an error, we include the user name in the redacted message.
+		// It's safe to include this since it's hardcoded system user.
+		redactSafeUser := redact.SafeString(user.Normalized())
+
 		superPriv, ok := p.FindUser(user)
 		if !ok {
-			return fmt.Errorf(
+			return pgerror.Newf(
+				pgcode.InsufficientPrivilege,
 				"user %s does not have privileges over %s",
-				user,
+				redactSafeUser,
 				privilegeObject(parentID, objectType, objectName),
 			)
 		}
 
 		// The super users must match the allowed privilege set exactly.
 		if superPriv.Privileges != allowedSuperuserPrivileges.ToBitField() {
-			return fmt.Errorf(
-				"user %s must have exactly %s privileges on %s",
-				user,
-				allowedSuperuserPrivileges.SortedDisplayNames(),
+			return pgerror.Newf(
+				pgcode.InsufficientPrivilege,
+				"user %s must have exactly [%v] privileges on %s",
+				redactSafeUser,
+				allowedSuperuserPrivileges,
 				privilegeObject(parentID, objectType, objectName),
 			)
 		}
@@ -574,10 +582,10 @@ func (p *PrivilegeDescriptor) SetVersion(version PrivilegeDescVersion) {
 // privilegeObject is a helper function for privilege errors.
 func privilegeObject(
 	parentID catid.DescID, objectType privilege.ObjectType, objectName string,
-) string {
+) redact.RedactableString {
 	if parentID == keys.SystemDatabaseID ||
 		(parentID == catid.InvalidDescID && objectName == catconstants.SystemDatabaseName) {
-		return fmt.Sprintf("system %s %q", objectType, objectName)
+		return redact.Sprintf("system %s %q", objectType, objectName)
 	}
-	return fmt.Sprintf("%s %q", objectType, objectName)
+	return redact.Sprintf("%s %q", objectType, objectName)
 }

pkg/sql/catalog/catpb/privilege_test.go

@@ -17,6 +17,8 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/catid"
 	"github.com/cockroachdb/cockroach/pkg/testutils"
 	"github.com/cockroachdb/cockroach/pkg/util/leaktest"
+	"github.com/cockroachdb/redact"
+	"github.com/stretchr/testify/require"
 )
 
 func TestPrivilege(t *testing.T) {
@@ -704,3 +706,28 @@ func TestRevokeWithGrantOption(t *testing.T) {
 		}
 	}
 }
+
+// TestPrivilegeValidationErrorRedaction tests that privilege validation errors
+// are properly redacted.
+func TestPrivilegeValidationErrorRedaction(t *testing.T) {
+	defer leaktest.AfterTest(t)()
+
+	// Create a descriptor where root/admin don't have the required privileges
+	// This will trigger ValidateSuperuserPrivileges error
+	descriptor := catpb.NewCustomSuperuserPrivilegeDescriptor(
+		privilege.List{privilege.UPDATE}, // Wrong privilege (not ALL)
+		username.AdminRoleName(),
+	)
+
+	id := catid.DescID(bootstrap.TestingMinUserDescID())
+	err := descriptor.Validate(id, privilege.Table, "sensitive_table_name", catpb.DefaultSuperuserPrivileges)
+	require.Error(t, err)
+
+	nonRedactedMsg := redact.Sprint(err).StripMarkers()
+	expectedNonRedacted := `user root must have exactly [ALL] privileges on table "sensitive_table_name"`
+	require.Equal(t, expectedNonRedacted, nonRedactedMsg, "non-redacted message mismatch")
+
+	redactedMsg := redact.Sprint(err).Redact()
+	expectedRedacted := `user root must have exactly [ALL] privileges on table ‹×›`
+	require.Equal(t, expectedRedacted, string(redactedMsg), "redacted message mismatch")
+}