Skip to content
This repository was archived by the owner on Jul 28, 2021. It is now read-only.
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.

Commit 34fc892

Browse files
author
Mario Hros
committedFeb 29, 2020
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).
1 parent ffc565b commit 34fc892

File tree

16 files changed

+414
-198
lines changed

16 files changed

+414
-198
lines changed
 

‎README.md

+4-1
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,7 @@ Istio uses an Envoy proxy sidecar to mediate all inbound and outbound traffic fo
2929

3030
### Protecting frontend apps
3131

32-
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.
32+
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.
3333

3434
To view the user session information including the session tokens, you can look in the `Authorization` header.
3535

@@ -137,10 +137,13 @@ Depending on whether you're protecting frontend or backend applications, create
137137
| `clientSecretRef` | object | no | A reference secret that is used to authenticate the client. This can be used in place of the `clientSecret`. |
138138
| `clientSecretRef.name` | string |yes | The name of the Kubernetes Secret that contains the `clientSecret`. |
139139
| `clientSecretRef.key` | string | yes | The field within the Kubernetes Secret that contains the `clientSecret`. |
140+
| `callback` | string | **no | Callback URL or URI to return to from the IODC provider (default is relative `oidc/callback` appended to the original request path) |
140141
141142
142143
* 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.
143144
145+
** 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.
146+
144147
```
145148
apiVersion: "security.cloud.ibm.com/v1"
146149
kind: JwtConfig

‎adapter/client/callback.go

+80
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,80 @@
1+
package client
2+
3+
import (
4+
"path"
5+
"regexp"
6+
"strings"
7+
)
8+
9+
var regexpURL = regexp.MustCompile(`(https?)://([^/]+)(/?.*)`)
10+
11+
func callbackDefinitionForClient(c Client) string {
12+
callbackDef := ""
13+
14+
// also support empty clients (for tests without a valid client, using default callbackEndpoint)
15+
if c != nil {
16+
callbackDef = c.Callback()
17+
}
18+
19+
if callbackDef == "" {
20+
// if the callback is empty for the client, use the original behavior:
21+
// /oidc/callback is simply appended to the original path, resulting in many different callback URLs
22+
callbackDef = "oidc/callback"
23+
}
24+
return callbackDef
25+
}
26+
27+
// CallbackURLForTarget returns the absolute URL to which IdP should redirect after authenticating the user.
28+
// This is the speckial URL which is expected to be handled by the adapter under the provided Client configuration.
29+
func CallbackURLForTarget(c Client, requestScheme, requestHost, requestPath string) string {
30+
callbackDef := callbackDefinitionForClient(c)
31+
32+
if requestPath == "" {
33+
requestPath = "/"
34+
}
35+
36+
if callbackDef[0] == '/' {
37+
// callback path absolute on the request host
38+
return requestScheme + "://" + requestHost + callbackDef
39+
}
40+
41+
m := regexpURL.FindStringSubmatch(callbackDef)
42+
if len(m) > 1 {
43+
// callback is full URL
44+
return callbackDef
45+
}
46+
47+
// callback path relative to the target path
48+
if strings.HasSuffix(requestPath, callbackDef) {
49+
// relative callback path already appended, do not append twice
50+
// this can happen if this function is called in the actual callback handler
51+
// (happens after /authorize OIDC provider URL redirects back and we call OIDC provider API again)
52+
callbackDef = ""
53+
}
54+
return requestScheme + "://" + requestHost + path.Join(requestPath, callbackDef)
55+
}
56+
57+
// IsCallbackRequest returns true if the provided request should be handled by the adapter as part of the auth flow
58+
func IsCallbackRequest(c Client, requestScheme, requestHost, requestPath string) bool {
59+
callbackDef := callbackDefinitionForClient(c)
60+
61+
if requestPath == "" {
62+
requestPath = "/"
63+
}
64+
65+
if callbackDef[0] == '/' {
66+
// callback path absolute on the request host
67+
return strings.HasPrefix(requestPath, callbackDef)
68+
}
69+
70+
m := regexpURL.FindStringSubmatch(callbackDef)
71+
if len(m) == 4 {
72+
// callback is full URL, compare parts (case-insensitive just in case)
73+
return strings.EqualFold(m[1], requestScheme) &&
74+
strings.EqualFold(m[2], requestHost) &&
75+
strings.EqualFold(m[3], requestPath)
76+
}
77+
78+
// callback path relative to the target path, thus ending with callbackDef
79+
return strings.HasSuffix(requestPath, callbackDef)
80+
}

