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).

Author: Mario Hros

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.
 

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)
+}

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)
+		}
+	}
+}

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
 }

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,
 	}
 }

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

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()},
 			},
 		}

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

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
 	}
 }

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"
 )