auth: Add SAN-based auth validation for client certificates

[sanchit-CRL]

• Client cert auth only validated the Subject Distinguished Name (DN) and Common Name (CN)
• Modern certificate-based authN uses SANs instead of or in addition to Subject DNs or CN
• Multiple SAN entries in a certificate should be mappable to database users through identity mapping rules
• This PR implements SAN extraction from client certificates in standardised format (SAN:TYPE:value)
• Uses subset validation for root and node users: configured SANs in the start-up flag must all be present in the certificate.
• Implements OR logic between SAN and Subject DN validation when both are enabled as per the PRD and does not fallback to CN in case SAN is enabled.
• For NON-root/node users, SANs are used only for identity mapping, not validation.

Release note: None

Epic: CRDB-58375

[blathers-crl]

Your pull request contains more than 1000 changes. It is strongly encouraged to split big PRs into smaller chunks.

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

[cockroach-teamcity]

This change is 

[souravcrl]

Had a few comments/doubts for the changes. Please take a look. cc: @sanchit-CRL

@souravcrl reviewed 8 files and all commit messages, and made 13 comments.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @sanchit-CRL).

---

-- commits line 12 at r1:
QQ: not sure but should we say full subset validation as we are requiring all configured SANs to be present?

---

-- commits line 20 at r1:
nit: should we add an informs here for the relevant issue even if this does not close the issue?

---

pkg/sql/pgwire/auth_internal_test.go line 92 at r1 (raw file):

		expectedDbUser          string
	}{
		// ====================================================================

nit: not sure but i don't see this format of comments in the code anywhere. While they do enhance readability, they may make the commenting style in the codebase ambiguous. Should we stick to already prescribed or implemented code comment style patterns?

---

pkg/sql/pgwire/auth_internal_test.go line 100 at r1 (raw file):

			systemIdentity: "alice@example.com",
			sanIdentities:  []string{"SAN:DNS:example.com"},
			mockEnhancedMapRoleFunc: func(ctx context.Context, ids []string) ([]identmap.IdentityMapping, error) {

QQ: not sure but why are we mocking the EnhancedMapRole since that is what we are supposed to test for regressions here right with the inputs? Is there any other point in testing the checkClientUsernameMatchesMapping fn?

---

pkg/sql/pgwire/auth_internal_test.go line 294 at r1 (raw file):

			expectedDbUser:        "alice",
		},
	}

QQ: not sure but should we additionally consider some tests for regex first match of the username from the SAN mapped fields, i.e. str.match(re)[0]?

---

pkg/security/auth.go line 329 at r1 (raw file):

