allow single_access_token via http headers #728

sevgibson · 2020-08-28T22:34:26Z

Acts exactly like params, but uses headers instead.

curl -H "user_credentials: 4LiXF7FiGUppIPubBPey" https://www.domain.com

jaredbeck · 2020-08-28T22:40:28Z

Hi Scott, thanks for the contribution. Before we review this, can you please remind me of some of the security considerations re: putting plaintext secrets in HTTP headers? My dim recollection is there's no difference compared to e.g. a URL param, ie. the security of either approach depends on TLS and SNI? Can you please summarize the concerns and/or point me to some reading material? Thanks.

sevgibson · 2020-08-28T23:14:31Z

Thank you for your quick reply @jaredbeck !

I believe your recollection is exactly correct. And, in fact, OAuth generally uses a Header for the Authorization: Bearer token.

From https://en.wikipedia.org/wiki/HTTPS:

Because HTTPS piggybacks HTTP entirely on top of TLS, the entirety of the underlying
HTTP protocol can be encrypted. This includes the request URL (which particular web
page was requested), query parameters, headers, and cookies (which often contain
identifying information about the user).

I had trouble finding such a clear statement from the RFC, but it basically says that after the initial connection, ALL data must be sent as TLS "application data". This would include the URL, query parameters, headers, and payload body. https://tools.ietf.org/html/rfc2818#section-2

jaredbeck

Hi Scott, I've finished my first review. It looks like you've copied all the params methods (and tests) carefully, so it all looks good at first glance. I'd like to address the level of duplication first. Once we've addressed that, I think a final review will go smoothly. Thanks!

jaredbeck · 2020-09-10T02:08:45Z

lib/authlogic/session/base.rb

+        else
+          %i[all any].include?(single_access_allowed_request_types)
+        end
+      end


It seems headers_enabled_by_allowed_request_types? and params_enabled_by_allowed_request_types? have exactly the same contents. Let's fix that duplication by having a single method. What do you think about a name like request_type_allowed?, or single_access_token_allowed_by_request_type??

lib/authlogic/session/base.rb

jaredbeck · 2020-09-10T02:15:49Z

test/session_test/headers_test.rb

+          UserSession.single_access_allowed_request_types
+        )
+      end
+    end


To avoid duplicating ConfigTest verbatim, perhaps we should combine params_test.rb and headers_test.rb and call it single_access_token_test.rb.

@jaredbeck Note that I don't necessarily like adding a method that isn't really a test_ method to the class SessionTest::SingleAccessTokenTest::InstanceMethodsTest but it was the best way I could see to reduce the duplication, and not hide the actual assertions off in a helper file (which would be far worse for the reader).

lib/authlogic/session/base.rb

sevgibson · 2020-09-11T13:45:45Z

Overall, I realize I totally missed DRY the first time -- not a novice programmer, but new to submitting PRs to open source. Since this was security, I was a bit afraid to make any of these slight refactors without having a review by someone much more familiar with the code. Thank you for your patience!

jaredbeck

Other than my suggestion about maybe using Set, this LGTM.

Please add an item to the changelog under Unreleased -> Added.

@tiegz can you also take a quick look at this PR, please?

lib/authlogic/session/base.rb

Co-authored-by: Jared Beck <[email protected]>

tiegz

This looks like a useful addition to Authlogic, @sevgibson .

Only concern I have is: is this a breaking change if it's enabled by default?

Downstream users should already be aware that url-params can authenticate, if they have the single_access_token column. Will it be surprising then that someone could authenticate via a header suddenly too? (i.e. could single-access-token-replay attacks suddenly become easier to conceal?)

The answer might be "no", just curious what you all think.

lib/authlogic/session/base.rb

test/session_test/single_access_token_test.rb

jaredbeck · 2020-09-15T07:25:04Z

Thanks for the review, Tieg.

is this a breaking change if it's enabled by default? Downstream users should already be aware that url-params can authenticate, if they have the single_access_token column. Will it be surprising then that someone could authenticate via a header suddenly too? (i.e. could single-access-token-replay attacks suddenly become easier to conceal?)

It's not a breaking change as far as our API is concerned, and we haven't yet identified any meaningful impact on security.

