Support alternative token location #271

kubadz · 2019-10-02T10:52:42Z

Related issue

Proposed changes

Add additional optional configuration to jwt and oauth2_introspection authenticators allowing to set from where (which header or query parameter) the token should be received. The configuration is a token_from field in per-rule-configuration, as described in a linked issue.

Checklist

I have read the contributing guidelines
I have read the security policy
I confirm that this pull request does not address a security
vulnerability. If this pull request addresses a security vulnerability, I
confirm that I got green light (please contact
[email protected]) from the maintainers to push
the changes.
I have added tests that prove my fix is effective or that my feature works
I have added necessary documentation within the code base (if appropriate)
I have documented my changes in the
developer guide (if appropriate)

Further comments

…l pass if request and config is proper

aeneasr · 2019-10-03T12:28:29Z

helper/bearer.go

+	QueryParameter *string `json:"query_parameter"`
+}
+
+func BearerTokenFromRequest(r *http.Request, tokenLocation *BearerTokenLocation) string {


Would it make sense to test this independently?

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

aeneasr · 2019-10-05T10:42:11Z

helper/bearer.go

+		if tokenLocation.Header != nil {
+			token = r.Header.Get(*tokenLocation.Header)
+		} else if tokenLocation.QueryParameter != nil {
+			token = r.FormValue(*tokenLocation.QueryParameter)


same here:

Suggested change

token = r.FormValue(*tokenLocation.QueryParameter)

return r.FormValue(*tokenLocation.QueryParameter)

aeneasr · 2019-10-05T10:43:21Z

helper/bearer.go

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


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)

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

What do you think?

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.

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.

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

@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

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

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.

Ok, done 😄

aeneasr · 2019-10-05T10:44:31Z

helper/bearer.go

+			token = r.Header.Get(*tokenLocation.Header)
+		} else if tokenLocation.QueryParameter != nil {
+			token = r.FormValue(*tokenLocation.QueryParameter)
+		}


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

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

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

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?

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

…ither Header nor Query Param is configured

…ers or query parameters)

kubadz added 8 commits October 1, 2019 13:19

update go.mod (done by IDE)

b4d58b2

add configuration for specifying bearer token location

1472e65

fix test cases descriptions in authenticator jwt test

f2432d1

add unit tests for cases with bearer token in custom location

ce024c9

fix headers in unit tests (use canonicalized keys)

91cf8bf

implement fetching token from custom location

6cc7e65

add setup for oauth2_introspection test so the positive scenarios wil…

55c8eff

…l pass if request and config is proper

goimports fix

815e2bc

kubadz marked this pull request as ready for review October 3, 2019 10:15

kubadz mentioned this pull request Oct 3, 2019

Bump Oathkeeper image to support alternative token location kyma-project/kyma#5874

Merged

aeneasr reviewed Oct 3, 2019

View reviewed changes

kubadz mentioned this pull request Oct 4, 2019

oathkeeper: update authenticators documentation ory/docs#206

Merged

strekm approved these changes Oct 4, 2019

View reviewed changes

kubadz added 3 commits October 4, 2019 14:15

add unit tests for function getting token from request

5b0bd05

fix goimports

e710db1

add minor improvements to descriptions in json schema

594194e

aeneasr reviewed Oct 5, 2019

View reviewed changes

kubadz added 4 commits October 7, 2019 13:23

get token from default header if custom token location is set, but ne…

27669e3

…ither Header nor Query Param is configured

add test case

937fd44

do not require explicit "bearer" type in custom token locations (head…

e68a10d

…ers or query parameters)

update descriptions in json schema

4f51c8c

aeneasr approved these changes Oct 9, 2019

View reviewed changes

aeneasr merged commit fc85ac8 into ory:master Oct 9, 2019

kubadz deleted the alternative-token-location branch October 9, 2019 11:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support alternative token location #271

Support alternative token location #271

kubadz commented Oct 2, 2019 •

edited

Loading

aeneasr Oct 3, 2019

kubadz Oct 4, 2019

aeneasr Oct 5, 2019

aeneasr Oct 5, 2019

kubadz Oct 7, 2019

kubadz Oct 7, 2019

aeneasr Oct 7, 2019

kubadz Oct 7, 2019

aeneasr Oct 7, 2019

piotrmsc Oct 8, 2019

kubadz Oct 8, 2019 •

edited

Loading

aeneasr Oct 8, 2019 •

edited

Loading

kubadz Oct 8, 2019

aeneasr Oct 5, 2019

kubadz Oct 7, 2019

aeneasr Oct 7, 2019

kubadz Oct 7, 2019

aeneasr Oct 7, 2019

kubadz Oct 7, 2019

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

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

Support alternative token location #271

Support alternative token location #271

Conversation

kubadz commented Oct 2, 2019 • edited Loading

Related issue

Proposed changes

Checklist

Further comments

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kubadz Oct 8, 2019 • edited Loading

Choose a reason for hiding this comment

aeneasr Oct 8, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kubadz commented Oct 2, 2019 •

edited

Loading

kubadz Oct 8, 2019 •

edited

Loading

aeneasr Oct 8, 2019 •

edited

Loading