// SAN:TYPE:value format (e.g., SAN:URI:spiffe://..., SAN:DNS:example.com)
// This maintains the order of SAN attributes as URI -> IP -> DNS.
func extractSANsFromCertificate(cert *x509.Certificate) []string {

(supernit suggestion): should we define a SAN utility and move this and the subsequent SAN helper code there? I see this logic being used in the auth_methods.go authCert fn as well?

---

pkg/security/auth.go line 334 at r1 (raw file):

	// Extract URI SANs
	for _, uri := range cert.URIs {
		sans = append(sans, fmt.Sprintf("SAN:%s:%s", SANAttributeTypeURI.String(), uri.String()))

QQ: Not sure but any particular reason we are using this format for storing and retrieving SANs instead of a map or some other data container? Will we ever rely on the regex pattern for indexing or retrieval?

---

pkg/sql/pgwire/auth.go line 358 at r1 (raw file):

// from the external authentication system with the database user name
// that the user has requested to connect as."
func (c *conn) checkClientUsernameMatchesMapping(

nit: should we update the fn definition in line with the changed SAN entities mapping validation?

---

pkg/sql/pgwire/auth.go line 395 at r1 (raw file):

		if m == c.sessionArgs.User {
			ac.SetDbUser(m)
			return systemIdentity, nil

nit: Not sure but is the systemIdentity here the fn argument which we are returning here? This is unclear to me as to why if in the arguments itself we are expecting the systemIdentity to be passed, why do we need to return back the same. Again I would like to reiterate if we should consider moving this logic inside behaviors.MapRole which was anyways being invoked here and doing the necessary mapping. It can additionally map the SAN identities.

Although this is not part of PRD but may help with code cleanup.

---

pkg/security/auth_internal_test.go line 16 at r1 (raw file):

)

func TestValidateSANMatchForUser(t *testing.T) {

Not sure but for the tests should we additionally consider:

1. multiple DNS SAN fields in cert and in root/node san map and what happens if there is a order contradiction(i.e. the first is considered)
2. multiple IP SAN fields in cert and in root/node san map

---

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

	})
	if len(tlsState.PeerCertificates) > 0 && hbaEntry.GetOption("map") != "" {
		if clientCertSANRequired {

Not sure but this part of code seems related to https://reviewable.io/reviews/cockroachdb/cockroach/161992#-OlAJEoi0jOs1BQqaHCg. Should we add a utility just for SAN based logic?

---

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

			}
			b.SetSANIdentities(identityList)
		} else {

QQ: Not sure but I thought we will only consider common name if clientCertSAN is not set. Why are we setting it also for the case when this flag is both true or false?

[sanchit-CRL]

@sanchit-CRL made 12 comments.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @souravcrl).

---

-- commits line 12 at r1:

Previously, souravcrl wrote…

QQ: not sure but should we say full subset validation as we are requiring all configured SANs to be present?

Done.

---

-- commits line 20 at r1:

Previously, souravcrl wrote…

nit: should we add an informs here for the relevant issue even if this does not close the issue?

Which issue are you referring to ?

---

pkg/security/auth.go line 329 at r1 (raw file):

Previously, souravcrl wrote…

(supernit suggestion): should we define a SAN utility and move this and the subsequent SAN helper code there? I see this logic being used in the auth_methods.go authCert fn as well?

Exported ExtractSANsFromCertificate and used it in authCert

---

pkg/security/auth.go line 334 at r1 (raw file):

Previously, souravcrl wrote…

QQ: Not sure but any particular reason we are using this format for storing and retrieving SANs instead of a map or some other data container? Will we ever rely on the regex pattern for indexing or retrieval?

This is how it is expected in the PRD as well to provide SAN with it's type explicitly in the identity map, so without any change in the format the regex mapping can be done.
Also later (R3 in PRD) when we implement something similar to subject for each user (currently for SAN only root and node are supported) where we store SAN for user, then this can be directly used without changing any datatype. So yes, we will rely on this data for retrieval later.

---

pkg/sql/pgwire/auth.go line 358 at r1 (raw file):

Previously, souravcrl wrote…

nit: should we update the fn definition in line with the changed SAN entities mapping validation?

I didn't get what you are suggesting here

---

pkg/sql/pgwire/auth.go line 395 at r1 (raw file):

Previously, souravcrl wrote…

nit: Not sure but is the systemIdentity here the fn argument which we are returning here? This is unclear to me as to why if in the arguments itself we are expecting the systemIdentity to be passed, why do we need to return back the same. Again I would like to reiterate if we should consider moving this logic inside behaviors.MapRole which was anyways being invoked here and doing the necessary mapping. It can additionally map the SAN identities.

Although this is not part of PRD but may help with code cleanup.

This is done because the function returns the final matched system identity. Please check it also implements the SANIdentityMatching and that can needs to returns the corresponding system identity. So to make the consumer of the function checkClientUsernameMatchesMapping clean. this has been done.

Map function only uses identity mapper and uses systemIdentity/sanIdentity to return the DB user. It doesn't have the responsibility to match the mapped DB user with the session user. So the code seems clean to me, where each function does only whats required.

---

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

Previously, souravcrl wrote…

Not sure but this part of code seems related to https://reviewable.io/reviews/cockroachdb/cockroach/161992#-OlAJEoi0jOs1BQqaHCg. Should we add a utility just for SAN based logic?

Done.

---

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

Previously, souravcrl wrote…

QQ: Not sure but I thought we will only consider common name if clientCertSAN is not set. Why are we setting it also for the case when this flag is both true or false?

Fixed

---

pkg/security/auth_internal_test.go line 16 at r1 (raw file):

Previously, souravcrl wrote…

Not sure but for the tests should we additionally consider:

1. multiple DNS SAN fields in cert and in root/node san map and what happens if there is a order contradiction(i.e. the first is considered)
2. multiple IP SAN fields in cert and in root/node san map

There is no concept of order in this function implementation, it's just checking that all values must be present in the cert.

---

pkg/sql/pgwire/auth_internal_test.go line 92 at r1 (raw file):

Previously, souravcrl wrote…

nit: not sure but i don't see this format of comments in the code anywhere. While they do enhance readability, they may make the commenting style in the codebase ambiguous. Should we stick to already prescribed or implemented code comment style patterns?

removed

---

pkg/sql/pgwire/auth_internal_test.go line 100 at r1 (raw file):

Previously, souravcrl wrote…

QQ: not sure but why are we mocking the EnhancedMapRole since that is what we are supposed to test for regressions here right with the inputs? Is there any other point in testing the checkClientUsernameMatchesMapping fn?

checkClientUsernameMatchesMapping doesn't just call EnhancedMapRole, it's function is to map based on sanIdentites or without it. That is what the function itself is testing.
Also EnhancedMapRole internally just calls HbaEnhancedMapper, which already has it's own test cases. so there is no need of redundancy in unit tests.

---

pkg/sql/pgwire/auth_internal_test.go line 294 at r1 (raw file):

Previously, souravcrl wrote…

QQ: not sure but should we additionally consider some tests for regex first match of the username from the SAN mapped fields, i.e. str.match(re)[0]?

I am not sure, what is exactly expected here. this just checks for the final matchedIdentity and doesn't check the functionality of HbaEnhancedMapper

[souravcrl]

@souravcrl reviewed 2 files and all commit messages, made 7 comments, and resolved 2 discussions.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @sanchit-CRL).

---

-- commits line 20 at r1:

Previously, sanchit-CRL wrote…

Which issue are you referring to ?

Is there a public issue under which changes for SAN are being made? Typically I needed to file smaller story point issues in gh for the PRD requirement points. Each story point addressed a public facing concern. Each PR leading to resolving a public facing story point issue either had a informs or a fixes. Is there a different approach we are taking here?

---

pkg/security/auth.go line 334 at r1 (raw file):

Previously, sanchit-CRL wrote…

This is how it is expected in the PRD as well to provide SAN with it's type explicitly in the identity map, so without any change in the format the regex mapping can be done.
Also later (R3 in PRD) when we implement something similar to subject for each user (currently for SAN only root and node are supported) where we store SAN for user, then this can be directly used without changing any datatype. So yes, we will rely on this data for retrieval later.

The representation in PRD seems related to client input, not how we store or handle it internally. I was curious if hash map implementation was considered here since it could speed up retrieval.

R3 section of PRD seems to deal with role option for SAN which isn't planned currently, am I right?

---

pkg/sql/pgwire/auth.go line 358 at r1 (raw file):

Previously, sanchit-CRL wrote…

I didn't get what you are suggesting here

What I meant is description of the fn is outdated now which earlier said

// checkClientUsernameMatchesMapping uses the provided RoleMapper to
// verify that the client-provided username matches one of the
// mappings for the system identity.

Should we update this to reflect the new paradigm of username/SAN matching and other changes that were made to the user retrieved matching?

---

pkg/sql/pgwire/auth.go line 395 at r1 (raw file):

Previously, sanchit-CRL wrote…

This is done because the function returns the final matched system identity. Please check it also implements the SANIdentityMatching and that can needs to returns the corresponding system identity. So to make the consumer of the function checkClientUsernameMatchesMapping clean. this has been done.

Map function only uses identity mapper and uses systemIdentity/sanIdentity to return the DB user. It doesn't have the responsibility to match the mapped DB user with the session user. So the code seems clean to me, where each function does only whats required.

It seems to me that previously, the function checkClientUsernameMatchesMapping took only the mapper RoleMapper fn as argument and delegated the logic of actual mapping to the passed RoleMapper. It seems logical to have a overloaded RoleMapper for SAN and pass that here instead. The new SAN based roleMapper can do the actual mapping instead of increasing the scope of checkClientUsernameMatchesMapping.

---

pkg/security/auth_internal_test.go line 16 at r1 (raw file):

Previously, sanchit-CRL wrote…

There is no concept of order in this function implementation, it's just checking that all values must be present in the cert.

I wanted to confirm if we considered the case where an x509 cert has multiple DNS entries or multiple IP entries and we have identity map for each of those? They haven't been covered in the tests here it seems, so maybe a good addition.

---

pkg/sql/pgwire/auth_internal_test.go line 100 at r1 (raw file):

Previously, sanchit-CRL wrote…

checkClientUsernameMatchesMapping doesn't just call EnhancedMapRole, it's function is to map based on sanIdentites or without it. That is what the function itself is testing.
Also EnhancedMapRole internally just calls HbaEnhancedMapper, which already has it's own test cases. so there is no need of redundancy in unit tests.

I had a comment on this behavior of checkClientUsernameMatchesMapping. I can take it up there itself.

---

pkg/sql/pgwire/auth_internal_test.go line 294 at r1 (raw file):

Previously, sanchit-CRL wrote…

I am not sure, what is exactly expected here. this just checks for the final matchedIdentity and doesn't check the functionality of HbaEnhancedMapper

Have we considered the case where regex matched SAN is expected to generate the mapped db username?

[sanchit-CRL]

@sanchit-CRL made 6 comments and resolved 1 discussion.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @souravcrl).