I'm glad you mentioned replay attacks, though. Can you talk a bit more about that? An attacker intercepts the single_access_token? Perhaps we should only accept single_access_token over TLS connections. Now that would be a breaking change 😁

PS: I find the term "single access" misleading, because it sounds like "single use", but that's probably a discussion for another time.

Co-authored-by: Tieg Zaharia <[email protected]>

sevgibson · 2020-09-15T12:55:45Z

@tiegz @jaredbeck I agree that this should not be enabled by default. This makes perfect sense to me because my intent was not to make a change that would become active by default for existing apps. If you both agree, then I'll submit an update to default to nil which should be disabled. Please confirm if you both agree. Thank you!

sevgibson · 2020-09-15T13:11:13Z

Perhaps we should only accept single_access_token over TLS connections. Now that would be a breaking change 😁

💯

tiegz · 2020-09-15T14:48:13Z

I'm glad you mentioned replay attacks, though. Can you talk a bit more about that? An attacker intercepts the single_access_token?

Scenario: a user's single access token is accidentally leaked.

Currently: it would be easy to track down usages of that token by looking at url params in your logs
After this change: most people don't log all HTTP headers, so it would be easier for an attacker to use this token and make it look like they had just used session authentication (which might appear more legit).

That's my only worry -- but it assumes things about logs and importance of params vs headers, so maybe it's NBD.

Perhaps we should only accept single_access_token over TLS connections. Now that would be a breaking change 😁

Not a bad idea TBH!

PS: I find the term "single access" misleading, because it sounds like "single use", but that's probably a discussion for another time.

Totally agreed. At a past job, I created an Authlogic extension that wrapped database-backed tokens and used the single_access? feature in Authlogic. So you can land on a url with that token, but the token is pegged to that page, and gave you lesser power, and it would eventually expire. I've always thought that'd be useful as part of Authlogic. You could even use the single_access_token in the database as a key to derive these one-time-use tokens, so you don't need to store them anywhere (similar to JWTs).

tiegz · 2020-09-15T14:49:30Z

@tiegz @jaredbeck I agree that this should not be enabled by default. This makes perfect sense to me because my intent was not to make a change that would become active by default for existing apps. If you both agree, then I'll submit an update to default to nil which should be disabled. Please confirm if you both agree. Thank you!

I personally prefer that to avoid any surprises, but it's up to you @jaredbeck (and then maybe we could even enable it by default in the next major version?)

tiegz · 2020-09-15T14:52:24Z

Also, wanted to mention an experience I've had in the past:

I was using something very similar to single_access_token for authentication in the url. We discovered that the url was being cached in Facebook's caching layer because it was triggering 2FA SMS codes spuriously for users. Given that, plus how often people forget to scrub params in logs, it's worth considering how easy it is to leak a single_access_token [in url params].

tiegz · 2020-09-15T14:57:36Z

And another experience I just remembered 😆 :

Got hit by a credential-stuffing attack where the attacker was using Basic Auth to attempt mass logins. Luckily we had disabled Authlogic's Basic Auth (which I think was disabled as a default years ago too). But since we weren't logging the Basic Auth header anywhere, the only way I could figure that out was accidentally while looking at raw requests 🤮

[Lesson from that being: headers are less obvious than params]

sevgibson · 2020-09-15T15:33:18Z

@tiegz I appreciate how thoroughly you're considering the security implications, since of course most security holes (aside from intentional trojan horse) are due to holes that are simply overlooked. Having said that, I think that having this disabled by default leaves the assessment of the risks specifically related to using single_access_token in headers in the hands of the developer using the gem.

TBH, using single_access_token is a pretty low bar when compared with other ouath style protocols (JWT, etc). The problem of course is the level of effort when starting a new project. And once committed down that path, it becomes very difficult to transition.

jaredbeck

Some minor stuff. I think we're close.

lib/authlogic/session/base.rb

Co-authored-by: Jared Beck <[email protected]>

sevgibson · 2020-09-15T19:01:46Z

@jaredbeck I had to fix specs, and also decided to explicitly assert the defaults for both params_key and headers_key (since they are different now).

jaredbeck

LGTM. @tiegz please merge (squash) if you're happy

