From 8764ccfc8465134b02f5ed7085823812db698267 Mon Sep 17 00:00:00 2001 From: Mario Hros Date: Wed, 26 Feb 2020 23:00:08 +0100 Subject: [PATCH] Use state parameter to encode the original visited URL and support fixed OIDC callback URL Use OIDC state parameter which is later returned during the auth flow to encode the original visited URL instead of using a cookie. Using a cookie is unnecessary in this case and even causes race conditions if multiple requests are made to a different OIDC-protected targets at the same time. Additionally, the previously-used cookie for this auth flow purpose was named the same as the session cookie, which could even effectively overwrite an existing valid session cookie. This commit also implements support for fixed callback URI/URLs as many public OIDC providers require admins to configure a fixed return/callback URLs in advance for every client to prevent abuse (e.g. by uploading a fake OIDC handler script on a different URI using some unprotected uploader and stealing tokens returned by the OIDC provider to this callback URL). --- README.md | 6 +- adapter/client/callback.go | 80 ++++++ adapter/client/callback_test.go | 73 ++++++ adapter/client/client.go | 7 +- adapter/config/config.go | 6 +- adapter/pkg/apis/policies/v1/oidc_config.go | 15 +- adapter/policy/engine/engine.go | 11 +- adapter/strategy/strategy.go | 2 +- adapter/strategy/web/utils.go | 14 +- adapter/strategy/web/utils_test.go | 2 +- adapter/strategy/web/web.go | 236 ++++++++---------- adapter/strategy/web/web_test.go | 127 ++++++---- cmd/main.go | 1 + .../templates/deployment.yaml | 3 + helm/appidentityandaccessadapter/values.yaml | 4 + tests/fake/client.go | 26 +- 16 files changed, 415 insertions(+), 198 deletions(-) create mode 100644 adapter/client/callback.go create mode 100644 adapter/client/callback_test.go diff --git a/README.md b/README.md index 139d6c5..24b2c81 100644 --- a/README.md +++ b/README.md @@ -29,7 +29,7 @@ Istio uses an Envoy proxy sidecar to mediate all inbound and outbound traffic fo ### Protecting frontend apps -If you're using a browser based application, you can use the [Open ID Connect (OIDC)](https://openid.net/specs/openid-connect-core-1_0.html) / OAuth 2.0 `authorization_grant` flow to authenticate your users. When an unauthenticated user is detected, they are automatically redirected to the authentication page. When the authentication completes, the browser is redirected to an implicit `/oidc/callback` endpoint where the adapter intercepts the request. At this point, the adapter obtains tokens from the identity provider and then redirects the user back to their originally requested URL. +If you're using a browser based application, you can use the [Open ID Connect (OIDC)](https://openid.net/specs/openid-connect-core-1_0.html) / OAuth 2.0 `authorization_grant` flow to authenticate your users. When an unauthenticated user is detected, they are automatically redirected to the authentication page. When the authentication completes, the browser is redirected to an implicit `/oidc/callback` endpoint where the adapter intercepts the request. At this point, the adapter obtains tokens from the identity provider and then redirects the user back to their originally requested URL. Instead of the implicit `/oidc/callback` appended to the original URI, you can configure a different absolute URI or full URL to the callback in the OidcConfig. To view the user session information including the session tokens, you can look in the `Authorization` header. @@ -137,7 +137,11 @@ Depending on whether you're protecting frontend or backend applications, create | `clientSecretRef` | object | no | A reference secret that is used to authenticate the client. This can be used in place of the `clientSecret`. | | `clientSecretRef.name` | string |yes | The name of the Kubernetes Secret that contains the `clientSecret`. | | `clientSecretRef.key` | string | yes | The field within the Kubernetes Secret that contains the `clientSecret`. | + | `callback` | string | no | Callback URL or URI to return to from the IODC provider (default is relative `oidc/callback`) | + **Callback** + + The default callback is `oidc/callback` in the relative form, which means that for the original request path `/something`, the redirect URL into which the OIDC provider will redirect will be `scheme://host/something/oidc/callback`. Each URI you are protecting will have its own callback URL generated. Some providers only support a fixed return URL, though. To make a fixed URI, you can specify an absolute callback URI like `/oidc/callback` which will form a URL with original request scheme and host. Or you can set a full URL to the callback in the form `https://host/callback/url`. In any case, you are responsible to ensure the routes and Policy are all set up so that the resulting callback URL is handled by the adapter. * For backend applications: The OAuth 2.0 Bearer token spec defines a pattern for protecting APIs by using [JSON Web Tokens (JWTs)](https://tools.ietf.org/html/rfc7519.html). Using the following configuration as an example, define a `JwtConfig` CRD that contains the public key resource, which is used to validate token signatures. diff --git a/adapter/client/callback.go b/adapter/client/callback.go new file mode 100644 index 0000000..8a01b7d --- /dev/null +++ b/adapter/client/callback.go @@ -0,0 +1,80 @@ +package client + +import ( + "path" + "regexp" + "strings" +) + +var regexpURL = regexp.MustCompile(`(https?)://([^/]+)(/?.*)`) + +func callbackDefinitionForClient(c Client) string { + callbackDef := "" + + // also support empty clients (for tests without a valid client, using default callbackEndpoint) + if c != nil { + callbackDef = c.Callback() + } + + if callbackDef == "" { + // if the callback is empty for the client, use the original behavior: + // /oidc/callback is simply appended to the original path, resulting in many different callback URLs + callbackDef = "oidc/callback" + } + return callbackDef +} + +// CallbackURLForTarget returns the absolute URL to which IdP should redirect after authenticating the user. +// This is the speckial URL which is expected to be handled by the adapter under the provided Client configuration. +func CallbackURLForTarget(c Client, requestScheme, requestHost, requestPath string) string { + callbackDef := callbackDefinitionForClient(c) + + if requestPath == "" { + requestPath = "/" + } + + if callbackDef[0] == '/' { + // callback path absolute on the request host + return requestScheme + "://" + requestHost + callbackDef + } + + m := regexpURL.FindStringSubmatch(callbackDef) + if len(m) > 1 { + // callback is full URL + return callbackDef + } + + // callback path relative to the target path + if strings.HasSuffix(requestPath, callbackDef) { + // relative callback path already appended, do not append twice + // this can happen if this function is called in the actual callback handler + // (happens after /authorize OIDC provider URL redirects back and we call OIDC provider API again) + callbackDef = "" + } + return requestScheme + "://" + requestHost + path.Join(requestPath, callbackDef) +} + +// IsCallbackRequest returns true if the provided request should be handled by the adapter as part of the auth flow +func IsCallbackRequest(c Client, requestScheme, requestHost, requestPath string) bool { + callbackDef := callbackDefinitionForClient(c) + + if requestPath == "" { + requestPath = "/" + } + + if callbackDef[0] == '/' { + // callback path absolute on the request host + return strings.HasPrefix(requestPath, callbackDef) + } + + m := regexpURL.FindStringSubmatch(callbackDef) + if len(m) == 4 { + // callback is full URL, compare parts (case-insensitive just in case) + return strings.EqualFold(m[1], requestScheme) && + strings.EqualFold(m[2], requestHost) && + strings.EqualFold(m[3], requestPath) + } + + // callback path relative to the target path, thus ending with callbackDef + return strings.HasSuffix(requestPath, callbackDef) +} diff --git a/adapter/client/callback_test.go b/adapter/client/callback_test.go new file mode 100644 index 0000000..5fd23c6 --- /dev/null +++ b/adapter/client/callback_test.go @@ -0,0 +1,73 @@ +package client + +import ( + "net/url" + "testing" + + "github.com/ibm-cloud-security/app-identity-and-access-adapter/tests/fake" +) + +func TestCallbackURLForTarget(t *testing.T) { + tests := []struct { + CallbackDef string + Scheme, Host, Path string + Result string + }{ + { // http scheme test + "", + "http", "test.com", "/admin", + "http://test.com/admin/oidc/callback", + }, + { // default relative callback + "", + "https", "test.com", "/admin", + "https://test.com/admin/oidc/callback", + }, + { // relative URI + "custom/relative/path", + "https", "test.com", "/admin", + "https://test.com/admin/custom/relative/path", + }, + { // relative URI already appended + "custom/relative/path", + "https", "test.com", "/admin/custom/relative/path", + "https://test.com/admin/custom/relative/path", + }, + { // absolute URI + "/absolute/uri/callback", + "https", "test.com", "/admin", + "https://test.com/absolute/uri/callback", + }, + { // full URL (http) + "http://test.com/my/callback", + "http", "test.com", "/admin", + "http://test.com/my/callback", + }, + { // full URL (https) + "https://test.com/my/callback", + "https", "test.com", "/admin", + "https://test.com/my/callback", + }, + } + + for n, tst := range tests { + cli := fake.NewClientWithCallback(nil, tst.CallbackDef) + + res := CallbackURLForTarget(cli, tst.Scheme, tst.Host, tst.Path) + if res != tst.Result { + t.Fatalf("TestCallbackURLForTarget#%d CallbackURLForTarget failed.\n"+ + " Expected: %s\n Got: %s", n, tst.Result, res) + } + + resURL, err := url.Parse(res) + if err != nil { + t.Fatalf("TestCallbackURLForTarget#%d failed to parse URL returned from CallbackURLForTarget %s: %s", + n, res, err.Error()) + } + + if !IsCallbackRequest(cli, resURL.Scheme, resURL.Host, resURL.Path) { + t.Fatalf("TestCallbackURLForTarget#%d IsCallbackRequest check not returning true\n"+ + " scheme: %s, host: %s, path: %s", n, resURL.Scheme, resURL.Host, resURL.Path) + } + } +} diff --git a/adapter/client/client.go b/adapter/client/client.go index 5177b42..7872437 100644 --- a/adapter/client/client.go +++ b/adapter/client/client.go @@ -6,13 +6,14 @@ import ( "go.uber.org/zap" "github.com/ibm-cloud-security/app-identity-and-access-adapter/adapter/authserver" - "github.com/ibm-cloud-security/app-identity-and-access-adapter/adapter/pkg/apis/policies/v1" + v1 "github.com/ibm-cloud-security/app-identity-and-access-adapter/adapter/pkg/apis/policies/v1" ) // Client encapsulates an authn/z client object type Client interface { Name() string ID() string + Callback() string Secret() string AuthorizationServer() authserver.AuthorizationServerService ExchangeGrantCode(code string, redirectURI string) (*authserver.TokenResponse, error) @@ -32,6 +33,10 @@ func (c *remoteClient) ID() string { return c.ClientID } +func (c *remoteClient) Callback() string { + return c.ClientCallback +} + func (c *remoteClient) Secret() string { return c.ClientSecret } diff --git a/adapter/config/config.go b/adapter/config/config.go index 971c039..2827c24 100644 --- a/adapter/config/config.go +++ b/adapter/config/config.go @@ -14,9 +14,12 @@ type Config struct { // The blockKey is used to encrypt the cookie value // Valid lengths are 16, 24, or 32 bytes to select AES-128, AES-192, or AES-256. BlockKeySize IntOptions + // Use Secure attribute for session cookies. + // That ensures they are sent over HTTPS and should be enabled for production! + SecureCookies bool } -// defaultArgs returns the default configuration size +// NewConfig returns the default configuration func NewConfig() *Config { return &Config{ AdapterPort: uint16(47304), @@ -37,5 +40,6 @@ func NewConfig() *Config { }, Value: 16, }, + SecureCookies: false, } } diff --git a/adapter/pkg/apis/policies/v1/oidc_config.go b/adapter/pkg/apis/policies/v1/oidc_config.go index 0878e73..9b10e13 100644 --- a/adapter/pkg/apis/policies/v1/oidc_config.go +++ b/adapter/pkg/apis/policies/v1/oidc_config.go @@ -17,17 +17,18 @@ type OidcConfig struct { // OidcConfigSpec is the spec for a OidcConfig resource type OidcConfigSpec struct { - ClientName string - AuthMethod string `json:"authMethod"` - ClientID string `json:"clientId"` - DiscoveryURL string `json:"discoveryUrl"` - ClientSecret string `json:"clientSecret"` + ClientName string + AuthMethod string `json:"authMethod"` + ClientID string `json:"clientId"` + ClientCallback string `json:"callback"` + DiscoveryURL string `json:"discoveryUrl"` + ClientSecret string `json:"clientSecret"` ClientSecretRef ClientSecretRef `json:"clientSecretRef"` } type ClientSecretRef struct { - Name string `json:"name"` - Key string `json:"key"` + Name string `json:"name"` + Key string `json:"key"` } // +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object diff --git a/adapter/policy/engine/engine.go b/adapter/policy/engine/engine.go index ffdd925..be6fec1 100644 --- a/adapter/policy/engine/engine.go +++ b/adapter/policy/engine/engine.go @@ -5,13 +5,13 @@ import ( "errors" "strings" - "github.com/ibm-cloud-security/app-identity-and-access-adapter/adapter/pkg/apis/policies/v1" + v1 "github.com/ibm-cloud-security/app-identity-and-access-adapter/adapter/pkg/apis/policies/v1" "go.uber.org/zap" "github.com/ibm-cloud-security/app-identity-and-access-adapter/adapter/policy" policy2 "github.com/ibm-cloud-security/app-identity-and-access-adapter/adapter/policy/store/policy" - "github.com/ibm-cloud-security/app-identity-and-access-adapter/config/template" + authnz "github.com/ibm-cloud-security/app-identity-and-access-adapter/config/template" ) const ( @@ -53,6 +53,9 @@ func (m *engine) Evaluate(target *authnz.TargetMsg) (*Action, error) { // Strip custom path components if strings.HasSuffix(target.Path, callbackEndpoint) { + // Attempt to strip default /oidc/callback for backward compatibility. + // Now also a custom callbak can be configured in OidcConfig. Custom callbacks + // has to be explicitly supported in the routing so stripping them is not required. target.Path = strings.Split(target.Path, callbackEndpoint)[0] } else if strings.HasSuffix(target.Path, logoutEndpoint) { target.Path = strings.Split(target.Path, logoutEndpoint)[0] @@ -150,8 +153,8 @@ func createDefaultRules(action Action) []v1.Rule { case policy.OIDC: return []v1.Rule{ { - Claim: aud, - Match: "ANY", + Claim: aud, + Match: "ANY", Values: []string{action.Client.ID()}, }, } diff --git a/adapter/strategy/strategy.go b/adapter/strategy/strategy.go index 16e4bf6..cdf3559 100644 --- a/adapter/strategy/strategy.go +++ b/adapter/strategy/strategy.go @@ -6,7 +6,7 @@ import ( policy "istio.io/api/policy/v1beta1" "github.com/ibm-cloud-security/app-identity-and-access-adapter/adapter/policy/engine" - "github.com/ibm-cloud-security/app-identity-and-access-adapter/config/template" + authnz "github.com/ibm-cloud-security/app-identity-and-access-adapter/config/template" ) // Strategy defines the entry point to an authentication handler diff --git a/adapter/strategy/web/utils.go b/adapter/strategy/web/utils.go index 2e43861..0775edb 100644 --- a/adapter/strategy/web/utils.go +++ b/adapter/strategy/web/utils.go @@ -11,7 +11,7 @@ import ( "go.uber.org/zap" "github.com/ibm-cloud-security/app-identity-and-access-adapter/adapter/client" - "github.com/ibm-cloud-security/app-identity-and-access-adapter/config/template" + authnz "github.com/ibm-cloud-security/app-identity-and-access-adapter/config/template" ) const ( @@ -82,7 +82,7 @@ func buildTokenCookieName(base string, c client.Client) string { // generateSessionIDCookie creates a new sessionId cookie // if the provided value is empty and new id is randomly generated -func generateSessionIDCookie(c client.Client, value *string) *http.Cookie { +func generateSessionIDCookie(c client.Client, value *string, secure bool) *http.Cookie { var v = randString(15) if value != nil { v = *value @@ -91,8 +91,12 @@ func generateSessionIDCookie(c client.Client, value *string) *http.Cookie { Name: buildTokenCookieName(sessionCookie, c), Value: v, Path: "/", - Secure: false, // TODO: replace on release - HttpOnly: false, - Expires: time.Now().Add(time.Hour * time.Duration(2160)), // 90 days + Secure: secure, + SameSite: http.SameSiteLaxMode, + HttpOnly: true, // Cookie available to HTTP protocol only (no JS access) + //TODO: possible to use Expires instead of Max-Age once Istio supports it, + // see https://github.com/istio/istio/pull/21270 + //Expires: time.Now().Add(time.Hour * time.Duration(2160)), // 90 days + MaxAge: 90 * 24 * 60 * 60, // 90 days } } diff --git a/adapter/strategy/web/utils_test.go b/adapter/strategy/web/utils_test.go index d1fb23e..b20d228 100644 --- a/adapter/strategy/web/utils_test.go +++ b/adapter/strategy/web/utils_test.go @@ -6,7 +6,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/ibm-cloud-security/app-identity-and-access-adapter/adapter/client" - "github.com/ibm-cloud-security/app-identity-and-access-adapter/config/template" + authnz "github.com/ibm-cloud-security/app-identity-and-access-adapter/config/template" "github.com/ibm-cloud-security/app-identity-and-access-adapter/tests/fake" ) diff --git a/adapter/strategy/web/web.go b/adapter/strategy/web/web.go index 81e95cf..818320f 100644 --- a/adapter/strategy/web/web.go +++ b/adapter/strategy/web/web.go @@ -3,11 +3,12 @@ package webstrategy import ( "errors" "net/http" + "net/url" "strings" "sync" "time" - "k8s.io/api/core/v1" + v1 "k8s.io/api/core/v1" "k8s.io/client-go/kubernetes" "github.com/gogo/googleapis/google/rpc" @@ -30,7 +31,7 @@ import ( "github.com/ibm-cloud-security/app-identity-and-access-adapter/adapter/policy/engine" "github.com/ibm-cloud-security/app-identity-and-access-adapter/adapter/strategy" "github.com/ibm-cloud-security/app-identity-and-access-adapter/adapter/validator" - "github.com/ibm-cloud-security/app-identity-and-access-adapter/config/template" + authnz "github.com/ibm-cloud-security/app-identity-and-access-adapter/config/template" ) const ( @@ -69,10 +70,10 @@ type Encryptor interface { Decode(name, value string, dst interface{}) error } -// OidcCookie represents a token stored in browser -type OidcCookie struct { - Value string - Expiration time.Time +// OidcState represents a state passed to the OIDC provider during auth flow and later validated +type OidcState struct { + OriginalURL string + Expiration time.Time } // New creates an instance of an OIDC protection agent. @@ -87,7 +88,7 @@ func New(ctx *config.Config, kubeClient kubernetes.Interface) strategy.Strategy // Instantiate the secure cookie encryption instance by // reading or creating a new HMAC and AES symmetric key - _, err := w.getSecureCookie() + _, err := w.getEncryptionSecret() if err != nil { zap.L().Warn("Could not sync signing / encryption secrets, will retry later", zap.Error(err)) } @@ -102,16 +103,18 @@ func (w *WebStrategy) HandleAuthnZRequest(r *authnz.HandleAuthnZRequest, action return w.handleLogout(r, action) } - if strings.HasSuffix(r.Instance.Request.Path, callbackEndpoint) { + request := r.Instance.Request + + if client.IsCallbackRequest(action.Client, request.Scheme, request.Host, request.Path) { if r.Instance.Request.Params.Error != "" { - zap.L().Debug("An error occurred during authentication", zap.String("error_query_param", r.Instance.Request.Params.Error)) - return w.handleErrorCallback(errors.New(r.Instance.Request.Params.Error)) - } else if r.Instance.Request.Params.Code != "" { + zap.L().Debug("An error occurred during authentication", zap.String("error_query_param", request.Params.Error)) + return w.handleErrorCallback(errors.New(request.Params.Error)) + } else if request.Params.Code != "" { zap.L().Debug("Received authorization code") - return w.handleAuthorizationCodeCallback(r.Instance.Request.Params.Code, r.Instance.Request, action) + return w.handleAuthorizationCodeCallback(r.Instance.Request.Params.Code, request, action) } else { zap.L().Debug("Unexpected response on callback endpoint /oidc/callback. Triggering re-authentication.") - return w.handleAuthorizationCodeFlow(r.Instance.Request, action) + return w.handleAuthorizationCodeFlow(request, action) } } @@ -209,7 +212,7 @@ func (w *WebStrategy) handleRefreshTokens(sessionID string, session *authserver. return nil, nil } else { zap.L().Debug("Updated tokens using refresh token", zap.String("client_name", c.Name()), zap.String("session_id", sessionID)) - cookie := generateSessionIDCookie(c, &sessionID) + cookie := generateSessionIDCookie(c, &sessionID, w.ctx.SecureCookies) w.tokenCache.Store(cookie.Value, tokens) return &authnz.HandleAuthnZResponse{ Result: &v1beta1.CheckResult{Status: status.WithMessage(rpc.OK, "User is authenticated")}, @@ -250,48 +253,37 @@ func (w *WebStrategy) handleLogout(r *authnz.HandleAuthnZRequest, action *engine redirectURL := strings.Split(r.Instance.Request.Path, logoutEndpoint)[0] cookies := []*http.Cookie{ { - Name: cookieName, - Value: "deleted", - Path: "/", - Expires: time.Now().Add(-100 * time.Hour), + Name: cookieName, + Value: "deleted", + Path: "/", + Secure: w.ctx.SecureCookies, + //TODO: possible to use Expires instead of Max-Age once Istio supports it, + // see https://github.com/istio/istio/pull/21270 + //Expires: time.Now().Add(time.Hour * time.Duration(2160)), // 90 days + MaxAge: 90 * 24 * 60 * 60, // 90 days }, } return buildSuccessRedirectResponse(redirectURL, cookies), nil } -/* -func (w *WebStrategy) handleLogout(path string, action *engine.Action) (*authnz.HandleAuthnZResponse, error) { - // Uncomment when we go stateless - expiration := time.Now().Add(-100 * time.Hour) - cookies := make([]*http.Cookie, 3) - for i, name := range []string{accessTokenCookie, idTokenCookie, refreshTokenCookie} { - cookies[i] = &http.Cookie{ - Name: buildTokenCookieName(name, action.Client), - Value: "deleted", - Path: "/", - Expires: expiration, - } - } - redirectURL := strings.Split(path, logoutEndpoint)[0] - return buildSuccessRedirectResponse(redirectURL, cookies), nil -} -*/ - // handleAuthorizationCodeCallback processes a successful OAuth 2.0 callback containing a authorization code func (w *WebStrategy) handleAuthorizationCodeCallback(code interface{}, request *authnz.RequestMsg, action *engine.Action) (*authnz.HandleAuthnZResponse, error) { - err := w.validateState(request, action.Client) + state, err := w.validateState(action.Client.ID(), request) if err != nil { zap.L().Info("OIDC callback: could not validate state parameter", zap.Error(err), zap.String("client_name", action.Client.Name())) return w.handleErrorCallback(err) } - redirectURI := buildRequestURL(request) + // Set the redirect URL back to this adapter + // Some providers require this callback to be the same as the one used for /autorization (handleAuthorizationCodeFlow) + redirectURL := client.CallbackURLForTarget(action.Client, request.Scheme, request.Host, request.Path) // Exchange authorization grant code for tokens - response, err := action.Client.ExchangeGrantCode(code.(string), redirectURI) + response, err := action.Client.ExchangeGrantCode(code.(string), redirectURL) if err != nil { - zap.L().Info("OIDC callback: Could not retrieve tokens", zap.Error(err), zap.String("client_name", action.Client.Name())) + zap.L().Info("OIDC callback: Could not retrieve tokens", zap.Error(err), + zap.String("client_name", action.Client.Name()), zap.String("redirect_uri", redirectURL)) return w.handleErrorCallback(err) } @@ -310,28 +302,35 @@ func (w *WebStrategy) handleAuthorizationCodeCallback(code interface{}, request return w.handleErrorCallback(validationErr) } - cookie := generateSessionIDCookie(action.Client, nil) + cookie := generateSessionIDCookie(action.Client, nil, w.ctx.SecureCookies) w.tokenCache.Store(cookie.Value, response) - zap.L().Debug("OIDC callback: created new active session: ", zap.String("client_name", action.Client.Name()), zap.String("session_id", cookie.Value)) + zap.L().Debug("OIDC callback: created new active session: ", zap.String("session_id", cookie.Value), zap.String("client_name", action.Client.Name())) + + // redirect to the original URL (as specified in the state) + redirectURL = state.OriginalURL + // overwrite redirect URL if requested by the action if action.RedirectUri != "" { - redirectURI = action.RedirectUri + redirectURL = action.RedirectUri } - return buildSuccessRedirectResponse(redirectURI, []*http.Cookie{cookie}), nil + return buildSuccessRedirectResponse(redirectURL, []*http.Cookie{cookie}), nil } // handleAuthorizationCodeFlow initiates an OAuth 2.0 / OIDC authorization_code grant flow. func (w *WebStrategy) handleAuthorizationCodeFlow(request *authnz.RequestMsg, action *engine.Action) (*authnz.HandleAuthnZResponse, error) { - redirectURI := buildRequestURL(request) + callbackEndpoint - // / Build and store session state - state, stateCookie, err := w.buildStateParam(action.Client) + // set the redirect URL back to this adapter + redirectURL := client.CallbackURLForTarget(action.Client, request.Scheme, request.Host, request.Path) + + // build and store the OIDC state with the original URL + originalRequestURL := buildRequestURL(request) + state, err := w.buildStateParam(action.Client.ID(), originalRequestURL) if err != nil { zap.L().Info("Could not generate state parameter", zap.Error(err), zap.String("client_name", action.Client.Name())) return nil, err } - zap.S().Debugf("Initiating redirect to identity provider using redirect URL: %s", redirectURI) + zap.S().Debugf("Initiating redirect to identity provider using redirect URL: %s", redirectURL) return &authnz.HandleAuthnZResponse{ Result: &v1beta1.CheckResult{ Status: rpc.Status{ @@ -340,8 +339,7 @@ func (w *WebStrategy) handleAuthorizationCodeFlow(request *authnz.RequestMsg, ac Details: []*types.Any{status.PackErrorDetail(&policy.DirectHttpResponse{ Code: policy.Found, // Response Mixer remaps on request Headers: map[string]string{ - location: generateAuthorizationURL(action.Client, redirectURI, state), - setCookie: stateCookie.String(), + location: generateAuthorizationURL(action.Client, redirectURL, state), }, })}, }, @@ -349,28 +347,30 @@ func (w *WebStrategy) handleAuthorizationCodeFlow(request *authnz.RequestMsg, ac }, nil } -// getSecureCookie retrieves the SecureCookie encryption struct in a +// getEncryptionSecret retrieves the secret (used to encrypt/decrypt the state) in a // thread safe manner. -func (w *WebStrategy) getSecureCookie() (Encryptor, error) { +func (w *WebStrategy) getEncryptionSecret() (Encryptor, error) { // Allow all threads to check if instance already exists if w.encrpytor != nil { return w.encrpytor, nil } + w.mutex.Lock() + defer w.mutex.Unlock() + // Once this thread has the lock check again to see if it had been set while waiting if w.encrpytor != nil { - w.mutex.Unlock() return w.encrpytor, nil } + // We need to generate a new key set - sc, err := w.generateSecureCookie() - w.mutex.Unlock() + sc, err := w.getOrGenerateEncryptionSecret() return sc, err } -// generateSecureCookie instantiates a SecureCookie instance using either the preconfigured -// key secret cookie or a dynamically generated pair. -func (w *WebStrategy) generateSecureCookie() (Encryptor, error) { +// getOrGenerateEncryptionSecret returns the encryptor based on the pre-configured secret +// or it generates a new secret if none has been pre-configured +func (w *WebStrategy) getOrGenerateEncryptionSecret() (Encryptor, error) { var secret interface{} // Check if key set was already configured. This should occur during helm install secret, err := networking.Retry(3, 1, func() (interface{}, error) { @@ -381,7 +381,7 @@ func (w *WebStrategy) generateSecureCookie() (Encryptor, error) { zap.S().Infof("Secret %v not found: %v. Another will be generated.", defaultKeySecret, err) secret, err = w.generateKeySecret(w.ctx.HashKeySize.Value, w.ctx.BlockKeySize.Value) if err != nil { - zap.L().Info("Failed to retrieve tokens", zap.Error(err)) + zap.L().Info("Failed to generate secret", zap.Error(err)) return nil, err } } @@ -416,65 +416,54 @@ func (w *WebStrategy) generateKeySecret(hashKeySize int, blockKeySize int) (inte }) } -// generateEncryptedCookie creates an encodes and encrypts cookieData into an http.Cookie -func (w *WebStrategy) generateEncryptedCookie(cookieName string, cookieData *OidcCookie) (*http.Cookie, error) { +// encryptState encodes and encrypts OidcState +func (w *WebStrategy) encryptState(clientID string, state *OidcState) (string, error) { // encode the struct - data, err := bson.Marshal(&cookieData) + data, err := bson.Marshal(&state) if err != nil { - zap.L().Warn("Could not marshal cookie data", zap.Error(err)) - return nil, err + zap.L().Warn("Could not marshal the state object", zap.Error(err)) + return "", err } - sc, err := w.getSecureCookie() + sc, err := w.getEncryptionSecret() if err != nil { - zap.L().Warn("Could not get secure cookie instance", zap.Error(err)) - return nil, err + zap.L().Warn("Could not get encryptor", zap.Error(err)) + return "", err } // create the cookie - if encoded, err := sc.Encode(cookieName, data); err == nil { - return &http.Cookie{ - Name: cookieName, - Value: encoded, - Path: "/", - Secure: false, // TODO: DO NOT RELEASE WITHOUT THIS FLAG SET TO TRUE - HttpOnly: false, - Expires: time.Now().Add(time.Hour * time.Duration(4)), - }, nil + if encoded, err := sc.Encode(clientID, data); err == nil { + return encoded, nil } else { - zap.S().Error("Error encoding cookie: length: %s", len(encoded)) - return nil, err + zap.S().Error("Error encoding state", zap.Error(err), zap.Int("len", len(encoded))) + return "", err } } -// parseAndValidateCookie parses a raw http.Cookie, performs basic validation -// and returns an OIDCCookie -func (w *WebStrategy) parseAndValidateCookie(cookie *http.Cookie) *OidcCookie { - if cookie == nil { - zap.L().Debug("Cookie does not exist") +// decryptAndValidateState decrypts and decodes the state string, performs basic validation +// and returns the parsed OidcState +func (w *WebStrategy) decryptAndValidateState(clientID string, encryptedState string) *OidcState { + if encryptedState == "" { + zap.L().Debug("Empty encoded state string") return nil } - sc, err := w.getSecureCookie() + sc, err := w.getEncryptionSecret() if err != nil { - zap.L().Debug("Error getting securecookie", zap.Error(err)) + zap.L().Debug("Error getting encryptor", zap.Error(err)) return nil } value := []byte{} - if err := sc.Decode(cookie.Name, cookie.Value, &value); err != nil { - zap.L().Debug("Could not decode cookie:", zap.Error(err)) + if err := sc.Decode(clientID, encryptedState, &value); err != nil { + zap.L().Debug("Could not decode encrypted state:", zap.Error(err)) return nil } - cookieObj := OidcCookie{} + cookieObj := OidcState{} if err := bson.Unmarshal(value, &cookieObj); err != nil { - zap.L().Debug("Could not unmarshal cookie:", zap.Error(err)) - return nil - } - if cookieObj.Value == "" { - zap.L().Debug("Cookie does not have a token value") + zap.L().Debug("Could not unmarshal the state object:", zap.Error(err)) return nil } if cookieObj.Expiration.Before(time.Now()) { - zap.S().Debug("Cookies have expired: %v - %v", cookieObj.Expiration, time.Now()) + zap.S().Debug("State parameter has expired: %v - %v", cookieObj.Expiration, time.Now()) return nil } return &cookieObj @@ -504,55 +493,44 @@ func buildSuccessRedirectResponse(redirectURI string, cookies []*http.Cookie) *a } } -func (w *WebStrategy) buildStateParam(c client.Client) (string, *http.Cookie, error) { - state := randString(10) - cookieName := buildTokenCookieName(sessionCookie, c) - cookie, err := w.generateEncryptedCookie(cookieName, &OidcCookie{ - Value: state, - Expiration: time.Now().Add(time.Hour), +// buildState param creates encrypted string holding the action metadata to pass to the OIDC provider as the state +func (w *WebStrategy) buildStateParam(clientID, originalURL string) (string, error) { + encState, err := w.encryptState(clientID, &OidcState{ + OriginalURL: originalURL, + Expiration: time.Now().Add(10 * time.Minute), }) - return state, cookie, err + return encState, err } // validateState ensures the callback request state parameter matches the state stored in an encrypted session cookie // Follows from OAuth 2.0 specification https://tools.ietf.org/html/rfc6749#section-4.1.1 -func (w *WebStrategy) validateState(request *authnz.RequestMsg, c client.Client) error { - // Get state cookie from header - header := http.Header{ - "Cookie": {request.Headers.Cookies}, - } - r := http.Request{Header: header} - name := buildTokenCookieName(sessionCookie, c) - oidcStateCookie, err := r.Cookie(name) +func (w *WebStrategy) validateState(clientID string, request *authnz.RequestMsg) (*OidcState, error) { + // Ensure state is returned on request from identity provider + if request.Params.State == "" { + stateError := errors.New("state parameter not provided") + zap.L().Info("OIDC callback: missing state parameter from identity provider") + return nil, stateError + } + + // Unescape state parameter + encryptedState, err := url.QueryUnescape(request.Params.State) if err != nil { - return errors.New("state parameter not provided") + stateError := errors.New("bad state parameter") + zap.L().Info("OIDC callback: bad state parameter", zap.String("state", request.Params.State)) + return nil, stateError } // Parse encrypted state cookie - storedHttpCookie := w.parseAndValidateCookie(oidcStateCookie) + state := w.decryptAndValidateState(clientID, encryptedState) // Ensure state cookie is returned - if storedHttpCookie == nil { - stateError := errors.New("missing state parameter") - zap.L().Info("OIDC callback: missing stored state parameter", zap.Error(err)) - return stateError - } - - // Ensure state is returned on request from identity provider - if request.Params.State == "" { - stateError := errors.New("missing state parameter from callback") - zap.L().Info("OIDC callback: missing state parameter from identity provider", zap.Error(err)) - return stateError - } - - // Validate cookie state with stored state - if request.Params.State != storedHttpCookie.Value { + if state == nil { stateError := errors.New("invalid state parameter") - zap.L().Info("OIDC callback: missing or invalid state parameter", zap.Error(err)) - return stateError + zap.L().Info("OIDC callback: state parameter invalid or malformed") + return nil, stateError } zap.L().Debug("OIDC callback: validated state parameter") - return nil + return state, nil } diff --git a/adapter/strategy/web/web_test.go b/adapter/strategy/web/web_test.go index 0fe4233..acdb836 100644 --- a/adapter/strategy/web/web_test.go +++ b/adapter/strategy/web/web_test.go @@ -3,12 +3,13 @@ package webstrategy import ( "errors" "net/http" + "net/url" "strings" "sync" "testing" "time" - "github.com/ibm-cloud-security/app-identity-and-access-adapter/adapter/pkg/apis/policies/v1" + v1 "github.com/ibm-cloud-security/app-identity-and-access-adapter/adapter/pkg/apis/policies/v1" "github.com/ibm-cloud-security/app-identity-and-access-adapter/adapter/validator" "github.com/gogo/protobuf/types" @@ -22,14 +23,15 @@ import ( "github.com/ibm-cloud-security/app-identity-and-access-adapter/adapter/config" err "github.com/ibm-cloud-security/app-identity-and-access-adapter/adapter/errors" "github.com/ibm-cloud-security/app-identity-and-access-adapter/adapter/policy/engine" - "github.com/ibm-cloud-security/app-identity-and-access-adapter/config/template" + authnz "github.com/ibm-cloud-security/app-identity-and-access-adapter/config/template" "github.com/ibm-cloud-security/app-identity-and-access-adapter/tests/fake" ) const ( - defaultHost = "tests.io" - defaultBasePath = "/api" - defaultState = "session" + defaultHost = "tests.io" + defaultBasePath = "/api" + defaultState = "session" + defaultOriginalURL = "https://" + defaultHost + defaultBasePath ) var defaultHashKey = securecookie.GenerateRandomKey(16) @@ -72,6 +74,45 @@ func TestHandleNewAuthorizationRequest(t *testing.T) { int32(16), nil, }, + { // New Authentication with a custom absolute-form callback URI + generateAuthnzRequest("", "", "", "", ""), + &engine.Action{ + Client: fake.NewClientWithCallback(nil, "/my/callback"), + }, + &v1beta1.DirectHttpResponse{ + Code: 302, + Headers: map[string]string{"location": generateAuthorizationURL(fake.NewClient(nil), "https://tests.io/my/callback", "")}, + }, + "Redirecting to identity provider", + int32(16), + nil, + }, + { // New Authentication with a custom relative-form callback URI + generateAuthnzRequest("", "", "", "", ""), + &engine.Action{ + Client: fake.NewClientWithCallback(nil, "my/callback"), + }, + &v1beta1.DirectHttpResponse{ + Code: 302, + Headers: map[string]string{"location": generateAuthorizationURL(fake.NewClient(nil), "https://tests.io/api/my/callback", "")}, + }, + "Redirecting to identity provider", + int32(16), + nil, + }, + { // New Authentication with a custom full callback URL + generateAuthnzRequest("", "", "", "", ""), + &engine.Action{ + Client: fake.NewClientWithCallback(nil, "https://my-adapter-domain.com/my/callback"), + }, + &v1beta1.DirectHttpResponse{ + Code: 302, + Headers: map[string]string{"location": generateAuthorizationURL(fake.NewClient(nil), "https://my-adapter-domain.com/my/callback", "")}, + }, + "Redirecting to identity provider", + int32(16), + nil, + }, } for _, test := range tests { @@ -190,6 +231,7 @@ func TestRefreshTokenFlow(t *testing.T) { t.Run("refresh", func(t *testing.T) { t.Parallel() api := WebStrategy{ + ctx: config.NewConfig(), tokenCache: new(sync.Map), encrpytor: defaultSecureCookie, tokenUtil: MockValidator{ @@ -281,9 +323,12 @@ func TestCodeCallback(t *testing.T) { }, } - encryptCookie := func(c *OidcCookie) string { - cookie, _ := api.generateEncryptedCookie("oidc-cookie-id", c) - return cookie.String() + encryptState := func(c *OidcState) string { + state, err := api.encryptState(fake.NewClient(nil).ID(), c) + if err != nil { + panic(err) + } + return url.QueryEscape(state) } var tests = []struct { @@ -294,7 +339,7 @@ func TestCodeCallback(t *testing.T) { message string err error }{ - { // missing state cookie + { // missing state "", nil, generateAuthnzRequest("", "code", "", callbackEndpoint, ""), @@ -304,50 +349,40 @@ func TestCodeCallback(t *testing.T) { "state parameter not provided", nil, }, - { // invalid state - missing + { // bad state (unable to url-decode) "", nil, - generateAuthnzRequest(encryptCookie(&OidcCookie{Value: ""}), "code", "", callbackEndpoint, defaultState), + generateAuthnzRequest("", "code", "", callbackEndpoint, "inva%FFli%X0d-state"), &v1beta1.DirectHttpResponse{ Code: 401, }, - "missing state parameter", + "bad state parameter", nil, }, - { // invalid state - missing expiration + { // invalid state - not a properly encrypted state string "", nil, - generateAuthnzRequest(encryptCookie(&OidcCookie{Value: defaultState}), "code", "", callbackEndpoint, defaultState), - &v1beta1.DirectHttpResponse{ - Code: 401, - }, - "missing state parameter", - nil, - }, - { // invalid state - mismatch - "", - nil, - generateAuthnzRequest(encryptCookie(defaultSessionOidcCookie()), "code", "", callbackEndpoint, "session2"), + generateAuthnzRequest("", "code", "", callbackEndpoint, "invalidencryptedstate"), &v1beta1.DirectHttpResponse{ Code: 401, }, "invalid state parameter", nil, }, - { // state not returned from IdP + { // invalid state - expired (empty expiration date) "", nil, - generateAuthnzRequest(encryptCookie(defaultSessionOidcCookie()), "code", "", callbackEndpoint, ""), + generateAuthnzRequest("", "code", "", callbackEndpoint, encryptState(&OidcState{})), &v1beta1.DirectHttpResponse{ Code: 401, }, - "missing state parameter from callback", + "invalid state parameter", nil, }, { // could not exchange grant code "", defaultFailureTokenResponse("problem getting tokens"), - generateAuthnzRequest(encryptCookie(defaultSessionOidcCookie()), "code", "", callbackEndpoint, defaultState), + generateAuthnzRequest("", "code", "", callbackEndpoint, encryptState(&OidcState{Expiration: time.Now().Add(1 * time.Minute)})), &v1beta1.DirectHttpResponse{ Code: 401, }, @@ -361,7 +396,7 @@ func TestCodeCallback(t *testing.T) { IdentityToken: "invalid_token", }, }, - generateAuthnzRequest(encryptCookie(defaultSessionOidcCookie()), "code", "", callbackEndpoint, defaultState), + generateAuthnzRequest("", "code", "", callbackEndpoint, encryptState(&OidcState{Expiration: time.Now().Add(1 * time.Minute)})), &v1beta1.DirectHttpResponse{ Code: 401, }, @@ -371,11 +406,11 @@ func TestCodeCallback(t *testing.T) { { // success "", defaultSuccessTokenResponse(), - generateAuthnzRequest(encryptCookie(defaultSessionOidcCookie()), "code", "", callbackEndpoint, defaultState), + generateAuthnzRequest("", "code", "", callbackEndpoint, encryptState(&OidcState{Expiration: time.Now().Add(1 * time.Minute), OriginalURL: defaultOriginalURL})), &v1beta1.DirectHttpResponse{ Code: 302, Headers: map[string]string{ - "location": "https://" + defaultHost + defaultBasePath, + "location": defaultOriginalURL, setCookie: "oidc-test-id", }, }, @@ -385,7 +420,7 @@ func TestCodeCallback(t *testing.T) { { // success w/new redirect "https://" + defaultHost + "/another_path", defaultSuccessTokenResponse(), - generateAuthnzRequest(encryptCookie(defaultSessionOidcCookie()), "code", "", callbackEndpoint, defaultState), + generateAuthnzRequest("", "code", "", callbackEndpoint, encryptState(&OidcState{Expiration: time.Now().Add(1 * time.Minute)})), &v1beta1.DirectHttpResponse{ Code: 302, Headers: map[string]string{ @@ -443,6 +478,7 @@ func TestLogout(t *testing.T) { } api := WebStrategy{ + ctx: config.NewConfig(), tokenUtil: MockValidator{}, } @@ -466,13 +502,15 @@ func compareDirectHttpResponses(t *testing.T, r *authnz.HandleAuthnZResponse, ex continue } assert.Equal(t, expected.Code, response.Code) - for key, value := range expected.Headers { - if key == setCookie { + for expectKey, expectValue := range expected.Headers { + if expectKey == setCookie { assert.NotEmpty(t, response.Headers[setCookie]) continue } - if !strings.HasPrefix(response.Headers[key], value) { - assert.Fail(t, "incorrect header value: '"+key+"'\n found: "+value+"\n expected:"+response.Headers[key]) + if !strings.HasPrefix(response.Headers[expectKey], expectValue) { + assert.Fail(t, "incorrect header value: '"+expectKey+ + "'\n found: "+response.Headers[expectKey]+ + "\n expected: "+expectValue) } } @@ -480,10 +518,10 @@ func compareDirectHttpResponses(t *testing.T, r *authnz.HandleAuthnZResponse, ex } } -func defaultSessionOidcCookie() *OidcCookie { - return &OidcCookie{ - Value: defaultState, - Expiration: time.Now().Add(time.Hour), +func defaultSessionOidcCookie() *OidcState { + return &OidcState{ + OriginalURL: defaultOriginalURL, + Expiration: time.Now().Add(time.Hour), } } @@ -543,8 +581,11 @@ func createCookie() *http.Cookie { Name: "oidc-cookie-id", Value: defaultState, Path: "/", - Secure: false, // TODO: replace on release - HttpOnly: false, - Expires: time.Now().Add(time.Hour * time.Duration(2160)), + Secure: false, // disabled for tests + HttpOnly: true, // Cookie available to HTTP protocol only (no JS access) + //TODO: possible to use Expires instead of Max-Age once Istio supports it, + // see https://github.com/istio/istio/pull/21270 + //Expires: time.Now().Add(time.Hour * time.Duration(2160)), // 90 days + MaxAge: 90 * 24 * 60 * 60, // 90 days } } diff --git a/cmd/main.go b/cmd/main.go index 5ddbbe4..22f4ba1 100644 --- a/cmd/main.go +++ b/cmd/main.go @@ -35,6 +35,7 @@ func getCmd() *cobra.Command { f.Int8VarP(&sa.Level, "level", "l", sa.Level, "Set output log level. Range [-1, 7].") f.VarP(&sa.HashKeySize, "hash-key", "", "The size of the HMAC signature key. It is recommended to use a key with 32 or 64 bytes.") f.VarP(&sa.BlockKeySize, "block-key", "", "The size of the AES blockKey size used to encrypt the cookie value. Valid lengths are 16, 24, or 32.") + f.BoolVarP(&sa.SecureCookies, "secure-cookies", "", sa.SecureCookies, "Use Secure attribute for session cookies to ensure they are sent over HTTPS only.") return cmd } diff --git a/helm/appidentityandaccessadapter/templates/deployment.yaml b/helm/appidentityandaccessadapter/templates/deployment.yaml index ca172c4..f6f0115 100644 --- a/helm/appidentityandaccessadapter/templates/deployment.yaml +++ b/helm/appidentityandaccessadapter/templates/deployment.yaml @@ -30,6 +30,9 @@ spec: - "--level={{ .Values.logging.level }}" - "--hash-key={{ .Values.keys.hashKeySize }}" - "--block-key={{ .Values.keys.blockKeySize }}" + {{ if .Values.secureCookies }} + - "--secure-cookies" + {{ end }} imagePullPolicy: {{ .Values.image.pullPolicy}} ports: diff --git a/helm/appidentityandaccessadapter/values.yaml b/helm/appidentityandaccessadapter/values.yaml index 34f349b..6e0f9c5 100644 --- a/helm/appidentityandaccessadapter/values.yaml +++ b/helm/appidentityandaccessadapter/values.yaml @@ -57,3 +57,7 @@ logging: ## from -1 to 7 ## See zapcore: https://godoc.org/go.uber.org/zap/zapcore#Level level: 0 + +# Use Secure attribute for session cookies. +# That ensures they are sent over HTTPS and should be enabled for production! +secureCookies: false \ No newline at end of file diff --git a/tests/fake/client.go b/tests/fake/client.go index 40cff44..222b058 100644 --- a/tests/fake/client.go +++ b/tests/fake/client.go @@ -8,11 +8,12 @@ type TokenResponse struct { } type Client struct { - Server authserver.AuthorizationServerService - TokenResponse *TokenResponse - ClientName string - ClientID string - ClientSecret string + Server authserver.AuthorizationServerService + TokenResponse *TokenResponse + ClientName string + ClientID string + ClientCallback string + ClientSecret string } func NewClient(tokenResponse *TokenResponse) *Client { @@ -25,6 +26,17 @@ func NewClient(tokenResponse *TokenResponse) *Client { } } +func NewClientWithCallback(tokenResponse *TokenResponse, callback string) *Client { + return &Client{ + Server: NewAuthServer(), + ClientName: "name", + ClientID: "id", + ClientCallback: callback, + ClientSecret: "secret", + TokenResponse: tokenResponse, + } +} + func (m *Client) Name() string { return m.ClientName } @@ -33,6 +45,10 @@ func (m *Client) ID() string { return m.ClientID } +func (m *Client) Callback() string { + return m.ClientCallback +} + func (m *Client) Secret() string { return m.ClientSecret }