Merge #147706

147706: oidcccl, server: add role synchronisation for DB Console OIDC logins r=souravcrl a=shriramters

Previously, the OIDC callback (`/oidc/v1/callback`) authenticated
Admin-UI users but did not reconcile their IdP groups with SQL roles.
This was inadequate because operators who already rely on automatic
GRANT/REVOKE for JWT and LDAP flows had no equivalent mechanism for
OIDC log-ins, leading to inconsistent privileges and manual role
management.
To address this, this patch introduces group-to-role synchronisation
for OIDC:

* Adds three cluster settings
  `server.oidc_authentication.authorization.enabled`,
  `server.oidc_authentication.group_claim`, and
  `server.oidc_authentication.userinfo_group_key`, mirroring the JWT
  knobs.
* Parses the configured claim from the verified ID-token (array or
  string), normalises, dedupes and sorts it.
* Falls back to the user-info endpoint when the claim is absent and a
  group key is configured.
* Converts groups to validated SQL usernames and invokes
  `EnsureUserOnlyBelongsToRoles`.
* Threads the node’s `*sql.ExecutorConfig` into the OIDC stack.

Epic: CRDB-48763
Fixes: CRDB-51161

Release note (security update): CockroachDB can now synchronise SQL
role membership from the groups claim provided by an OpenID Connect
(OIDC) Identity Provider when
`server.oidc_authentication.authorization.enabled = true`.
.
At login the DB Console gathers the `groups` claim from both the
verified ID token and, when available, the access token (if it is a
JWT).  Any groups found in either token are combined and
de-duplicated.  If no claim is present in either token, the server
will query the provider's `/userinfo` endpoint and extract groups from
there (using `server.oidc_authentication.userinfo_group_key`) as a
final fallback.
.
The resulting list of groups is normalised to SQL role names and
compared to the user’s current role memberships.  Any newly required
roles are GRANTed and any stale ones are REVOKEd, matching the
behaviour already available for JWT and LDAP-based role
synchronisation.

Co-authored-by: Shriram Ravindranathan <[email protected]>

Author: craig[bot]

pkg/ccl/oidcccl/BUILD.bazel

