Skip to content

Commit

Permalink
rpc: prevent cross-tenant RPCs
Browse files Browse the repository at this point in the history 
Prior to this patch, it was possible for a server for tenant 123
to perform RPCs to a server for enant 456.
This patch disables that.

Release note: None
  • Loading branch information
@knz
knz committed Jan 29, 2023
1 parent 69dd453 commit 7c2d43a
Show file tree
Hide file tree
Showing 3 changed files with 45 additions and 22 deletions.
16 changes: 14 additions & 2 deletions pkg/rpc/auth.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -143,8 +143,20 @@ func (a kvAuth) authenticate(ctx context.Context) (roachpb.TenantID, error) {
//
// Is this a connection from another SQL tenant server?
if security.IsTenantCertificate(clientCert) {
// Incoming connection originating from a tenant SQL server. Let through.
// The other server is able to use any of this server's RPCs.
// Incoming connection originating from a tenant SQL server.
tid, err := tenantFromCommonName(clientCert.Subject.CommonName)
if err != nil {
return roachpb.TenantID{}, err
}
// Verify that our peer is a service for the same tenant
// as ourselves (we don't want to allow tenant 123 to
// serve requests for a client coming from tenant 456).
if tid != a.tenant.tenantID {
return roachpb.TenantID{}, authErrorf("this tenant (%v) cannot serve requests from a server for tenant %v", a.tenant.tenantID, tid)
}

// We return an unset tenant ID, to bypass authorization checks:
// the other server is able to use any of this server's RPCs.
return roachpb.TenantID{}, nil
}
}
Expand Down
45 changes: 27 additions & 18 deletions pkg/rpc/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"
"io"
"testing"

Expand Down Expand Up @@ -90,40 +91,48 @@ func TestWrappedServerStream(t *testing.T) {
require.Equal(t, 3, recv)
}

func TestTenantFromCert(t *testing.T) {
func TestAuthenticateTenant(t *testing.T) {
defer leaktest.AfterTest(t)()
correctOU := []string{security.TenantsOU}
stid := roachpb.SystemTenantID
tenTen := roachpb.MustMakeTenantID(10)
for _, tc := range []struct {
systemID roachpb.TenantID
ous []string
commonName string
expTenID roachpb.TenantID
expErr string
tenantScope uint64
}{
{ous: correctOU, commonName: "10", expTenID: roachpb.MustMakeTenantID(10)},
{ous: correctOU, commonName: roachpb.MinTenantID.String(), expTenID: roachpb.MinTenantID},
{ous: correctOU, commonName: roachpb.MaxTenantID.String(), expTenID: roachpb.MaxTenantID},
{ous: correctOU, commonName: roachpb.SystemTenantID.String() /* "system" */, expErr: `could not parse tenant ID from Common Name \(CN\)`},
{ous: correctOU, commonName: "-1", expErr: `could not parse tenant ID from Common Name \(CN\)`},
{ous: correctOU, commonName: "0", expErr: `invalid tenant ID 0 in Common Name \(CN\)`},
{ous: correctOU, commonName: "1", expErr: `invalid tenant ID 1 in Common Name \(CN\)`},
{ous: correctOU, commonName: "root", expErr: `could not parse tenant ID from Common Name \(CN\)`},
{ous: correctOU, commonName: "other", expErr: `could not parse tenant ID from Common Name \(CN\)`},
{ous: []string{"foo"}, commonName: "other", expErr: `client certificate CN=other,OU=foo cannot be used to perform RPC on tenant {1}`},
{ous: nil, commonName: "other", expErr: `client certificate CN=other cannot be used to perform RPC on tenant {1}`},
{ous: append([]string{"foo"}, correctOU...), commonName: "other", expErr: `could not parse tenant ID from Common Name`},
{ous: nil, commonName: "root"},
{ous: nil, commonName: "root", tenantScope: 10, expErr: "client certificate CN=root cannot be used to perform RPC on tenant {1}"},
{systemID: stid, ous: correctOU, commonName: "10", expTenID: tenTen},
{systemID: stid, ous: correctOU, commonName: roachpb.MinTenantID.String(), expTenID: roachpb.MinTenantID},
{systemID: stid, ous: correctOU, commonName: roachpb.MaxTenantID.String(), expTenID: roachpb.MaxTenantID},
{systemID: stid, ous: correctOU, commonName: roachpb.SystemTenantID.String() /* "system" */, expErr: `could not parse tenant ID from Common Name \(CN\)`},
{systemID: stid, ous: correctOU, commonName: "-1", expErr: `could not parse tenant ID from Common Name \(CN\)`},
{systemID: stid, ous: correctOU, commonName: "0", expErr: `invalid tenant ID 0 in Common Name \(CN\)`},
{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: 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: 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\)`},
} {
t.Run(tc.commonName, func(t *testing.T) {
t.Run(fmt.Sprintf("from %v to %v", tc.commonName, tc.systemID), func(t *testing.T) {
cert := &x509.Certificate{
Subject: pkix.Name{
CommonName: tc.commonName,
OrganizationalUnit: tc.ous,
},
}
if tc.tenantScope > 0 {
tenantSANs, err := security.MakeTenantURISANs(username.MakeSQLUsernameFromPreNormalizedString(tc.commonName), []roachpb.TenantID{roachpb.MustMakeTenantID(tc.tenantScope)})
tenantSANs, err := security.MakeTenantURISANs(
username.MakeSQLUsernameFromPreNormalizedString(tc.commonName),
[]roachpb.TenantID{roachpb.MustMakeTenantID(tc.tenantScope)})
require.NoError(t, err)
cert.URIs = append(cert.URIs, tenantSANs...)
}
Expand All @@ -135,7 +144,7 @@ func TestTenantFromCert(t *testing.T) {
p := peer.Peer{AuthInfo: tlsInfo}
ctx := peer.NewContext(context.Background(), &p)

tenID, err := rpc.TestingAuthenticateTenant(ctx)
tenID, err := rpc.TestingAuthenticateTenant(ctx, tc.systemID)

if tc.expErr == "" {
require.Equal(t, tc.expTenID, tenID)
Expand Down
6 changes: 4 additions & 2 deletions pkg/rpc/helpers_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,8 +36,10 @@ func TestingNewWrappedServerStream(

// TestingAuthenticateTenant performs authentication of a tenant from a context
// for testing.
func TestingAuthenticateTenant(ctx context.Context) (roachpb.TenantID, error) {
return kvAuth{tenant: tenantAuthorizer{tenantID: roachpb.SystemTenantID}}.authenticate(ctx)
func TestingAuthenticateTenant(
ctx context.Context, serverTenantID roachpb.TenantID,
) (roachpb.TenantID, error) {
return kvAuth{tenant: tenantAuthorizer{tenantID: serverTenantID}}.authenticate(ctx)
}

// TestingAuthorizeTenantRequest performs authorization of a tenant request
Expand Down

0 comments on commit 7c2d43a

Please sign in to comment.