Support hashed tokens

[jeremywadsack]

This is related to #217 but I thought it was worth a separate discussion.

I'd like to have a callback that lets me add more security around token storage. As the token is effectively a password, I'd like to be able to one-way hash it.

I think the easiest implementation for that would be to create a custom TokenComparator that hashes the provided token before comparing to the stored one. But currently SimpleTokenAuthentication :: TokenAuthenticationHandler#token_comparator is private.

Additionally, I would add some before_save handler that happens after the token is created (although see my comments on #292). That would be where I'd add my hashing logic. I can add that to the model myself, by adding after acts_as_token_authenticatable, so it may not need to be a hook, just a documented example to show how to do it.

But I feel that this should be a strategy that simple_token_authentication includes. That is, a configuration option:

class User < ActiveRecord::Base
  acts_as_token_authenticatable hashed_token: true

[gonzalo-bulnes]

Hi, I like the idea @jeremywadsack. The token comparator is currently private because I didn't want to extend the gem API unnecessarily. However, as you can notice, the dependency on the TokenComparator itself is encapsulated by an accessor, which was waiting for a use case for comparator switching logic 😉

I've commented before about not considering worth hashing authentication tokens, that being said, it was a while ago and I'm not against reconsidering it... If you're ready to give it a go, I'll be happy to assist you in the implementation.

[jeremywadsack]

@gozalo-bulnes thanks for the thoughtful response. Sorry I missed that previous discussion. Here's my counter argument: - Agree that client's protection of token is likely to be less secure than our database but we have very little control over that. That doesn't mean we shouldn't do everything we can to secure what we can protect. - If someone gets a client's token from their side, that's one account compromised. If someone gets access to our database, that's all accounts compromised. This is exactly the nature of the large security breaches over the last few years. It would be detrimental to our business. I do recognize that for a large percentage of projects, access to the database with tokens would also mean access to the core data, making token hashing moot. - As we use tokens for API access, resetting tokens is not painless. Their automated data feeds would break and they'd likely have to engage their IT staff to rotate the token on their end. Does that make sense? Am I missing something or over-thinking this?

On Sun, Aug 6, 2017 at 6:10 AM Gonzalo Bulnes Guilpain < ***@***.***> wrote: Hi, I like the idea @jeremywadsack <https://github.com/jeremywadsack>. The token comparator is currently private because I didn't want to extend the gem API unnecessarily. However, as you can notice, the dependency on the TokenComparator itself is encapsulated by an accessor <https://github.com/gonzalo-bulnes/simple_token_authentication/blob/v1.15.1/lib/simple_token_authentication/token_authentication_handler.rb#L86-L88>, which was waiting for a use case for comparator switching logic 😉 I've commented before about not considering worth hashing authentication tokens <#82 (comment)>, that being said, it was a while ago and I'm not against reconsidering it... If you're ready to give it a go, I'll be happy to assist you in the implementation. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#300 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAOurMPHeIlm0La2RTSqFSM9KtNttmTHks5sVbtTgaJpZM4NSTEN> .

-- Jeremy Wadsack

[gonzalo-bulnes]

Hi @jeremywadsack,

I don't think you're over-thinking it, and your scenario makes sense. It's true that for API clients the price of rotating tokens is higher than for users who sign in / sign out (i.e. who have other means of authentication).

Would it be enough to provide a generic HashedTokenComparator and the corresponding option? (e.g. acts_as_token_authenticatable compare_tokens_with: hashed_token_comparator, considering that we can think better about those names.)

Providing an alternate (but predefined) token comparator was my first thought, since the hashing algorithm would benefit from being public anyway. (The security of a good algorithm would only depend on the properties of the hashing function anyway, and review helps getting things right.)

But that wouldn't give you a chance to use a salt... As I see it, the tokens being reasonably long pseudo-random strings, rainbow tables are unlikely to be used... I mean that they would be rather expensive and there would likely be cheaper ways to gain access than storing such massive databases of hashes. That being said, what do you think? Would you be comfortable with that?

(Also, without thinking much about it I would say that any hashing function that is implemented in the Ruby standard library, would be fine to me.)

[jeremywadsack]

@gonzalo-bulnes All of that makes sense. Also, that's a fair point about the salt/IV versus rainbow tables on long, random strings. If I suggested a salt, I was probably thinking about being able to decrypt the key (to show a client their key), but I don't think that's really valuable. I think I'd prefer to stick with the basic case of a one-way hash and not worry about the use of a salt.

I can take a stab at a built-in hashing implementation. SHA2 is probably fine, but I don't have enough security knowledge to make a statement on that.

[gonzalo-bulnes]

I'll ask around about SHA2 on my side, if you open the PR early I can help you adding the config option, or anything else : )

I find convenient to use PRs in the following mindset, so no need to wait for things to be done:

When you need feedback or help, or you think the branch is ready for merging, open a pull request (source)

[jeremywadsack]

While I haven't had time to look into making changes, I have been thinking about the best approach here. For long-lived tokens, we should treat them like passwords. Under Rails 5 you can use has_secure_password to hash a field with bcrypt (and handle verification). Moving forward, I suggest that we could change the comparator to be aware of has_secure_password (in whatever way is needed if any). Ideally then the only change the developer using the gem needs to make is to use the has_secure_password process when setting up the token field and the gem "just works".

This is speculative. I haven't investigated what it would take to make that happen.

[philayres]

Did either of you make progress with hashing of tokens? TBH I blindly assumed this was happening when I picked this gem, then looked at the DB and had a nasty surprise.

@jeremywadsack did you happen to implement something specific on your side? I'd be happy to take anything you have and work with you both to produce a PR.

[jeremywadsack]

@philayres I have made no progress on this, but I'd love to have help if you have time to dig into it. Everything that I know (at this moment) is in this thread. :)

[philayres]

I'm about to make a pull request that implements persisting authentication tokens as digests generated using BCrypt hashing from Devise. The challenge was not so much in the hashing, but more in allowing it to continue to operate fast enough to be used for API authentication.

Please take a look at the pull request, which will reference this issue, in a moment or two. I'm obviously open to discussion. Anyway, it took some effort, and extra pairs of eyes to validate it will be much appreciated.

[davidalee]

#344 has been open for some time, is there any way we can look to get that PR reviewed? It addresses a pretty significant security vulnerability and seems like something we'd want by default.