---

-- commits line 20 at r1:

Previously, souravcrl wrote…

Is there a public issue under which changes for SAN are being made? Typically I needed to file smaller story point issues in gh for the PRD requirement points. Each story point addressed a public facing concern. Each PR leading to resolving a public facing story point issue either had a informs or a fixes. Is there a different approach we are taking here?

No, there isn't a public facing issue here, all of it has been managed in JIRA and not gh

---

pkg/security/auth.go line 334 at r1 (raw file):

Previously, souravcrl wrote…

The representation in PRD seems related to client input, not how we store or handle it internally. I was curious if hash map implementation was considered here since it could speed up retrieval.

R3 section of PRD seems to deal with role option for SAN which isn't planned currently, am I right?

Even upto a 100 SAN entries, a list or hash map won't have any performance hit, and I don't see it growing more than that in any practical scenario. Rather list works better in case of fewer elements due to no overhead of hashing.
Yes, it is not planned currently, but has been done keeping that in mind so that regex mapping is simpler without modifying how SAN is stored. It's the same, either manipulate at the time of storing or manipulate at the time of regex match. I chose the previous

---

pkg/sql/pgwire/auth.go line 395 at r1 (raw file):

Previously, souravcrl wrote…

It seems to me that previously, the function checkClientUsernameMatchesMapping took only the mapper RoleMapper fn as argument and delegated the logic of actual mapping to the passed RoleMapper. It seems logical to have a overloaded RoleMapper for SAN and pass that here instead. The new SAN based roleMapper can do the actual mapping instead of increasing the scope of checkClientUsernameMatchesMapping.

