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.

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

feat(oauth2) optionally hash client secret #5610

Merged
merged 1 commit into from
Mar 26, 2020

Conversation

bungle
Copy link
Member

@bungle bungle commented Feb 25, 2020

Summary

This was originally implemented with #4866 by @janza.

After some review comments, and the introduction of DAO transformations, I decided to make changes, thus opening this new PR.

This PR adds a new boolean column hash_secret to oauth2_credentials that is used to determine whether or not the client_secret will be hashed.

The PR adds support for pbkdf2 (and contains code for argon2, and bcrypt if we want to enable them later). The plugin will also check if the client_secret needs to be rehashed (on usage). One caveat. If you run this on cluster and some nodes have argon2 and some don't, it is possible that you cannot use the credentials on those that don't. So keep your environments similar.

Issues resolved

Close #4866, Fix #1237 (at least on OAuth2)

@bungle bungle added pr/do not merge pr/discussion This PR is being debated. Probably just a few details. labels Feb 25, 2020
@bungle bungle requested a review from p0pr0ck5 February 25, 2020 23:41
@bungle bungle force-pushed the feat/oauth2-hash-client-secret branch 14 times, most recently from 5bd8b75 to 5aab605 Compare February 26, 2020 12:58
@bungle bungle added pr/please review and removed pr/discussion This PR is being debated. Probably just a few details. pr/do not merge labels Feb 26, 2020
@bungle bungle force-pushed the feat/oauth2-hash-client-secret branch 2 times, most recently from 79a82de to 1fca9e0 Compare February 26, 2020 13:01
@bungle bungle requested a review from hishamhm February 26, 2020 13:54
@bungle bungle force-pushed the feat/oauth2-hash-client-secret branch 4 times, most recently from 5447383 to 238987b Compare February 27, 2020 09:02
kong/plugins/oauth2/migrations/004_200_to_210.lua Outdated Show resolved Hide resolved
kong/plugins/oauth2/secret.lua Show resolved Hide resolved
kong/plugins/oauth2/secret.lua Show resolved Hide resolved
kong/plugins/oauth2/secret.lua Show resolved Hide resolved
@p0pr0ck5
Copy link
Contributor

Why are we supporting more than one hash algorithm? It seems to complicate things substantially and I don’t see it being configurable. And argon it not used anywhere else in this project- let’s not add another dependency if we don’t need it?

@p0pr0ck5
Copy link
Contributor

Also, what is the logic behind making hash behavior configurable? We don't have such an option for basic-auth.

@bungle
Copy link
Member Author

bungle commented Feb 28, 2020

Why are we supporting more than one hash algorithm? It seems to complicate things substantially and I don’t see it being configurable. And argon it not used anywhere else in this project- let’s not add another dependency if we don’t need it?

bcrypt is also not used anywhere. The reason was:

  1. argon2id is recommended and probably gonna be NIST (EE can ship it) (https://password-hashing.net/)
  2. bcrypt will install properly by with just luarocks and not NIST compatible (argon2-ffi and lua c-wrapper still need system library too)

@bungle
Copy link
Member Author

bungle commented Feb 28, 2020

Also, what is the logic behind making hash behavior configurable? We don't have such an option for basic-auth.

In many places like:

  1. bintray
  2. luarocks

etc.

You can go to your settings and fetch the API key (and I would say it is very handy albeit less secure). In some contexts like Github I think the flow is: create app secret and it shows you after creation, and after that it is impossible (very cumbersome as you probably also need to configure this and that, that you already had to one which key you cannot get anymore, but yes, more secure too). So because there is legacy in this plugin we wanted to keep it optional and backward compatible and support both scenarios.

Both scenarios have good and bad sides, even other than what I explained above.

@bungle
Copy link
Member Author

bungle commented Feb 28, 2020

Also, what is the logic behind making hash behavior configurable? We don't have such an option for basic-auth.

For basic-auth I think we didn't even think about that during that point. It is also using hashing function (basic sha1 hash with salt, non-pbkdf2 type) that is not particularly made for hashing passwords. Also it is rare to see any use to store password in plain (e.g. with keycloak I cannot find my password anywhere, but I can see clients and their secrets).

@bungle bungle force-pushed the feat/oauth2-hash-client-secret branch from 8dd4ede to 1ce3df0 Compare February 28, 2020 17:37
@bungle
Copy link
Member Author

bungle commented Feb 28, 2020

At some point if @fffonion can provide us password hashing functions with lua-resty-openssl, I guess we can remove these dependencies, though we might need to write migrations then.

@fffonion
Copy link
Contributor

fffonion commented Mar 2, 2020

@bungle lua-resty-openssl 0.6.0rc0 now has kdf which supports PBKDF2 and others. 😄

@bungle
Copy link
Member Author

bungle commented Mar 10, 2020

@fffonion, ok perhaps we should then wait for lua-resty-openssl bump.

@bungle bungle force-pushed the feat/oauth2-hash-client-secret branch from 1ce3df0 to 31e7ff5 Compare March 10, 2020 15:51
@fffonion
Copy link
Contributor

@bungle I just released 0.6.0, the kdf api should be available now : )

@bungle bungle force-pushed the feat/oauth2-hash-client-secret branch 2 times, most recently from 2784211 to a6d48d8 Compare March 19, 2020 17:36
@bungle
Copy link
Member Author

bungle commented Mar 19, 2020

This is now updated to use pbkdf2 with lua-resty-openssl. Other algorithms are disabled, but can be enabled if we need to support them at some point. The new dependencies were removed from .rockspec (so brypt or argon2-ffi is not included anymore there).

PBKDF2 will output hash like this (contains default settings):

$pbkdf2-sha512$i=10000,l=32$RsW9il33SHbxQzywGmNkRw$EnvHL59Y0vyxTmjuiArdi6Kmxk3/6TrM1q9bIR3d/zw

@bungle bungle removed request for hishamhm and p0pr0ck5 March 20, 2020 08:08
@bungle bungle force-pushed the feat/oauth2-hash-client-secret branch from a6d48d8 to ef6bf5d Compare March 24, 2020 16:48
@bungle bungle requested a review from kikito March 25, 2020 05:56
@bungle bungle force-pushed the feat/oauth2-hash-client-secret branch from ef6bf5d to 3b03783 Compare March 25, 2020 17:54
### Summary

This was originally implemented with #4866 by @janza.

After some review comments, and the introduction of DAO transformations,
I decided to make changes, thus opening this new PR.

This PR adds a new `boolean` column `hash_secret` to `oauth2_credentials`
that is used to determine whether or not the `client_secret` will be hashed.

The PR adds support for `argon2`, and `bcrypt` and it uses `argon2` if the
system library for argon2 is installed. Otherwise it will fallback to
`bcrypt`. The plugin will also check if the `client_secret` needs to be
rehashed (on usage). One caveat. If you run this on cluster and some nodes
have `argon2` and some don't, it is possible that you cannot use the
credentials on those that don't. So keep your environments similar.

### Issues resolved

Close #4866, Fix #1237 (at least on OAuth2)
@bungle bungle force-pushed the feat/oauth2-hash-client-secret branch from 3b03783 to 2d164ff Compare March 26, 2020 15:22
@bungle bungle merged commit 946568c into next Mar 26, 2020
@bungle bungle deleted the feat/oauth2-hash-client-secret branch March 26, 2020 16:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants