Merge #155531 #156018 #156066 #156081

155531: identmap: use case-insensitive matching r=rafiss a=rafiss

All usernames in CockroachDB are normalized to lower-case. Previously, the identity map would not account for this, and would use a case sensitive match to check the identities to be mapped.

Epic: None
Release note (bug fix): The username remapping functionality specified by the server.identity_map.configuration cluster setting now matches identities and usernames with a case-insensitive comparison.


156018: changefeedccl: skip TestChangefeedKafkaMessageTooLarge under duress r=andyyang890 a=arulajmani

Closes #155333

Release note: None

156066: kvcoord: always check for buffered writes when checking for locks r=miraradeva a=stevendanna

When reading through code for an unrelated reason we noted that there was a call checking for acquired locks but not buffered writes. This is concerning since the reasoning for why we need to check for acquired locks almost always implies we should also check for buffered writes.

In the particular case of RollbackToSavepoint, I believe the omission has no impact. Namely:

  - Previous writes have been cleared from the buffer and thus aren't in danger of failing to be rolled back.

  - Subsequent writes will bump the sequence number and thus those writes are not in danger of being erroneously rolled back.

  - Subsequent reads cannot read the data that should have been rolled back because it will have been cleared from the buffer.

Further, for real-world SQL transactions, we never have a buffered write without having at least 1 lock.

However, for the avoidance fo doubt, we now check both and add a new method to avoid missing checks in the future.

Fixes #155975
Release note: None

156081: kvnemesis: bump the timeout r=miraradeva a=arulajmani

Medium -> large.

Closes #155985 Closes #155852

Epic: none

Release note: None

Co-authored-by: Rafi Shamim <[email protected]>
Co-authored-by: Arul Ajmani <[email protected]>
Co-authored-by: Steven Danna <[email protected]>

Author: 4 people

pkg/acceptance/compose/gss/docker-compose.yml

@@ -24,7 +24,7 @@ services:
       timeout: 10s
       retries: 25
   psql:
-    image: us-east1-docker.pkg.dev/crl-ci-images/cockroach/acceptance-gss-psql:20241009-173040
+    image: us-east1-docker.pkg.dev/crl-ci-images/cockroach/acceptance-gss-psql:20251023-150915
     user: "${UID}:${GID}"
     depends_on:
       cockroach:

pkg/acceptance/compose/gss/psql/gss_test.go

@@ -109,12 +109,12 @@ func TestGSS(t *testing.T) {
 			user:     "tester",
 			gssErr:   "",
 		},