It is not increasing the scope of checkClientUsernameMatchesMapping, even now the it is only delegating the mapping to either EnhanceRoleMapper or RoleMapper what it was done before.

cockroach/pkg/sql/pgwire/auth.go

Line 346 in 1cc0b55

func (c *conn) checkClientUsernameMatchesMapping(

The mapper function has no context of the session user and matching the mapped user against it.

The functionality is exactly the same as before, it just additionally makes a choice whether to check SAN mapping or single predefined system identity mapping.

---

pkg/security/auth_internal_test.go line 16 at r1 (raw file):

Previously, souravcrl wrote…

I wanted to confirm if we considered the case where an x509 cert has multiple DNS entries or multiple IP entries and we have identity map for each of those? They haven't been covered in the tests here it seems, so maybe a good addition.

I have added

---

pkg/sql/pgwire/auth_internal_test.go line 100 at r1 (raw file):

Previously, souravcrl wrote…

I had a comment on this behavior of checkClientUsernameMatchesMapping. I can take it up there itself.

Sure

---

pkg/sql/pgwire/auth_internal_test.go line 294 at r1 (raw file):

Previously, souravcrl wrote…

Have we considered the case where regex matched SAN is expected to generate the mapped db username?

Since this test mocks the EnhancedMapRole, that is not tested. It is already tested within tests for HbaEnhancedMapper and I don't think it's necessary for this test

[souravcrl]

@souravcrl reviewed 3 files and all commit messages, made 5 comments, and resolved 1 discussion.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @sanchit-CRL).

---

-- commits line 20 at r1:

Previously, sanchit-CRL wrote…

No, there isn't a public facing issue here, all of it has been managed in JIRA and not gh

Excalate is used to sync issues b/w gh and jira and we are supposed to comment only on gh. I wanted some more clarity if possible.

---

pkg/security/auth.go line 334 at r1 (raw file):

Previously, sanchit-CRL wrote…

Even upto a 100 SAN entries, a list or hash map won't have any performance hit, and I don't see it growing more than that in any practical scenario. Rather list works better in case of fewer elements due to no overhead of hashing.
Yes, it is not planned currently, but has been done keeping that in mind so that regex mapping is simpler without modifying how SAN is stored. It's the same, either manipulate at the time of storing or manipulate at the time of regex match. I chose the previous

DNS and IP entries in x509 certificates used can be proportionately more compared to the size of number of entries in the identity map for CN. A hashmap can help in that regard to quickly process the entries. We can also possibly consider a pre processed regex pattern for matching entries in the certs as we support regex also in the certs.

A secondary concern is the URI fields can have crdb prefixes for internal use not to be used for user role mapping. Should we consider filtering out those entries before doing the mapping?

---

pkg/sql/pgwire/auth.go line 395 at r1 (raw file):

