Skip to content

Commit

Permalink
pkg/security: relax requirement to follow CRDB URI SAN scheme
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@abarganier
abarganier committed Sep 1, 2022
1 parent 9911916 commit 815e6e0
Show file tree
Hide file tree
Showing 4 changed files with 86 additions and 57 deletions.
2 changes: 1 addition & 1 deletion pkg/rpc/auth.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -156,7 +156,7 @@ func (a kvAuth) authenticate(ctx context.Context) (roachpb.TenantID, error) {
// NodeUser. This is not a security concern, as RootUser has access to
// read and write all data, merely good hygiene. For example, there is
// no reason to permit the root user to send raw Raft RPCs.
certUserScope, err := security.GetCertificateUserScope(&tlsInfo.State)
certUserScope, err := security.GetCertificateUserScope(ctx, &tlsInfo.State)
if err != nil {
return roachpb.TenantID{}, err
}
Expand Down
16 changes: 11 additions & 5 deletions pkg/security/auth.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import (
"github.com/cockroachdb/cockroach/pkg/roachpb"
"github.com/cockroachdb/cockroach/pkg/security/password"
"github.com/cockroachdb/cockroach/pkg/security/username"
"github.com/cockroachdb/cockroach/pkg/util/log"
"github.com/cockroachdb/cockroach/pkg/util/syncutil"
"github.com/cockroachdb/errors"
)
Expand Down Expand Up @@ -101,7 +102,7 @@ func getCertificatePrincipals(cert *x509.Certificate) []string {

// GetCertificateUserScope extracts the certificate scopes from a client certificate.
func GetCertificateUserScope(
tlsState *tls.ConnectionState,
ctx context.Context, tlsState *tls.ConnectionState,
) (userScopes []CertificateUserScope, _ error) {
if tlsState == nil {
return nil, errors.Errorf("request is not using TLS")
Expand All @@ -116,15 +117,20 @@ func GetCertificateUserScope(
for _, uri := range peerCert.URIs {
tenantID, user, err := ParseTenantURISAN(uri.String())
if err != nil {
return nil, err
log.Errorf(ctx, "unable to parse tenant URI SAN, defaulting to global certificate: %v", err)
// We want to fallback to using the global scope
// in the case where our URI SAN scheme isn't followed
// directly. Continue to handle once we finish looping.
// See: https://github.com/cockroachdb/cockroach/issues/87235
continue
}
scope := CertificateUserScope{
Username: user,
TenantID: tenantID,
}
userScopes = append(userScopes, scope)
}
if len(userScopes) == 0 {
if len(userScopes) == 0 || len(userScopes) != len(peerCert.URIs) {
users := getCertificatePrincipals(peerCert)
for _, user := range users {
scope := CertificateUserScope{
Expand All @@ -150,12 +156,12 @@ func Contains(sl []string, s string) bool {
// UserAuthCertHook builds an authentication hook based on the security
// mode and client certificate.
func UserAuthCertHook(
insecureMode bool, tlsState *tls.ConnectionState, tenantID roachpb.TenantID,
ctx context.Context, insecureMode bool, tlsState *tls.ConnectionState, tenantID roachpb.TenantID,
) (UserAuthHook, error) {
var certUserScope []CertificateUserScope
if !insecureMode {
var err error
certUserScope, err = GetCertificateUserScope(tlsState)
certUserScope, err = GetCertificateUserScope(ctx, tlsState)
if err != nil {
return nil, err
}
Expand Down
121 changes: 72 additions & 49 deletions pkg/security/auth_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -85,58 +85,80 @@ func makeFakeTLSState(t *testing.T, spec string) *tls.ConnectionState {

func TestGetCertificateUserScope(t *testing.T) {
defer leaktest.AfterTest(t)()
// Nil TLS state.
if _, err := security.GetCertificateUserScope(nil); err == nil {
t.Error("unexpected success")
}
ctx := context.Background()
t.Run("nil TLS state", func(t *testing.T) {
if _, err := security.GetCertificateUserScope(ctx, nil); err == nil {
t.Error("unexpected success")
}
})

// No certificates.
if _, err := security.GetCertificateUserScope(makeFakeTLSState(t, "")); err == nil {
t.Error("unexpected success")
}
t.Run("no certificates", func(t *testing.T) {
if _, err := security.GetCertificateUserScope(ctx, makeFakeTLSState(t, "")); err == nil {
t.Error("unexpected success")
}
})

// Good request: single certificate.
if userScopes, err := security.GetCertificateUserScope(makeFakeTLSState(t, "foo")); err != nil {
t.Error(err)
} else {
require.Equal(t, 1, len(userScopes))
require.Equal(t, "foo", userScopes[0].Username)
require.True(t, userScopes[0].Global)
}
t.Run("good request: single certificate", func(t *testing.T) {
if userScopes, err := security.GetCertificateUserScope(ctx, makeFakeTLSState(t, "foo")); err != nil {
t.Error(err)
} else {
require.Equal(t, 1, len(userScopes))
require.Equal(t, "foo", userScopes[0].Username)
require.True(t, userScopes[0].Global)
}
})

// Request with multiple certs, but only one chain (eg: origin certs are client and CA).
if userScopes, err := security.GetCertificateUserScope(makeFakeTLSState(t, "foo;CA")); err != nil {
t.Error(err)
} else {
require.Equal(t, 1, len(userScopes))
require.Equal(t, "foo", userScopes[0].Username)
require.True(t, userScopes[0].Global)
}
t.Run("request with multiple certs, but only one chain (eg: origin certs are client and CA)", func(t *testing.T) {
if userScopes, err := security.GetCertificateUserScope(ctx, makeFakeTLSState(t, "foo;CA")); err != nil {
t.Error(err)
} else {
require.Equal(t, 1, len(userScopes))
require.Equal(t, "foo", userScopes[0].Username)
require.True(t, userScopes[0].Global)
}
})

// Always use the first certificate.
if userScopes, err := security.GetCertificateUserScope(makeFakeTLSState(t, "foo;bar")); err != nil {
t.Error(err)
} else {
require.Equal(t, 1, len(userScopes))
require.Equal(t, "foo", userScopes[0].Username)
require.True(t, userScopes[0].Global)
}
t.Run("always use the first certificate", func(t *testing.T) {
if userScopes, err := security.GetCertificateUserScope(ctx, makeFakeTLSState(t, "foo;bar")); err != nil {
t.Error(err)
} else {
require.Equal(t, 1, len(userScopes))
require.Equal(t, "foo", userScopes[0].Username)
require.True(t, userScopes[0].Global)
}
})

// Extract all of the principals from the first certificate.
if userScopes, err := security.GetCertificateUserScope(makeFakeTLSState(t, "foo,dns:bar,dns:blah;CA")); err != nil {
t.Error(err)
} else {
require.Equal(t, 3, len(userScopes))
require.True(t, userScopes[0].Global)
}
if userScopes, err := security.GetCertificateUserScope(makeFakeTLSState(t, "foo,uri:crdb://tenant/123/user/foo;CA")); err != nil {
t.Error(err)
} else {
require.Equal(t, 1, len(userScopes))
require.Equal(t, "foo", userScopes[0].Username)
require.Equal(t, roachpb.MakeTenantID(123), userScopes[0].TenantID)
require.False(t, userScopes[0].Global)
}
t.Run("extract all of the principals from the first certificate", func(t *testing.T) {
if userScopes, err := security.GetCertificateUserScope(ctx, makeFakeTLSState(t, "foo,dns:bar,dns:blah;CA")); err != nil {
t.Error(err)
} else {
require.Equal(t, 3, len(userScopes))
require.True(t, userScopes[0].Global)
}
})

t.Run("extracts username, tenantID from tenant URI SAN", func(t *testing.T) {
if userScopes, err := security.GetCertificateUserScope(ctx, makeFakeTLSState(t, "foo,uri:crdb://tenant/123/user/foo;CA")); err != nil {
t.Error(err)
} else {
require.Equal(t, 1, len(userScopes))
require.Equal(t, "foo", userScopes[0].Username)
require.Equal(t, roachpb.MakeTenantID(123), userScopes[0].TenantID)
require.False(t, userScopes[0].Global)
}
})

t.Run("falls back to global client cert when crdb URI SAN schem is not followed", func(t *testing.T) {
if userScopes, err := security.GetCertificateUserScope(
ctx,
makeFakeTLSState(t, "sanuri,uri:mycompany:sv:rootclient:dev:usw1")); err != nil {
t.Error(err)
} else {
require.Equal(t, 1, len(userScopes))
require.Equal(t, "sanuri", userScopes[0].Username)
require.True(t, userScopes[0].Global)
}
})
}

func TestSetCertPrincipalMap(t *testing.T) {
Expand Down Expand Up @@ -165,6 +187,7 @@ func TestSetCertPrincipalMap(t *testing.T) {
func TestGetCertificateUsersMapped(t *testing.T) {
defer leaktest.AfterTest(t)()
defer func() { _ = security.SetCertPrincipalMap(nil) }()
ctx := context.Background()

testCases := []struct {
spec string
Expand Down Expand Up @@ -196,7 +219,7 @@ func TestGetCertificateUsersMapped(t *testing.T) {
if err := security.SetCertPrincipalMap(vals); err != nil {
t.Fatal(err)
}
userScopes, err := security.GetCertificateUserScope(makeFakeTLSState(t, c.spec))
userScopes, err := security.GetCertificateUserScope(ctx, makeFakeTLSState(t, c.spec))
if err != nil {
t.Fatal(err)
}
Expand Down Expand Up @@ -268,7 +291,7 @@ func TestAuthenticationHook(t *testing.T) {
if err != nil {
t.Fatal(err)
}
hook, err := security.UserAuthCertHook(tc.insecure, makeFakeTLSState(t, tc.tlsSpec), tc.tenantID)
hook, err := security.UserAuthCertHook(ctx, tc.insecure, makeFakeTLSState(t, tc.tlsSpec), tc.tenantID)
if (err == nil) != tc.buildHookSuccess {
t.Fatalf("expected success=%t, got err=%v", tc.buildHookSuccess, err)
}
Expand Down
4 changes: 2 additions & 2 deletions pkg/sql/pgwire/auth_methods.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -400,7 +400,7 @@ func scramAuthenticator(
// and "cert-scram-sha-256" when the SQL client provides a TLS client
// certificate.
func authCert(
_ context.Context,
ctx context.Context,
_ AuthConn,
tlsState tls.ConnectionState,
execCfg *sql.ExecutorConfig,
Expand All @@ -422,7 +422,7 @@ func authCert(
tlsState.PeerCertificates[0].Subject.CommonName = tree.Name(
tlsState.PeerCertificates[0].Subject.CommonName,
).Normalize()
hook, err := security.UserAuthCertHook(false /*insecure*/, &tlsState, execCfg.RPCContext.TenantID)
hook, err := security.UserAuthCertHook(ctx, false /*insecure*/, &tlsState, execCfg.RPCContext.TenantID)
if err != nil {
return err
}
Expand Down

0 comments on commit 815e6e0

Please sign in to comment.