feat(oauth2) hash client secret

[janza]

Summary

Hashing is done using sha256 provided by openresty library. The change
is backwards compatible by validating hashed secrets only if they are
stored in the right format: s256|hashed_secret|salt. If the secret
doesn't match the format the code assumes secret is stored raw. Most of
the code is based on hashing code used in basic-auth plugin.

Issues resolved

Closes #1237

[CLAassistant]

All committers have signed the CLA.

[bungle]

@janza, do you think it should be configurable? What I mean is that it is quite common case to go to e.g. developer portal, and grab the secret. If it is hashed, you need to generate a new secret. Or am I following this correctly?

[janza]

Most of the services that use secret hashing only show the secret once when the client is created. There is no way to retrieve it unless a new secret is generated. Secrets should be treated the same as passwords — services don't provide a way to see account password, only the option to reset it.

That said, in case there are users using kong in the way you descibed it could be an option to add a configuration flag for backwards compatibility.

[bungle]

@janza, yes, I somewhat think it depends on situation. I use a lot of services where this is implemented so that you can grab the secret anytime (of course first by signing in, and perhaps presenting 2-factor), and e.g. keycloak allows you to just go to admin and clients to get one. Sometimes it is easier, albeit less secure, just to get secret than create a new client or change the secret (which could affect other components). Of course public key crypto could be used here too... but at the moment the plugin does not support it, e.g. private_key_jwt.

[janza]

I'm open to adding configuration option if that would help with getting this patch merged.

I think that in case users don't want secrets to be hashed they should be responsible for providing ways to protect secrets (additional password verification, 2FA, ...) and/or en/decryption before calling into kong oauth2 api — that seems out of scope for kong itself.

[hishamhm]

I'm open to adding configuration option if that would help with getting this patch merged.

I think that would be the safer way to approach this!

[janza]

Looking at the codebase I think the best option would be to add a boolean flag to create/update endpoints for credentials /consumers/:consumers/oauth2/:oauth2_credentials. If the flag is set those credentials will be hashed, or otherwise just stored raw.

Another idea is to add this configuration flag to plugin config but there's no obvious way to get oauth2 plugin configuration in the oauth2_credentials dao methods.

Any recommendations?

[hishamhm]

@janza I believe we would need both flags: in the DAO so we can identify which keys are hashed and which ones are not (keeping backwards compatibility), and in the plugin config so that it knows when to apply the DAO flag.

[janza]

Current code is already backwards compatible with unhashed keys - hashed keys are stored in a certain format in the DB so we can identify hashed and unhashed keys so I don't think it's needed.

[hishamhm]

@janza but this would cause problems in case someone's unhashed secret is s256|a|a, right? Since keys can be arbitrary, it seems to me that the information that states that the key is hashed must live out-of-band, and not encoded as part of the string (otherwise any possible encoding could be a valid unhashed value).

[janza]

That is correct and if it's not an acceptable issue we'll also need to have this configuration as part of the DAO.

What is not clear to me, in the code, is how to get this flag from plugin configuration into oauth2_credentials dao. Specificaly in the before hook I'm missing some code to fetch current plugin configuration. If you could give some insight in now to properly do that that I can try and implement it.

[p0pr0ck5]

We should use peer-reviewed hashing algorithms like bcrypt instead of rolling our own here, which automatically handle things like salts and performance tunables