Previously, sanchit-CRL wrote…

It is not increasing the scope of checkClientUsernameMatchesMapping, even now the it is only delegating the mapping to either EnhanceRoleMapper or RoleMapper what it was done before.

cockroach/pkg/sql/pgwire/auth.go

Line 346 in 1cc0b55

func (c *conn) checkClientUsernameMatchesMapping(

The mapper function has no context of the session user and matching the mapped user against it.

The functionality is exactly the same as before, it just additionally makes a choice whether to check SAN mapping or single predefined system identity mapping.

The function now additionally takes in the AuthBehaviors struct as an argument. It verifies the SAN identities length, earlier it just delegated the processing of the system Identity to the MapRole. It also verifies if len(mappings) == 0 or not.

Since we are anyways disabling the generic role mapper in favor of the EnhancedMapRole when the flag for ClientCertSANRequired is set to true, so doing this check again here seems redundant. I think setting the desired map role was already covered here:

cockroach/pkg/sql/pgwire/auth_methods.go

Line 429 in f2ded8c

clientCertSANRequired := security.ClientCertSANRequired.Get(&execCfg.Settings.SV)

---

pkg/security/auth_internal_test.go line 16 at r1 (raw file):

Previously, sanchit-CRL wrote…

I have added

I see that for the DNS test to be true, it needs to set both entries in rootSANsToSet. Is the second entry rootSANsToSet disregarded here(like if rootSANsToSet: "DNS=a.com,URI=b,IP=5.6.7.8,IP=1.2.3.4" and certSANs has no entry for "SAN:DNS:a.com", will the mapping fail or will it succeed? Seems a bit ambiguous if multiple entries are present which is getting mapped.

---

pkg/sql/pgwire/auth_internal_test.go line 294 at r1 (raw file):

Previously, sanchit-CRL wrote…

Since this test mocks the EnhancedMapRole, that is not tested. It is already tested within tests for HbaEnhancedMapper and I don't think it's necessary for this test

I wanted to understand if this is possible to reproduce the test in a integration test, rather than just in a unit test as this can also help improve the comprehensibility of the mapping logic for future work.

[sanchit-CRL]

@sanchit-CRL made 5 comments.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @souravcrl).

---

-- commits line 20 at r1:

Previously, souravcrl wrote…

Excalate is used to sync issues b/w gh and jira and we are supposed to comment only on gh. I wanted some more clarity if possible.

I am aware Exalate is used to sync issues, but this doesn't have a public facinf issue. it's driven via an EPIC which is already listet in the commit.

---

pkg/security/auth.go line 334 at r1 (raw file):

Previously, souravcrl wrote…

DNS and IP entries in x509 certificates used can be proportionately more compared to the size of number of entries in the identity map for CN. A hashmap can help in that regard to quickly process the entries. We can also possibly consider a pre processed regex pattern for matching entries in the certs as we support regex also in the certs.

A secondary concern is the URI fields can have crdb prefixes for internal use not to be used for user role mapping. Should we consider filtering out those entries before doing the mapping?

What order of practical IP and DNS entries are we talking here ? what currently has been implemented easily handle 100s of such entries without over complicating the code. We can always augment all of this if there is a requirement in future, but in the current scope I don't think it is required as it is not adding any additional value.

I don't think URI fields will have crdb prefix for SQL auth, even if they are we cannot say with surity if it is intentionally placed by the customer, nor we do not prohibit the user from creating a cert having crdb prefix. So removing it is not adding any value

---

pkg/sql/pgwire/auth.go line 395 at r1 (raw file):

Previously, souravcrl wrote…

The function now additionally takes in the AuthBehaviors struct as an argument. It verifies the SAN identities length, earlier it just delegated the processing of the system Identity to the MapRole. It also verifies if len(mappings) == 0 or not.

Since we are anyways disabling the generic role mapper in favor of the EnhancedMapRole when the flag for ClientCertSANRequired is set to true, so doing this check again here seems redundant. I think setting the desired map role was already covered here:

cockroach/pkg/sql/pgwire/auth_methods.go

Line 429 in f2ded8c

clientCertSANRequired := security.ClientCertSANRequired.Get(&execCfg.Settings.SV)

I am not sure which line of code are you talking about in your comment
"so doing this check again here seems redundant." can you clarify what is redundant ?

---

pkg/security/auth_internal_test.go line 16 at r1 (raw file):

Previously, souravcrl wrote…

I see that for the DNS test to be true, it needs to set both entries in rootSANsToSet. Is the second entry rootSANsToSet disregarded here(like if rootSANsToSet: "DNS=a.com,URI=b,IP=5.6.7.8,IP=1.2.3.4" and certSANs has no entry for "SAN:DNS:a.com", will the mapping fail or will it succeed? Seems a bit ambiguous if multiple entries are present which is getting mapped.

This is not a mapping function, this function just validates if all the already configured SAN for node or root are present in the cert.
mapping was a different function, this function is not testing it.

---

pkg/sql/pgwire/auth_internal_test.go line 294 at r1 (raw file):

Previously, souravcrl wrote…

I wanted to understand if this is possible to reproduce the test in a integration test, rather than just in a unit test as this can also help improve the comprehensibility of the mapping logic for future work.

I can write an integration test in a separate PR, but I won't be able do it in the same as the length of PR is almost 1000 lines and the guidelines suggest to not have it more than that.

[souravcrl]

@souravcrl made 4 comments.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @sanchit-CRL).

---

-- commits line 20 at r1:

Previously, sanchit-CRL wrote…

I am aware Exalate is used to sync issues, but this doesn't have a public facinf issue. it's driven via an EPIC which is already listet in the commit.

I am not understanding this. Should we not have public facing issues in future commits, and only jira epics for reference in the PR?

---

pkg/security/auth.go line 334 at r1 (raw file):

Previously, sanchit-CRL wrote…

What order of practical IP and DNS entries are we talking here ? what currently has been implemented easily handle 100s of such entries without over complicating the code. We can always augment all of this if there is a requirement in future, but in the current scope I don't think it is required as it is not adding any additional value.

I don't think URI fields will have crdb prefix for SQL auth, even if they are we cannot say with surity if it is intentionally placed by the customer, nor we do not prohibit the user from creating a cert having crdb prefix. So removing it is not adding any value

Are tenant certificates URI fields prefixed with crdb and should these certificates be allowed for sql auth? Also on the cc clusters do we have these certificates used and how are we limiting them?

---

pkg/sql/pgwire/auth.go line 395 at r1 (raw file):

Previously, sanchit-CRL wrote…

I am not sure which line of code are you talking about in your comment
"so doing this check again here seems redundant." can you clarify what is redundant ?

I am not understanding actually why we need a check of length of SAN identities at all. Since we had set the role mapper previously in the authCert fn, why can't we directly use it here instead of overriding with behaviors.MapRole incase if len(sanIdentities) > 0. If this is what we wanted in the first place, can you clarify the behavior going forward then?

---

pkg/security/auth_internal_test.go line 16 at r1 (raw file):

Previously, sanchit-CRL wrote…

This is not a mapping function, this function just validates if all the already configured SAN for node or root are present in the cert.
mapping was a different function, this function is not testing it.

I seem to get confused. For the case rootSANsToSet: "DNS=a.com,URI=b,IP=5.6.7.8,IP=1.2.3.4" and certSANs has no entry for "SAN:DNS:a.com", will the mapping fail or will it succeed. I just wanted to understand that.

[sanchit-CRL]

@sanchit-CRL made 4 comments.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on souravcrl).

