Skip to content

Commit

Permalink
auth: add region-based callback URLs for OIDC
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@dhartunian
dhartunian committed Jan 22, 2021
1 parent 10ba90c commit 32e1c77
Show file tree
Hide file tree
Showing 8 changed files with 354 additions and 102 deletions.
2 changes: 1 addition & 1 deletion docs/generated/settings/settings.html
View file
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@
<tr><td><code>server.oidc_authentication.enabled</code></td><td>boolean</td><td><code>false</code></td><td>enables or disabled OIDC login for the DB Console (this feature is experimental)</td></tr>
<tr><td><code>server.oidc_authentication.principal_regex</code></td><td>string</td><td><code>(.+)</code></td><td>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)</td></tr>
<tr><td><code>server.oidc_authentication.provider_url</code></td><td>string</td><td><code></code></td><td>sets OIDC provider URL ({provider_url}/.well-known/openid-configuration must resolve) (this feature is experimental)</td></tr>
<tr><td><code>server.oidc_authentication.redirect_url</code></td><td>string</td><td><code>https://localhost:8080/oidc/v1/callback</code></td><td>sets OIDC redirect URL (base HTTP URL, likely your load balancer, must route to the path /oidc/v1/callback) (this feature is experimental)</td></tr>
<tr><td><code>server.oidc_authentication.redirect_url</code></td><td>string</td><td><code>https://localhost:8080/oidc/v1/callback</code></td><td>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)</td></tr>
<tr><td><code>server.oidc_authentication.scopes</code></td><td>string</td><td><code>openid</code></td><td>sets OIDC scopes to include with authentication request (space delimited list of strings, required to start with `openid`) (this feature is experimental)</td></tr>
<tr><td><code>server.rangelog.ttl</code></td><td>duration</td><td><code>720h0m0s</code></td><td>if nonzero, range log entries older than this duration are deleted every 10m0s. Should not be lowered below 24 hours.</td></tr>
<tr><td><code>server.remote_debugging.mode</code></td><td>string</td><td><code>local</code></td><td>set to enable remote debugging, localhost-only or disable (any, local, off)</td></tr>
Expand Down
6 changes: 5 additions & 1 deletion pkg/ccl/oidcccl/BUILD.bazel
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ go_library(
visibility = ["//visibility:public"],
deps = [
"//pkg/ccl/utilccl",
"//pkg/roachpb",
"//pkg/server",
"//pkg/server/serverpb",
"//pkg/server/telemetry",
Expand All @@ -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",
Expand Down
149 changes: 73 additions & 76 deletions pkg/ccl/oidcccl/authentication_oidc.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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"
)
Expand Down Expand Up @@ -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
Expand All @@ -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),
Expand Down Expand Up @@ -195,21 +205,47 @@ 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,
}

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
Expand All @@ -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,
Expand All @@ -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 {
Expand Down Expand Up @@ -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 {
Expand All @@ -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
Expand Down
78 changes: 78 additions & 0 deletions pkg/ccl/oidcccl/authentication_oidc_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
})
}
}
Loading

0 comments on commit 32e1c77

Please sign in to comment.