tiegz · 2020-09-21T02:13:27Z

Looks really close to me, but I can see 2 blockers:

we need to access controller.request.headers instead of controllers.headers (which is for response headers)
to comply with CGI, Rack servers will uppercase the header name, convert hyphens to underscores, and prefix with "HTTP_". Maybe we can do that in the headers_credentials method, similar to how Rails does it?

sevgibson · 2020-09-21T15:38:06Z

Looks really close to me, but I can see 2 blockers:

we need to access controller.request.headers instead of controllers.headers (which is for response headers)

to comply with CGI, Rack servers will uppercase the header name, convert hyphens to underscores, and prefix with "HTTP_". Maybe we can do that in the headers_credentials method, similar to how Rails does it?

@tiegz Ok, makes sense -- this may take me a little time, so marking this as draft for now, and will request review again when both are done. Thanks, both for working through this PR with me!

tiegz

@sevgibson I've added PR suggestions for the 2 fixes I mentioned. I've tested this locally and it fixes the feature for me.

tiegz · 2020-11-26T01:06:30Z

lib/authlogic/session/base.rb

+        # Setting headers_key to nil is the accepted way to disable
+        # single_access_token in headers.
+        return nil if headers_key.nil?
+        controller.headers[headers_key]


Suggested change

controller.headers[headers_key]

controller.request.headers[headers_key]

tiegz · 2020-11-26T01:06:44Z

test/test_helper.rb

@@ -269,6 +269,14 @@ def unset_params
      controller.params["user_credentials"] = nil
    end

+    def set_headers_for(user)
+      controller.headers["user_credentials"] = user.single_access_token


Suggested change

controller.headers["user_credentials"] = user.single_access_token

controller.request.headers["user_credentials"] = user.single_access_token

tiegz · 2020-11-26T01:06:51Z

test/test_helper.rb

+    end
+
+    def unset_headers
+      controller.headers["user_credentials"] = nil


Suggested change

controller.headers["user_credentials"] = nil

controller.request.headers["user_credentials"] = nil

tiegz · 2020-11-26T01:07:08Z

lib/authlogic/controller_adapters/abstract_adapter.rb

+      def headers
+        controller.headers
+      end
+


Suggested change

def headers

controller.headers

end

tiegz · 2020-11-26T01:08:39Z

lib/authlogic/session/base.rb

@@ -1849,6 +1894,10 @@ def params_key
        build_key(self.class.params_key)
      end

+      def headers_key
+        build_key(self.class.headers_key)


Suggested change

build_key(self.class.headers_key)

# Rack servers will uppercase header names, convert hyphens to undercsores, and prefix with "HTTP_" in order

# to comply with CGI. We translate these converted headers in a similar way to Rails:

# https://github.com/rails/rails/blob/6-0-stable/actionpack/lib/action_dispatch/http/headers.rb#L123-L126

"HTTP_" + build_key(self.class.headers_key).upcase.tr("-", "_")

sevgibson added 2 commits August 28, 2020 17:08

allow single_access_token via http headers

ffc4bd0

remove unnecessary setup

fdf5999

jaredbeck requested changes Sep 10, 2020

View reviewed changes

sevgibson added 2 commits September 11, 2020 09:34

DRY it up

fab002d

forgot to modify test module name

4254ef2

sevgibson requested a review from jaredbeck September 11, 2020 13:45

jaredbeck approved these changes Sep 14, 2020

View reviewed changes

lib/authlogic/session/base.rb Outdated Show resolved Hide resolved

sevgibson and others added 2 commits September 14, 2020 18:45

Use suggested Set intersect instead of array &

48398df

Co-authored-by: Jared Beck <[email protected]>

add changelog entry

781e807

tiegz reviewed Sep 15, 2020

View reviewed changes

lib/authlogic/session/base.rb Outdated Show resolved Hide resolved

test/session_test/single_access_token_test.rb Outdated Show resolved Hide resolved

sevgibson and others added 2 commits September 15, 2020 08:31

punctuation

1d521eb

Co-authored-by: Tieg Zaharia <[email protected]>

test method naming clarity

2d6c1ab

Co-authored-by: Tieg Zaharia <[email protected]>

fix whitespace

