From 5898f276144622720e36e8c25b3ec91f620de8a1 Mon Sep 17 00:00:00 2001 From: Adam Storm Date: Thu, 21 Jan 2021 12:20:51 -0500 Subject: [PATCH 1/3] sql: Implement ALTER TABLE from REGIONAL BY TABLE to GLOBAL Implement ALTER TABLE ... SET LOCALITY GLOBAL for tables starting as REGIONAL BY ROW. Release note (sql change): Implement ALTER TABLE ... SET LOCALITY GLOBAL for tables starting as REGIONAL BY ROW. --- .../testdata/logic_test/alter_table_locality | 91 +++++++++- pkg/sql/alter_table_locality.go | 166 +++++++++++------- pkg/sql/catalog/descriptor.go | 3 + pkg/sql/catalog/tabledesc/structured.go | 17 ++ pkg/sql/region_util.go | 47 ++++- 5 files changed, 252 insertions(+), 72 deletions(-) diff --git a/pkg/ccl/logictestccl/testdata/logic_test/alter_table_locality b/pkg/ccl/logictestccl/testdata/logic_test/alter_table_locality index da81698c6a50..09330ef79091 100644 --- a/pkg/ccl/logictestccl/testdata/logic_test/alter_table_locality +++ b/pkg/ccl/logictestccl/testdata/logic_test/alter_table_locality @@ -319,9 +319,36 @@ DATABASE alter_locality_test ALTER DATABASE alter_locality_test CONFIGURE ZONE constraints = '{+region=ap-southeast-2: 1, +region=ca-central-1: 1, +region=us-east-1: 1}', lease_preferences = '[[+region=ca-central-1]]' -statement error unimplemented: implementation pending +statement ok ALTER TABLE regional_by_table_in_primary_region SET LOCALITY GLOBAL +query TT +SHOW CREATE TABLE regional_by_table_in_primary_region +---- +regional_by_table_in_primary_region CREATE TABLE public.regional_by_table_in_primary_region ( + i INT8 NULL, + FAMILY "primary" (i, rowid) +) LOCALITY GLOBAL + +query TT +SHOW ZONE CONFIGURATION FOR TABLE regional_by_table_in_primary_region +---- +TABLE regional_by_table_in_primary_region ALTER TABLE regional_by_table_in_primary_region CONFIGURE ZONE USING + range_min_bytes = 134217728, + range_max_bytes = 536870912, + gc.ttlseconds = 90000, + num_replicas = 3, + constraints = '{+region=ap-southeast-2: 1, +region=ca-central-1: 1, +region=us-east-1: 1}', + lease_preferences = '[[+region=ca-central-1]]' + +# Drop the table and recreate it to get back to original state (this is required because alter table +# from global to regional by table is not yet implemented). +statement ok +DROP TABLE regional_by_table_in_primary_region + +statement ok +CREATE TABLE regional_by_table_in_primary_region (i int) LOCALITY REGIONAL BY TABLE IN PRIMARY REGION + statement error unimplemented: implementation pending ALTER TABLE regional_by_table_in_primary_region SET LOCALITY REGIONAL BY ROW @@ -347,7 +374,7 @@ DATABASE alter_locality_test ALTER DATABASE alter_locality_test CONFIGURE ZONE constraints = '{+region=ap-southeast-2: 1, +region=ca-central-1: 1, +region=us-east-1: 1}', lease_preferences = '[[+region=ca-central-1]]' -# drop and recreate the table to get it back to the "no region" state +# Drop and recreate the table to get it back to the "no region" state. statement ok DROP TABLE regional_by_table_no_region @@ -376,7 +403,7 @@ TABLE regional_by_table_no_region ALTER TABLE regional_by_table_no_region CONFI constraints = '{+region=ap-southeast-2: 3}', lease_preferences = '[[+region=ap-southeast-2]]' -# drop and recreate the table to get it back to the "no region" state +# Drop and recreate the table to get it back to the "no region" state. statement ok DROP TABLE regional_by_table_no_region @@ -405,9 +432,36 @@ DATABASE alter_locality_test ALTER DATABASE alter_locality_test CONFIGURE ZONE constraints = '{+region=ap-southeast-2: 1, +region=ca-central-1: 1, +region=us-east-1: 1}', lease_preferences = '[[+region=ca-central-1]]' -statement error unimplemented: implementation pending +statement ok ALTER TABLE regional_by_table_no_region SET LOCALITY GLOBAL +query TT +SHOW CREATE TABLE regional_by_table_no_region +---- +regional_by_table_no_region CREATE TABLE public.regional_by_table_no_region ( + i INT8 NULL, + FAMILY "primary" (i, rowid) +) LOCALITY GLOBAL + +query TT +SHOW ZONE CONFIGURATION FOR TABLE regional_by_table_no_region +---- +TABLE regional_by_table_no_region ALTER TABLE regional_by_table_no_region CONFIGURE ZONE USING + range_min_bytes = 134217728, + range_max_bytes = 536870912, + gc.ttlseconds = 90000, + num_replicas = 3, + constraints = '{+region=ap-southeast-2: 1, +region=ca-central-1: 1, +region=us-east-1: 1}', + lease_preferences = '[[+region=ca-central-1]]' + +# Drop the table and recreate it to get back to original state (this is required because alter table +# from global to regional by table is not yet implemented). +statement ok +DROP TABLE regional_by_table_no_region + +statement ok +CREATE TABLE regional_by_table_no_region (i int) LOCALITY REGIONAL BY TABLE + statement error unimplemented: implementation pending ALTER TABLE regional_by_table_no_region SET LOCALITY REGIONAL BY ROW @@ -483,8 +537,35 @@ DATABASE alter_locality_test ALTER DATABASE alter_locality_test CONFIGURE ZONE constraints = '{+region=ap-southeast-2: 1, +region=ca-central-1: 1, +region=us-east-1: 1}', lease_preferences = '[[+region=ca-central-1]]' -statement error unimplemented: implementation pending +statement ok ALTER TABLE regional_by_table_in_us_east SET LOCALITY GLOBAL +query TT +SHOW CREATE TABLE regional_by_table_in_us_east +---- +regional_by_table_in_us_east CREATE TABLE public.regional_by_table_in_us_east ( + i INT8 NULL, + FAMILY "primary" (i, rowid) +) LOCALITY GLOBAL + +query TT +SHOW ZONE CONFIGURATION FOR TABLE regional_by_table_in_us_east +---- +TABLE regional_by_table_in_us_east ALTER TABLE regional_by_table_in_us_east CONFIGURE ZONE USING + range_min_bytes = 134217728, + range_max_bytes = 536870912, + gc.ttlseconds = 90000, + num_replicas = 3, + constraints = '{+region=ap-southeast-2: 1, +region=ca-central-1: 1, +region=us-east-1: 1}', + lease_preferences = '[[+region=ca-central-1]]' + +# Drop the table and recreate it to get back to original state (this is required because alter table +# from global to regional by table is not yet implemented). +statement ok +DROP TABLE regional_by_table_in_us_east + +statement ok +CREATE TABLE regional_by_table_in_us_east (i int) LOCALITY REGIONAL BY TABLE IN "us-east-1" + statement error unimplemented: implementation pending ALTER TABLE regional_by_table_in_us_east SET LOCALITY REGIONAL BY ROW diff --git a/pkg/sql/alter_table_locality.go b/pkg/sql/alter_table_locality.go index 28e3ba44e91a..c1680ca90e0b 100644 --- a/pkg/sql/alter_table_locality.go +++ b/pkg/sql/alter_table_locality.go @@ -12,16 +12,15 @@ package sql import ( "context" - "fmt" "github.com/cockroachdb/cockroach/pkg/sql/catalog/catalogkv" + "github.com/cockroachdb/cockroach/pkg/sql/catalog/dbdesc" "github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb" "github.com/cockroachdb/cockroach/pkg/sql/catalog/tabledesc" "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode" "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror" "github.com/cockroachdb/cockroach/pkg/sql/privilege" "github.com/cockroachdb/cockroach/pkg/sql/sem/tree" - "github.com/cockroachdb/cockroach/pkg/sql/sessiondata" "github.com/cockroachdb/cockroach/pkg/util/errorutil/unimplemented" "github.com/cockroachdb/cockroach/pkg/util/log/eventpb" "github.com/cockroachdb/errors" @@ -71,36 +70,91 @@ func (n *alterTableSetLocalityNode) Next(runParams) (bool, error) { return false func (n *alterTableSetLocalityNode) Values() tree.Datums { return tree.Datums{} } func (n *alterTableSetLocalityNode) Close(context.Context) {} -func (n *alterTableSetLocalityNode) alterTableLocalityGlobalToRegionalByTable( - params runParams, -) error { +func (n *alterTableSetLocalityNode) alterTableLocalityGlobalToRegionalByTable() error { return unimplemented.New("alter table locality from GLOBAL to REGIONAL BY TABLE", "implementation pending") } func (n *alterTableSetLocalityNode) alterTableLocalityRegionalByTableToGlobal( - params runParams, + params runParams, desc *dbdesc.Immutable, ) error { - return unimplemented.New("alter table locality from REGIONAL BY TABLE to GLOBAL", "implementation pending") + const operation string = "alter table locality REGIONAL BY TABLE to GLOBAL" + if err := assertIsMultiRegionDatabase(desc, operation); err != nil { + return err + } + if !n.tableDesc.IsLocalityRegionalByTable() { + return errors.AssertionFailedf( + "invalid call %q on incorrect table locality. %v", + operation, + n.tableDesc.LocalityConfig, + ) + } + + // Set LocalityConfig to GLOBAL. + n.tableDesc.LocalityConfig = &descpb.TableDescriptor_LocalityConfig{ + Locality: &descpb.TableDescriptor_LocalityConfig_Global_{ + Global: &descpb.TableDescriptor_LocalityConfig_Global{}, + }, + } + + resolvedSchema, err := params.p.Descriptors().GetImmutableSchemaByID( + params.ctx, + params.p.txn, + n.tableDesc.GetParentSchemaID(), + tree.SchemaLookupFlags{}) + if err != nil { + return err + } + + tableName := tree.MakeTableNameWithSchema( + tree.Name(desc.Name), + tree.Name(resolvedSchema.Name), + tree.Name(n.tableDesc.GetName()), + ) + + // Validate the new locality before updating the table descriptor. + if err := tabledesc.ValidateTableLocalityConfig( + tableName.String(), + n.tableDesc.LocalityConfig, + desc, + ); err != nil { + return err + } + + // Write out the table descriptor update. + if err := params.p.writeSchemaChange( + params.ctx, + n.tableDesc, + descpb.InvalidMutationID, + tree.AsStringWithFQNames(&n.n, params.Ann()), + ); err != nil { + return err + } + + // Update the zone configuration. + if err := params.p.applyZoneConfigFromTableLocalityConfig( + params.ctx, + tableName, + n.tableDesc.TableDesc(), + *desc.RegionConfig, + ); err != nil { + return err + } + + return nil } func (n *alterTableSetLocalityNode) alterTableLocalityRegionalByTableToRegionalByTable( - params runParams, + params runParams, desc *dbdesc.Immutable, ) error { - // Ensure that the database is multi-region enabled. - dbDesc, err := catalogkv.MustGetDatabaseDescByID( - params.ctx, - params.extendedEvalCtx.Txn, - params.extendedEvalCtx.EvalContext.Codec, - n.tableDesc.GetParentID(), - ) - if err != nil { + const operation string = "alter table locality REGIONAL BY TABLE to REGIONAL BY TABLE" + if err := assertIsMultiRegionDatabase(desc, operation); err != nil { return err } - if !dbDesc.IsMultiRegion() { + if !n.tableDesc.IsLocalityRegionalByTable() { return errors.AssertionFailedf( - "invalid call to alter the table locality on a non-multi-region database. %v %v", - dbDesc, - n, + "invalid call %q on incorrect table locality. %v", + operation, + n.tableDesc.LocalityConfig, ) } @@ -135,11 +189,25 @@ func (n *alterTableSetLocalityNode) alterTableLocalityRegionalByTableToRegionalB } // Validate the new locality before updating the table descriptor. - tableName := tree.MakeTableName(tree.Name(dbDesc.Name), tree.Name(n.tableDesc.GetName())) + resolvedSchema, err := params.p.Descriptors().GetImmutableSchemaByID( + params.ctx, + params.p.txn, + n.tableDesc.GetParentSchemaID(), + tree.SchemaLookupFlags{}) + if err != nil { + return err + } + + tableName := tree.MakeTableNameWithSchema( + tree.Name(desc.Name), + tree.Name(resolvedSchema.Name), + tree.Name(n.tableDesc.GetName()), + ) + if err := tabledesc.ValidateTableLocalityConfig( tableName.String(), n.tableDesc.LocalityConfig, - dbDesc, + desc, ); err != nil { return err } @@ -154,42 +222,14 @@ func (n *alterTableSetLocalityNode) alterTableLocalityRegionalByTableToRegionalB return err } - // Update the table's zone configuration. If we're altering to the PRIMARY REGION, - // we don't want to set a zone configuration (in fact, we'll remove any existing zone - // configuration below). - if !alterToPrimaryRegion { - if err := params.p.applyZoneConfigFromTableLocalityConfig( - params.ctx, - tableName, - n.tableDesc.TableDesc(), - *dbDesc.RegionConfig, - ); err != nil { - return err - } - } else { - // TODO(#multiregion): We should check to see if the zone configuration has been updated - // by the user. If it has, we need to warn, and only proceed if sql_safe_updates is disabled. - - // Table is placed in the PRIMARY REGION. Remove the table's zone configuration so - // that it can be inherited from the database. - fullTableName := tree.MakeTableNameWithSchema( - tableName.CatalogName, - tableName.SchemaName, - tableName.ObjectName, - ) - sql := fmt.Sprintf("ALTER TABLE %s CONFIGURE ZONE DISCARD", fullTableName.String()) - if _, err := params.p.ExtendedEvalContext().ExecCfg.InternalExecutor.ExecEx( - params.ctx, - "table-multiregion-discard-zone-config", - params.extendedEvalCtx.Txn, - sessiondata.InternalExecutorOverride{ - User: params.p.SessionData().User(), - }, - sql, - ); err != nil { - return err - } - return nil + // Update the table's zone configuration. + if err := params.p.applyZoneConfigFromTableLocalityConfig( + params.ctx, + tableName, + n.tableDesc.TableDesc(), + *desc.RegionConfig, + ); err != nil { + return err } return nil @@ -197,7 +237,7 @@ func (n *alterTableSetLocalityNode) alterTableLocalityRegionalByTableToRegionalB func (n *alterTableSetLocalityNode) startExec(params runParams) error { // Ensure that the database is multi-region enabled. - dbDesc, err := catalogkv.MustGetDatabaseDescByID( + desc, err := catalogkv.MustGetDatabaseDescByID( params.ctx, params.extendedEvalCtx.Txn, params.extendedEvalCtx.EvalContext.Codec, @@ -206,7 +246,7 @@ func (n *alterTableSetLocalityNode) startExec(params runParams) error { if err != nil { return err } - if !dbDesc.IsMultiRegion() { + if !desc.IsMultiRegion() { return pgerror.Newf( pgcode.InvalidTableDefinition, "cannot alter a table's LOCALITY if its database is not multi-region enabled", @@ -238,7 +278,7 @@ func (n *alterTableSetLocalityNode) startExec(params runParams) error { // GLOBAL to REGIONAL BY ROW return unimplemented.New("alter table locality to REGIONAL BY ROW", "implementation pending") case tree.LocalityLevelTable: - if err = n.alterTableLocalityGlobalToRegionalByTable(params); err != nil { + if err = n.alterTableLocalityGlobalToRegionalByTable(); err != nil { return err } default: @@ -247,14 +287,14 @@ func (n *alterTableSetLocalityNode) startExec(params runParams) error { case *descpb.TableDescriptor_LocalityConfig_RegionalByTable_: switch newLocality.LocalityLevel { case tree.LocalityLevelGlobal: - err = n.alterTableLocalityRegionalByTableToGlobal(params) + err = n.alterTableLocalityRegionalByTableToGlobal(params, desc) if err != nil { return err } case tree.LocalityLevelRow: return unimplemented.New("alter table locality to REGIONAL BY ROW", "implementation pending") case tree.LocalityLevelTable: - err = n.alterTableLocalityRegionalByTableToRegionalByTable(params) + err = n.alterTableLocalityRegionalByTableToRegionalByTable(params, desc) if err != nil { return err } diff --git a/pkg/sql/catalog/descriptor.go b/pkg/sql/catalog/descriptor.go index 7595941c806d..410e8dd02803 100644 --- a/pkg/sql/catalog/descriptor.go +++ b/pkg/sql/catalog/descriptor.go @@ -248,6 +248,9 @@ type TableDescriptor interface { WritableColumns() []descpb.ColumnDescriptor GetLocalityConfig() *descpb.TableDescriptor_LocalityConfig + IsLocalityRegionalByRow() bool + IsLocalityRegionalByTable() bool + IsLocalityGlobal() bool } // Index is an interface around the index descriptor types. diff --git a/pkg/sql/catalog/tabledesc/structured.go b/pkg/sql/catalog/tabledesc/structured.go index 1b00f14f1856..43de5dde8796 100644 --- a/pkg/sql/catalog/tabledesc/structured.go +++ b/pkg/sql/catalog/tabledesc/structured.go @@ -4149,3 +4149,20 @@ func (desc *Mutable) SetOffline(reason string) { desc.State = descpb.DescriptorState_OFFLINE desc.OfflineReason = reason } + +// IsLocalityRegionalByRow returns whether or not the table is REGIONAL BY ROW table +func (desc *wrapper) IsLocalityRegionalByRow() bool { + return desc.LocalityConfig.GetRegionalByRow() != nil +} + +// IsLocalityRegionalByTable returns whether or not the table is REGIONAL BY TABLE table +// TODO (arulajmani): We can pull out this first check once all multi-region tables contain a LocalityConfig. +func (desc *wrapper) IsLocalityRegionalByTable() bool { + return desc.LocalityConfig == nil || + desc.LocalityConfig.GetRegionalByTable() != nil +} + +// IsLocalityGlobal returns whether or not the table is GLOBAL table +func (desc *wrapper) IsLocalityGlobal() bool { + return desc.LocalityConfig.GetGlobal() != nil +} diff --git a/pkg/sql/region_util.go b/pkg/sql/region_util.go index 3804dd0deba7..c46358aab4de 100644 --- a/pkg/sql/region_util.go +++ b/pkg/sql/region_util.go @@ -12,6 +12,7 @@ package sql import ( "context" + "fmt" "sort" "strings" @@ -47,6 +48,20 @@ func (s *liveClusterRegions) toStrings() []string { return ret } +// assertIsMultiRegionDatabase ensures that we're in a multi-region database, and +// if not, returns an assertion error. Takes an "operation" string which describes +// the operation in progress which requires database to be multi-region. +func assertIsMultiRegionDatabase(desc *dbdesc.Immutable, operation string) error { + if !desc.IsMultiRegion() { + return errors.AssertionFailedf( + "invalid call to %q on a non-multi-region database. %v", + operation, + desc, + ) + } + return nil +} + // getLiveClusterRegions returns a set of live region names in the cluster. // A region name is deemed active if there is at least one alive node // in the cluster in with locality set to a given region. @@ -184,8 +199,8 @@ func constraintsConjunctionForRegionalLocality( // zoneConfigFromTableLocalityConfig generates a desired ZoneConfig based // on the locality config for the database. -// This function can return a nil zonepb.ZoneConfig, meaning no update is required -// to any zone config. +// This function can return a nil zonepb.ZoneConfig, meaning no table level zone +// configuration is required. // TODO(#multiregion,aayushshah15): properly configure this for region survivability and leaseholder // preferences when new zone configuration parameters merge. func zoneConfigFromTableLocalityConfig( @@ -225,6 +240,9 @@ func zoneConfigFromTableLocalityConfig( func (p *planner) applyZoneConfigForMultiRegion( ctx context.Context, zs tree.ZoneSpecifier, zc *zonepb.ZoneConfig, desc string, ) error { + // TODO(#multiregion): We should check to see if the zone configuration has been updated + // by the user. If it has, we need to warn, and only proceed if sql_safe_updates is disabled. + // Convert the partially filled zone config to re-run as a SQL command. // This avoid us having to modularize planNode logic from set_zone_config // and the optimizer. @@ -314,9 +332,11 @@ func (p *planner) applyZoneConfigFromTableLocalityConfig( if err != nil { return err } - // If we do not have to configure anything, exit early. + + // This means that the table doesn't need an explicit zone configuration. Drop + // one if it already exists and return. if localityZoneConfig == nil { - return nil + return p.dropZoneConfigForTable(ctx, explicitTblName) } return p.applyZoneConfigForMultiRegion( @@ -341,6 +361,25 @@ func (p *planner) applyZoneConfigFromDatabaseRegionConfig( ) } +// dropZoneConfigForTable drops the zone configuration for a table if one exists. +func (p *planner) dropZoneConfigForTable(ctx context.Context, name tree.TableName) error { + // TODO(#multiregion): We should check to see if the zone configuration has been updated + // by the user. If it has, we need to warn, and only proceed if sql_safe_updates is disabled. + sql := fmt.Sprintf("ALTER TABLE %s CONFIGURE ZONE DISCARD", name.String()) + if _, err := p.ExtendedEvalContext().ExecCfg.InternalExecutor.ExecEx( + ctx, + "table-multiregion-discard-zone-config", + p.ExtendedEvalContext().Txn, + sessiondata.InternalExecutorOverride{ + User: p.SessionData().User(), + }, + sql, + ); err != nil { + return err + } + return nil +} + // forEachTableWithLocalityConfigInDatabase loops through each schema and table // for a table with a LocalityConfig configured. // NOTE: this function uses cached table and schema descriptors. As a result, it may From 32e1c77643a927c4d384241bcb60052cd3c5fef1 Mon Sep 17 00:00:00 2001 From: David Hartunian Date: Thu, 3 Dec 2020 13:57:31 -0500 Subject: [PATCH 2/3] auth: add region-based callback URLs for OIDC Modifies `server.oidc_authentication.redirect_url` cluster setting to accept valid JSON strings with a `redirect_urls` field that can support region-based OIDC auth flows. In addition to a simple string callback URL, here is an example of valid JSON that this setting can accept: ``` '{ "redirect_urls": { "us-east-1": "https://localhost:8080/oidc/v1/callback", "eu-west-1": "example.com" } }' ``` Prerequisites to using the multi-region callback URLs: 1. `region` locality flag is available and set 2. `server.oidc_authentication.redirect_url` setting is set as valid JSON containing the `redirect_urls` object with a key that matches the `region` locality flag value on this node When prerequisites above are met, the `callback_uri` OAuth param is set to the region-specific value from the JSON setting upon redirect to the auth provider. If you are using region-specific configuration, and do not have the `region` locality set, or try using OIDC in a region without a corresponding entry in the JSON, OIDC will fail to run. If you are using simple string-based configuration of a single redirect URL, OIDC will always use it regardless of your region locality configuration. Be aware that the auth provider will likely need to be updated to know about all possible redirect URLs it may get triggered with. Resolves #56517 Release note (security update): Adds ability to set region-specific callback URLs in the OIDC config. The `server.oidc_authentication.redirect_url` cluster setting can now accept JSON as an alternative to the basic URL string setting. If a JSON value is set, it *must* contain a `redirect_url` key that maps to an object with key, value pairs where the key is a `region` matching an existing locality setting, and the value is a callback URL. --- docs/generated/settings/settings.html | 2 +- pkg/ccl/oidcccl/BUILD.bazel | 6 +- pkg/ccl/oidcccl/authentication_oidc.go | 149 ++++++++++---------- pkg/ccl/oidcccl/authentication_oidc_test.go | 78 ++++++++++ pkg/ccl/oidcccl/settings.go | 145 ++++++++++++++++--- pkg/ccl/oidcccl/settings_test.go | 69 +++++++++ pkg/server/authentication.go | 2 + pkg/server/server.go | 5 +- 8 files changed, 354 insertions(+), 102 deletions(-) create mode 100644 pkg/ccl/oidcccl/settings_test.go diff --git a/docs/generated/settings/settings.html b/docs/generated/settings/settings.html index f044a7d0a9a3..0236916accf6 100644 --- a/docs/generated/settings/settings.html +++ b/docs/generated/settings/settings.html @@ -54,7 +54,7 @@ server.oidc_authentication.enabledbooleanfalseenables or disabled OIDC login for the DB Console (this feature is experimental) server.oidc_authentication.principal_regexstring(.+)regular expression to apply to extracted principal (see claim_json_key setting) to translate to SQL user (golang regex format, must include 1 grouping to extract) (this feature is experimental) server.oidc_authentication.provider_urlstringsets OIDC provider URL ({provider_url}/.well-known/openid-configuration must resolve) (this feature is experimental) -server.oidc_authentication.redirect_urlstringhttps://localhost:8080/oidc/v1/callbacksets OIDC redirect URL (base HTTP URL, likely your load balancer, must route to the path /oidc/v1/callback) (this feature is experimental) +server.oidc_authentication.redirect_urlstringhttps://localhost:8080/oidc/v1/callbacksets OIDC redirect URL via a URL string or a JSON string containing a required `redirect_urls` key with an object that maps from region keys to URL strings (URLs should point to your load balancer and must route to the path /oidc/v1/callback) (this feature is experimental) server.oidc_authentication.scopesstringopenidsets OIDC scopes to include with authentication request (space delimited list of strings, required to start with `openid`) (this feature is experimental) server.rangelog.ttlduration720h0m0sif nonzero, range log entries older than this duration are deleted every 10m0s. Should not be lowered below 24 hours. server.remote_debugging.modestringlocalset to enable remote debugging, localhost-only or disable (any, local, off) diff --git a/pkg/ccl/oidcccl/BUILD.bazel b/pkg/ccl/oidcccl/BUILD.bazel index fabb3f857950..94348715b80f 100644 --- a/pkg/ccl/oidcccl/BUILD.bazel +++ b/pkg/ccl/oidcccl/BUILD.bazel @@ -11,6 +11,7 @@ go_library( visibility = ["//visibility:public"], deps = [ "//pkg/ccl/utilccl", + "//pkg/roachpb", "//pkg/server", "//pkg/server/serverpb", "//pkg/server/telemetry", @@ -30,7 +31,10 @@ go_library( go_test( name = "oidcccl_test", - srcs = ["authentication_oidc_test.go"], + srcs = [ + "authentication_oidc_test.go", + "settings_test.go", + ], embed = [":oidcccl"], deps = [ "//pkg/base", diff --git a/pkg/ccl/oidcccl/authentication_oidc.go b/pkg/ccl/oidcccl/authentication_oidc.go index fb175a55ef6c..f8656dd45446 100644 --- a/pkg/ccl/oidcccl/authentication_oidc.go +++ b/pkg/ccl/oidcccl/authentication_oidc.go @@ -16,6 +16,7 @@ import ( "strings" "github.com/cockroachdb/cockroach/pkg/ccl/utilccl" + "github.com/cockroachdb/cockroach/pkg/roachpb" "github.com/cockroachdb/cockroach/pkg/server" "github.com/cockroachdb/cockroach/pkg/server/telemetry" "github.com/cockroachdb/cockroach/pkg/settings/cluster" @@ -24,6 +25,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/util/log" "github.com/cockroachdb/cockroach/pkg/util/syncutil" "github.com/cockroachdb/cockroach/pkg/util/uuid" + "github.com/cockroachdb/errors" "github.com/coreos/go-oidc" "golang.org/x/oauth2" ) @@ -123,16 +125,16 @@ type oidcAuthenticationServer struct { } type oidcAuthenticationConf struct { - clientID string - clientSecret string - redirectURL string - providerURL string - scopes string - enabled bool - claimJSONKey string - principalRegex *regexp.Regexp - buttonText string - autoLogin bool + clientID string + clientSecret string + redirectURLConf redirectURLConf + providerURL string + scopes string + enabled bool + claimJSONKey string + principalRegex *regexp.Regexp + buttonText string + autoLogin bool } // GetUIConf is used to extract certain parts of the OIDC @@ -147,23 +149,31 @@ func (s *oidcAuthenticationServer) GetOIDCConf() ui.OIDCUIConf { } } -func reloadConfig(ctx context.Context, server *oidcAuthenticationServer, st *cluster.Settings) { +func reloadConfig( + ctx context.Context, + server *oidcAuthenticationServer, + locality roachpb.Locality, + st *cluster.Settings, +) { server.mutex.Lock() defer server.mutex.Unlock() - reloadConfigLocked(ctx, server, st) + reloadConfigLocked(ctx, server, locality, st) } func reloadConfigLocked( - ctx context.Context, server *oidcAuthenticationServer, st *cluster.Settings, + ctx context.Context, + server *oidcAuthenticationServer, + locality roachpb.Locality, + st *cluster.Settings, ) { conf := oidcAuthenticationConf{ - clientID: OIDCClientID.Get(&st.SV), - clientSecret: OIDCClientSecret.Get(&st.SV), - redirectURL: OIDCRedirectURL.Get(&st.SV), - providerURL: OIDCProviderURL.Get(&st.SV), - scopes: OIDCScopes.Get(&st.SV), - claimJSONKey: OIDCClaimJSONKey.Get(&st.SV), - enabled: OIDCEnabled.Get(&st.SV), + clientID: OIDCClientID.Get(&st.SV), + clientSecret: OIDCClientSecret.Get(&st.SV), + redirectURLConf: mustParseOIDCRedirectURL(OIDCRedirectURL.Get(&st.SV)), + providerURL: OIDCProviderURL.Get(&st.SV), + scopes: OIDCScopes.Get(&st.SV), + claimJSONKey: OIDCClaimJSONKey.Get(&st.SV), + enabled: OIDCEnabled.Get(&st.SV), // The success of this line is guaranteed by the validation of the setting principalRegex: regexp.MustCompile(OIDCPrincipalRegex.Get(&st.SV)), buttonText: OIDCButtonText.Get(&st.SV), @@ -195,13 +205,19 @@ func reloadConfigLocked( return } - // Validation of the scope setting will require that we have the `openid` scope + // Validation of the scope setting will require that we have the `openid` scope. scopesForOauth := strings.Split(server.conf.scopes, " ") + redirectURL, err := getRegionSpecificRedirectURL(locality, server.conf.redirectURLConf) + if err != nil { + log.Warningf(ctx, "unable to initialize OIDC provider, disabling OIDC: %v", err) + return + } + server.oauth2Config = oauth2.Config{ ClientID: server.conf.clientID, ClientSecret: server.conf.clientSecret, - RedirectURL: server.conf.redirectURL, + RedirectURL: redirectURL, Endpoint: provider.Endpoint(), Scopes: scopesForOauth, @@ -209,7 +225,27 @@ func reloadConfigLocked( server.verifier = provider.Verifier(&oidc.Config{ClientID: server.conf.clientID}) server.initialized = true - log.Infof(ctx, "initialized oidc server") + log.Infof(ctx, "initialized OIDC server") +} + +// getRegionSpecificRedirectURL will query the localities and see if we have +// regions configured. If we do, it will ask the configuration for a +// region-specific redirect, otherwise it will query for one without a region. +func getRegionSpecificRedirectURL(locality roachpb.Locality, conf redirectURLConf) (string, error) { + if len(locality.Tiers) > 0 { + region, containsRegion := locality.Find("region") + if containsRegion { + if redirectURL, ok := conf.getForRegion(region); ok { + return redirectURL, nil + } + return "", errors.Newf("OIDC: no matching redirect URL found for region %s", region) + } + } + s, ok := conf.get() + if !ok { + return "", errors.New("OIDC: redirect URL config expects region setting, which is unset") + } + return s, nil } // ConfigureOIDC attaches handlers to the server `mux` that @@ -221,6 +257,7 @@ func reloadConfigLocked( var ConfigureOIDC = func( serverCtx context.Context, st *cluster.Settings, + locality roachpb.Locality, mux *http.ServeMux, userLoginFromSSO func(ctx context.Context, username string) (*http.Cookie, error), ambientCtx log.AmbientContext, @@ -239,7 +276,7 @@ var ConfigureOIDC = func( defer oidcAuthentication.mutex.Unlock() if oidcAuthentication.enabled && !oidcAuthentication.initialized { - reloadConfigLocked(ctx, oidcAuthentication, st) + reloadConfigLocked(ctx, oidcAuthentication, locality, st) } if !oidcAuthentication.enabled { @@ -348,7 +385,7 @@ var ConfigureOIDC = func( defer oidcAuthentication.mutex.Unlock() if oidcAuthentication.enabled && !oidcAuthentication.initialized { - reloadConfigLocked(ctx, oidcAuthentication, st) + reloadConfigLocked(ctx, oidcAuthentication, locality, st) } if !oidcAuthentication.enabled { @@ -371,77 +408,37 @@ var ConfigureOIDC = func( ) }) - reloadConfig(serverCtx, oidcAuthentication, st) + reloadConfig(serverCtx, oidcAuthentication, locality, st) OIDCEnabled.SetOnChange(&st.SV, func() { - reloadConfig( - ambientCtx.AnnotateCtx(context.Background()), - oidcAuthentication, - st, - ) + reloadConfig(ambientCtx.AnnotateCtx(context.Background()), oidcAuthentication, locality, st) }) OIDCClientID.SetOnChange(&st.SV, func() { - reloadConfig( - ambientCtx.AnnotateCtx(context.Background()), - oidcAuthentication, - st, - ) + reloadConfig(ambientCtx.AnnotateCtx(context.Background()), oidcAuthentication, locality, st) }) OIDCClientSecret.SetOnChange(&st.SV, func() { - reloadConfig( - ambientCtx.AnnotateCtx(context.Background()), - oidcAuthentication, - st, - ) + reloadConfig(ambientCtx.AnnotateCtx(context.Background()), oidcAuthentication, locality, st) }) OIDCRedirectURL.SetOnChange(&st.SV, func() { - reloadConfig( - ambientCtx.AnnotateCtx(context.Background()), - oidcAuthentication, - st, - ) + reloadConfig(ambientCtx.AnnotateCtx(context.Background()), oidcAuthentication, locality, st) }) OIDCProviderURL.SetOnChange(&st.SV, func() { - reloadConfig( - ambientCtx.AnnotateCtx(context.Background()), - oidcAuthentication, - st, - ) + reloadConfig(ambientCtx.AnnotateCtx(context.Background()), oidcAuthentication, locality, st) }) OIDCScopes.SetOnChange(&st.SV, func() { - reloadConfig( - ambientCtx.AnnotateCtx(context.Background()), - oidcAuthentication, - st, - ) + reloadConfig(ambientCtx.AnnotateCtx(context.Background()), oidcAuthentication, locality, st) }) OIDCClaimJSONKey.SetOnChange(&st.SV, func() { - reloadConfig( - ambientCtx.AnnotateCtx(context.Background()), - oidcAuthentication, - st, - ) + reloadConfig(ambientCtx.AnnotateCtx(context.Background()), oidcAuthentication, locality, st) }) OIDCPrincipalRegex.SetOnChange(&st.SV, func() { - reloadConfig( - ambientCtx.AnnotateCtx(context.Background()), - oidcAuthentication, - st, - ) + reloadConfig(ambientCtx.AnnotateCtx(context.Background()), oidcAuthentication, locality, st) }) OIDCButtonText.SetOnChange(&st.SV, func() { - reloadConfig( - ambientCtx.AnnotateCtx(context.Background()), - oidcAuthentication, - st, - ) + reloadConfig(ambientCtx.AnnotateCtx(context.Background()), oidcAuthentication, locality, st) }) OIDCAutoLogin.SetOnChange(&st.SV, func() { - reloadConfig( - ambientCtx.AnnotateCtx(context.Background()), - oidcAuthentication, - st, - ) + reloadConfig(ambientCtx.AnnotateCtx(context.Background()), oidcAuthentication, locality, st) }) return oidcAuthentication, nil diff --git a/pkg/ccl/oidcccl/authentication_oidc_test.go b/pkg/ccl/oidcccl/authentication_oidc_test.go index e4a3d809efca..fa2957cdef7e 100644 --- a/pkg/ccl/oidcccl/authentication_oidc_test.go +++ b/pkg/ccl/oidcccl/authentication_oidc_test.go @@ -287,3 +287,81 @@ func TestOIDCStateEncodeDecode(t *testing.T) { t.Fatal("state didn't match when decoded") } } + +func Test_getRegionSpecificRedirectURL(t *testing.T) { + type args struct { + locality roachpb.Locality + conf redirectURLConf + } + tests := []struct { + name string + args args + want string + wantErr bool + }{ + // Single redirect configurations + {"single redirect: empty locality", args{ + locality: roachpb.Locality{}, + conf: redirectURLConf{sru: &singleRedirectURL{RedirectURL: "correct.example.com"}}, + }, "correct.example.com", false}, + {"single redirect: locality with no region", args{ + locality: roachpb.Locality{Tiers: []roachpb.Tier{{Key: "who", Value: "knows"}}}, + conf: redirectURLConf{sru: &singleRedirectURL{RedirectURL: "correct.example.com"}}, + }, "correct.example.com", false}, + {"single redirect: locality with region", args{ + locality: roachpb.Locality{Tiers: []roachpb.Tier{{Key: "region", Value: "us-east-1"}}}, + conf: redirectURLConf{sru: &singleRedirectURL{RedirectURL: "correct.example.com"}}, + }, "correct.example.com", false}, + // Multi-region configurations + {"multi-region config: empty locality", args{ + locality: roachpb.Locality{}, + conf: redirectURLConf{mrru: &multiRegionRedirectURLs{RedirectURLs: nil}}, + }, "", true}, + {"multi-region config: locality with no region", args{ + locality: roachpb.Locality{Tiers: []roachpb.Tier{{Key: "who", Value: "knows"}}}, + conf: redirectURLConf{mrru: &multiRegionRedirectURLs{RedirectURLs: nil}}, + }, "", true}, + {"multi-region config: locality with region but no corresponding URL in config", args{ + locality: roachpb.Locality{Tiers: []roachpb.Tier{{Key: "region", Value: "us-east-1"}}}, + conf: redirectURLConf{mrru: &multiRegionRedirectURLs{RedirectURLs: nil}}, + }, "", true}, + {"multi-region config: locality with region and corresponding url", args{ + locality: roachpb.Locality{Tiers: []roachpb.Tier{{Key: "region", Value: "us-east-1"}}}, + conf: redirectURLConf{mrru: &multiRegionRedirectURLs{ + RedirectURLs: map[string]string{ + "us-east-1": "correct.example.com", + "us-west-2": "incorrect.example.com", + }, + }}, + }, "correct.example.com", false}, + {"multi-region config: locality with region and corresponding url 2", args{ + locality: roachpb.Locality{Tiers: []roachpb.Tier{{Key: "region", Value: "us-west-2"}}}, + conf: redirectURLConf{mrru: &multiRegionRedirectURLs{ + RedirectURLs: map[string]string{ + "us-east-1": "incorrect.example.com", + "us-west-2": "correct.example.com", + }, + }}, + }, "correct.example.com", false}, + {"multi-region config: locality with unexpected region", args{ + locality: roachpb.Locality{Tiers: []roachpb.Tier{{Key: "region", Value: "us-central-2"}}}, + conf: redirectURLConf{mrru: &multiRegionRedirectURLs{ + RedirectURLs: map[string]string{ + "us-east-1": "incorrect.example.com", + "us-west-2": "correct.example.com", + }, + }}, + }, "", true}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, err := getRegionSpecificRedirectURL(tt.args.locality, tt.args.conf) + if tt.wantErr { + require.Error(t, err) + } else { + require.NoError(t, err) + } + require.Equal(t, got, tt.want) + }) + } +} diff --git a/pkg/ccl/oidcccl/settings.go b/pkg/ccl/oidcccl/settings.go index 1e2f321aaa49..b512eb205e8c 100644 --- a/pkg/ccl/oidcccl/settings.go +++ b/pkg/ccl/oidcccl/settings.go @@ -9,6 +9,8 @@ package oidcccl import ( + "bytes" + "encoding/json" "net/url" "regexp" "strings" @@ -33,7 +35,7 @@ const ( OIDCAutoLoginSettingName = baseOIDCSettingName + "autologin" ) -// OIDCEnabled enables or disabled OIDC login for the DB Console +// OIDCEnabled enables or disabled OIDC login for the DB Console. var OIDCEnabled = func() *settings.BoolSetting { s := settings.RegisterBoolSetting( OIDCEnabledSettingName, @@ -44,7 +46,7 @@ var OIDCEnabled = func() *settings.BoolSetting { return s }() -// OIDCClientID is the OIDC client id +// OIDCClientID is the OIDC client id. var OIDCClientID = func() *settings.StringSetting { s := settings.RegisterStringSetting( OIDCClientIDSettingName, @@ -55,7 +57,7 @@ var OIDCClientID = func() *settings.StringSetting { return s }() -// OIDCClientSecret is the OIDC client secret +// OIDCClientSecret is the OIDC client secret. var OIDCClientSecret = func() *settings.StringSetting { s := settings.RegisterStringSetting( OIDCClientSecretSettingName, @@ -66,34 +68,130 @@ var OIDCClientSecret = func() *settings.StringSetting { return s }() -// OIDCRedirectURL is the cluster URL to redirect to after OIDC auth completes +type redirectURLConf struct { + mrru *multiRegionRedirectURLs + sru *singleRedirectURL +} + +// getForRegion is used when we have a cluster with regions configured. +// Both configuration types can return valid responses here. +func (conf *redirectURLConf) getForRegion(region string) (string, bool) { + if conf.mrru != nil { + s, ok := conf.mrru.RedirectURLs[region] + return s, ok + } + if conf.sru != nil { + return conf.sru.RedirectURL, true + } + return "", false +} + +// get is used in the case where regions are not configured on the cluster. +// Only a singleRedirectURL configuration can return a valid result here. +func (conf *redirectURLConf) get() (string, bool) { + if conf.sru != nil { + return conf.sru.RedirectURL, true + } + return "", false +} + +// multiRegionRedirectURLs is a struct that defines a valid JSON body for the +// OIDCRedirectURL cluster setting in multi-region environments. +type multiRegionRedirectURLs struct { + RedirectURLs map[string]string `json:"redirect_urls"` +} + +// singleRedirectURL is a struct containing a string that stores a single +// redirect URL in the case where the configuration only has a single one. +type singleRedirectURL struct { + RedirectURL string +} + +// mustParseOIDCRedirectURL will read in a string that's from the +// `OIDCRedirectURL` setting. We know from the validation that runs on that +// setting that any value that's not valid JSON that deserializes into the +// `multiRegionRedirectURLs` struct will be a URL. +func mustParseOIDCRedirectURL(s string) redirectURLConf { + var mrru = multiRegionRedirectURLs{} + decoder := json.NewDecoder(bytes.NewReader([]byte(s))) + err := decoder.Decode(&mrru) + if err != nil { + return redirectURLConf{sru: &singleRedirectURL{RedirectURL: s}} + } + return redirectURLConf{mrru: &mrru} +} + +func validateOIDCRedirectURL(values *settings.Values, s string) error { + var mrru = multiRegionRedirectURLs{} + + var jsonCheck json.RawMessage + if json.Unmarshal([]byte(s), &jsonCheck) != nil { + // If we know the string is *not* valid JSON, fall back to assuming basic URL + // string to use the simple redirect URL configuration option + if _, err := url.Parse(s); err != nil { + return errors.Wrap(err, "OIDC redirect URL not valid") + } + return nil + } + + decoder := json.NewDecoder(bytes.NewReader([]byte(s))) + decoder.DisallowUnknownFields() + if err := decoder.Decode(&mrru); err != nil { + return errors.Wrap(err, "OIDC redirect JSON not valid") + } + for _, route := range mrru.RedirectURLs { + if _, err := url.Parse(route); err != nil { + return errors.Wrapf(err, "OIDC redirect JSON contains invalid URL: %s", route) + } + } + return nil +} + +// OIDCRedirectURL is the cluster URL to redirect to after OIDC auth completes. +// This can be set to a simple string that Go can parse as a valid URL (although +// it's incredibly permissive) or as a string that can be parsed as valid JSON +// and deserialized into an instance of multiRegionRedirectURLs or +// singleRedirectURL defined above which implement the `callbackRedirecter` +// interface. In the latter case, it is expected that each node will use a +// callback URL that matches its own `region` locality tag. +// +// Example valid values: +// - 'https://cluster.example.com:8080/oidc/v1/callback' +// - '{ +// "redirect_urls": { +// "us-east-1": "https://localhost:8080/oidc/v1/callback", +// "eu-west-1": "example.com" +// } +// }' +// +// In a multi-region cluster where this setting is set to a URL string, we will +// use the same callback URL on all auth requests. In a multi-region setting +// where the cluster's region is not listed in the `redirect_urls` object, we +// will use the required `default_url` callback URL. var OIDCRedirectURL = func() *settings.StringSetting { s := settings.RegisterValidatedStringSetting( OIDCRedirectURLSettingName, - "sets OIDC redirect URL (base HTTP URL, likely your load balancer, must route to the path /oidc/v1/callback) (this feature is experimental)", + "sets OIDC redirect URL via a URL string or a JSON string containing a required "+ + "`redirect_urls` key with an object that maps from region keys to URL strings "+ + "(URLs should point to your load balancer and must route to the path /oidc/v1/callback) "+ + "(this feature is experimental)", "https://localhost:8080/oidc/v1/callback", - func(values *settings.Values, s string) error { - _, err := url.Parse(s) - if err != nil { - return err - } - return nil - }, + validateOIDCRedirectURL, ) s.SetReportable(true) s.SetVisibility(settings.Public) return s }() -// OIDCProviderURL is the location of the OIDC discovery document for the auth provider +// OIDCProviderURL is the location of the OIDC discovery document for the auth +// provider. var OIDCProviderURL = func() *settings.StringSetting { s := settings.RegisterValidatedStringSetting( OIDCProviderURLSettingName, "sets OIDC provider URL ({provider_url}/.well-known/openid-configuration must resolve) (this feature is experimental)", "", func(values *settings.Values, s string) error { - _, err := url.Parse(s) - if err != nil { + if _, err := url.Parse(s); err != nil { return err } return nil @@ -104,7 +202,7 @@ var OIDCProviderURL = func() *settings.StringSetting { return s }() -// OIDCScopes contains the list of scopes to request from the auth provider +// OIDCScopes contains the list of scopes to request from the auth provider. var OIDCScopes = func() *settings.StringSetting { s := settings.RegisterValidatedStringSetting( OIDCScopesSettingName, @@ -123,7 +221,7 @@ var OIDCScopes = func() *settings.StringSetting { return s }() -// OIDCClaimJSONKey is the key of the claim to extract from the OIDC id_token +// OIDCClaimJSONKey is the key of the claim to extract from the OIDC id_token. var OIDCClaimJSONKey = func() *settings.StringSetting { s := settings.RegisterStringSetting( OIDCClaimJSONKeySettingName, @@ -134,8 +232,8 @@ var OIDCClaimJSONKey = func() *settings.StringSetting { return s }() -// OIDCPrincipalRegex is a regular expression to apply to the OIDC id_token claim value to conver -// it to a DB principal +// OIDCPrincipalRegex is a regular expression to apply to the OIDC id_token +// claim value to conver it to a DB principal. var OIDCPrincipalRegex = func() *settings.StringSetting { s := settings.RegisterValidatedStringSetting( OIDCPrincipalRegexSettingName, @@ -143,8 +241,7 @@ var OIDCPrincipalRegex = func() *settings.StringSetting { "translate to SQL user (golang regex format, must include 1 grouping to extract) (this feature is experimental)", "(.+)", func(values *settings.Values, s string) error { - _, err := regexp.Compile(s) - if err != nil { + if _, err := regexp.Compile(s); err != nil { return errors.Wrapf(err, "unable to initialize %s setting, regex does not compile", OIDCPrincipalRegexSettingName) } @@ -155,7 +252,8 @@ var OIDCPrincipalRegex = func() *settings.StringSetting { return s }() -// OIDCButtonText is a string to display on the button in the DB Console to login with OIDC +// OIDCButtonText is a string to display on the button in the DB Console to +// login with OIDC. var OIDCButtonText = func() *settings.StringSetting { s := settings.RegisterStringSetting( OIDCButtonTextSettingName, @@ -166,7 +264,8 @@ var OIDCButtonText = func() *settings.StringSetting { return s }() -// OIDCAutoLogin is a boolean that enables automatic redirection to OIDC auth in the DB Console +// OIDCAutoLogin is a boolean that enables automatic redirection to OIDC auth in +// the DB Console. var OIDCAutoLogin = func() *settings.BoolSetting { s := settings.RegisterBoolSetting( OIDCAutoLoginSettingName, diff --git a/pkg/ccl/oidcccl/settings_test.go b/pkg/ccl/oidcccl/settings_test.go new file mode 100644 index 000000000000..ef1503214e9b --- /dev/null +++ b/pkg/ccl/oidcccl/settings_test.go @@ -0,0 +1,69 @@ +// Copyright 2020 The Cockroach Authors. +// +// Licensed as a CockroachDB Enterprise file under the Cockroach Community +// License (the "License"); you may not use this file except in compliance with +// the License. You may obtain a copy of the License at +// +// https://github.com/cockroachdb/cockroach/blob/master/licenses/CCL.txt + +package oidcccl + +import ( + "testing" + + "github.com/stretchr/testify/require" +) + +func Test_validateAndParseOIDCRedirectURL(t *testing.T) { + tests := []struct { + name string + setting string + wantErr bool + expectMultiRegion *multiRegionRedirectURLs + expectSingleURL *singleRedirectURL + }{ + {"empty string", + "", false, + nil, &singleRedirectURL{RedirectURL: ""}}, + {"URL string", + "https://some.long.example.com/a/b/c/d/#", false, + nil, + &singleRedirectURL{RedirectURL: "https://some.long.example.com/a/b/c/d/#"}}, + {"json contains unexpected keys", + "{\"wrong\":\"key\"}", true, + nil, nil}, + {"json with extra nonsense", + "{\"redirect_urls\": {\"key\":\"example.com\"},\"wrong\":\"key\"}", true, + nil, nil}, + {"json with redirect URLs wrong type", + "{\"redirect_urls\":\"key\"}", true, + nil, nil}, + {"json with invalid redirect URL", + "{\"redirect_urls\":{\"key\":\"{}{}{}http://example.com\"}}", true, + nil, nil}, + {"json with redirect URLs correct type", + "{\"redirect_urls\": {\"key\":\"http://example.com\"}}", false, + &multiRegionRedirectURLs{ + map[string]string{"key": "http://example.com"}, + }, + nil}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if err := validateOIDCRedirectURL(nil, tt.setting); (err != nil) != tt.wantErr { + t.Errorf("validateOIDCRedirectURL() error = %v, wantErr %v", err, tt.wantErr) + } + if !tt.wantErr { + parsedRedirect := mustParseOIDCRedirectURL(tt.setting) + + if tt.expectMultiRegion != nil { + require.Equal(t, parsedRedirect.mrru, tt.expectMultiRegion) + + } + if tt.expectSingleURL != nil { + require.Equal(t, parsedRedirect.sru, tt.expectSingleURL) + } + } + }) + } +} diff --git a/pkg/server/authentication.go b/pkg/server/authentication.go index 0d12ffc8b79c..fcbde38e6cf3 100644 --- a/pkg/server/authentication.go +++ b/pkg/server/authentication.go @@ -21,6 +21,7 @@ import ( "strconv" "time" + "github.com/cockroachdb/cockroach/pkg/roachpb" "github.com/cockroachdb/cockroach/pkg/security" "github.com/cockroachdb/cockroach/pkg/server/serverpb" "github.com/cockroachdb/cockroach/pkg/settings" @@ -76,6 +77,7 @@ type OIDC interface { var ConfigureOIDC = func( ctx context.Context, st *cluster.Settings, + locality roachpb.Locality, mux *http.ServeMux, userLoginFromSSO func(ctx context.Context, username string) (*http.Cookie, error), ambientCtx log.AmbientContext, diff --git a/pkg/server/server.go b/pkg/server/server.go index b686374270d1..85fd464921a5 100644 --- a/pkg/server/server.go +++ b/pkg/server/server.go @@ -1676,7 +1676,10 @@ func (s *Server) PreStart(ctx context.Context) error { // OIDC Configuration must happen prior to the UI Handler being defined below so that we have // the system settings initialized for it to pick up from the oidcAuthenticationServer. - oidc, err := ConfigureOIDC(ctx, s.ClusterSettings(), &s.mux, s.authentication.UserLoginFromSSO, s.cfg.AmbientCtx, s.ClusterID()) + oidc, err := ConfigureOIDC( + ctx, s.ClusterSettings(), s.cfg.Locality, + &s.mux, s.authentication.UserLoginFromSSO, s.cfg.AmbientCtx, s.ClusterID(), + ) if err != nil { return err } From 73d3246dbedf5a0234fe6bc12285842d4c301372 Mon Sep 17 00:00:00 2001 From: Jordan Lewis Date: Fri, 4 Sep 2020 15:37:24 -0400 Subject: [PATCH 3/3] sql: improve error message for search_path with commas It's easy to accidentally surround your search path with quotes when setting it, because you'd think that most things in `SET` syntax are quoted. But you are not supposed to quote things in set search_path, and it can lead to confusing scenarios. Now, if you try to set search_path to a string containing a comma, which we don't support anyway, the error message will be a bit friendlier. Release note (sql change): improve error message when people use set search_path incorrectly, or with a schema that legitimately has a comma in its name Release justification: error-message-only change --- pkg/sql/vars.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/pkg/sql/vars.go b/pkg/sql/vars.go index e6ace038c037..bf96700d97c8 100644 --- a/pkg/sql/vars.go +++ b/pkg/sql/vars.go @@ -843,8 +843,10 @@ var varGen = map[string]sessionVar{ // TODO(knz): if/when we want to support this, we'll need to change // the interface between GetStringVal() and Set() to take string // arrays instead of a single string. - return "", unimplemented.Newf("schema names containing commas in search_path", - "schema name %q not supported in search_path", s) + return "", + errors.WithHintf(unimplemented.NewWithIssuef(53971, + `schema name %q has commas so is not supported in search_path.`, s), + `Did you mean to omit quotes? SET search_path = %s`, s) } buf.WriteString(comma) buf.WriteString(s)