pkg/security: relax requirement to follow CRDB URI SAN cert scheme

[abarganier]

Recently, a customer upgraded to v22.1.6, a recent patch release which
contains the new tenant-scoped client certificates and asssociated
authorization logic updates.

The new authz logic required that the SAN URIs included in the client
certificate followed the URI SAN scheme:
crdb://tenant/<tenant_id>/user/<tenant_username>

However, for customers that use URI SANs that do not follow this
convention or do not have the flexibility to alter the URI SAN,
this was preventing them from using their existing certificates.
This would generate an error when attempting to connect to a
SQL shell.

One example URI SAN is as follows:
mycompany:sv:rootclient:dev:usw1

This is a certificate that worked with the legacy behavior, but
is rejected by the new authz logic.

We should update the authz logic to be less strict about the URI SAN
following our own scheme. If we are unable to parse the URI SAN then
we should fallback to using the globally scoped client certificate
instead, enabling backwards compatibility.

This patch does just that, logging an error in the case where we are
unable to parse the URI SAN and instead falling back to the legacy
behavior, producing a global user scope for the certificate.

Release note: none

Release justification: low risk, necessary fix to enable customers
using custom URI SAN schemes to continue using their existing certificates
on newer CRDB versions.

Addresses #87235

[cockroach-teamcity]

This change is 

[knz]

The behavior without this PR is indeed a problem (#87235), but the change in this PR does not really improve the situation.

The better approach would be to understand that the SAN field can contain multiple entries. It's possible for a certificate to contain both a mycompany: URI and also a crdb: URI.

The logic in crdb should detect (and parse) crdb: URIs and ignore the non-crdb: URIs.

If it parses a crdb: URI, and an error gets encountered (e.g. because the crdb: URI has invalid syntax.) that error should not be ignored.

Reviewed 1 of 4 files at r1.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @abarganier, @catj-cockroach, @jeffswenson, @jonstjohn, and @rimadeodhar)

---

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

	peerCert := tlsState.PeerCertificates[0]
	for _, uri := range peerCert.URIs {
		tenantID, user, err := ParseTenantURISAN(uri.String())

Only call this function if the URI scheme is crdb.
(If the scheme is not crdb, ignore the URI and try the next URI instead).

---

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

		tenantID, user, err := ParseTenantURISAN(uri.String())
		if err != nil {
			log.Errorf(ctx, "unable to parse tenant URI SAN, defaulting to global certificate: %v", err)

Don't ignore the error if we decided to parse the URI.

---

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

		userScopes = append(userScopes, scope)
	}
	if len(userScopes) == 0 || len(userScopes) != len(peerCert.URIs) {

I don't understand this length comparison. What is it trying to check?

[kpatron-cockroachlabs]

I agree with @knz that we should not immediately fall back to global certs if any one SAN field doesn't match our format, given the likelihood that customer certs will include extra non-CRDB related SANs (or crdb: SANs for other clusters, etc).

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @abarganier, @catj-cockroach, @jeffswenson, @jonstjohn, and @rimadeodhar)

[abarganier]

Thanks for the feedback all. I agree with the proposed changes, I'll get a patch up tomorrow.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @abarganier, @catj-cockroach, @jeffswenson, @jonstjohn, and @rimadeodhar)

[abarganier]

I've updated the authz logic as requested. We now look through all the URI SANs included in the certificate for one with the expected crdb:// prefix. If we find a prefix match, we attempt to parse the URI SAN as before, in which case we take a strict approach where we throw an error if the format isn't the expected crdb://tenant/<tenant_id>/user/<tenant_username>.

In the case where none of the provided URI SANs have the expected crdb:// prefix, only then do we fallback to the global cert.

I've added some tests to cover these scenarios. Thanks again for everyone's input/help.

PTAL.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @catj-cockroach, @jeffswenson, @jonstjohn, @knz, and @rimadeodhar)

---

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

Previously, knz (Raphael 'kena' Poss) wrote…

Only call this function if the URI scheme is crdb.
(If the scheme is not crdb, ignore the URI and try the next URI instead).

Done.

---

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

Previously, knz (Raphael 'kena' Poss) wrote…

Don't ignore the error if we decided to parse the URI.

Done. Added some tests around these scenarios.

---

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

Previously, knz (Raphael 'kena' Poss) wrote…

I don't understand this length comparison. What is it trying to check?

This was a misunderstanding on my part - removed. As before, we should only fall back to the global cert if none of the URIs were of the expected CRDB format.

[knz]

nice

Reviewed 5 of 5 files at r2, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @abarganier, @catj-cockroach, @jeffswenson, @jonstjohn, @kpatron-cockroachlabs, and @rimadeodhar)

---

pkg/security/auth.go line 117 at r2 (raw file):

	peerCert := tlsState.PeerCertificates[0]
	for _, uri := range peerCert.URIs {
		if URISANHasCRDBPrefix(uri.String()) {

best store uri.String() in an intermediate var.

[abarganier]

TFTR!

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @catj-cockroach, @jeffswenson, @jonstjohn, @knz, @kpatron-cockroachlabs, and @rimadeodhar)

---

pkg/security/auth.go line 117 at r2 (raw file):

Previously, knz (Raphael 'kena' Poss) wrote…

best store uri.String() in an intermediate var.

Done.

[abarganier]

bors r=knz

[craig]

Build failed (retrying...):

• Bazel Essential CI (Cockroach)

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)