X-Git-Url: https://git.arvados.org/arvados.git/blobdiff_plain/5d56a1af42f64df57ef7a1bcef6d016ff2310900..HEAD:/lib/controller/localdb/login_oidc.go diff --git a/lib/controller/localdb/login_oidc.go b/lib/controller/localdb/login_oidc.go index 6182469ac3..d91cdddc01 100644 --- a/lib/controller/localdb/login_oidc.go +++ b/lib/controller/localdb/login_oidc.go @@ -14,8 +14,10 @@ import ( "errors" "fmt" "io" + "net" "net/http" "net/url" + "regexp" "strings" "sync" "text/template" @@ -28,9 +30,10 @@ import ( "git.arvados.org/arvados.git/sdk/go/auth" "git.arvados.org/arvados.git/sdk/go/ctxlog" "git.arvados.org/arvados.git/sdk/go/httpserver" - "github.com/coreos/go-oidc" + "github.com/coreos/go-oidc/v3/oidc" lru "github.com/hashicorp/golang-lru" "github.com/jmoiron/sqlx" + "github.com/lib/pq" "github.com/sirupsen/logrus" "golang.org/x/oauth2" "google.golang.org/api/option" @@ -43,6 +46,7 @@ var ( tokenCacheNegativeTTL = time.Minute * 5 tokenCacheTTL = time.Minute * 10 tokenCacheRaceWindow = time.Minute + pqCodeUniqueViolation = pq.ErrorCode("23505") ) type oidcLoginController struct { @@ -64,10 +68,11 @@ type oidcLoginController struct { // https://people.googleapis.com/) peopleAPIBasePath string - provider *oidc.Provider // initialized by setup() - oauth2conf *oauth2.Config // initialized by setup() - verifier *oidc.IDTokenVerifier // initialized by setup() - mu sync.Mutex // protects setup() + provider *oidc.Provider // initialized by setup() + endSessionURL *url.URL // initialized by setup() + oauth2conf *oauth2.Config // initialized by setup() + verifier *oidc.IDTokenVerifier // initialized by setup() + mu sync.Mutex // protects setup() } // Initialize ctrl.provider and ctrl.oauth2conf. @@ -97,11 +102,46 @@ func (ctrl *oidcLoginController) setup() error { ClientID: ctrl.ClientID, }) ctrl.provider = provider + var claims struct { + EndSessionEndpoint string `json:"end_session_endpoint"` + } + err = provider.Claims(&claims) + if err != nil { + return fmt.Errorf("error parsing OIDC discovery metadata: %v", err) + } else if claims.EndSessionEndpoint == "" { + ctrl.endSessionURL = nil + } else { + u, err := url.Parse(claims.EndSessionEndpoint) + if err != nil { + return fmt.Errorf("OIDC end_session_endpoint is not a valid URL: %v", err) + } else if u.Scheme != "https" { + return fmt.Errorf("OIDC end_session_endpoint MUST use HTTPS but does not: %v", u.String()) + } else { + ctrl.endSessionURL = u + } + } return nil } func (ctrl *oidcLoginController) Logout(ctx context.Context, opts arvados.LogoutOptions) (arvados.LogoutResponse, error) { - return logout(ctx, ctrl.Cluster, opts) + err := ctrl.setup() + if err != nil { + return arvados.LogoutResponse{}, fmt.Errorf("error setting up OpenID Connect provider: %s", err) + } + resp, err := logout(ctx, ctrl.Cluster, opts) + if err != nil { + return arvados.LogoutResponse{}, err + } + creds, credsOK := auth.FromContext(ctx) + if ctrl.endSessionURL != nil && credsOK && len(creds.Tokens) > 0 { + values := ctrl.endSessionURL.Query() + values.Set("client_id", ctrl.ClientID) + values.Set("post_logout_redirect_uri", resp.RedirectLocation) + u := *ctrl.endSessionURL + u.RawQuery = values.Encode() + resp.RedirectLocation = u.String() + } + return resp, err } func (ctrl *oidcLoginController) Login(ctx context.Context, opts arvados.LoginOptions) (arvados.LoginResponse, error) { @@ -114,6 +154,9 @@ func (ctrl *oidcLoginController) Login(ctx context.Context, opts arvados.LoginOp if opts.ReturnTo == "" { return loginError(errors.New("missing return_to parameter")) } + if err := validateLoginRedirectTarget(ctrl.Parent.cluster, opts.ReturnTo); err != nil { + return loginError(fmt.Errorf("invalid return_to parameter: %s", err)) + } state := ctrl.newOAuth2State([]byte(ctrl.Cluster.SystemRootToken), opts.Remote, opts.ReturnTo) var authparams []oauth2.AuthCodeOption for k, v := range ctrl.AuthParams { @@ -147,10 +190,39 @@ func (ctrl *oidcLoginController) Login(ctx context.Context, opts arvados.LoginOp return loginError(err) } ctxRoot := auth.NewContext(ctx, &auth.Credentials{Tokens: []string{ctrl.Cluster.SystemRootToken}}) - return ctrl.Parent.UserSessionCreate(ctxRoot, rpc.UserSessionCreateOptions{ - ReturnTo: state.Remote + "," + state.ReturnTo, + resp, err := ctrl.Parent.UserSessionCreate(ctxRoot, rpc.UserSessionCreateOptions{ + ReturnTo: state.Remote + ",https://controller.api.client.invalid", AuthInfo: *authinfo, }) + if err != nil { + return resp, err + } + // Extract token from rails' UserSessionCreate response, and + // attach it to our caller's desired ReturnTo URL. The Rails + // handler explicitly disallows sending the real ReturnTo as a + // belt-and-suspenders defence against Rails accidentally + // exposing an additional login relay. + u, err := url.Parse(resp.RedirectLocation) + if err != nil { + return resp, err + } + token := u.Query().Get("api_token") + if token == "" { + resp.RedirectLocation = state.ReturnTo + } else { + u, err := url.Parse(state.ReturnTo) + if err != nil { + return resp, err + } + q := u.Query() + if q == nil { + q = url.Values{} + } + q.Set("api_token", token) + u.RawQuery = q.Encode() + resp.RedirectLocation = u.String() + } + return resp, nil } func (ctrl *oidcLoginController) UserAuthenticate(ctx context.Context, opts arvados.UserAuthenticateOptions) (arvados.APIClientAuthorization, error) { @@ -333,7 +405,7 @@ func OIDCAccessTokenAuthorizer(cluster *arvados.Cluster, getdb func(context.Cont // We want ctrl to be nil if the chosen controller is not a // *oidcLoginController, so we can ignore the 2nd return value // of this type cast. - ctrl, _ := NewConn(cluster).loginController.(*oidcLoginController) + ctrl, _ := NewConn(context.Background(), cluster, getdb).loginController.(*oidcLoginController) cache, err := lru.New2Q(tokenCacheSize) if err != nil { panic(err) @@ -388,6 +460,9 @@ func (ta *oidcTokenAuthorizer) WrapCalls(origFunc api.RoutableFunc) api.Routable } } +// Matches error from oidc UserInfo() when receiving HTTP status 5xx +var re5xxError = regexp.MustCompile(`^5\d\d `) + // registerToken checks whether tok is a valid OIDC Access Token and, // if so, ensures that an api_client_authorizations row exists so that // RailsAPI will accept it as an Arvados token. @@ -408,11 +483,8 @@ func (ta *oidcTokenAuthorizer) registerToken(ctx context.Context, tok string) er // cached positive result aca := cached.(arvados.APIClientAuthorization) var expiring bool - if aca.ExpiresAt != "" { - t, err := time.Parse(time.RFC3339Nano, aca.ExpiresAt) - if err != nil { - return fmt.Errorf("error parsing expires_at value: %w", err) - } + if !aca.ExpiresAt.IsZero() { + t := aca.ExpiresAt expiring = t.Before(time.Now().Add(time.Minute)) } if !expiring { @@ -460,6 +532,7 @@ func (ta *oidcTokenAuthorizer) registerToken(ctx context.Context, tok string) er return fmt.Errorf("error setting up OpenID Connect provider: %s", err) } if ok, err := ta.checkAccessTokenScope(ctx, tok); err != nil || !ok { + // Note checkAccessTokenScope logs any interesting errors ta.cache.Add(tok, time.Now().Add(tokenCacheNegativeTTL)) return err } @@ -468,6 +541,21 @@ func (ta *oidcTokenAuthorizer) registerToken(ctx context.Context, tok string) er } userinfo, err := ta.ctrl.provider.UserInfo(ctx, oauth2.StaticTokenSource(oauth2Token)) if err != nil { + if neterr := net.Error(nil); errors.As(err, &neterr) || re5xxError.MatchString(err.Error()) { + // If this token is in fact a valid OIDC + // token, but we failed to validate it here + // because of a network problem or internal + // server error, we error out now with a 5xx + // error, indicating to the client that they + // can try again. If we didn't error out now, + // the unrecognized token would eventually + // cause a 401 error further down the stack, + // which the caller would interpret as an + // unrecoverable failure. + ctxlog.FromContext(ctx).WithError(err).Debugf("treating OIDC UserInfo lookup error type %T as transient; failing request instead of forwarding token blindly", err) + return err + } + ctxlog.FromContext(ctx).WithError(err).WithField("HMAC", hmac).Debug("UserInfo failed (not an OIDC token?), caching negative result") ta.cache.Add(tok, time.Now().Add(tokenCacheNegativeTTL)) return nil } @@ -482,7 +570,6 @@ func (ta *oidcTokenAuthorizer) registerToken(ctx context.Context, tok string) er // it's expiring. exp := time.Now().UTC().Add(tokenCacheTTL + tokenCacheRaceWindow) - var aca arvados.APIClientAuthorization if updating { _, err = tx.ExecContext(ctx, `update api_client_authorizations set expires_at=$1 where api_token=$2`, exp, hmac) if err != nil { @@ -490,23 +577,44 @@ func (ta *oidcTokenAuthorizer) registerToken(ctx context.Context, tok string) er } ctxlog.FromContext(ctx).WithField("HMAC", hmac).Debug("(*oidcTokenAuthorizer)registerToken: updated api_client_authorizations row") } else { - aca, err = ta.ctrl.Parent.CreateAPIClientAuthorization(ctx, ta.ctrl.Cluster.SystemRootToken, *authinfo) + aca, err := ta.ctrl.Parent.CreateAPIClientAuthorization(ctx, ta.ctrl.Cluster.SystemRootToken, *authinfo) if err != nil { return err } - _, err = tx.ExecContext(ctx, `update api_client_authorizations set api_token=$1, expires_at=$2 where uuid=$3`, hmac, exp, aca.UUID) + _, err = tx.ExecContext(ctx, `savepoint upd`) if err != nil { + return err + } + _, err = tx.ExecContext(ctx, `update api_client_authorizations set api_token=$1, expires_at=$2 where uuid=$3`, hmac, exp, aca.UUID) + if e, ok := err.(*pq.Error); ok && e.Code == pqCodeUniqueViolation { + // unique_violation, given that the above + // query did not find a row with matching + // api_token, means another thread/process + // also received this same token and won the + // race to insert it -- in which case this + // thread doesn't need to update the database. + // Discard the redundant row. + _, err = tx.ExecContext(ctx, `rollback to savepoint upd`) + if err != nil { + return err + } + _, err = tx.ExecContext(ctx, `delete from api_client_authorizations where uuid=$1`, aca.UUID) + if err != nil { + return err + } + ctxlog.FromContext(ctx).WithField("HMAC", hmac).Debug("(*oidcTokenAuthorizer)registerToken: api_client_authorizations row inserted by another thread") + } else if err != nil { + ctxlog.FromContext(ctx).Errorf("%#v", err) return fmt.Errorf("error adding OIDC access token to database: %w", err) + } else { + ctxlog.FromContext(ctx).WithFields(logrus.Fields{"UUID": aca.UUID, "HMAC": hmac}).Debug("(*oidcTokenAuthorizer)registerToken: inserted api_client_authorizations row") } - aca.APIToken = hmac - ctxlog.FromContext(ctx).WithFields(logrus.Fields{"UUID": aca.UUID, "HMAC": hmac}).Debug("(*oidcTokenAuthorizer)registerToken: inserted api_client_authorizations row") } err = tx.Commit() if err != nil { return err } - aca.ExpiresAt = exp.Format(time.RFC3339Nano) - ta.cache.Add(tok, aca) + ta.cache.Add(tok, arvados.APIClientAuthorization{ExpiresAt: exp}) return nil } @@ -541,6 +649,6 @@ func (ta *oidcTokenAuthorizer) checkAccessTokenScope(ctx context.Context, tok st return true, nil } } - ctxlog.FromContext(ctx).WithFields(logrus.Fields{"have": claims.Scope, "need": ta.ctrl.AcceptAccessTokenScope}).Infof("unacceptable access token scope") + ctxlog.FromContext(ctx).WithFields(logrus.Fields{"have": claims.Scope, "need": ta.ctrl.AcceptAccessTokenScope}).Info("unacceptable access token scope") return false, httpserver.ErrorWithStatus(errors.New("unacceptable access token scope"), http.StatusUnauthorized) }