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

Fix hash-related method signatures #531

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

gustavocovas
Copy link
Contributor

...to prevent hash functions that do not produce unique values

Description

Closes #530.

Proposed Changes

Since pbkdf2 works with a func() Hash.hash type, returning such type at GetValidHashFunction in api/auth/authmongo.go should simplify the calls to pbkdf2.Key. Our hash functions passed to pbkdf2.Key should not produce unique values anymore (#526), and we can upgrade our Dockerfile to Go 1.16.

Testing

Test locally and check that the panic: crypto/hmac: hash generation function does not produce unique values is not called anymore.

make test
make install
source .env
make run-client
docker logs -f -t <huskyci-api-container-id>

You might want to make more checks. I believe that the stored hashes for users passwords in a production system should not be valid anymore, since apparently we were not using the pbkdf2 library correctly.

Copy link
Contributor

@GabhenDM GabhenDM left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code LGTM!

Indeed it's a breaking change, i ran "make install" on master branch, switched to the fixed branch and rebuilt the API and got the following error:

[HUSKYCI][*] poc-golang-gosec -> https://github.com/globocom/huskyCI.git
[HUSKYCI][ERROR] Sending request to huskyCI: Unauthorized Husky-Token ZjMxYjU3YTUtMjkwMS00ZjliLWFlYzgtMWVmMDNmODE5ZTk0OnllSVAyMEdsNDNPcW9XazdBaFhlZ1ZMdkZUdGpaVl9tWkxHbGxuNmhGbXc9
make: *** [Makefile:155: run-client] Error 1

So it also affects client access tokens to the API.

Maybe we can think of a script to be executed on build and regenerate all tokens/hashes?

Also got this log (manually debugging :D)

huskyCI_API  | 2021/03/23 03:26:06 {"version":"1.1","host":"af2ed42488eb","short_message":" [Hash value from random data is different]","full_message":" [Hash value from random data is different]","timestamp":1616469966,"level":6,"action":"testToken","app":"undefined","file":"/go/src/github.com/globocom/huskyCI/api/log/log.go","info":"VALIDATE","line":31,"tags":"undefined"}

That error is returned from the ValidateRandomData (api/token/token.go line 83), as expected on the hash part of the token it fails. 🐼

@@ -1,4 +1,4 @@
FROM golang:1.15
FROM golang:1.16
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if we need to fix on this specific version, could just allow for latest image.

@rogeriobastos
Copy link

Hi guys, I've been testing this fix in a fresh new installation and it works fine.
I think this fix shouldn't be blocked to new users.

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.

Verify pbkdf2 hash function implementation
4 participants