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.

Sign up for GitHub

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

Jump to bottom

[basicauth] Encrypt password field #33

Closed
thibaultcha opened this issue Feb 25, 2015 · 10 comments
Closed

[basicauth] Encrypt password field #33

thibaultcha opened this issue Feb 25, 2015 · 10 comments
Assignees
@subnetmarco
Labels
idea/new plugin [legacy] those issues belong to Kong Nation, since GitHub issues are reserved for bug reports.

Comments

@thibaultcha
Copy link
Member

api_keys and password (stored in secret_key field) must be hashed.

@ahmadnassri What did we decided already? The public_key being the salt?
@thibaultcha thibaultcha added this to the RC1 milestone Feb 25, 2015
@ahmadnassri
Copy link
Contributor

public_key can't be the salt, its public, therefor not-secure. use the internal id (uuid) as salt

@thibaultcha
Copy link
Member Author

Ah yes, it was with the id! Right, thanks

@subnetmarco
Copy link
Member

I suggest using SHA1

@subnetmarco
Copy link
Member

The only problem with encrypting keys is that the user will be able to retrieve it only the first time and never again. Of course this is not ideal.

I spoke with @thibaultcha and I agree that the only field that should be encrypted in the password in Basic Authentication, but not the secret_key for other authentication types.

@thibaultcha thibaultcha removed this from the 0.1.0-beta2 milestone Mar 3, 2015
@thibaultcha thibaultcha added api and removed TODO labels Mar 25, 2015
@thibaultcha thibaultcha changed the title Encrypt secret_key field [basicauth] Encrypt password field Mar 27, 2015
@thibaultcha thibaultcha added idea/new plugin [legacy] those issues belong to Kong Nation, since GitHub issues are reserved for bug reports. and removed api labels Mar 27, 2015
@thibaultcha thibaultcha modified the milestone: 1.0.0 Mar 27, 2015
@subnetmarco subnetmarco modified the milestones: 0.3.0, 0.2.0 Apr 22, 2015
@subnetmarco subnetmarco modified the milestones: 0.3.1, 0.3.0 Jun 2, 2015
@thibaultcha thibaultcha removed this from the 0.3.1 milestone Jun 8, 2015
@bortels
Copy link

bortels commented Aug 1, 2015

Any news on this? Non-encrypted secrets at rest are a giant audit flag (for me, in particular, for PCI).

I would suggest that both the basic_auth and secret_key be hashed; really, if you are not going to pass it back to the backend API, there is no good reason to store it plaintext, including not being able to retrieve it more than once; if the secret key is lost (or stolen or compromised) - you just generate a new one.

@sonicaghi
Copy link
Member

@bortels we have setup a roadmap here. We're working around the clock to deliver many new features, unfortunately "encrypt password" is not planned for the next release.

However, if you guys can help with a PR along the way, we would be more than happy to merge it for the v. 0.5

@tyiss
Copy link

tyiss commented Aug 26, 2015

moving forward with this, first draft with basic authentication plugin

configuration will add a new key 'encryption_method' which will hold:

  • plain - plain passwords - default, for backward compatibility
  • sha1 - sha1 for salted password with consumer id.

some code related questions:

  • is there a way to use plugin 'conf' in the api.lua. to get encryption_method .
  • for sha1 i use open-resty bundled https://github.com/openresty/lua-resty-string
    there is no issue using this for test api using kong proxy.
    but when i run the tests - it was failing. first because package.path was not pointing directly.
    a manual fix was giving me error regarding no ffi (no Luajit) in tests.
    how can i solve this ?

@thibaultcha
Copy link
Member Author

Shouldn't we use bcrypt?

@tyiss
Copy link

tyiss commented Aug 26, 2015

bcrypt is fine by me.
are you suggesting having it instead of sha1 or along side with it (letting the user to decide).

@thibaultcha
Copy link
Member Author

Having both would be better of course yeah