-		// Verify case-sensitivity.
+		// Verify names are matched without case-sensitivity.
 		{
 			conf:     `host all all all gss map=demo`,
 			identMap: `demo /^(.*)@MY.EX$ \1`,
 			user:     "tester",
-			gssErr:   `system identity "[email protected]" did not map to a database role`,
+			gssErr:   ``,
 		},
 		// Validating the use of "map" as a filter.
 		{

pkg/ccl/changefeedccl/changefeed_test.go

@@ -11041,6 +11041,8 @@ func TestChangefeedKafkaMessageTooLarge(t *testing.T) {
 	defer leaktest.AfterTest(t)()
 	defer log.Scope(t).Close(t)
 
+	skip.UnderDuress(t, "prone to overload")
+
 	testFn := func(t *testing.T, s TestServer, f cdctest.TestFeedFactory) {
 		if KafkaV2Enabled.Get(&s.Server.ClusterSettings().SV) {
 			// This is already covered for the v2 sink in another test: TestKafkaSinkClientV2_Resize

pkg/kv/kvclient/kvcoord/txn_coord_sender.go

@@ -527,8 +527,7 @@ func (tc *TxnCoordSender) Send(
 		return nil, pErr
 	}
 
-	if ba.IsSingleEndTxnRequest() && !tc.interceptorAlloc.txnPipeliner.hasAcquiredLocks() &&
-		!tc.interceptorAlloc.txnWriteBuffer.hasBufferedWrites() {
+	if ba.IsSingleEndTxnRequest() && !tc.hasAcquiredLocksOrBufferedWritesLocked() {
 		return nil, tc.finalizeNonLockingTxnLocked(ctx, ba)
 	}
 
@@ -1724,3 +1723,7 @@ func (tc *TxnCoordSender) hasPerformedWritesLocked() bool {
 func (tc *TxnCoordSender) hasBufferedWritesLocked() bool {
 	return tc.interceptorAlloc.txnWriteBuffer.hasBufferedWrites()
 }
+
+func (tc *TxnCoordSender) hasAcquiredLocksOrBufferedWritesLocked() bool {
+	return tc.interceptorAlloc.txnPipeliner.hasAcquiredLocks() || tc.interceptorAlloc.txnWriteBuffer.hasBufferedWrites()
+}

pkg/kv/kvclient/kvcoord/txn_coord_sender_savepoints.go

@@ -89,12 +89,12 @@ func (tc *TxnCoordSender) CreateSavepoint(ctx context.Context) (kv.SavepointToke
 	// We also increment the write sequence if any writes are buffered since
 	// a buffered write will not appears as a lock acquisition in the
 	// pipeliner. For SQL-generated transactions, this additional check is
-	// unnecessary since we won't ever have buffered writes wihout also at
+	// unnecessary since we won't ever have buffered writes without also at
 	// least 1 locked row. However, for other types of KV transactions such
 	// as those generated by KVNemesis we may have blind Put or Delete
 	// requests that would be erroneously rolled back if we didn't increment
 	// the sequence here.
-	if tc.interceptorAlloc.txnPipeliner.hasAcquiredLocks() || tc.interceptorAlloc.txnWriteBuffer.hasBufferedWrites() {
+	if tc.hasAcquiredLocksOrBufferedWritesLocked() {
 		if err := tc.interceptorAlloc.txnSeqNumAllocator.stepWriteSeqLocked(ctx); err != nil {
 			return nil, err
 		}
@@ -160,7 +160,7 @@ func (tc *TxnCoordSender) RollbackToSavepoint(ctx context.Context, s kv.Savepoin
 	// TODO(nvanbenschoten): once #113765 is resolved, we should make this
 	// unconditional and push the write sequence increment into
 	// txnSeqNumAllocator.rollbackToSavepointLocked.
-	if tc.interceptorAlloc.txnPipeliner.hasAcquiredLocks() {
+	if tc.hasAcquiredLocksOrBufferedWritesLocked() {
 		tc.mu.txn.AddIgnoredSeqNumRange(
 			enginepb.IgnoredSeqNumRange{
 				Start: sp.seqNum, End: tc.interceptorAlloc.txnSeqNumAllocator.writeSeq,

pkg/kv/kvnemesis/BUILD.bazel

@@ -62,7 +62,7 @@ go_library(
 
 go_test(
     name = "kvnemesis_test",
-    size = "medium",
+    size = "large",
     srcs = [
         "applier_test.go",
         "engine_test.go",

pkg/sql/pgwire/identmap/ident_map.go

@@ -85,9 +85,12 @@ func From(r io.Reader) (*Conf, error) {
 		var sysPattern *regexp.Regexp
 		var err error
 		if sysName := parts[2]; sysName[0] == '/' {
-			sysPattern, err = regexp.Compile(sysName[1:])
+			// Use case-insensitive matching since system identities (e.g., certificate CNs)
+			// are normalized to lowercase before being passed to the user map.
+			sysPattern, err = regexp.Compile("(?i)" + sysName[1:])
 		} else {
-			sysPattern, err = regexp.Compile("^" + regexp.QuoteMeta(sysName) + "$")
+			// Use case-insensitive matching for literal patterns as well.
+			sysPattern, err = regexp.Compile("(?i)^" + regexp.QuoteMeta(sysName) + "$")
 		}
 		if err != nil {
 			return nil, errors.Wrapf(err, "unable to parse line %d", lineNo)
@@ -143,16 +146,19 @@ func (c *Conf) Map(mapName, systemIdentity string) ([]username.SQLUsername, bool
 	var names []username.SQLUsername
 	seen := make(map[string]bool)
 	for _, elt := range elts {
-		if n := elt.substitute(systemIdentity); n != "" && !seen[n] {
+		if n := elt.substitute(systemIdentity); n != "" {
 			// We're returning this as a for-validation username since a
 			// pattern-based mapping could still result in invalid characters
 			// being incorporated into the input.
 			u, err := username.MakeSQLUsernameFromUserInput(n, username.PurposeValidation)
 			if err != nil {
 				return nil, true, err
 			}
-			names = append(names, u)
-			seen[n] = true
+			normalized := u.Normalized()
+			if !seen[normalized] {
+				names = append(names, u)
+				seen[normalized] = true
+			}
 		}
 	}
 	return names, true, nil

pkg/sql/pgwire/identmap/ident_map_test.go

@@ -18,14 +18,14 @@ func TestIdentityMapElement(t *testing.T) {
 	exactMatch := func(sysName, dbname string) element {
 		return element{
 			dbUser:       dbname,
-			pattern:      regexp.MustCompile("^" + regexp.QuoteMeta(sysName) + "$"),
+			pattern:      regexp.MustCompile("(?i)^" + regexp.QuoteMeta(sysName) + "$"),
 			substituteAt: -1,
 		}
 	}
 	regexMatch := func(sysName, dbName string) element {
 		return element{
 			dbUser:       dbName,
-			pattern:      regexp.MustCompile(sysName),
+			pattern:      regexp.MustCompile("(?i)" + sysName),
 			substituteAt: strings.Index(dbName, `\1`),
 		}
 	}
@@ -65,6 +65,33 @@ func TestIdentityMapElement(t *testing.T) {
 			principal: "[email protected]",
 			expected:  "",
 		},
+		// Case-insensitive exact match tests.
+		{
+			elt:       exactMatch("CaRlItO", "carl"),
+			principal: "carlito",
+			expected:  "carl",
+		},
+		{
+			elt:       exactMatch("carlito", "carl"),
+			principal: "CARLITO",
+			expected:  "carl",
+		},
+		{
+			elt:       exactMatch("CaRlItO", "carl"),
+			principal: "CaRlItO",
+			expected:  "carl",
+		},
+		// Case-insensitive regex match tests.
+		{
+			elt:       regexMatch("^(.*)@CockroachLabs.com$", `\1`),
+			principal: "[email protected]",
+			expected:  "carl",
+		},
+		{
+			elt:       regexMatch("^(.*)@cockroachlabs.com$", `\1`),
+			principal: "[email protected]",
+			expected:  "Carl",
+		},
 	}
 
 	for idx, tc := range tcs {
@@ -130,3 +157,101 @@ bar      [email protected]       carl        # Duplicate behavior
 	_, mapFound, _ = m.Map("bar", "something")
 	a.True(mapFound)
 }
+
+func TestIdentityMapCaseInsensitive(t *testing.T) {
+	a := assert.New(t)
+	data := `
+# Test case-insensitive matching for both literal and regex patterns.
+# This is important because certificate CNs are normalized to lowercase
+# before being passed to the user map.
+certmap      AbC123DeF456                  cert_user
+certmap      /^([A-Z0-9]+)@example.com$   user_\1
+emailmap     /^(.*)@COMPANY.COM$           \1
+`
+
+	m, err := From(strings.NewReader(data))
+	if !a.NoError(err) {
+		return
+	}
+
+	// Test literal pattern matching is case-insensitive.
+	elts, _, err := m.Map("certmap", "abc123def456")
+	if a.NoError(err) && a.Len(elts, 1) {
+		a.Equal("cert_user", elts[0].Normalized())
+	}
+
+	// Test uppercase version also matches.
+	elts, _, err = m.Map("certmap", "ABC123DEF456")
+	if a.NoError(err) && a.Len(elts, 1) {
+		a.Equal("cert_user", elts[0].Normalized())
+	}
+
+	// Test mixed case also matches.
+	elts, _, err = m.Map("certmap", "AbC123dEf456")
+	if a.NoError(err) && a.Len(elts, 1) {
+		a.Equal("cert_user", elts[0].Normalized())
+	}
+
+	// Test regex pattern matching is case-insensitive with substitution.
+	elts, _, err = m.Map("certmap", "[email protected]")
+	if a.NoError(err) && a.Len(elts, 1) {
+		a.Equal("user_abc123", elts[0].Normalized())
+	}
+
+	// Test uppercase email also matches. Note that the captured group is lowercased
+	// during SQL username normalization.
+	elts, _, err = m.Map("certmap", "[email protected]")
+	if a.NoError(err) && a.Len(elts, 1) {
+		a.Equal("user_abc123", elts[0].Normalized())
+	}
+
+	// Test pattern with uppercase domain matches lowercase input.
+	elts, _, err = m.Map("emailmap", "[email protected]")
+	if a.NoError(err) && a.Len(elts, 1) {
+		a.Equal("john.doe", elts[0].Normalized())
+	}
+
+	// Test pattern with uppercase domain matches uppercase input.
+	// The captured username is normalized to lowercase.
+	elts, _, err = m.Map("emailmap", "[email protected]")
+	if a.NoError(err) && a.Len(elts, 1) {
+		a.Equal("jane.doe", elts[0].Normalized())
+	}
+}
+
+func TestIdentityMapCaseInsensitiveDeduplication(t *testing.T) {
+	a := assert.New(t)
+	data := `
+# Test that deduplication works correctly with case-insensitive usernames.
+# If multiple rules produce usernames that normalize to the same value,
+# only the first one should be returned.
+testmap      [email protected]        carl
+testmap      /^(.*)@cockroachlabs.com$     \1
+`
+
+	m, err := From(strings.NewReader(data))
+	if !a.NoError(err) {
+		return
+	}
+
+	// When we pass in [email protected] (note the different casing):
+	// - First rule matches and produces: carl
+	// - Second rule matches and produces: Carl (which normalizes to carl)
+	// We should only get one result since they normalize to the same username.
+	elts, _, err := m.Map("testmap", "[email protected]")
+	if a.NoError(err) && a.Len(elts, 1) {
+		a.Equal("carl", elts[0].Normalized())
+	}
+
+	// Also test with lowercase input to verify both rules match but deduplicate.
+	elts, _, err = m.Map("testmap", "[email protected]")
+	if a.NoError(err) && a.Len(elts, 1) {
+		a.Equal("carl", elts[0].Normalized())
+	}
+
+	// Test with a completely uppercase input.
+	elts, _, err = m.Map("testmap", "[email protected]")
+	if a.NoError(err) && a.Len(elts, 1) {
+		a.Equal("carl", elts[0].Normalized())
+	}
+}

pkg/sql/pgwire/testdata/auth/identity_map

@@ -48,13 +48,13 @@ host     all      all  all     cert-password map=testing
 # testing testuser2 carl              # Cert that doesn't correspond to a db user
 # testing [email protected] carl   # Cert with a non-SQL principal baked in
 # Active configuration:
-# map-name system-username         database-username
-testing    ^testuser$              carl
-testing    (.*)@cockroachlabs.com  \1                # substituteAt=0
-testing    ^testuser$              another_carl
-testing    ^will_be_carl$          carl
-testing    ^testuser2$             carl
-testing    ^testuser@example\.com$ carl
+# map-name system-username             database-username
+testing    (?i)^testuser$              carl
+testing    (?i)(.*)@cockroachlabs.com  \1                # substituteAt=0
+testing    (?i)^testuser$              another_carl
+testing    (?i)^will_be_carl$          carl
+testing    (?i)^testuser2$             carl
+testing    (?i)^testuser@example\.com$ carl
 
 sql
 CREATE USER carl WITH PASSWORD 'doggo';
@@ -274,7 +274,7 @@ host     all      all  all     cert-password map=testing
 # testing testuser root               # Exact remapping
 # Active configuration:
 # map-name system-username database-username
-testing    ^testuser$      root
+testing    (?i)^testuser$  root
 
 connect user=testuser database=mydb
 ----
@@ -303,7 +303,7 @@ host     all      all  all     cert-password map=testing
 # testing testuser node               # Exact remapping
 # Active configuration:
 # map-name system-username database-username
-testing    ^testuser$      node
+testing    (?i)^testuser$  node
 
 connect user=testuser database=mydb
 ----