---

-- commits line 20 at r1:

Previously, souravcrl wrote…

I am not understanding this. Should we not have public facing issues in future commits, and only jira epics for reference in the PR?

I will take this offline and see if I need to do this for future commits

---

pkg/security/auth.go line 334 at r1 (raw file):

Previously, souravcrl wrote…

Are tenant certificates URI fields prefixed with crdb and should these certificates be allowed for sql auth? Also on the cc clusters do we have these certificates used and how are we limiting them?

We are not limiting them currently as it is not expected for customers to know an internal pattern, if this comes as a requirement later I can look into it then.

---

pkg/sql/pgwire/auth.go line 395 at r1 (raw file):

Previously, souravcrl wrote…

I am not understanding actually why we need a check of length of SAN identities at all. Since we had set the role mapper previously in the authCert fn, why can't we directly use it here instead of overriding with behaviors.MapRole incase if len(sanIdentities) > 0. If this is what we wanted in the first place, can you clarify the behavior going forward then?

Okay I think I get what you are asking, EnhancedRoleMapper is set in case the clustersetting to enable SAN is applied. In context to this function we do not have access to the cluster setting so I used the length of SAN.
I have updated the condition based on the EnhancedRoleMapper set, instead of length of SAN and updated it in the comments as well

---

pkg/security/auth_internal_test.go line 16 at r1 (raw file):

Previously, souravcrl wrote…