tyiss pushed a commit to tyiss/kong that referenced this issue Sep 2, 2015
adds sha1 support for basic auth configuration (issue Kong#33)
920b45a
@tyiss tyiss mentioned this issue Sep 2, 2015
Closed
@thibaultcha thibaultcha mentioned this issue Sep 7, 2015
Merged
thibaultcha added a commit that referenced this issue Sep 10, 2015
@thibaultcha
feat(basic-auth) password encryption
8c253cb 
On the contrary of #527, this only allows for sha1 encryption. The
reason is that due to the current architecture, we cannot support two
types at the same time (supporting plain is a bad practice anyways).
Because a basicauth_credential has no relation to a plugin entity (they
are **not** semantically related anyways), we cannot now how the
password is stored/encrypted.

I also took the opportunity of 0.5.0 and the migration script to make
that decision. The migration script will be updated to also migrate the
current passwords.

This does a bit more than #527:

- unit tests
- support encryption in unit test mode (with a mock using a vendor sha1
  library)
- comparison of the hash at the proxy level (for actual authentication)

Resolves #33
thibaultcha added a commit that referenced this issue Sep 15, 2015
@thibaultcha
feat(basic-auth) password encryption
ba41444 
On the contrary of #527, this only allows for sha1 encryption. The
reason is that due to the current architecture, we cannot support two
types at the same time (supporting plain is a bad practice anyways).
Because a basicauth_credential has no relation to a plugin entity (they
are **not** semantically related anyways), we cannot now how the
password is stored/encrypted.

I also took the opportunity of 0.5.0 and the migration script to make
that decision. The migration script will be updated to also migrate the
current passwords.

This does a bit more than #527:

- unit tests
- support encryption in unit test mode (with a mock using a vendor sha1
  library)
- comparison of the hash at the proxy level (for actual authentication)

Resolves #33
thibaultcha added a commit that referenced this issue Sep 21, 2015
@thibaultcha
feat(basic-auth) password encryption
0539840 
On the contrary of #527, this only allows for sha1 encryption. The
reason is that due to the current architecture, we cannot support two
types at the same time (supporting plain is a bad practice anyways).
Because a basicauth_credential has no relation to a plugin entity (they
are **not** semantically related anyways), we cannot now how the
password is stored/encrypted.

I also took the opportunity of 0.5.0 and the migration script to make
that decision. The migration script will be updated to also migrate the
current passwords.

This does a bit more than #527:

- unit tests
- support encryption in unit test mode (with a mock using a vendor sha1
  library)
- comparison of the hash at the proxy level (for actual authentication)

Resolves #33
@subnetmarco subnetmarco mentioned this issue Oct 5, 2015
Closed
@kncs kncs mentioned this issue Jun 29, 2017
Closed
gszr pushed a commit that referenced this issue Jun 10, 2021
@hbagdi
fix: skip sending status code after body is sent
3e4c63d 
This commit fixes a bug where the plugin tries to send HTTP status code
after the response body is sent for `/metrics` endpoint.

This resulted in error logs in Kong.
A test has been added to ensure that no error logs are generated when
`/metrics` is scraped by Prometheus.

Another test has been added to ensure that the response body only
contains either metrics or comments describing the metrics.
This test is added to catch regression for
#4077, where Kong injected HTML
generated by the default Lapis handler.

Fixes #32 
From #33
gszr pushed a commit that referenced this issue Jul 7, 2021
@fffonion
chore(acme) release: 0.2.6 (#33)
16df703
gszr pushed a commit that referenced this issue Aug 6, 2021
@hbagdi @gszr
fix(prometheus) skip sending status code after body is sent
a787020 
This commit fixes a bug where the plugin tries to send HTTP status code
after the response body is sent for `/metrics` endpoint.

This resulted in error logs in Kong.
A test has been added to ensure that no error logs are generated when
`/metrics` is scraped by Prometheus.

Another test has been added to ensure that the response body only
contains either metrics or comments describing the metrics.
This test is added to catch regression for
#4077, where Kong injected HTML
generated by the default Lapis handler.

Fixes #32
From #33
gszr pushed a commit that referenced this issue Oct 26, 2021
@james-callahan
Merge commit 'refs/pull/36/head' of https://github.com/Kong/kong-plug…
a563f51 
…in-zipkin

Fixes #33
gszr pushed a commit that referenced this issue Oct 27, 2021
@james-callahan
Merge commit 'refs/pull/36/head' of https://github.com/Kong/kong-plug…
4257229 
…in-zipkin

Fixes #33
hutchic added a commit that referenced this issue Jun 10, 2022
@hutchic
chore(rename) rename the default package to Kong (#33)
c715c63 
* chore(rename) rename the default package to Kong

* feat(compatibility) add a provides to maintain backwards compatibility
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@subnetmarco subnetmarco

Labels
idea/new plugin [legacy] those issues belong to Kong Nation, since GitHub issues are reserved for bug reports.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
6 participants
@ahmadnassri @bortels @subnetmarco @thibaultcha @sonicaghi @tyiss