From afdb4ff70fa7cadf19c48b9ac37e070f89169a18 Mon Sep 17 00:00:00 2001 From: Arul Ajmani Date: Wed, 8 Mar 2023 20:15:27 -0500 Subject: [PATCH 1/2] tenantcapabilities: add commentary, improve method name We were missing commentary for the functions on the Reader interface. While here, we also rename the method that retrieves the global tenant capability state to denote as such. Release note: None --- pkg/multitenant/tenantcapabilities/capabilities.go | 4 +++- .../tenantcapabilitiesauthorizer/authorizer_test.go | 4 ++-- .../tenantcapabilities/tenantcapabilitieswatcher/watcher.go | 4 ++-- pkg/sql/crdb_internal.go | 2 +- 4 files changed, 8 insertions(+), 6 deletions(-) diff --git a/pkg/multitenant/tenantcapabilities/capabilities.go b/pkg/multitenant/tenantcapabilities/capabilities.go index 891782e6e998..85c71407fd80 100644 --- a/pkg/multitenant/tenantcapabilities/capabilities.go +++ b/pkg/multitenant/tenantcapabilities/capabilities.go @@ -36,8 +36,10 @@ type Watcher interface { // Reader provides access to the global tenant capability state. The global // tenant capability state may be arbitrarily stale. type Reader interface { + // GetCapabilities returns the tenant capabilities for the specified tenant. GetCapabilities(id roachpb.TenantID) (_ TenantCapabilities, found bool) - GetCapabilitiesMap() map[roachpb.TenantID]TenantCapabilities + // GetGlobalCapabilityState returns the capability state for all tenants. + GetGlobalCapabilityState() map[roachpb.TenantID]TenantCapabilities } // Authorizer performs various kinds of capability checks for requests issued diff --git a/pkg/multitenant/tenantcapabilities/tenantcapabilitiesauthorizer/authorizer_test.go b/pkg/multitenant/tenantcapabilities/tenantcapabilitiesauthorizer/authorizer_test.go index b7a7618603b1..bfd88a19769d 100644 --- a/pkg/multitenant/tenantcapabilities/tenantcapabilitiesauthorizer/authorizer_test.go +++ b/pkg/multitenant/tenantcapabilities/tenantcapabilitiesauthorizer/authorizer_test.go @@ -136,8 +136,8 @@ func (m mockReader) GetCapabilities( return cp, found } -// GetCapabilitiesMap implements the tenantcapabilities.Reader interface. -func (m mockReader) GetCapabilitiesMap() map[roachpb.TenantID]tenantcapabilities.TenantCapabilities { +// GetGlobalCapabilityState implements the tenantcapabilities.Reader interface. +func (m mockReader) GetGlobalCapabilityState() map[roachpb.TenantID]tenantcapabilities.TenantCapabilities { return m } diff --git a/pkg/multitenant/tenantcapabilities/tenantcapabilitieswatcher/watcher.go b/pkg/multitenant/tenantcapabilities/tenantcapabilitieswatcher/watcher.go index c54f3c5e84bf..928ad26467fd 100644 --- a/pkg/multitenant/tenantcapabilities/tenantcapabilitieswatcher/watcher.go +++ b/pkg/multitenant/tenantcapabilities/tenantcapabilitieswatcher/watcher.go @@ -84,8 +84,8 @@ func (w *Watcher) GetCapabilities( return cp, found } -// GetCapabilitiesMap implements the tenantcapabilities.Reader interface. -func (w *Watcher) GetCapabilitiesMap() map[roachpb.TenantID]tenantcapabilities.TenantCapabilities { +// GetGlobalCapabilityState implements the tenantcapabilities.Reader interface. +func (w *Watcher) GetGlobalCapabilityState() map[roachpb.TenantID]tenantcapabilities.TenantCapabilities { w.mu.RLock() defer w.mu.RUnlock() result := make(map[roachpb.TenantID]tenantcapabilities.TenantCapabilities, len(w.mu.store)) diff --git a/pkg/sql/crdb_internal.go b/pkg/sql/crdb_internal.go index c14314516c72..9792102f63af 100644 --- a/pkg/sql/crdb_internal.go +++ b/pkg/sql/crdb_internal.go @@ -7579,7 +7579,7 @@ CREATE TABLE crdb_internal.node_tenant_capabilities_cache ( tenantID roachpb.TenantID tenantCapabilities tenantcapabilities.TenantCapabilities } - tenantCapabilitiesMap := tenantCapabilitiesReader.GetCapabilitiesMap() + tenantCapabilitiesMap := tenantCapabilitiesReader.GetGlobalCapabilityState() tenantCapabilitiesEntries := make([]tenantCapabilitiesEntry, 0, len(tenantCapabilitiesMap)) for tenantID, tenantCapabilities := range tenantCapabilitiesMap { tenantCapabilitiesEntries = append( From 8bf4365a6d5c8cd0967f5053eefe555a1776246c Mon Sep 17 00:00:00 2001 From: Arul Ajmani Date: Wed, 8 Mar 2023 20:16:42 -0500 Subject: [PATCH 2/2] tenantcapabilitiesauthorizer: introduce kill-switch cluster setting This patch introduces a new (non-public) cluster setting called `tenant_capabilities.authorizer.enabled` which acts as a kill switch for the Authorizer. It is inteded to be a safety switch to turn off capability checks performed by the Authorizer, in case we need to turn off the subsystem. As such, it isn't inteded to be used during normal cluster operation. Release note: None --- .../tenantcapabilitiesauthorizer/BUILD.bazel | 3 ++ .../authorizer.go | 38 ++++++++++--- .../authorizer_test.go | 41 ++++++++++++-- .../testdata/authorizer_enabled | 53 +++++++++++++++++++ 4 files changed, 125 insertions(+), 10 deletions(-) create mode 100644 pkg/multitenant/tenantcapabilities/tenantcapabilitiesauthorizer/testdata/authorizer_enabled diff --git a/pkg/multitenant/tenantcapabilities/tenantcapabilitiesauthorizer/BUILD.bazel b/pkg/multitenant/tenantcapabilities/tenantcapabilitiesauthorizer/BUILD.bazel index ce8c3412dc4a..36d93c632138 100644 --- a/pkg/multitenant/tenantcapabilities/tenantcapabilitiesauthorizer/BUILD.bazel +++ b/pkg/multitenant/tenantcapabilities/tenantcapabilitiesauthorizer/BUILD.bazel @@ -13,6 +13,7 @@ go_library( "//pkg/kv/kvpb", "//pkg/multitenant/tenantcapabilities", "//pkg/roachpb", + "//pkg/settings", "//pkg/settings/cluster", "//pkg/util/log", "@com_github_cockroachdb_errors//:errors", @@ -29,10 +30,12 @@ go_test( "//pkg/multitenant/tenantcapabilities", "//pkg/multitenant/tenantcapabilities/tenantcapabilitiestestutils", "//pkg/roachpb", + "//pkg/settings", "//pkg/settings/cluster", "//pkg/testutils/datapathutils", "//pkg/util/leaktest", "@com_github_cockroachdb_datadriven//:datadriven", + "@com_github_stretchr_testify//require", ], ) diff --git a/pkg/multitenant/tenantcapabilities/tenantcapabilitiesauthorizer/authorizer.go b/pkg/multitenant/tenantcapabilities/tenantcapabilitiesauthorizer/authorizer.go index b99d24010a5b..d38b6d71f81a 100644 --- a/pkg/multitenant/tenantcapabilities/tenantcapabilitiesauthorizer/authorizer.go +++ b/pkg/multitenant/tenantcapabilities/tenantcapabilitiesauthorizer/authorizer.go @@ -16,11 +16,24 @@ import ( "github.com/cockroachdb/cockroach/pkg/kv/kvpb" "github.com/cockroachdb/cockroach/pkg/multitenant/tenantcapabilities" "github.com/cockroachdb/cockroach/pkg/roachpb" + "github.com/cockroachdb/cockroach/pkg/settings" "github.com/cockroachdb/cockroach/pkg/settings/cluster" "github.com/cockroachdb/cockroach/pkg/util/log" "github.com/cockroachdb/errors" ) +// authorizerEnabled dictates whether the Authorizer performs any capability +// checks or not. It is intended as an escape hatch to turn off the tenant +// capabilities infrastructure; as such, it is intended to be a sort of hammer +// of last resort, and isn't expected to be used during normal cluster +// operation. +var authorizerEnabled = settings.RegisterBoolSetting( + settings.SystemOnly, + "tenant_capabilities.authorizer.enabled", + "enables authorization based on capability checks for incoming (secondary tenant) requests", + true, +) + // Authorizer is a concrete implementation of the tenantcapabilities.Authorizer // interface. It's safe for concurrent use. type Authorizer struct { @@ -50,8 +63,8 @@ func New(settings *cluster.Settings, knobs *tenantcapabilities.TestingKnobs) *Au func (a *Authorizer) HasCapabilityForBatch( ctx context.Context, tenID roachpb.TenantID, ba *kvpb.BatchRequest, ) error { - if tenID.IsSystem() { - return nil // the system tenant is allowed to do as it pleases. + if a.elideCapabilityChecks(ctx, tenID) { + return nil } if a.capabilitiesReader == nil { return errors.AssertionFailedf("programming error: trying to perform capability check when no reader exists") @@ -156,8 +169,8 @@ func (a *Authorizer) BindReader(reader tenantcapabilities.Reader) { } func (a *Authorizer) HasNodeStatusCapability(ctx context.Context, tenID roachpb.TenantID) error { - if tenID.IsSystem() { - return nil // the system tenant is allowed to do as it pleases. + if a.elideCapabilityChecks(ctx, tenID) { + return nil } cp, found := a.capabilitiesReader.GetCapabilities(tenID) if !found { @@ -173,8 +186,8 @@ func (a *Authorizer) HasNodeStatusCapability(ctx context.Context, tenID roachpb. } func (a *Authorizer) HasTSDBQueryCapability(ctx context.Context, tenID roachpb.TenantID) error { - if tenID.IsSystem() { - return nil // the system tenant is allowed to do as it pleases. + if a.elideCapabilityChecks(ctx, tenID) { + return nil } cp, found := a.capabilitiesReader.GetCapabilities(tenID) if !found { @@ -188,3 +201,16 @@ func (a *Authorizer) HasTSDBQueryCapability(ctx context.Context, tenID roachpb.T } return nil } + +// elideCapabilityChecks returns true if capability checks should be skipped for +// the supplied tenant. +func (a *Authorizer) elideCapabilityChecks(ctx context.Context, tenID roachpb.TenantID) bool { + if tenID.IsSystem() { + return true // the system tenant is allowed to do as it pleases + } + if !authorizerEnabled.Get(&a.settings.SV) { + log.VInfof(ctx, 3, "authorizer turned off; eliding capability checks") + return true + } + return false +} diff --git a/pkg/multitenant/tenantcapabilities/tenantcapabilitiesauthorizer/authorizer_test.go b/pkg/multitenant/tenantcapabilities/tenantcapabilitiesauthorizer/authorizer_test.go index bfd88a19769d..6af9c7150bfe 100644 --- a/pkg/multitenant/tenantcapabilities/tenantcapabilitiesauthorizer/authorizer_test.go +++ b/pkg/multitenant/tenantcapabilities/tenantcapabilitiesauthorizer/authorizer_test.go @@ -13,15 +13,18 @@ package tenantcapabilitiesauthorizer import ( "context" "fmt" + "strconv" "testing" "github.com/cockroachdb/cockroach/pkg/multitenant/tenantcapabilities" "github.com/cockroachdb/cockroach/pkg/multitenant/tenantcapabilities/tenantcapabilitiestestutils" "github.com/cockroachdb/cockroach/pkg/roachpb" + "github.com/cockroachdb/cockroach/pkg/settings" "github.com/cockroachdb/cockroach/pkg/settings/cluster" "github.com/cockroachdb/cockroach/pkg/testutils/datapathutils" "github.com/cockroachdb/cockroach/pkg/util/leaktest" "github.com/cockroachdb/datadriven" + "github.com/stretchr/testify/require" ) // TestDataDriven runs datadriven tests against the Authorizer interface. The @@ -52,24 +55,35 @@ import ( // ---- // ok // -// // "has-tsdb-query-capability": performas a capability check to be able to // make TSDB queries. Example: // // has-tsdb-query-capability ten=11 // ---- // ok - +// +// "set-bool-cluster-setting": overrides the specified boolean cluster setting +// to the given value. Currently, only the authorizerEnabled cluster setting is +// supported. +// +// set-bool-cluster-setting name=tenant_capabilities.authorizer.enabled value=false +// ---- +// ok func TestDataDriven(t *testing.T) { defer leaktest.AfterTest(t)() datadriven.Walk(t, datapathutils.TestDataPath(t), func(t *testing.T, path string) { + clusterSettings := cluster.MakeTestingClusterSettings() + ctx := context.Background() mockReader := mockReader(make(map[roachpb.TenantID]tenantcapabilities.TenantCapabilities)) - authorizer := New(cluster.MakeTestingClusterSettings(), nil /* TestingKnobs */) + authorizer := New(clusterSettings, nil /* TestingKnobs */) authorizer.BindReader(mockReader) datadriven.RunTest(t, path, func(t *testing.T, d *datadriven.TestData) string { - tenID := tenantcapabilitiestestutils.GetTenantID(t, d) + var tenID roachpb.TenantID + if d.HasArg("ten") { + tenID = tenantcapabilitiestestutils.GetTenantID(t, d) + } switch d.Cmd { case "upsert": _, caps, err := tenantcapabilitiestestutils.ParseTenantCapabilityUpsert(t, d) @@ -106,6 +120,18 @@ func TestDataDriven(t *testing.T) { return "ok" } return err.Error() + case "set-bool-cluster-setting": + var settingName string + d.ScanArgs(t, "name", &settingName) + setting, ok := supportedClusterSettings[settingName] + if !ok { + t.Fatalf("cluster setting %s not supported", settingName) + } + var valStr string + d.ScanArgs(t, "value", &valStr) + val, err := strconv.ParseBool(valStr) + require.NoError(t, err) + setting.Override(ctx, &clusterSettings.SV, val) default: return fmt.Sprintf("unknown command %s", d.Cmd) } @@ -114,6 +140,13 @@ func TestDataDriven(t *testing.T) { }) } +// supportedClusterSettings is a map, keyed by cluster setting name, of all +// boolean cluster settings that can be altered when running datadriven tests +// for the Authorizer. +var supportedClusterSettings = map[string]*settings.BoolSetting{ + authorizerEnabled.Key(): authorizerEnabled, +} + type mockReader map[roachpb.TenantID]tenantcapabilities.TenantCapabilities var _ tenantcapabilities.Reader = mockReader{} diff --git a/pkg/multitenant/tenantcapabilities/tenantcapabilitiesauthorizer/testdata/authorizer_enabled b/pkg/multitenant/tenantcapabilities/tenantcapabilitiesauthorizer/testdata/authorizer_enabled new file mode 100644 index 000000000000..c22bf964834f --- /dev/null +++ b/pkg/multitenant/tenantcapabilities/tenantcapabilitiesauthorizer/testdata/authorizer_enabled @@ -0,0 +1,53 @@ +upsert ten=10 can_admin_scatter=false can_admin_split=false can_view_node_info=false can_view_tsdb_metrics=false +---- +ok + +has-capability-for-batch ten=10 cmds=(AdminScatter, Scan) +---- +client tenant does not have capability "can_admin_scatter" (*kvpb.AdminScatterRequest) + +has-node-status-capability ten=10 +---- +client tenant does not have capability to query cluster node metadata + +has-tsdb-query-capability ten=10 +---- +client tenant does not have capability to query timeseries data + +# Disable the Authorizer. +set-bool-cluster-setting name=tenant_capabilities.authorizer.enabled value=false +---- +ok + +# Now that the Authorizer is disabled, all the checks that were previously +# failing should continue to fail. + +has-capability-for-batch ten=10 cmds=(AdminScatter, Scan) +---- +ok + +has-node-status-capability ten=10 +---- +ok + +has-tsdb-query-capability ten=10 +---- +ok + +# Enable the Authorizer again and ensure we start failing capability checks +# again. +set-bool-cluster-setting name=tenant_capabilities.authorizer.enabled value=true +---- +ok + +has-capability-for-batch ten=10 cmds=(AdminScatter, Scan) +---- +client tenant does not have capability "can_admin_scatter" (*kvpb.AdminScatterRequest) + +has-node-status-capability ten=10 +---- +client tenant does not have capability to query cluster node metadata + +has-tsdb-query-capability ten=10 +---- +client tenant does not have capability to query timeseries data