‎adapter/client/callback_test.go

+73
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,73 @@
1+
package client
2+
3+
import (
4+
"net/url"
5+
"testing"
6+
7+
"github.com/ibm-cloud-security/app-identity-and-access-adapter/tests/fake"
8+
)
9+
10+
func TestCallbackURLForTarget(t *testing.T) {
11+
tests := []struct {
12+
CallbackDef string
13+
Scheme, Host, Path string
14+
Result string
15+
}{
16+
{ // http scheme test
17+
"",
18+
"http", "test.com", "/admin",
19+
"http://test.com/admin/oidc/callback",
20+
},
21+
{ // default relative callback
22+
"",
23+
"https", "test.com", "/admin",
24+
"https://test.com/admin/oidc/callback",
25+
},
26+
{ // relative URI
27+
"custom/relative/path",
28+
"https", "test.com", "/admin",
29+
"https://test.com/admin/custom/relative/path",
30+
},
31+
{ // relative URI already appended
32+
"custom/relative/path",
33+
"https", "test.com", "/admin/custom/relative/path",
34+
"https://test.com/admin/custom/relative/path",
35+
},
36+
{ // absolute URI
37+
"/absolute/uri/callback",
38+
"https", "test.com", "/admin",
39+
"https://test.com/absolute/uri/callback",
40+
},
41+
{ // full URL (http)
42+
"http://test.com/my/callback",
43+
"http", "test.com", "/admin",
44+
"http://test.com/my/callback",
45+
},
46+
{ // full URL (https)
47+
"https://test.com/my/callback",
48+
"https", "test.com", "/admin",
49+
"https://test.com/my/callback",
50+
},
51+
}
52+
53+
for n, tst := range tests {
54+
cli := fake.NewClientWithCallback(nil, tst.CallbackDef)
55+
56+
res := CallbackURLForTarget(cli, tst.Scheme, tst.Host, tst.Path)
57+
if res != tst.Result {
58+
t.Fatalf("TestCallbackURLForTarget#%d CallbackURLForTarget failed.\n"+
59+
" Expected: %s\n Got: %s", n, tst.Result, res)
60+
}
61+
62+
resURL, err := url.Parse(res)
63+
if err != nil {
64+
t.Fatalf("TestCallbackURLForTarget#%d failed to parse URL returned from CallbackURLForTarget %s: %s",
65+
n, res, err.Error())
66+
}
67+
68+
if !IsCallbackRequest(cli, resURL.Scheme, resURL.Host, resURL.Path) {
69+
t.Fatalf("TestCallbackURLForTarget#%d IsCallbackRequest check not returning true\n"+
70+
" scheme: %s, host: %s, path: %s", n, resURL.Scheme, resURL.Host, resURL.Path)
71+
}
72+
}
73+
}

‎adapter/client/client.go

+6-1
Original file line numberDiff line numberDiff line change
@@ -6,13 +6,14 @@ import (
66
"go.uber.org/zap"
77

88
"github.com/ibm-cloud-security/app-identity-and-access-adapter/adapter/authserver"
9-
"github.com/ibm-cloud-security/app-identity-and-access-adapter/adapter/pkg/apis/policies/v1"
9+
v1 "github.com/ibm-cloud-security/app-identity-and-access-adapter/adapter/pkg/apis/policies/v1"
1010
)
1111

1212
// Client encapsulates an authn/z client object
1313
type Client interface {
1414
Name() string
1515
ID() string
16+
Callback() string
1617
Secret() string
1718
AuthorizationServer() authserver.AuthorizationServerService
1819
ExchangeGrantCode(code string, redirectURI string) (*authserver.TokenResponse, error)
@@ -32,6 +33,10 @@ func (c *remoteClient) ID() string {
3233
return c.ClientID
3334
}
3435

36+
func (c *remoteClient) Callback() string {
37+
return c.ClientCallback
38+
}
39+
3540
func (c *remoteClient) Secret() string {
3641
return c.ClientSecret
3742
}

‎adapter/config/config.go

+5-1
Original file line numberDiff line numberDiff line change
@@ -14,9 +14,12 @@ type Config struct {
1414
// The blockKey is used to encrypt the cookie value
1515
// Valid lengths are 16, 24, or 32 bytes to select AES-128, AES-192, or AES-256.
1616
BlockKeySize IntOptions
17+
// Use Secure attribute for session cookies.
18+
// That ensures they are sent over HTTPS and should be enabled for production!
19+
SecureCookies bool
1720
}
1821

19-
// defaultArgs returns the default configuration size
22+
// NewConfig returns the default configuration
2023
func NewConfig() *Config {
2124
return &Config{
2225
AdapterPort: uint16(47304),
@@ -37,5 +40,6 @@ func NewConfig() *Config {
3740
},
3841
Value: 16,
3942
},
43+
SecureCookies: false,
4044
}
4145
}