f28220f

jaredbeck requested changes Sep 15, 2020

View reviewed changes

lib/authlogic/session/base.rb Outdated Show resolved Hide resolved

lib/authlogic/session/base.rb Show resolved Hide resolved

lib/authlogic/session/base.rb Show resolved Hide resolved

sevgibson and others added 3 commits September 15, 2020 14:51

Update lib/authlogic/session/base.rb

84d991a

Co-authored-by: Jared Beck <[email protected]>

default headers_key is nil

8ae7b9a

Update lib/authlogic/session/base.rb

a25489f

Co-authored-by: Jared Beck <[email protected]>

sevgibson requested a review from jaredbeck September 15, 2020 19:06

jaredbeck approved these changes Sep 15, 2020

View reviewed changes

sevgibson marked this pull request as draft September 21, 2020 23:24

tiegz requested changes Nov 26, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

allow single_access_token via http headers #728

allow single_access_token via http headers #728

sevgibson commented Aug 28, 2020

jaredbeck commented Aug 28, 2020

sevgibson commented Aug 28, 2020

jaredbeck left a comment

jaredbeck Sep 10, 2020

jaredbeck Sep 10, 2020

sevgibson Sep 11, 2020

sevgibson commented Sep 11, 2020

jaredbeck left a comment •

edited

Loading

tiegz left a comment

jaredbeck commented Sep 15, 2020

sevgibson commented Sep 15, 2020 •

edited

Loading

sevgibson commented Sep 15, 2020

tiegz commented Sep 15, 2020

tiegz commented Sep 15, 2020

tiegz commented Sep 15, 2020 •

edited

Loading

tiegz commented Sep 15, 2020 •

edited

Loading

sevgibson commented Sep 15, 2020

jaredbeck left a comment

sevgibson commented Sep 15, 2020

jaredbeck left a comment •

edited

Loading

tiegz commented Sep 21, 2020

sevgibson commented Sep 21, 2020

tiegz left a comment

tiegz Nov 26, 2020

tiegz Nov 26, 2020

tiegz Nov 26, 2020

tiegz Nov 26, 2020

tiegz Nov 26, 2020

	controller.headers[headers_key]
	controller.request.headers[headers_key]

	controller.headers["user_credentials"] = user.single_access_token
	controller.request.headers["user_credentials"] = user.single_access_token

-        build_key(self.class.headers_key)
+        # Rack servers will uppercase header names, convert hyphens to undercsores, and prefix with "HTTP_" in order
+        # to comply with CGI. We translate these converted headers in a similar way to Rails:
+        # https://github.com/rails/rails/blob/6-0-stable/actionpack/lib/action_dispatch/http/headers.rb#L123-L126
+        "HTTP_" + build_key(self.class.headers_key).upcase.tr("-", "_")

allow single_access_token via http headers #728

Are you sure you want to change the base?

allow single_access_token via http headers #728

Conversation

sevgibson commented Aug 28, 2020

jaredbeck commented Aug 28, 2020

sevgibson commented Aug 28, 2020

jaredbeck left a 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

sevgibson commented Sep 11, 2020

jaredbeck left a comment • edited Loading

Choose a reason for hiding this comment

tiegz left a comment

Choose a reason for hiding this comment

jaredbeck commented Sep 15, 2020

sevgibson commented Sep 15, 2020 • edited Loading

sevgibson commented Sep 15, 2020

tiegz commented Sep 15, 2020

tiegz commented Sep 15, 2020

tiegz commented Sep 15, 2020 • edited Loading

tiegz commented Sep 15, 2020 • edited Loading

sevgibson commented Sep 15, 2020

jaredbeck left a comment

Choose a reason for hiding this comment

sevgibson commented Sep 15, 2020

jaredbeck left a comment • edited Loading

Choose a reason for hiding this comment

tiegz commented Sep 21, 2020

sevgibson commented Sep 21, 2020

tiegz left a 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

jaredbeck left a comment •

edited

Loading

sevgibson commented Sep 15, 2020 •

edited

Loading

tiegz commented Sep 15, 2020 •

edited

Loading

tiegz commented Sep 15, 2020 •

edited

Loading

jaredbeck left a comment •

edited

Loading