From 8aa0ec17c56d2303411dba99915646a5dbdbc538 Mon Sep 17 00:00:00 2001 From: Harshavardhana Date: Tue, 26 Dec 2023 12:38:42 -0800 Subject: [PATCH] simplify the provider config init, loading and allow reachable IDPs (#3168) fixes https://github.com/minio/console/issues/3018 --- Makefile | 12 ++-- pkg/auth/idp/oauth2/config.go | 79 ++++++++++++++++++++++++ pkg/auth/idp/oauth2/provider.go | 87 +++++++------------------- restapi/user_login.go | 106 ++++++++++++++++++-------------- 4 files changed, 166 insertions(+), 118 deletions(-) diff --git a/Makefile b/Makefile index a382939de..7adde0705 100644 --- a/Makefile +++ b/Makefile @@ -254,19 +254,19 @@ test-initialize-minio-nginx: test-start-docker-minio-w-redirect-url test-start-d cleanup-minio-nginx: @(docker stop minio test-nginx & docker network rm test-network) +# https://stackoverflow.com/questions/19200235/golang-tests-in-sub-directory +# Note: go test ./... will run tests on the current folder and all subfolders. +# This is needed because tests can be in the folder or sub-folder(s), let's include them all please!. test: @echo "execute test and get coverage" - # https://stackoverflow.com/questions/19200235/golang-tests-in-sub-directory - # Note: go test ./... will run tests on the current folder and all subfolders. - # This is needed because tests can be in the folder or sub-folder(s), let's include them all please!. @(cd restapi && mkdir -p coverage && GO111MODULE=on go test ./... -test.v -coverprofile=coverage/coverage.out) +# https://stackoverflow.com/questions/19200235/golang-tests-in-sub-directory +# Note: go test ./... will run tests on the current folder and all subfolders. +# This is since tests in pkg folder are in subfolders and were not executed. test-pkg: @echo "execute test and get coverage" - # https://stackoverflow.com/questions/19200235/golang-tests-in-sub-directory - # Note: go test ./... will run tests on the current folder and all subfolders. - # This is since tests in pkg folder are in subfolders and were not executed. @(cd pkg && mkdir -p coverage && GO111MODULE=on go test ./... -test.v -coverprofile=coverage/coverage-pkg.out) coverage: diff --git a/pkg/auth/idp/oauth2/config.go b/pkg/auth/idp/oauth2/config.go index f726a1e73..1dd859d52 100644 --- a/pkg/auth/idp/oauth2/config.go +++ b/pkg/auth/idp/oauth2/config.go @@ -20,11 +20,16 @@ package oauth2 import ( "crypto/sha1" + "fmt" + "net/http" "strings" "github.com/minio/console/pkg/auth/token" + "github.com/minio/minio-go/v7/pkg/set" "github.com/minio/pkg/v2/env" "golang.org/x/crypto/pbkdf2" + "golang.org/x/oauth2" + xoauth2 "golang.org/x/oauth2" ) // ProviderConfig - OpenID IDP Configuration for console. @@ -41,8 +46,82 @@ type ProviderConfig struct { RoleArn string // can be empty } +// GetOauth2Provider instantiates a new oauth2 client using the configured credentials +// it returns a *Provider object that contains the necessary configuration to initiate an +// oauth2 authentication flow. +// +// We only support Authentication with the Authorization Code Flow - spec: +// https://openid.net/specs/openid-connect-core-1_0.html#CodeFlowAuth +func (pc ProviderConfig) GetOauth2Provider(name string, scopes []string, r *http.Request, idpClient, stsClient *http.Client) (provider *Provider, err error) { + var ddoc DiscoveryDoc + ddoc, err = parseDiscoveryDoc(r.Context(), pc.URL, idpClient) + if err != nil { + return nil, err + } + + supportedResponseTypes := set.NewStringSet() + for _, responseType := range ddoc.ResponseTypesSupported { + // FIXME: ResponseTypesSupported is a JSON array of strings - it + // may not actually have strings with spaces inside them - + // making the following code unnecessary. + for _, s := range strings.Fields(responseType) { + supportedResponseTypes.Add(s) + } + } + + isSupported := requiredResponseTypes.Difference(supportedResponseTypes).IsEmpty() + if !isSupported { + return nil, fmt.Errorf("expected 'code' response type - got %s, login not allowed", ddoc.ResponseTypesSupported) + } + + // If provided scopes are empty we use the user configured list or a default list. + if len(scopes) == 0 { + for _, s := range strings.Split(pc.Scopes, ",") { + w := strings.TrimSpace(s) + if w == "" { + continue + } + scopes = append(scopes, w) + } + if len(scopes) == 0 { + scopes = defaultScopes + } + } + + redirectURL := pc.RedirectCallback + if pc.RedirectCallbackDynamic { + // dynamic redirect if set, will generate redirect URLs + // dynamically based on incoming requests. + redirectURL = getLoginCallbackURL(r) + } + + // add "openid" scope always. + scopes = append(scopes, "openid") + + client := new(Provider) + client.oauth2Config = &xoauth2.Config{ + ClientID: pc.ClientID, + ClientSecret: pc.ClientSecret, + RedirectURL: redirectURL, + Endpoint: oauth2.Endpoint{ + AuthURL: ddoc.AuthEndpoint, + TokenURL: ddoc.TokenEndpoint, + }, + Scopes: scopes, + } + + client.IDPName = name + client.UserInfo = pc.Userinfo + + client.provHTTPClient = idpClient + client.stsHTTPClient = stsClient + + return client, nil +} + // GetStateKeyFunc - return the key function used to generate the authorization // code flow state parameter. + func (pc ProviderConfig) GetStateKeyFunc() StateKeyFunc { return func() []byte { return pbkdf2.Key([]byte(pc.HMACPassphrase), []byte(pc.HMACSalt), 4096, 32, sha1.New) diff --git a/pkg/auth/idp/oauth2/provider.go b/pkg/auth/idp/oauth2/provider.go index 062eafa5c..cabdca561 100644 --- a/pkg/auth/idp/oauth2/provider.go +++ b/pkg/auth/idp/oauth2/provider.go @@ -154,7 +154,7 @@ var requiredResponseTypes = set.CreateStringSet("code") // We only support Authentication with the Authorization Code Flow - spec: // https://openid.net/specs/openid-connect-core-1_0.html#CodeFlowAuth func NewOauth2ProviderClient(scopes []string, r *http.Request, httpClient *http.Client) (*Provider, error) { - ddoc, err := parseDiscoveryDoc(GetIDPURL(), httpClient) + ddoc, err := parseDiscoveryDoc(r.Context(), GetIDPURL(), httpClient) if err != nil { return nil, err } @@ -211,6 +211,15 @@ func NewOauth2ProviderClient(scopes []string, r *http.Request, httpClient *http. var defaultScopes = []string{"openid", "profile", "email"} +// NewOauth2ProviderClientByName returns a provider if present specified by the input name of the provider. +func (ois OpenIDPCfg) NewOauth2ProviderClientByName(name string, scopes []string, r *http.Request, idpClient, stsClient *http.Client) (provider *Provider, err error) { + oi, ok := ois[name] + if !ok { + return nil, fmt.Errorf("%s IDP provider does not exist", name) + } + return oi.GetOauth2Provider(name, scopes, r, idpClient, stsClient) +} + // NewOauth2ProviderClient instantiates a new oauth2 client using the // `OpenIDPCfg` configuration struct. It returns a *Provider object that // contains the necessary configuration to initiate an oauth2 authentication @@ -218,70 +227,18 @@ var defaultScopes = []string{"openid", "profile", "email"} // // We only support Authentication with the Authorization Code Flow - spec: // https://openid.net/specs/openid-connect-core-1_0.html#CodeFlowAuth -func (o OpenIDPCfg) NewOauth2ProviderClient(name string, scopes []string, r *http.Request, idpClient, stsClient *http.Client) (*Provider, error) { - ddoc, err := parseDiscoveryDoc(o[name].URL, idpClient) - if err != nil { - return nil, err - } - - supportedResponseTypes := set.NewStringSet() - for _, responseType := range ddoc.ResponseTypesSupported { - // FIXME: ResponseTypesSupported is a JSON array of strings - it - // may not actually have strings with spaces inside them - - // making the following code unnecessary. - for _, s := range strings.Fields(responseType) { - supportedResponseTypes.Add(s) +func (ois OpenIDPCfg) NewOauth2ProviderClient(scopes []string, r *http.Request, idpClient, stsClient *http.Client) (provider *Provider, providerCfg ProviderConfig, err error) { + for name, oi := range ois { + provider, err = oi.GetOauth2Provider(name, scopes, r, idpClient, stsClient) + if err != nil { + // Upon error look for the next IDP. + continue } + // Upon success return right away. + providerCfg = oi + break } - isSupported := requiredResponseTypes.Difference(supportedResponseTypes).IsEmpty() - - if !isSupported { - return nil, fmt.Errorf("expected 'code' response type - got %s, login not allowed", ddoc.ResponseTypesSupported) - } - - // If provided scopes are empty we use the user configured list or a default - // list. - if len(scopes) == 0 { - scopesTmp := strings.Split(o[name].Scopes, ",") - for _, s := range scopesTmp { - w := strings.TrimSpace(s) - if w != "" { - scopes = append(scopes, w) - } - } - if len(scopes) == 0 { - scopes = defaultScopes - } - } - - redirectURL := o[name].RedirectCallback - if o[name].RedirectCallbackDynamic { - // dynamic redirect if set, will generate redirect URLs - // dynamically based on incoming requests. - redirectURL = getLoginCallbackURL(r) - } - - // add "openid" scope always. - scopes = append(scopes, "openid") - - client := new(Provider) - client.oauth2Config = &xoauth2.Config{ - ClientID: o[name].ClientID, - ClientSecret: o[name].ClientSecret, - RedirectURL: redirectURL, - Endpoint: oauth2.Endpoint{ - AuthURL: ddoc.AuthEndpoint, - TokenURL: ddoc.TokenEndpoint, - }, - Scopes: scopes, - } - - client.IDPName = name - client.UserInfo = o[name].Userinfo - - client.provHTTPClient = idpClient - client.stsHTTPClient = stsClient - return client, nil + return provider, providerCfg, err } type User struct { @@ -427,9 +384,9 @@ func validateOauth2State(state string, keyFunc StateKeyFunc) error { // parseDiscoveryDoc parses a discovery doc from an OAuth provider // into a DiscoveryDoc struct that have the correct endpoints -func parseDiscoveryDoc(ustr string, httpClient *http.Client) (DiscoveryDoc, error) { +func parseDiscoveryDoc(ctx context.Context, ustr string, httpClient *http.Client) (DiscoveryDoc, error) { d := DiscoveryDoc{} - req, err := http.NewRequest(http.MethodGet, ustr, nil) + req, err := http.NewRequestWithContext(ctx, http.MethodGet, ustr, nil) if err != nil { return d, err } diff --git a/restapi/user_login.go b/restapi/user_login.go index daca4e17e..cb5abbad3 100644 --- a/restapi/user_login.go +++ b/restapi/user_login.go @@ -177,58 +177,68 @@ func isKubernetes() bool { } // getLoginDetailsResponse returns information regarding the Console authentication mechanism. -func getLoginDetailsResponse(params authApi.LoginDetailParams, openIDProviders oauth2.OpenIDPCfg) (*models.LoginDetails, *CodedAPIError) { - ctx, cancel := context.WithCancel(params.HTTPRequest.Context()) - defer cancel() +func getLoginDetailsResponse(params authApi.LoginDetailParams, openIDProviders oauth2.OpenIDPCfg) (ld *models.LoginDetails, apiErr *CodedAPIError) { loginStrategy := models.LoginDetailsLoginStrategyForm var redirectRules []*models.RedirectRule r := params.HTTPRequest + var loginDetails *models.LoginDetails - if len(openIDProviders) >= 1 { + if len(openIDProviders) > 0 { loginStrategy = models.LoginDetailsLoginStrategyRedirect - for name, provider := range openIDProviders { - // initialize new oauth2 client - oauth2Client, err := openIDProviders.NewOauth2ProviderClient(name, nil, r, GetConsoleHTTPClient("", getClientIP(params.HTTPRequest)), GetConsoleHTTPClient(getMinIOServer(), getClientIP(params.HTTPRequest))) - if err != nil { - return nil, ErrorWithContext(ctx, err, ErrOauth2Provider) - } - // Validate user against IDP - identityProvider := &auth.IdentityProvider{ - KeyFunc: provider.GetStateKeyFunc(), - Client: oauth2Client, - } - - displayName := fmt.Sprintf("Login with SSO (%s)", name) - serviceType := "" - - if provider.DisplayName != "" { - displayName = provider.DisplayName - } - - if provider.RoleArn != "" { - splitRoleArn := strings.Split(provider.RoleArn, ":") - - if len(splitRoleArn) > 2 { - serviceType = splitRoleArn[2] - } - } - - redirectRule := models.RedirectRule{ - Redirect: identityProvider.GenerateLoginURL(), - DisplayName: displayName, - ServiceType: serviceType, - } - - redirectRules = append(redirectRules, &redirectRule) - } } + + for name, provider := range openIDProviders { + // initialize new oauth2 client + + oauth2Client, err := provider.GetOauth2Provider(name, nil, r, GetConsoleHTTPClient("", getClientIP(params.HTTPRequest)), + GetConsoleHTTPClient(getMinIOServer(), getClientIP(params.HTTPRequest))) + if err != nil { + continue + } + + // Validate user against IDP + identityProvider := &auth.IdentityProvider{ + KeyFunc: provider.GetStateKeyFunc(), + Client: oauth2Client, + } + + displayName := fmt.Sprintf("Login with SSO (%s)", name) + serviceType := "" + + if provider.DisplayName != "" { + displayName = provider.DisplayName + } + + if provider.RoleArn != "" { + splitRoleArn := strings.Split(provider.RoleArn, ":") + + if len(splitRoleArn) > 2 { + serviceType = splitRoleArn[2] + } + } + + redirectRule := models.RedirectRule{ + Redirect: identityProvider.GenerateLoginURL(), + DisplayName: displayName, + ServiceType: serviceType, + } + + redirectRules = append(redirectRules, &redirectRule) + } + + if len(openIDProviders) > 0 && len(redirectRules) == 0 { + loginStrategy = models.LoginDetailsLoginStrategyForm + // No IDP configured fallback to username/password + } + loginDetails = &models.LoginDetails{ LoginStrategy: loginStrategy, RedirectRules: redirectRules, IsK8S: isKubernetes(), AnimatedLogin: getConsoleAnimatedLogin(), } + return loginDetails, nil } @@ -248,7 +258,7 @@ func getLoginOauth2AuthResponse(params authApi.LoginOauth2AuthParams, openIDProv r := params.HTTPRequest lr := params.Body - if openIDProviders != nil { + if len(openIDProviders) > 0 { // we read state rState := *lr.State @@ -258,22 +268,24 @@ func getLoginOauth2AuthResponse(params authApi.LoginOauth2AuthParams, openIDProv } var requestItems oauth2.LoginURLParams - - err = json.Unmarshal(decodedRState, &requestItems) - - if err != nil { + if err = json.Unmarshal(decodedRState, &requestItems); err != nil { return nil, ErrorWithContext(ctx, err) } IDPName := requestItems.IDPName state := requestItems.State - providerCfg := openIDProviders[IDPName] - oauth2Client, err := openIDProviders.NewOauth2ProviderClient(IDPName, nil, r, GetConsoleHTTPClient("", getClientIP(params.HTTPRequest)), GetConsoleHTTPClient(getMinIOServer(), getClientIP(params.HTTPRequest))) + providerCfg, ok := openIDProviders[IDPName] + if !ok { + return nil, ErrorWithContext(ctx, fmt.Errorf("selected IDP %s does not exist", IDPName)) + } + + // Initialize new identity provider with new oauth2Client per IDPName + oauth2Client, err := providerCfg.GetOauth2Provider(IDPName, nil, r, GetConsoleHTTPClient("", getClientIP(params.HTTPRequest)), + GetConsoleHTTPClient(getMinIOServer(), getClientIP(params.HTTPRequest))) if err != nil { return nil, ErrorWithContext(ctx, err) } - // initialize new identity provider identityProvider := auth.IdentityProvider{ KeyFunc: providerCfg.GetStateKeyFunc(),