Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support alternative token location #271

Merged
merged 15 commits into from
Oct 9, 2019
Merged

Support alternative token location #271

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
b4d58b2
update go.mod (done by IDE)
kubadz Oct 1, 2019
1472e65
add configuration for specifying bearer token location
kubadz Oct 1, 2019
f2432d1
fix test cases descriptions in authenticator jwt test
kubadz Oct 1, 2019
ce024c9
add unit tests for cases with bearer token in custom location
kubadz Oct 2, 2019
91cf8bf
fix headers in unit tests (use canonicalized keys)
kubadz Oct 2, 2019
6cc7e65
implement fetching token from custom location
kubadz Oct 2, 2019
55c8eff
add setup for oauth2_introspection test so the positive scenarios wil…
kubadz Oct 2, 2019
815e2bc
goimports fix
kubadz Oct 2, 2019
5b0bd05
add unit tests for function getting token from request
kubadz Oct 4, 2019
e710db1
fix goimports
kubadz Oct 4, 2019
594194e
add minor improvements to descriptions in json schema
kubadz Oct 4, 2019
27669e3
get token from default header if custom token location is set, but ne…
kubadz Oct 7, 2019
937fd44
add test case
kubadz Oct 7, 2019
e68a10d
do not require explicit "bearer" type in custom token locations (head…
kubadz Oct 8, 2019
4f51c8c
update descriptions in json schema
kubadz Oct 8, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 32 additions & 0 deletions .schemas/authenticators.jwt.schema.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,38 @@
},
"scope_strategy": {
"$ref": "https://raw.githubusercontent.com/ory/oathkeeper/master/.schemas/scope_strategy.schema.json#"
},
"token_from": {
"title": "Token From",
"description": "The location of the bearer token.\n If not configured, the token will be received from a default location - 'Authorization' header.\n Only one location (header or query) can be specified.",
"oneOf": [
{
"type": "object",
"required": [
"header"
],
"properties": {
"header": {
"title": "Header",
"type": "string",
"description": "The header that must contain a Bearer token for request authentication. It can't be set along with query_parameter."
}
}
},
{
"type": "object",
"required": [
"query_parameter"
],
"properties": {
"query_parameter": {
"title": "Query Parameter",
"type": "string",
"description": "The query parameter that must contain a Bearer token for request authentication. It can't be set along with header."
}
}
}
]
}
},
"additionalProperties": false
Expand Down
32 changes: 32 additions & 0 deletions .schemas/authenticators.oauth2_introspection.schema.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,38 @@
"items": {
"type": "string"
}
},
"token_from": {
"title": "Token From",
"description": "The location of the bearer token.\n If not configured, the token will be received from a default location - 'Authorization' header.\n Only one location (header or query) can be specified.",
"oneOf": [
{
"type": "object",
"required": [
"header"
],
"properties": {
"header": {
"title": "Header",
"type": "string",
"description": "The header that must contain a Bearer token for request authentication. It can't be set along with query_parameter."
}
}
},
{
"type": "object",
"required": [
"query_parameter"
],
"properties": {
"query_parameter": {
"title": "Query Parameter",
"type": "string",
"description": "The query parameter that must contain a Bearer token for request authentication. It can't be set along with header."
}
}
}
]
}
},
"required": [
Expand Down
1 change: 1 addition & 0 deletions go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ require (
github.com/blang/semver v3.5.1+incompatible
github.com/bxcodec/faker v2.0.1+incompatible
github.com/cenkalti/backoff v2.1.1+incompatible
github.com/codegangsta/negroni v1.0.0 // indirect
github.com/dgrijalva/jwt-go v3.2.0+incompatible
github.com/fsnotify/fsnotify v1.4.7
github.com/ghodss/yaml v1.0.0
Expand Down
25 changes: 22 additions & 3 deletions helper/bearer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,9 +25,28 @@ import (
"strings"
)

func BearerTokenFromRequest(r *http.Request) string {
auth := r.Header.Get("Authorization")
split := strings.SplitN(auth, " ", 2)
const (
defaultAuthorizationHeader = "Authorization"
)

type BearerTokenLocation struct {
Header *string `json:"header"`
QueryParameter *string `json:"query_parameter"`
}

func BearerTokenFromRequest(r *http.Request, tokenLocation *BearerTokenLocation) string {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense to test this independently?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah. That might be a good idea :)
I added tests.

var token string
if tokenLocation != nil {
if tokenLocation.Header != nil {
token = r.Header.Get(*tokenLocation.Header)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't it make more sense to return the value here directly?

Suggested change
token = r.Header.Get(*tokenLocation.Header)
return r.Header.Get(*tokenLocation.Header)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can't return the value directly there, as in your suggestion, because we need to return only the token without the "bearer" part. However, I thought about moving lines 49-53 to a separate function. Then the line of your suggestion would look like:

return getToken(r.Header.Get(*tokenLocation.Header))

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The bearer is only relevant when the header is Authorization as as means of switching authorization modes (e.g. basic, bearer, ...).

However, an explicit query parameter such as ?token=1234 would not include the bearer instruction as the parameter is already explicitly bound to a token. The same applies if we define a custom header such as X-MyApplication-AuthToken. Here too, the context is explicit and clear.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. Then of course we can do it like you suggested :)
From the issue and comments I understood that we only want to change location of the token in the means of changing Header name or switching to query param, without tampering with the value of the Header.
So for example you can change header name in some proxy without changing the value of header itself.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think in query param schema should not be present in the value but in custom header it coud be, this gives more guarantee/flexibility that some libs in developer application will still work. OAuth for example expects Bearer scheme

That would be two authenticators with two separate custom headers IMO

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aeneasr We do not give the possibility to override the scheme and IMO it should remain as it is but with the option to use different header

Copy link
Contributor Author

@kubadz kubadz Oct 8, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aeneasr the last word is yours 😄 Do you want the "bearer" in custom header value to be optional, required or not valid?

Copy link
Member

@aeneasr aeneasr Oct 8, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's very confusing to have the "bearer" in the custom header and the query parameter. If you look at the documentation it also does not become very clear. This is something I believe will make things very awkward when people are trying to use these features as I've never ever in my life have seen something like X-Token-Auth: bearer bla nor ?token=bearer%20bla. So please remove the "bearer" splitting for these two parameters and, if unclear, explicitly document it with examples in the docs.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, done 😄

} else if tokenLocation.QueryParameter != nil {
token = r.FormValue(*tokenLocation.QueryParameter)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here:

Suggested change
token = r.FormValue(*tokenLocation.QueryParameter)
return r.FormValue(*tokenLocation.QueryParameter)

}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible that both if / else if do not get executed? Then token would be empty.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is not possible, as JSON schema for jwt and oauth2_introspection authenticators would not allow for that.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we make this explicit in the code by simply doing a if/else?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can, but if somehow QueryParameter would be a nil, than the program will crash on dereferencing a nil pointer. With "else if" the function will simply return an empty string, like if there was no token in the configured header/query parameter (what may be a proper behavior if there was "alternative token location" set, but not configured - even though with the current schema it is not possible).

I see an alternative solution here: returning an error saying that rule is misconfigured. But I would have to add error handling in the places where this function is called.

What do you think?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, how about simply falling back to the authorization header in the case where both are nil?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, done

} else {
token = r.Header.Get(defaultAuthorizationHeader)
}

split := strings.SplitN(token, " ", 2)
if len(split) != 2 || !strings.EqualFold(split[0], "bearer") {
return ""
}
Expand Down
23 changes: 12 additions & 11 deletions pipeline/authn/authenticator_jwt.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,12 +21,13 @@ type AuthenticatorJWTRegistry interface {
}

type AuthenticatorOAuth2JWTConfiguration struct {
Scope []string `json:"required_scope"`
Audience []string `json:"target_audience"`
Issuers []string `json:"trusted_issuers"`
AllowedAlgorithms []string `json:"allowed_algorithms"`
JWKSURLs []string `json:"jwks_urls"`
ScopeStrategy string `json:"scope_strategy"`
Scope []string `json:"required_scope"`
Audience []string `json:"target_audience"`
Issuers []string `json:"trusted_issuers"`
AllowedAlgorithms []string `json:"allowed_algorithms"`
JWKSURLs []string `json:"jwks_urls"`
ScopeStrategy string `json:"scope_strategy"`
BearerTokenLocation *helper.BearerTokenLocation `json:"token_from"`
}

type AuthenticatorJWT struct {
Expand Down Expand Up @@ -67,16 +68,16 @@ func (a *AuthenticatorJWT) Config(config json.RawMessage) (*AuthenticatorOAuth2J
}

func (a *AuthenticatorJWT) Authenticate(r *http.Request, config json.RawMessage, _ pipeline.Rule) (*AuthenticationSession, error) {
token := helper.BearerTokenFromRequest(r)
if token == "" {
return nil, errors.WithStack(ErrAuthenticatorNotResponsible)
}

cf, err := a.Config(config)
if err != nil {
return nil, err
}

token := helper.BearerTokenFromRequest(r, cf.BearerTokenLocation)
if token == "" {
return nil, errors.WithStack(ErrAuthenticatorNotResponsible)
}

if len(cf.AllowedAlgorithms) == 0 {
cf.AllowedAlgorithms = []string{"RS256"}
}
Expand Down
89 changes: 78 additions & 11 deletions pipeline/authn/authenticator_jwt_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,13 +67,14 @@ func TestAuthenticatorJWT(t *testing.T) {

t.Run("method=authenticate", func(t *testing.T) {
for k, tc := range []struct {
setup func()
d string
r *http.Request
config string
expectErr bool
expectCode int
expectSess *AuthenticationSession
setup func()
d string
r *http.Request
config string
expectErr bool
expectExactErr error
expectCode int
expectSess *AuthenticationSession
}{
{
d: "should fail because no payloads",
Expand All @@ -85,6 +86,69 @@ func TestAuthenticatorJWT(t *testing.T) {
r: &http.Request{Header: http.Header{"Authorization": []string{"bearer invalid.token.sign"}}},
expectErr: true,
},
{
d: "should return error saying that authenticator is not responsible for validating the request, as the token was not provided in a proper location (default)",
r: &http.Request{Header: http.Header{"Foobar": []string{"bearer " + gen(keys[1], jwt.MapClaims{
"sub": "sub",
"exp": now.Add(time.Hour).Unix(),
})}}},
expectErr: true,
expectExactErr: ErrAuthenticatorNotResponsible,
},
{
d: "should return error saying that authenticator is not responsible for validating the request, as the token was not provided in a proper location (custom header)",
r: &http.Request{Header: http.Header{"Authorization": []string{"bearer " + gen(keys[1], jwt.MapClaims{
"sub": "sub",
"exp": now.Add(time.Hour).Unix(),
})}}},
config: `{"token_from": {"header": "X-Custom-Header"}}`,
expectErr: true,
expectExactErr: ErrAuthenticatorNotResponsible,
},
{
d: "should return error saying that authenticator is not responsible for validating the request, as the token was not provided in a proper location (custom query parameter)",
r: &http.Request{
Form: map[string][]string{
"someOtherQueryParam": []string{
fmt.Sprintf("bearer %s", gen(keys[1], jwt.MapClaims{
"sub": "sub",
"exp": now.Add(time.Hour).Unix(),
})),
},
},
Header: http.Header{"Authorization": []string{"bearer " + gen(keys[1], jwt.MapClaims{
"sub": "sub",
"exp": now.Add(time.Hour).Unix(),
})}},
},
config: `{"token_from": {"query_parameter": "token"}}`,
expectErr: true,
expectExactErr: ErrAuthenticatorNotResponsible,
},
{
d: "should pass because the valid JWT token was provided in a proper location (custom header)",
r: &http.Request{Header: http.Header{"X-Custom-Header": []string{"bearer " + gen(keys[1], jwt.MapClaims{
"sub": "sub",
"exp": now.Add(time.Hour).Unix(),
})}}},
config: `{"token_from": {"header": "X-Custom-Header"}}`,
expectErr: false,
},
{
d: "should pass because the valid JWT token was provided in a proper location (custom query parameter)",
r: &http.Request{
Form: map[string][]string{
"token": []string{
fmt.Sprintf("bearer %s", gen(keys[1], jwt.MapClaims{
"sub": "sub",
"exp": now.Add(time.Hour).Unix(),
})),
},
},
},
config: `{"token_from": {"query_parameter": "token"}}`,
expectErr: false,
},
{
d: "should pass because JWT is valid",
r: &http.Request{Header: http.Header{"Authorization": []string{"bearer " + gen(keys[1], jwt.MapClaims{
Expand Down Expand Up @@ -187,7 +251,7 @@ func TestAuthenticatorJWT(t *testing.T) {
expectCode: 401,
},
{
d: "should pass because JWT is missing scope",
d: "should fail because JWT is missing scope",
r: &http.Request{Header: http.Header{"Authorization": []string{"bearer " + gen(keys[2], jwt.MapClaims{
"sub": "sub",
"exp": now.Add(time.Hour).Unix(),
Expand All @@ -197,7 +261,7 @@ func TestAuthenticatorJWT(t *testing.T) {
expectErr: true,
},
{
d: "should pass because JWT issuer is untrusted",
d: "should fail because JWT issuer is untrusted",
r: &http.Request{Header: http.Header{"Authorization": []string{"bearer " + gen(keys[1], jwt.MapClaims{
"sub": "sub",
"exp": now.Add(time.Hour).Unix(),
Expand All @@ -207,7 +271,7 @@ func TestAuthenticatorJWT(t *testing.T) {
expectErr: true,
},
{
d: "should pass because JWT is missing audience",
d: "should fail because JWT is missing audience",
r: &http.Request{Header: http.Header{"Authorization": []string{"bearer " + gen(keys[1], jwt.MapClaims{
"sub": "sub",
"exp": now.Add(time.Hour).Unix(),
Expand Down Expand Up @@ -235,10 +299,13 @@ func TestAuthenticatorJWT(t *testing.T) {
tc.config, _ = sjson.Set(tc.config, "jwks_urls", keys)
session, err := a.Authenticate(tc.r, json.RawMessage([]byte(tc.config)), nil)
if tc.expectErr {
require.Error(t, err)
if tc.expectCode != 0 {
assert.Equal(t, tc.expectCode, herodot.ToDefaultError(err, "").StatusCode(), "Status code mismatch")
}
require.Error(t, err)
if tc.expectExactErr != nil {
assert.EqualError(t, err, tc.expectExactErr.Error())
}
} else {
require.NoError(t, err, "%#v", errors.Cause(err))
}
Expand Down
15 changes: 8 additions & 7 deletions pipeline/authn/authenticator_oauth2_introspection.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,12 +20,13 @@ import (
)

type AuthenticatorOAuth2IntrospectionConfiguration struct {
Scopes []string `json:"required_scope"`
Audience []string `json:"target_audience"`
Issuers []string `json:"trusted_issuers"`
PreAuth *AuthenticatorOAuth2IntrospectionPreAuthConfiguration `json:"pre_authorization"`
ScopeStrategy string `json:"scope_strategy"`
IntrospectionURL string `json:"introspection_url"`
Scopes []string `json:"required_scope"`
Audience []string `json:"target_audience"`
Issuers []string `json:"trusted_issuers"`
PreAuth *AuthenticatorOAuth2IntrospectionPreAuthConfiguration `json:"pre_authorization"`
ScopeStrategy string `json:"scope_strategy"`
IntrospectionURL string `json:"introspection_url"`
BearerTokenLocation *helper.BearerTokenLocation `json:"token_from"`
}

type AuthenticatorOAuth2IntrospectionPreAuthConfiguration struct {
Expand Down Expand Up @@ -70,7 +71,7 @@ func (a *AuthenticatorOAuth2Introspection) Authenticate(r *http.Request, config
return nil, err
}

token := helper.BearerTokenFromRequest(r)
token := helper.BearerTokenFromRequest(r, cf.BearerTokenLocation)
if token == "" {
return nil, errors.WithStack(ErrAuthenticatorNotResponsible)
}
Expand Down
Loading