Skip to content

Commit

Permalink
rpc: simplify and enhance the client cert validation
Browse files Browse the repository at this point in the history 
TLDR: this commit enhances various aspects of TLS client cert
validation. Between other things, it makes the error clearer in case
of authentication failure.

Example, before:
```
ERROR: requested user demo is not authorized for tenant {2}
```

Example, after:
```
ERROR: certificate authentication failed for user "demo"
DETAIL: The client certificate is valid for "root" on all tenants.
```

----

Changes to client cert authentication:

The error message was improved.

Changes to tenant authentication in the RPC subsystem:

Prior to this change, there was a function `getActiveNodeOrUserScope`
whose purpose as not obvious at first sight -- it was merely checking
that the client cert contained the `root` or `node` identity.

This commit reduces the complexity of this function to make it more
maintainable.

Additionally, upon an authn error the text of the error was a bit
complex to read, and did not include details about the client cert.
This commit enhances the error and makes it show the user scopes
included in the client cert.

Finally, the function contained an exception to prevent the `node`
principal from running RPCs on a non-system tenant. This was too
restrictive; it prevents us (in a later commit) from reusing the
node cert for tenant-tenant connections when using shared-process
multitenancy. So this commit removes that exception.

Release note: None
  • Loading branch information
@knz
knz committed Jan 31, 2023
1 parent 8621e75 commit 37543f1
Show file tree
Hide file tree
Showing 5 changed files with 86 additions and 53 deletions.
45 changes: 17 additions & 28 deletions pkg/rpc/auth.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@ import (
"github.com/cockroachdb/cockroach/pkg/security"
"github.com/cockroachdb/cockroach/pkg/security/username"
"github.com/cockroachdb/cockroach/pkg/util/grpcutil"
"github.com/cockroachdb/errors"
"google.golang.org/grpc"
"google.golang.org/grpc/codes"
"google.golang.org/grpc/credentials"
Expand Down Expand Up @@ -184,42 +183,32 @@ func (a kvAuth) authenticate(ctx context.Context) (roachpb.TenantID, error) {
}

// Confirm that the user scope is node/root. Otherwise, return an authentication error.
_, err = getActiveNodeOrUserScope(certUserScope, username.RootUser, a.tenant.tenantID)
if err != nil {
return roachpb.TenantID{}, authErrorf("client certificate %s cannot be used to perform RPC on tenant %d", clientCert.Subject, a.tenant.tenantID)
if err := checkRootOrNodeInScope(certUserScope, a.tenant.tenantID); err != nil {
return roachpb.TenantID{}, err
}

// User is node/root user authorized for this tenant, return success.
return roachpb.TenantID{}, nil
}

// getActiveNodeOrUserScope returns a node user scope if one is present in the set of certificate user scopes. If node
// user scope is not present, it returns the user scope corresponding to the username parameter. The node user scope
// will always override the user scope for authentication.
func getActiveNodeOrUserScope(
certUserScope []security.CertificateUserScope, user string, serverTenantID roachpb.TenantID,
) (security.CertificateUserScope, error) {
var userScope security.CertificateUserScope
// checkRootOrNodeInScope checks that the root or node principals are
// present in the cert user scopes.
func checkRootOrNodeInScope(
certUserScope []security.CertificateUserScope, serverTenantID roachpb.TenantID,
) (err error) {
for _, scope := range certUserScope {
// If we get a scope that matches the Node user, immediately return.
if scope.Username == username.NodeUser {
if scope.Global {
return scope, nil
}
// Confirm that the certificate scope and serverTenantID are the system tenant.
if scope.TenantID == roachpb.SystemTenantID && serverTenantID == roachpb.SystemTenantID {
return scope, nil
}
// Only consider global scopes or scopes that match this server.
if !(scope.Global || scope.TenantID == serverTenantID) {
continue
}
if scope.Username == user && (scope.TenantID == serverTenantID || scope.Global) {
userScope = scope

// If we get a scope that matches the Node user, immediately return.
if scope.Username == username.NodeUser || scope.Username == username.RootUser {
return nil
}
}
// Double check we're not returning an empty scope
if userScope.Username == "" {
return userScope, errors.New("could not find active user scope for client certificate")
}

// Only return userScope if we haven't found a NodeUser.
return userScope, nil
return authErrorf(
"need root or node client cert to perform RPCs on this server (this is tenant %v; cert is valid for %s)",
serverTenantID, security.FormatUserScopes(certUserScope))
}
12 changes: 9 additions & 3 deletions pkg/rpc/auth_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -113,14 +113,20 @@ func TestAuthenticateTenant(t *testing.T) {
{systemID: stid, ous: correctOU, commonName: "1", expErr: `invalid tenant ID 1 in Common Name \(CN\)`},
{systemID: stid, ous: correctOU, commonName: "root", expErr: `could not parse tenant ID from Common Name \(CN\)`},
{systemID: stid, ous: correctOU, commonName: "other", expErr: `could not parse tenant ID from Common Name \(CN\)`},
{systemID: stid, ous: []string{"foo"}, commonName: "other", expErr: `client certificate CN=other,OU=foo cannot be used to perform RPC on tenant {1}`},
{systemID: stid, ous: nil, commonName: "other", expErr: `client certificate CN=other cannot be used to perform RPC on tenant {1}`},
{systemID: stid, ous: []string{"foo"}, commonName: "other",
expErr: `need root or node client cert to perform RPCs on this server \(this is tenant system; cert is valid for "other" on all tenants\)`},
{systemID: stid, ous: nil, commonName: "other",
expErr: `need root or node client cert to perform RPCs on this server \(this is tenant system; cert is valid for "other" on all tenants\)`},
{systemID: stid, ous: append([]string{"foo"}, correctOU...), commonName: "other", expErr: `could not parse tenant ID from Common Name`},
{systemID: stid, ous: nil, commonName: "root"},
{systemID: stid, ous: nil, commonName: "root", tenantScope: 10, expErr: "client certificate CN=root cannot be used to perform RPC on tenant {1}"},
{systemID: stid, ous: nil, commonName: "node"},
{systemID: stid, ous: nil, commonName: "root", tenantScope: 10,
expErr: `need root or node client cert to perform RPCs on this server \(this is tenant system; cert is valid for "root" on tenant 10\)`},
{systemID: tenTen, ous: correctOU, commonName: "10", expTenID: roachpb.TenantID{}},
{systemID: tenTen, ous: correctOU, commonName: "123", expErr: `this tenant \(10\) cannot serve requests from a server for tenant 123`},
{systemID: tenTen, ous: correctOU, commonName: "1", expErr: `invalid tenant ID 1 in Common Name \(CN\)`},
{systemID: tenTen, ous: nil, commonName: "root"},
{systemID: tenTen, ous: nil, commonName: "node"},
} {
t.Run(fmt.Sprintf("from %v to %v", tc.commonName, tc.systemID), func(t *testing.T) {
cert := &x509.Certificate{
Expand Down
22 changes: 20 additions & 2 deletions pkg/security/auth.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -169,7 +169,7 @@ func UserAuthCertHook(
}

if !clientConnection && !systemIdentity.IsNodeUser() {
return errors.Errorf("user %s is not allowed", systemIdentity)
return errors.Errorf("user %q is not allowed", systemIdentity)
}

// If running in insecure mode, we have nothing to verify it against.
Expand All @@ -187,10 +187,28 @@ func UserAuthCertHook(
if ValidateUserScope(certUserScope, systemIdentity.Normalized(), tenantID) {
return nil
}
return errors.Errorf("requested user %s is not authorized for tenant %d", systemIdentity, tenantID)
return errors.WithDetailf(errors.Errorf("certificate authentication failed for user %q", systemIdentity),
"The client certificate is valid for %s.", FormatUserScopes(certUserScope))
}, nil
}

// FormatUserScopes formats a list of scopes in a human-readable way,
// suitable for e.g. inclusion in error messages.
func FormatUserScopes(certUserScope []CertificateUserScope) string {
var buf strings.Builder
comma := ""
for _, scope := range certUserScope {
fmt.Fprintf(&buf, "%s%q on ", comma, scope.Username)
if scope.Global {
buf.WriteString("all tenants")
} else {
fmt.Fprintf(&buf, "tenant %v", scope.TenantID)
}
comma = ", "
}
return buf.String()
}

// IsTenantCertificate returns true if the passed certificate indicates an
// inbound Tenant connection.
func IsTenantCertificate(cert *x509.Certificate) bool {
Expand Down
58 changes: 39 additions & 19 deletions pkg/security/auth_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import (
"crypto/tls"
"crypto/x509"
"crypto/x509/pkix"
"fmt"
"net/url"
"strings"
"testing"
Expand Down Expand Up @@ -272,40 +273,50 @@ func TestAuthenticationHook(t *testing.T) {
publicHookSuccess bool
privateHookSuccess bool
tenantID roachpb.TenantID
expectedErr string
}{
// Insecure mode, empty username.
{true, "", username.SQLUsername{}, "", true, false, false, roachpb.SystemTenantID},
{true, "", username.SQLUsername{}, "", true, false, false, roachpb.SystemTenantID, `user is missing`},
// Insecure mode, non-empty username.
{true, "", fooUser, "", true, true, false, roachpb.SystemTenantID},
{true, "", fooUser, "", true, true, false, roachpb.SystemTenantID, `user "foo" is not allowed`},
// Secure mode, no TLS state.
{false, "", username.SQLUsername{}, "", false, false, false, roachpb.SystemTenantID},
{false, "", username.SQLUsername{}, "", false, false, false, roachpb.SystemTenantID, `no client certificates in request`},
// Secure mode, bad user.
{false, "foo", username.NodeUserName(), "", true, false, false, roachpb.SystemTenantID},
{false, "foo", username.NodeUserName(), "", true, false, false, roachpb.SystemTenantID,
`certificate authentication failed for user "node"`},
// Secure mode, node user.
{false, username.NodeUser, username.NodeUserName(), "", true, true, true, roachpb.SystemTenantID},
{false, username.NodeUser, username.NodeUserName(), "", true, true, true, roachpb.SystemTenantID, ``},
// Secure mode, node cert and unrelated user.
{false, username.NodeUser, fooUser, "", true, false, false, roachpb.SystemTenantID},
{false, username.NodeUser, fooUser, "", true, false, false, roachpb.SystemTenantID,
`certificate authentication failed for user "foo"`},
// Secure mode, root user.
{false, username.RootUser, username.NodeUserName(), "", true, false, false, roachpb.SystemTenantID},
{false, username.RootUser, username.NodeUserName(), "", true, false, false, roachpb.SystemTenantID,
`certificate authentication failed for user "node"`},
// Secure mode, tenant cert, foo user.
{false, "(Tenants)foo", fooUser, "", true, false, false, roachpb.SystemTenantID},
{false, "(Tenants)foo", fooUser, "", true, false, false, roachpb.SystemTenantID,
`using tenant client certificate as user certificate is not allowed`},
// Secure mode, multiple cert principals.
{false, "foo,dns:bar", fooUser, "", true, true, false, roachpb.SystemTenantID},
{false, "foo,dns:bar", barUser, "", true, true, false, roachpb.SystemTenantID},
{false, "foo,dns:bar", fooUser, "", true, true, false, roachpb.SystemTenantID, `user "foo" is not allowed`},
{false, "foo,dns:bar", barUser, "", true, true, false, roachpb.SystemTenantID, `user "bar" is not allowed`},
// Secure mode, principal map.
{false, "foo,dns:bar", blahUser, "foo:blah", true, true, false, roachpb.SystemTenantID},
{false, "foo,dns:bar", blahUser, "bar:blah", true, true, false, roachpb.SystemTenantID},
{false, "foo,uri:crdb://tenant/123/user/foo", fooUser, "", true, true, false, roachpb.MustMakeTenantID(123)},
{false, "foo,uri:crdb://tenant/123/user/foo", fooUser, "", true, false, false, roachpb.SystemTenantID},
{false, "foo", fooUser, "", true, true, false, roachpb.MustMakeTenantID(123)},
{false, "foo,uri:crdb://tenant/1/user/foo", fooUser, "", true, false, false, roachpb.MustMakeTenantID(123)},
{false, "foo,uri:crdb://tenant/123/user/foo", blahUser, "", true, false, false, roachpb.MustMakeTenantID(123)},
{false, "foo,dns:bar", blahUser, "foo:blah", true, true, false, roachpb.SystemTenantID, `user "blah" is not allowed`},
{false, "foo,dns:bar", blahUser, "bar:blah", true, true, false, roachpb.SystemTenantID, `user "blah" is not allowed`},
{false, "foo,uri:crdb://tenant/123/user/foo", fooUser, "", true, true, false, roachpb.MustMakeTenantID(123),
`user "foo" is not allowed`},
{false, "foo,uri:crdb://tenant/123/user/foo", fooUser, "", true, false, false, roachpb.SystemTenantID,
`certificate authentication failed for user "foo"`},
{false, "foo", fooUser, "", true, true, false, roachpb.MustMakeTenantID(123),
`user "foo" is not allowed`},
{false, "foo,uri:crdb://tenant/1/user/foo", fooUser, "", true, false, false, roachpb.MustMakeTenantID(123),
`certificate authentication failed for user "foo"`},
{false, "foo,uri:crdb://tenant/123/user/foo", blahUser, "", true, false, false, roachpb.MustMakeTenantID(123),
`certificate authentication failed for user "blah"`},
}

ctx := context.Background()

for _, tc := range testCases {
t.Run("", func(t *testing.T) {
for i, tc := range testCases {
t.Run(fmt.Sprintf("%d: tls:%s user:%v tenant:%v", i, tc.tlsSpec, tc.username, tc.tenantID), func(t *testing.T) {
err := security.SetCertPrincipalMap(strings.Split(tc.principalMap, ","))
if err != nil {
t.Fatal(err)
Expand All @@ -315,16 +326,25 @@ func TestAuthenticationHook(t *testing.T) {
t.Fatalf("expected success=%t, got err=%v", tc.buildHookSuccess, err)
}
if err != nil {
require.Regexp(t, tc.expectedErr, err.Error())
return
}
err = hook(ctx, tc.username, true /* clientConnection */)
if (err == nil) != tc.publicHookSuccess {
t.Fatalf("expected success=%t, got err=%v", tc.publicHookSuccess, err)
}
if err != nil {
require.Regexp(t, tc.expectedErr, err.Error())
return
}
err = hook(ctx, tc.username, false /* clientConnection */)
if (err == nil) != tc.privateHookSuccess {
t.Fatalf("expected success=%t, got err=%v", tc.privateHookSuccess, err)
}
if err != nil {
require.Regexp(t, tc.expectedErr, err.Error())
return
}
})
}
}
2 changes: 1 addition & 1 deletion pkg/server/authentication_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -833,7 +833,7 @@ func TestGRPCAuthentication(t *testing.T) {
for _, subsystem := range subsystems {
t.Run(fmt.Sprintf("bad-user/%s", subsystem.name), func(t *testing.T) {
err := subsystem.sendRPC(ctx, conn)
if exp := `client certificate CN=testuser,O=Cockroach cannot be used to perform RPC on tenant {1}`; !testutils.IsError(err, exp) {
if exp := `need root or node client cert to perform RPCs on this server`; !testutils.IsError(err, exp) {
t.Errorf("expected %q error, but got %v", exp, err)
}
})
Expand Down

0 comments on commit 37543f1

Please sign in to comment.