‎adapter/pkg/apis/policies/v1/oidc_config.go

+8-7
Original file line numberDiff line numberDiff line change
@@ -17,17 +17,18 @@ type OidcConfig struct {
1717

1818
// OidcConfigSpec is the spec for a OidcConfig resource
1919
type OidcConfigSpec struct {
20-
ClientName string
21-
AuthMethod string `json:"authMethod"`
22-
ClientID string `json:"clientId"`
23-
DiscoveryURL string `json:"discoveryUrl"`
24-
ClientSecret string `json:"clientSecret"`
20+
ClientName string
21+
AuthMethod string `json:"authMethod"`
22+
ClientID string `json:"clientId"`
23+
ClientCallback string `json:"callback"`
24+
DiscoveryURL string `json:"discoveryUrl"`
25+
ClientSecret string `json:"clientSecret"`
2526
ClientSecretRef ClientSecretRef `json:"clientSecretRef"`
2627
}
2728

2829
type ClientSecretRef struct {
29-
Name string `json:"name"`
30-
Key string `json:"key"`
30+
Name string `json:"name"`
31+
Key string `json:"key"`
3132
}
3233

3334
// +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object

‎adapter/policy/engine/engine.go

+7-4
Original file line numberDiff line numberDiff line change
@@ -5,13 +5,13 @@ import (
55
"errors"
66
"strings"
77

8-
"github.com/ibm-cloud-security/app-identity-and-access-adapter/adapter/pkg/apis/policies/v1"
8+
v1 "github.com/ibm-cloud-security/app-identity-and-access-adapter/adapter/pkg/apis/policies/v1"
99

1010
"go.uber.org/zap"
1111

1212
"github.com/ibm-cloud-security/app-identity-and-access-adapter/adapter/policy"
1313
policy2 "github.com/ibm-cloud-security/app-identity-and-access-adapter/adapter/policy/store/policy"
14-
"github.com/ibm-cloud-security/app-identity-and-access-adapter/config/template"
14+
authnz "github.com/ibm-cloud-security/app-identity-and-access-adapter/config/template"
1515
)
1616

1717
const (
@@ -53,6 +53,9 @@ func (m *engine) Evaluate(target *authnz.TargetMsg) (*Action, error) {
5353

5454
// Strip custom path components
5555
if strings.HasSuffix(target.Path, callbackEndpoint) {
56+
// Attempt to strip default /oidc/callback for backward compatibility.
57+
// Now also a custom callbak can be configured in OidcConfig. Custom callbacks
58+
// has to be explicitly supported in the routing so stripping them is not required.
5659
target.Path = strings.Split(target.Path, callbackEndpoint)[0]
5760
} else if strings.HasSuffix(target.Path, logoutEndpoint) {
5861
target.Path = strings.Split(target.Path, logoutEndpoint)[0]
@@ -150,8 +153,8 @@ func createDefaultRules(action Action) []v1.Rule {
150153
case policy.OIDC:
151154
return []v1.Rule{
152155
{
153-
Claim: aud,
154-
Match: "ANY",
156+
Claim: aud,
157+
Match: "ANY",
155158
Values: []string{action.Client.ID()},
156159
},
157160
}

‎adapter/strategy/strategy.go

+1-1
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@ import (
66
policy "istio.io/api/policy/v1beta1"
77

88
"github.com/ibm-cloud-security/app-identity-and-access-adapter/adapter/policy/engine"
9-
"github.com/ibm-cloud-security/app-identity-and-access-adapter/config/template"
9+
authnz "github.com/ibm-cloud-security/app-identity-and-access-adapter/config/template"
1010
)
1111

1212
// Strategy defines the entry point to an authentication handler

‎adapter/strategy/web/utils.go

+9-5
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@ import (
1111
"go.uber.org/zap"
1212

1313
"github.com/ibm-cloud-security/app-identity-and-access-adapter/adapter/client"
14-
"github.com/ibm-cloud-security/app-identity-and-access-adapter/config/template"
14+
authnz "github.com/ibm-cloud-security/app-identity-and-access-adapter/config/template"
1515
)
1616

1717
const (
@@ -82,7 +82,7 @@ func buildTokenCookieName(base string, c client.Client) string {
8282

8383
// generateSessionIDCookie creates a new sessionId cookie
8484
// if the provided value is empty and new id is randomly generated
85-
func generateSessionIDCookie(c client.Client, value *string) *http.Cookie {
85+
func generateSessionIDCookie(c client.Client, value *string, secure bool) *http.Cookie {
8686
var v = randString(15)
8787
if value != nil {
8888
v = *value
@@ -91,8 +91,12 @@ func generateSessionIDCookie(c client.Client, value *string) *http.Cookie {
9191
Name: buildTokenCookieName(sessionCookie, c),
9292
Value: v,
9393
Path: "/",
94-
Secure: false, // TODO: replace on release
95-
HttpOnly: false,
96-
Expires: time.Now().Add(time.Hour * time.Duration(2160)), // 90 days
94+
Secure: secure,
95+
SameSite: http.SameSiteLaxMode,
96+
HttpOnly: true, // Cookie available to HTTP protocol only (no JS access)
97+
//TODO: possible to use Expires instead of Max-Age once Istio supports it,
98+
// see https://github.com/istio/istio/pull/21270
99+
//Expires: time.Now().Add(time.Hour * time.Duration(2160)), // 90 days
100+
MaxAge: 90 * 24 * 60 * 60, // 90 days
97101
}
98102
}

‎adapter/strategy/web/utils_test.go

+1-1
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@ import (
66
"github.com/stretchr/testify/assert"
77

88
"github.com/ibm-cloud-security/app-identity-and-access-adapter/adapter/client"
9-
"github.com/ibm-cloud-security/app-identity-and-access-adapter/config/template"
9+
authnz "github.com/ibm-cloud-security/app-identity-and-access-adapter/config/template"
1010
"github.com/ibm-cloud-security/app-identity-and-access-adapter/tests/fake"
1111
)
1212

‎adapter/strategy/web/web.go

+107-129
Large diffs are not rendered by default.

‎adapter/strategy/web/web_test.go

+84-43
Original file line numberDiff line numberDiff line change
@@ -3,12 +3,13 @@ package webstrategy
33
import (
44
"errors"
55
"net/http"
6+
"net/url"
67
"strings"
78
"sync"
89
"testing"
910
"time"
1011

11-
"github.com/ibm-cloud-security/app-identity-and-access-adapter/adapter/pkg/apis/policies/v1"
12+
v1 "github.com/ibm-cloud-security/app-identity-and-access-adapter/adapter/pkg/apis/policies/v1"
1213
"github.com/ibm-cloud-security/app-identity-and-access-adapter/adapter/validator"
1314

1415
"github.com/gogo/protobuf/types"
@@ -22,14 +23,15 @@ import (
2223
"github.com/ibm-cloud-security/app-identity-and-access-adapter/adapter/config"
2324
err "github.com/ibm-cloud-security/app-identity-and-access-adapter/adapter/errors"
2425
"github.com/ibm-cloud-security/app-identity-and-access-adapter/adapter/policy/engine"
25-
"github.com/ibm-cloud-security/app-identity-and-access-adapter/config/template"
26+
authnz "github.com/ibm-cloud-security/app-identity-and-access-adapter/config/template"
2627
"github.com/ibm-cloud-security/app-identity-and-access-adapter/tests/fake"
2728
)
2829

2930
const (
30-
defaultHost = "tests.io"
31-
defaultBasePath = "/api"
32-
defaultState = "session"
31+
defaultHost = "tests.io"
32+
defaultBasePath = "/api"
33+
defaultState = "session"
34+
defaultOriginalURL = "https://" + defaultHost + defaultBasePath
3335
)
3436

3537
var defaultHashKey = securecookie.GenerateRandomKey(16)
@@ -72,6 +74,45 @@ func TestHandleNewAuthorizationRequest(t *testing.T) {
7274
int32(16),
7375
nil,
7476
},
77+
{ // New Authentication with a custom absolute-form callback URI
78+
generateAuthnzRequest("", "", "", "", ""),
79+
&engine.Action{
80+
Client: fake.NewClientWithCallback(nil, "/my/callback"),
81+
},
82+
&v1beta1.DirectHttpResponse{
83+
Code: 302,
84+
Headers: map[string]string{"location": generateAuthorizationURL(fake.NewClient(nil), "https://tests.io/my/callback", "")},
85+
},
86+
"Redirecting to identity provider",
87+
int32(16),
88+
nil,
89+
},
90+
{ // New Authentication with a custom relative-form callback URI
91+
generateAuthnzRequest("", "", "", "", ""),
92+
&engine.Action{
93+
Client: fake.NewClientWithCallback(nil, "my/callback"),
94+
},
95+
&v1beta1.DirectHttpResponse{
96+
Code: 302,
97+
Headers: map[string]string{"location": generateAuthorizationURL(fake.NewClient(nil), "https://tests.io/api/my/callback", "")},
98+
},
99+
"Redirecting to identity provider",
100+
int32(16),
101+
nil,
102+
},
103+
{ // New Authentication with a custom full callback URL
104+
generateAuthnzRequest("", "", "", "", ""),
105+
&engine.Action{
106+
Client: fake.NewClientWithCallback(nil, "https://my-adapter-domain.com/my/callback"),
107+
},
108+
&v1beta1.DirectHttpResponse{
109+
Code: 302,
110+
Headers: map[string]string{"location": generateAuthorizationURL(fake.NewClient(nil), "https://my-adapter-domain.com/my/callback", "")},
111+
},
112+
"Redirecting to identity provider",
113+
int32(16),
114+
nil,
115+
},
75116
}
76117

77118
for _, test := range tests {
@@ -190,6 +231,7 @@ func TestRefreshTokenFlow(t *testing.T) {
190231
t.Run("refresh", func(t *testing.T) {
191232
t.Parallel()
192233
api := WebStrategy{
234+
ctx: config.NewConfig(),
193235
tokenCache: new(sync.Map),
194236
encrpytor: defaultSecureCookie,
195237
tokenUtil: MockValidator{
@@ -281,9 +323,12 @@ func TestCodeCallback(t *testing.T) {
281323
},
282324
}
283325

284-
encryptCookie := func(c *OidcCookie) string {
285-
cookie, _ := api.generateEncryptedCookie("oidc-cookie-id", c)
286-
return cookie.String()
326+
encryptState := func(c *OidcState) string {
327+
state, err := api.encryptState(fake.NewClient(nil).ID(), c)
328+
if err != nil {
329+
panic(err)
330+
}
331+
return url.QueryEscape(state)
287332
}
288333

289334
var tests = []struct {
@@ -294,7 +339,7 @@ func TestCodeCallback(t *testing.T) {
294339
message string
295340
err error
296341
}{
297-
{ // missing state cookie
342+
{ // missing state
298343
"",
299344
nil,
300345
generateAuthnzRequest("", "code", "", callbackEndpoint, ""),
@@ -304,50 +349,40 @@ func TestCodeCallback(t *testing.T) {
304349
"state parameter not provided",
305350
nil,
306351
},
307-
{ // invalid state - missing
352+
{ // bad state (unable to url-decode)
308353
"",
309354
nil,
310-
generateAuthnzRequest(encryptCookie(&OidcCookie{Value: ""}), "code", "", callbackEndpoint, defaultState),
355+
generateAuthnzRequest("", "code", "", callbackEndpoint, "inva%FFli%X0d-state"),
311356
&v1beta1.DirectHttpResponse{
312357
Code: 401,
313358
},
314-
"missing state parameter",
359+
"bad state parameter",
315360
nil,
316361
},
317-
{ // invalid state - missing expiration
362+
{ // invalid state - not a properly encrypted state string
318363
"",
319364
nil,
320-
generateAuthnzRequest(encryptCookie(&OidcCookie{Value: defaultState}), "code", "", callbackEndpoint, defaultState),
321-
&v1beta1.DirectHttpResponse{
322-
Code: 401,
323-
},
324-
"missing state parameter",
325-
nil,
326-
},
327-
{ // invalid state - mismatch
328-
"",
329-
nil,
330-
generateAuthnzRequest(encryptCookie(defaultSessionOidcCookie()), "code", "", callbackEndpoint, "session2"),
365+
generateAuthnzRequest("", "code", "", callbackEndpoint, "invalidencryptedstate"),
331366
&v1beta1.DirectHttpResponse{
332367
Code: 401,
333368
},
334369
"invalid state parameter",
335370
nil,
336371
},
337-
{ // state not returned from IdP
372+
{ // invalid state - expired (empty expiration date)
338373
"",
339374
nil,
340-
generateAuthnzRequest(encryptCookie(defaultSessionOidcCookie()), "code", "", callbackEndpoint, ""),
375+
generateAuthnzRequest("", "code", "", callbackEndpoint, encryptState(&OidcState{})),
341376
&v1beta1.DirectHttpResponse{
342377
Code: 401,
343378
},
344-
"missing state parameter from callback",
379+
"invalid state parameter",
345380
nil,
346381
},
347382
{ // could not exchange grant code
348383
"",
349384
defaultFailureTokenResponse("problem getting tokens"),
350-
generateAuthnzRequest(encryptCookie(defaultSessionOidcCookie()), "code", "", callbackEndpoint, defaultState),
385+
generateAuthnzRequest("", "code", "", callbackEndpoint, encryptState(&OidcState{Expiration: time.Now().Add(1 * time.Minute)})),
351386
&v1beta1.DirectHttpResponse{
352387
Code: 401,
353388
},
@@ -361,7 +396,7 @@ func TestCodeCallback(t *testing.T) {
361396
IdentityToken: "invalid_token",
362397
},
363398
},
364-
generateAuthnzRequest(encryptCookie(defaultSessionOidcCookie()), "code", "", callbackEndpoint, defaultState),
399+
generateAuthnzRequest("", "code", "", callbackEndpoint, encryptState(&OidcState{Expiration: time.Now().Add(1 * time.Minute)})),
365400
&v1beta1.DirectHttpResponse{
366401
Code: 401,
367402
},
@@ -371,11 +406,11 @@ func TestCodeCallback(t *testing.T) {
371406
{ // success
372407
"",
373408
defaultSuccessTokenResponse(),
374-
generateAuthnzRequest(encryptCookie(defaultSessionOidcCookie()), "code", "", callbackEndpoint, defaultState),
409+
generateAuthnzRequest("", "code", "", callbackEndpoint, encryptState(&OidcState{Expiration: time.Now().Add(1 * time.Minute), OriginalURL: defaultOriginalURL})),
375410
&v1beta1.DirectHttpResponse{
376411
Code: 302,
377412
Headers: map[string]string{
378-
"location": "https://" + defaultHost + defaultBasePath,
413+
"location": defaultOriginalURL,
379414
setCookie: "oidc-test-id",
380415
},
381416
},
@@ -385,7 +420,7 @@ func TestCodeCallback(t *testing.T) {
385420
{ // success w/new redirect
386421
"https://" + defaultHost + "/another_path",
387422
defaultSuccessTokenResponse(),
388-
generateAuthnzRequest(encryptCookie(defaultSessionOidcCookie()), "code", "", callbackEndpoint, defaultState),
423+
generateAuthnzRequest("", "code", "", callbackEndpoint, encryptState(&OidcState{Expiration: time.Now().Add(1 * time.Minute)})),
389424
&v1beta1.DirectHttpResponse{
390425
Code: 302,
391426
Headers: map[string]string{
@@ -443,6 +478,7 @@ func TestLogout(t *testing.T) {
443478
}
444479

445480
api := WebStrategy{
481+
ctx: config.NewConfig(),
446482
tokenUtil: MockValidator{},
447483
}
448484

@@ -466,24 +502,26 @@ func compareDirectHttpResponses(t *testing.T, r *authnz.HandleAuthnZResponse, ex
466502
continue
467503
}
468504
assert.Equal(t, expected.Code, response.Code)
469-
for key, value := range expected.Headers {
470-
if key == setCookie {
505+
for expectKey, expectValue := range expected.Headers {
506+
if expectKey == setCookie {
471507
assert.NotEmpty(t, response.Headers[setCookie])
472508
continue
473509
}
474-
if !strings.HasPrefix(response.Headers[key], value) {
475-
assert.Fail(t, "incorrect header value: '"+key+"'\n found: "+value+"\n expected:"+response.Headers[key])
510+
if !strings.HasPrefix(response.Headers[expectKey], expectValue) {
511+
assert.Fail(t, "incorrect header value: '"+expectKey+
512+
"'\n found: "+response.Headers[expectKey]+
513+
"\n expected: "+expectValue)
476514
}
477515
}
478516

479517
assert.Equal(t, expected.Body, response.Body)
480518
}
481519
}
482520

483-
func defaultSessionOidcCookie() *OidcCookie {
484-
return &OidcCookie{
485-
Value: defaultState,
486-
Expiration: time.Now().Add(time.Hour),
521+
func defaultSessionOidcCookie() *OidcState {
522+
return &OidcState{
523+
OriginalURL: defaultOriginalURL,
524+
Expiration: time.Now().Add(time.Hour),
487525
}
488526
}
489527

@@ -543,8 +581,11 @@ func createCookie() *http.Cookie {
543581
Name: "oidc-cookie-id",
544582
Value: defaultState,
545583
Path: "/",
546-
Secure: false, // TODO: replace on release
547-
HttpOnly: false,
548-
Expires: time.Now().Add(time.Hour * time.Duration(2160)),
584+
Secure: false, // disabled for tests
585+
HttpOnly: true, // Cookie available to HTTP protocol only (no JS access)
586+
//TODO: possible to use Expires instead of Max-Age once Istio supports it,
587+
// see https://github.com/istio/istio/pull/21270
588+
//Expires: time.Now().Add(time.Hour * time.Duration(2160)), // 90 days
589+
MaxAge: 90 * 24 * 60 * 60, // 90 days
549590
}
550591
}

‎cmd/main.go

+1
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@ func getCmd() *cobra.Command {
3535
f.Int8VarP(&sa.Level, "level", "l", sa.Level, "Set output log level. Range [-1, 7].")
3636
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.")
3737
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.")
38+
f.BoolVarP(&sa.SecureCookies, "secure-cookies", "", sa.SecureCookies, "Use Secure attribute for session cookies to ensure they are sent over HTTPS only.")
3839

3940
return cmd
4041
}

‎helm/appidentityandaccessadapter/templates/deployment.yaml

+3
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,9 @@ spec:
3030
- "--level={{ .Values.logging.level }}"
3131
- "--hash-key={{ .Values.keys.hashKeySize }}"
3232
- "--block-key={{ .Values.keys.blockKeySize }}"
33+
{{ if .Values.secureCookies }}
34+
- "--secure-cookies"
35+
{{ end }}
3336

3437
imagePullPolicy: {{ .Values.image.pullPolicy}}
3538
ports:

‎helm/appidentityandaccessadapter/values.yaml

+4
Original file line numberDiff line numberDiff line change
@@ -57,3 +57,7 @@ logging:
5757
## from -1 to 7
5858
## See zapcore: https://godoc.org/go.uber.org/zap/zapcore#Level
5959
level: 0
60+
61+
# Use Secure attribute for session cookies.
62+
# That ensures they are sent over HTTPS and should be enabled for production!
63+
secureCookies: false

‎tests/fake/client.go

+21-5
Original file line numberDiff line numberDiff line change
@@ -8,11 +8,12 @@ type TokenResponse struct {
88
}
99

1010
type Client struct {
11-
Server authserver.AuthorizationServerService
12-
TokenResponse *TokenResponse
13-
ClientName string
14-
ClientID string
15-
ClientSecret string
11+
Server authserver.AuthorizationServerService
12+
TokenResponse *TokenResponse
13+
ClientName string
14+
ClientID string
15+
ClientCallback string
16+
ClientSecret string
1617
}
1718

1819
func NewClient(tokenResponse *TokenResponse) *Client {
@@ -25,6 +26,17 @@ func NewClient(tokenResponse *TokenResponse) *Client {
2526
}
2627
}
2728

29+
func NewClientWithCallback(tokenResponse *TokenResponse, callback string) *Client {
30+
return &Client{
31+
Server: NewAuthServer(),
32+
ClientName: "name",
33+
ClientID: "id",
34+
ClientCallback: callback,
35+
ClientSecret: "secret",
36+
TokenResponse: tokenResponse,
37+
}
38+
}
39+
2840
func (m *Client) Name() string {
2941
return m.ClientName
3042
}
@@ -33,6 +45,10 @@ func (m *Client) ID() string {
3345
return m.ClientID
3446
}
3547

48+
func (m *Client) Callback() string {
49+
return m.ClientCallback
50+
}
51+
3652
func (m *Client) Secret() string {
3753
return m.ClientSecret
3854
}

0 commit comments

Comments
 (0)
This repository has been archived.