I seem to get confused. For the case rootSANsToSet: "DNS=a.com,URI=b,IP=5.6.7.8,IP=1.2.3.4" and certSANs has no entry for "SAN:DNS:a.com", will the mapping fail or will it succeed. I just wanted to understand that.

For a root and node, we do not use mapping. So there is no case of "mapping failing or succeeding". However if rootSAN is set as "DNS=a.com,URI=b,IP=5.6.7.8,IP=1.2.3.4" in the start-up flag and there is no entry for "SAN:DNS:a.com" in the certificate. Authentication for root will fail

[github-actions]

Potential Bug(s) Detected

The three-stage Claude Code analysis has identified potential bug(s) in this PR that may warrant investigation.

Next Steps:
Please review the detailed findings in the workflow run.

Note: When viewing the workflow output, scroll to the bottom to find the Final Analysis Summary.

After you review the findings, please tag the issue as follows:

• If the detected issue is real or was helpful in any way, please tag the issue with O-AI-Review-Real-Issue-Found
• If the detected issue was not helpful in any way, please tag the issue with O-AI-Review-Not-Helpful

[souravcrl]

@souravcrl reviewed 2 files and all commit messages, made 3 comments, and resolved 1 discussion.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on sanchit-CRL).

---

pkg/security/auth.go line 334 at r1 (raw file):

Previously, sanchit-CRL wrote…

We are not limiting them currently as it is not expected for customers to know an internal pattern, if this comes as a requirement later I can look into it then.

Noted.

---

pkg/security/auth_internal_test.go line 16 at r1 (raw file):

Previously, sanchit-CRL wrote…

For a root and node, we do not use mapping. So there is no case of "mapping failing or succeeding". However if rootSAN is set as "DNS=a.com,URI=b,IP=5.6.7.8,IP=1.2.3.4" in the start-up flag and there is no entry for "SAN:DNS:a.com" in the certificate. Authentication for root will fail

Should we actually fail here since anyone having multiple SAN mappings for DNS or IP will expect only 1 of them to match, not all of them? Is this expectation wrong that for DNS or IP mappings only 1 match is sufficient?

---

pkg/sql/pgwire/auth.go line 376 at r3 (raw file):

		}
		if len(mappings) == 0 {
			return "", errors.Newf("system identities %q did not map to any database role", sanIdentities)

SAN system

Code quote:

system

[sanchit-CRL]

@sanchit-CRL made 2 comments.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on souravcrl).

---

pkg/sql/pgwire/auth.go line 376 at r3 (raw file):

Previously, souravcrl wrote…

SAN system

system identities for a customer are external identities provided for auth. This seems correct to me.

---

pkg/security/auth_internal_test.go line 16 at r1 (raw file):

Previously, souravcrl wrote…

Should we actually fail here since anyone having multiple SAN mappings for DNS or IP will expect only 1 of them to match, not all of them? Is this expectation wrong that for DNS or IP mappings only 1 match is sufficient?

The customer has full control of what they want to validate. If they want to validate only one they can set that in the root start-up flag for SAN.
Any such product related discussion can be taken in the PRD, if the requirement change I will make changes in the code.

[souravcrl]

@souravcrl made 2 comments.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on sanchit-CRL).

---

pkg/sql/pgwire/auth.go line 376 at r3 (raw file):

Previously, sanchit-CRL wrote…

system identities for a customer are external identities provided for auth. This seems correct to me.

Should we say SAN system identities as this will make debugging using the error message easier.

---

pkg/security/auth_internal_test.go line 16 at r1 (raw file):

Previously, sanchit-CRL wrote…

The customer has full control of what they want to validate. If they want to validate only one they can set that in the root start-up flag for SAN.
Any such product related discussion can be taken in the PRD, if the requirement change I will make changes in the code.

Noted.

[sanchit-CRL]

@sanchit-CRL made 1 comment.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on souravcrl).

---

pkg/sql/pgwire/auth.go line 376 at r3 (raw file):

Previously, souravcrl wrote…

Should we say SAN system identities as this will make debugging using the error message easier.

The error message already has list of SAN identities, which makes it obvious in debugging

[souravcrl]

@souravcrl made 1 comment.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on sanchit-CRL).

---

pkg/sql/pgwire/auth.go line 376 at r3 (raw file):

Previously, sanchit-CRL wrote…

The error message already has list of SAN identities, which makes it obvious in debugging

I see for the non SAN system identity case, the error message is as follows:

errors.Newf("system identity %q did not map to a database role", systemIdentity)

How do we disambiguate between the 2 error messages if the number of SAN identities is also just 1? They seem to give out similar output and difficult to ascertain whether the error was due to a SAN misconfiguration or not.