@@ -4,6 +4,7 @@ go_library(
     name = "oidcccl",
     srcs = [
         "authentication_oidc.go",
+        "authorization_oidc.go",
         "claim_match.go",
         "settings.go",
         "state.go",
@@ -12,6 +13,7 @@ go_library(
     visibility = ["//visibility:public"],
     deps = [
         "//pkg/ccl/jwtauthccl",
+        "//pkg/ccl/securityccl/jwthelper",
         "//pkg/ccl/utilccl",
         "//pkg/roachpb",
         "//pkg/security/username",
@@ -21,6 +23,7 @@ go_library(
         "//pkg/server/telemetry",
         "//pkg/settings",
         "//pkg/settings/cluster",
+        "//pkg/sql",
         "//pkg/sql/pgwire",
         "//pkg/sql/pgwire/identmap",
         "//pkg/ui",
@@ -31,6 +34,7 @@ go_library(
         "//pkg/util/uuid",
         "@com_github_cockroachdb_errors//:errors",
         "@com_github_coreos_go_oidc//:go-oidc",
+        "@com_github_lestrrat_go_jwx_v2//jwt",
         "@org_golang_x_oauth2//:oauth2",
     ],
 )
@@ -40,6 +44,7 @@ go_test(
     size = "small",
     srcs = [
         "authentication_oidc_test.go",
+        "authorization_oidc_test.go",
         "main_test.go",
         "settings_test.go",
     ],
@@ -64,7 +69,11 @@ go_test(
         "//pkg/util/leaktest",
         "//pkg/util/log",
         "//pkg/util/randutil",
+        "@com_github_cockroachdb_errors//:errors",
         "@com_github_coreos_go_oidc//:go-oidc",
+        "@com_github_lestrrat_go_jwx_v2//jwa",
+        "@com_github_lestrrat_go_jwx_v2//jwk",
+        "@com_github_lestrrat_go_jwx_v2//jwt",
         "@com_github_stretchr_testify//require",
         "@org_golang_x_oauth2//:oauth2",
     ],

pkg/ccl/oidcccl/authentication_oidc.go

@@ -19,12 +19,13 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/ccl/jwtauthccl"
 	"github.com/cockroachdb/cockroach/pkg/ccl/utilccl"
 	"github.com/cockroachdb/cockroach/pkg/roachpb"
-	"github.com/cockroachdb/cockroach/pkg/security/username"
+	secuser "github.com/cockroachdb/cockroach/pkg/security/username"
 	"github.com/cockroachdb/cockroach/pkg/server"
 	"github.com/cockroachdb/cockroach/pkg/server/authserver"
 	"github.com/cockroachdb/cockroach/pkg/server/serverpb"
 	"github.com/cockroachdb/cockroach/pkg/server/telemetry"
 	"github.com/cockroachdb/cockroach/pkg/settings/cluster"
+	"github.com/cockroachdb/cockroach/pkg/sql"
 	"github.com/cockroachdb/cockroach/pkg/sql/pgwire"
 	"github.com/cockroachdb/cockroach/pkg/sql/pgwire/identmap"
 	"github.com/cockroachdb/cockroach/pkg/ui"
@@ -34,6 +35,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/util/uuid"
 	"github.com/cockroachdb/errors"
 	oidc "github.com/coreos/go-oidc"
+	"github.com/lestrrat-go/jwx/v2/jwt"
 	"golang.org/x/oauth2"
 )
 
@@ -131,6 +133,7 @@ type oidcAuthenticationServer struct {
 	// to help us gracefully recover from auth provider downtime without operator intervention.
 	enabled     bool
 	initialized bool
+	execCfg     *sql.ExecutorConfig
 }
 
 type oidcAuthenticationConf struct {
@@ -152,6 +155,9 @@ type oidcAuthenticationConf struct {
 	generateJWTAuthTokenSQLPort  int64
 	providerCustomCA             string
 	httpClient                   *httputil.Client
+	authZEnabled                 bool
+	groupClaim                   string
+	userinfoGroupKey             string
 }
 
 // GetOIDCConf is used to extract certain parts of the OIDC
@@ -187,37 +193,107 @@ type oidcManager struct {
 	oauth2Config *oauth2.Config
 	verifier     *oidc.IDTokenVerifier
 	httpClient   *httputil.Client
+	provider     *oidc.Provider
 }
 
-func (o *oidcManager) ExchangeVerifyGetClaims(
-	ctx context.Context, code string, idTokenKey string,
-) (map[string]json.RawMessage, error) {
+// ExchangeVerifyGetTokenInfo exchanges the auth-code, verifies the ID-token,
+// and extracts claims from both the ID and Access tokens if they are JWTs.
+// Access tokens are only processed if OIDC authorization is enabled for the
+// cluster.
+func (o *oidcManager) ExchangeVerifyGetTokenInfo(
+	ctx context.Context, code, idTokenKey string, authZEnabled bool,
+) (
+	idTokenClaims, accessTokenClaims map[string]json.RawMessage,
+	rawIDToken, rawAccessToken string,
+	err error,
+) {
 	credentials, err := o.Exchange(ctx, code)
 	if err != nil {
-		log.Errorf(ctx, "OIDC: failed to exchange code for token: %v", err)
-		return nil, err
+		return nil, nil, "", "", errors.Wrap(err, "OIDC: failed to exchange code for token")
 	}
 
-	rawIDToken, ok := credentials.Extra(idTokenKey).(string)
-	if !ok {
-		err := errors.New("OIDC: failed to extract ID token from the token credentials")
-		log.Error(ctx, "OIDC: failed to extract ID token from the token credentials")
-		return nil, err
+	// Build the list of tokens we must handle. For the ID token we verify
+	// signature and claims up-front; the access token is copied as-is.
+	// Since we use the OIDC Authorization Code flow (response_type=code),
+	// The token endpoint response must contain both id_token and access_token
+	// (refresh_token is optional). Their presence is therefore guaranteed here.
+	tokensToProcess := []struct {
+		name             string
+		getVerifiedToken func() (string, error)      // fetches the raw token; verifies when needed
+		claims           *map[string]json.RawMessage // destination for parsed claims
+		rawToken         *string                     // destination for the raw token string
+		required         bool
+	}{
+		{
+			name:     idTokenKey,
+			required: true,
+			getVerifiedToken: func() (string, error) {
+				val, _ := credentials.Extra(idTokenKey).(string)
+				if val == "" {
+					log.Ops.Warning(ctx, "OIDC: required id_token missing from credentials")
+					return "", errors.New("OIDC: required id_token missing from credentials")
+				}
+				// The ID token must be verified against the provider.
+				if _, err := o.Verify(ctx, val); err != nil {
+					return "", err
+				}
+				return val, nil
+			},
+			claims:   &idTokenClaims,
+			rawToken: &rawIDToken,
+		},
+		{
+			name:     "access_token",
+			required: authZEnabled,
+			getVerifiedToken: func() (string, error) {
+				val := credentials.AccessToken
+				if val == "" {
+					log.Ops.Warning(ctx, "OIDC: required access_token missing from credentials")
+					return "", errors.New("OIDC: required access_token missing from credentials")
+				}
+				return val, nil
+			},
+			claims:   &accessTokenClaims,
+			rawToken: &rawAccessToken,
+		},
 	}
 
-	idToken, err := o.Verify(ctx, rawIDToken)
-	if err != nil {
-		log.Errorf(ctx, "OIDC: unable to verify ID token: %v", err)
-		return nil, err
-	}
+	for _, tokenInfo := range tokensToProcess {
+		if !tokenInfo.required {
+			continue
+		}
+		var err error
+		*tokenInfo.rawToken, err = tokenInfo.getVerifiedToken()
+		if err != nil {
+			return nil, nil, "", "", errors.Wrapf(err, "OIDC: failed to verify %s", tokenInfo.name)
+		}
+		// Attempt to parse the token as a JWT to extract its claims.
+		// We keep this at INFO(1) because opaque tokens are normal for many IdPs;
+		// the message is useful for troubleshooting but not an operator-actionable error.
+		parsedToken, err := jwt.ParseInsecure([]byte(*tokenInfo.rawToken))
+		if err != nil {
+			log.VInfof(ctx, 1, "OIDC: could not parse %s as JWT (this is expected for opaque tokens): %v", tokenInfo.name, err)
+			continue // Not a JWT, so we can't get claims from it.
+		}
 
-	var claims map[string]json.RawMessage
-	if err := idToken.Claims(&claims); err != nil {
-		log.Errorf(ctx, "OIDC: unable to deserialize token claims: %v", err)
-		return nil, err
+		// Convert the jwt.Token into a map to extract all claims.
+		claimsMap, err := parsedToken.AsMap(ctx)
+		if err != nil {
+			return nil, nil, "", "", errors.Wrapf(err, "OIDC: failed to get claims as map from %s", tokenInfo.name)
+		}
+		// The claims map must be marshaled and unmarshaled to convert it to the desired type.
+		jsonBytes, err := json.Marshal(claimsMap)
+		if err != nil {
+			return nil, nil, "", "", errors.Wrapf(err, "OIDC: failed to marshal claims from %s", tokenInfo.name)
+		}
+		var claimsData map[string]json.RawMessage
+		if err := json.Unmarshal(jsonBytes, &claimsData); err != nil {
+			return nil, nil, "", "", errors.Wrapf(err, "OIDC: failed to deserialize claims from %s", tokenInfo.name)
+		}
+		*tokenInfo.claims = claimsData
 	}
 
-	return claims, nil
+	return idTokenClaims, accessTokenClaims, rawIDToken, rawAccessToken, nil
 }
 
 func (o *oidcManager) Verify(ctx context.Context, s string) (*oidc.IDToken, error) {
@@ -247,11 +323,17 @@ func (o oidcManager) AuthCodeURL(s string, option ...oauth2.AuthCodeOption) stri
 	return o.oauth2Config.AuthCodeURL(s, option...)
 }
 
+func (o *oidcManager) UserInfo(ctx context.Context, ts oauth2.TokenSource) (*oidc.UserInfo, error) {
+	octx := oidc.ClientContext(ctx, o.httpClient.Client)
+	return o.provider.UserInfo(octx, ts)
+}
+
 type IOIDCManager interface {
 	Verify(context.Context, string) (*oidc.IDToken, error)
 	Exchange(context.Context, string, ...oauth2.AuthCodeOption) (*oauth2.Token, error)
 	AuthCodeURL(string, ...oauth2.AuthCodeOption) string
-	ExchangeVerifyGetClaims(context.Context, string, string) (map[string]json.RawMessage, error)
+	ExchangeVerifyGetTokenInfo(context.Context, string, string, bool) (idTokenClaims, accessTokenClaims map[string]json.RawMessage, rawIDToken, rawAccessToken string, err error)
+	UserInfo(context.Context, oauth2.TokenSource) (*oidc.UserInfo, error)
 }
 
 var _ IOIDCManager = &oidcManager{}
@@ -291,6 +373,7 @@ var NewOIDCManager func(context.Context, oidcAuthenticationConf, string, []strin
 		verifier:     verifier,
 		oauth2Config: oauth2Config,
 		httpClient:   conf.httpClient,
+		provider:     provider,
 	}, nil
 }
 
@@ -337,6 +420,9 @@ func reloadConfigLocked(
 			httputil.WithDialerTimeout(clientTimeout),
 			httputil.WithCustomCAPEM(OIDCProviderCustomCA.Get(&st.SV)),
 		),
+		authZEnabled:     OIDCAuthZEnabled.Get(&st.SV),
+		groupClaim:       OIDCAuthGroupClaim.Get(&st.SV),
+		userinfoGroupKey: OIDCAuthUserinfoGroupKey.Get(&st.SV),
 	}
 
 	if !oidcAuthServer.conf.enabled && conf.enabled {
@@ -421,8 +507,11 @@ var ConfigureOIDC = func(
 	userLoginFromSSO func(ctx context.Context, username string) (*http.Cookie, error),
 	ambientCtx log.AmbientContext,
 	cluster uuid.UUID,
+	execCfg *sql.ExecutorConfig,
 ) (authserver.OIDC, error) {
-	oidcAuthentication := &oidcAuthenticationServer{}
+	oidcAuthentication := &oidcAuthenticationServer{
+		execCfg: execCfg,
+	}
 
 	// Don't want to use GRPC here since these endpoints require HTTP-Redirect behaviors and the
 	// callback endpoint will be receiving specialized parameters that grpc-gateway will only get
@@ -493,8 +582,11 @@ var ConfigureOIDC = func(
 			return
 		}
 
-		claims, err := oidcAuthentication.manager.ExchangeVerifyGetClaims(ctx, r.URL.Query().Get(codeKey), idTokenKey)
+		idTokenClaims, _, rawIDToken, rawAccessToken, err := oidcAuthentication.manager.
+			ExchangeVerifyGetTokenInfo(ctx, r.URL.Query().Get(codeKey), idTokenKey, oidcAuthentication.conf.authZEnabled)
+
 		if err != nil {
+			log.Errorf(ctx, "OIDC: failed to get and verify token: %v", err)
 			http.Error(w, genericCallbackHTTPError, http.StatusInternalServerError)
 			return
 		}
@@ -509,13 +601,20 @@ var ConfigureOIDC = func(
 		}
 
 		username, err := extractUsernameFromClaims(
-			ctx, claims, oidcAuthentication.conf.claimJSONKey, oidcAuthentication.conf.principalRegex,
+			ctx, idTokenClaims, oidcAuthentication.conf.claimJSONKey, oidcAuthentication.conf.principalRegex,
 		)
 		if err != nil {
 			http.Error(w, genericCallbackHTTPError, http.StatusInternalServerError)
 			return
 		}
 
+		// OIDC authorization
+		if err := oidcAuthentication.authorize(ctx, rawIDToken, rawAccessToken, username); err != nil {
+			log.Errorf(ctx, "OIDC authorization failed with error: %v", err)
+			http.Error(w, genericCallbackHTTPError, http.StatusForbidden)
+			return
+		}
+
 		cookie, err := userLoginFromSSO(ctx, username)
 		if err != nil {
 			log.Errorf(ctx, "OIDC: failed to complete authentication: unable to create session for %s: %v", username, err)
@@ -705,7 +804,7 @@ var ConfigureOIDC = func(
 					}
 				} else {
 					log.Infof(ctx, "OIDC: no identity map found for issuer %s; using %s without mapping", token.Issuer, tokenPrincipal)
-					if username, err := username.MakeSQLUsernameFromUserInput(tokenPrincipal, username.PurposeValidation); err != nil {
+					if username, err := secuser.MakeSQLUsernameFromUserInput(tokenPrincipal, secuser.PurposeValidation); err != nil {
 						acceptedUsernames = append(acceptedUsernames, username.Normalized())
 					}
 				}
@@ -829,6 +928,15 @@ var ConfigureOIDC = func(
 	OIDCAuthClientTimeout.SetOnChange(&st.SV, func(ctx context.Context) {
 		reloadConfig(ambientCtx.AnnotateCtx(ctx), oidcAuthentication, locality, st)
 	})
+	OIDCAuthZEnabled.SetOnChange(&st.SV, func(ctx context.Context) {
+		reloadConfig(ambientCtx.AnnotateCtx(ctx), oidcAuthentication, locality, st)
+	})
+	OIDCAuthGroupClaim.SetOnChange(&st.SV, func(ctx context.Context) {
+		reloadConfig(ambientCtx.AnnotateCtx(ctx), oidcAuthentication, locality, st)
+	})
+	OIDCAuthUserinfoGroupKey.SetOnChange(&st.SV, func(ctx context.Context) {
+		reloadConfig(ambientCtx.AnnotateCtx(ctx), oidcAuthentication, locality, st)
+	})
 
 	return oidcAuthentication, nil
 }

pkg/ccl/oidcccl/authentication_oidc_test.go

@@ -87,13 +87,21 @@ func (m mockOidcManager) AuthCodeURL(s string, option ...oauth2.AuthCodeOption)
 	return m.oauth2Config.AuthCodeURL(s, option...)
 }
 
-func (m mockOidcManager) ExchangeVerifyGetClaims(
-	ctx context.Context, s string, s2 string,
-) (map[string]json.RawMessage, error) {
-	x := map[string]json.RawMessage{}
-	// The email is surrounded by double quotes because it's a serialized JSON string.
-	x["email"] = json.RawMessage([]byte(fmt.Sprintf(`"%s"`, m.claimEmail)))
-	return x, nil
+func (m mockOidcManager) ExchangeVerifyGetTokenInfo(
+	ctx context.Context, code, idTokenKey string, _ bool,
+) (map[string]json.RawMessage, map[string]json.RawMessage, string, string, error) {
+	claims := map[string]json.RawMessage{
+		"email": json.RawMessage(`"[email protected]"`),
+	}
+	// Return nil for access token claims, and the raw token strings.
+	return claims, nil, "dummy-id-token", "dummy-access-token", nil
+}
+
+func (m mockOidcManager) UserInfo(
+	ctx context.Context, ts oauth2.TokenSource,
+) (*oidc.UserInfo, error) {
+	// This mock is not used in this test file, so it can return nil.
+	return nil, nil
 }
 
 var _ IOIDCManager = &mockOidcManager{}