authlogic icon indicating copy to clipboard operation
authlogic copied to clipboard

allow single_access_token via http headers

Open sevgibson opened this issue 4 years ago • 14 comments

Acts exactly like params, but uses headers instead.

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

sevgibson avatar Aug 28 '20 22:08 sevgibson

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.

jaredbeck avatar Aug 28 '20 22:08 jaredbeck

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

Screen Shot 2020-08-28 at 7 08 30 PM

sevgibson avatar Aug 28 '20 23:08 sevgibson

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!

sevgibson avatar Sep 11 '20 13:09 sevgibson

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.

jaredbeck avatar Sep 15 '20 07:09 jaredbeck

@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 avatar Sep 15 '20 12:09 sevgibson

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

💯

sevgibson avatar Sep 15 '20 13:09 sevgibson

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 avatar Sep 15 '20 14:09 tiegz

@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 avatar Sep 15 '20 14:09 tiegz

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 avatar Sep 15 '20 14:09 tiegz

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]

tiegz avatar Sep 15 '20 14:09 tiegz

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

sevgibson avatar Sep 15 '20 15:09 sevgibson

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

sevgibson avatar Sep 15 '20 19:09 sevgibson

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 avatar Sep 21 '20 02:09 tiegz

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!

sevgibson avatar Sep 21 '20 15:09 sevgibson