[sanchit-CRL]

@sanchit-CRL made 1 comment.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on souravcrl).

---

pkg/sql/pgwire/auth.go line 376 at r3 (raw file):

Previously, souravcrl wrote…

I see for the non SAN system identity case, the error message is as follows:

errors.Newf("system identity %q did not map to a database role", systemIdentity)

How do we disambiguate between the 2 error messages if the number of SAN identities is also just 1? They seem to give out similar output and difficult to ascertain whether the error was due to a SAN misconfiguration or not.

because SAN identities have a prefix SAN:
also there is a difference in the error message, system identities and system identity. these 2 are sufficient to distinguish.

[souravcrl]

Have only the comment concerning gh issue mapping for the PR.

@souravcrl reviewed 1 file and made 2 comments.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on sanchit-CRL).

---

pkg/sql/pgwire/auth.go line 376 at r3 (raw file):

Previously, sanchit-CRL wrote…

because SAN identities have a prefix SAN:
also there is a difference in the error message, system identities and system identity. these 2 are sufficient to distinguish.

Makes sense.

[souravcrl]

@souravcrl made 1 comment.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on sanchit-CRL).

---

pkg/sql/pgwire/auth.go line 376 at r3 (raw file):

Previously, souravcrl wrote…

Makes sense.

Sorry missed this earlier, what happens when the the number of SAN identities is 0. In that case we won't have the prefix and the error messages would look identical. This is a case when SAN maybe provided but not in the fields (URI, DNS, IP).

[sanchit-CRL]

I have already responded to your comment on the github issue, if needed I can do it for future PRs. I don't think it as a blocker since you already approved PRs which didn't have it. So I don't see it as a reason to block this any further.

@sanchit-CRL made 2 comments.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on souravcrl).

---

pkg/sql/pgwire/auth.go line 376 at r3 (raw file):

Previously, souravcrl wrote…

Sorry missed this earlier, what happens when the the number of SAN identities is 0. In that case we won't have the prefix and the error messages would look identical. This is a case when SAN maybe provided but not in the fields (URI, DNS, IP).

SAN entries cannot be 0, if the code is reaching this statement. You have already reviewed that code in the previous PR, please refer that.
And why do you say it would look identical, for any error we copy the statement and paste it in datadog/debugzip logs to search for the error. And it will differ by systems "identitIES" from system "identitY" which i told in the previous statement itself.
Also what do you mean by SAN is provided and not in fields (URI, DNS,IP). This also has been reviewed in previous PRs on how the SAN is extracted from the certificate.
Also I don't think this is something big enough to stall the progress of the PR.

[souravcrl]

Sorry for repeating myself, I just needed to know if this is the template we should follow. If this merits some discussion or if something is not clear, feel free to call it out. I have not blocked the PR since my comments are non blocking. I am just discussing the scope of the changes.

@souravcrl made 2 comments.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on sanchit-CRL).

---

pkg/sql/pgwire/auth.go line 376 at r3 (raw file):

And it will differ by systems "identitIES" from system "identitY" which i told in the previous statement itself.

I think this merits updating the error message.

Also I don't think this is something big enough to stall the progress of the PR.

My comments are non blocking. If you feel I have overstepped in my comments, you can ignore the comment and ask for a blanket approval. Sorry if you felt this way, it was not my intention.

[souravcrl]

As per offline discussion, I am approving the changes. Please take a look into my comment for adding SAN tags to the error messages and for adding the integration test. Rest LGTM.

@souravcrl made 1 comment.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on sanchit-CRL).

[sanchit-CRL]

@sanchit-CRL made 2 comments.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on souravcrl).

---

pkg/sql/pgwire/auth.go line 376 at r3 (raw file):

Previously, souravcrl wrote…

And it will differ by systems "identitIES" from system "identitY" which i told in the previous statement itself.

I think this merits updating the error message.

Also I don't think this is something big enough to stall the progress of the PR.

My comments are non blocking. If you feel I have overstepped in my comments, you can ignore the comment and ask for a blanket approval. Sorry if you felt this way, it was not my intention.

Added SAN prefex

---

pkg/sql/pgwire/auth_internal_test.go line 294 at r1 (raw file):

Previously, sanchit-CRL wrote…

I can write an integration test in a separate PR, but I won't be able do it in the same as the length of PR is almost 1000 lines and the guidelines suggest to not have it more than that.

#164138 created an issue

[sanchit-CRL]

/trunk merge

[trunk-io]

😎 Merged successfully - details.