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

FEATURE/MEDIUM: userList: generate random secure password #47

Merged
merged 1 commit into from
May 11, 2020
Merged

FEATURE/MEDIUM: userList: generate random secure password #47

merged 1 commit into from
May 11, 2020

Conversation

amelhusic
Copy link
Collaborator

This change previously hard coded password ("insecure-password" in HAProxy config) usage and instead use hashed password("password").
So, on every start up a random password is generated, hashed and then saved to HAProxy conf.
Generated password is stored in memory, while hashed password is saved to HAProxy conf.

@amelhusic amelhusic force-pushed the feat/password-encryption branch from fc1e569 to 5273053 Compare May 6, 2020 11:37
@aiharos aiharos requested a review from ShimmerGlass May 6, 2020 12:11
@amelhusic
Copy link
Collaborator Author

This is related to #12

ShimmerGlass
ShimmerGlass requested changes May 6, 2020
View reviewed changes
haproxy/config.go Outdated
b := make([]rune, n)
rand.Seed(time.Now().UnixNano())
for i := range b {
b[i] = dictionary[rand.Intn(len(dictionary))]
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should use crypto/rand instead of math/rand. math/rand is not a secure random source.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I applied suggested changes.

@amelhusic amelhusic force-pushed the feat/password-encryption branch from 5273053 to 146eb1a Compare May 6, 2020 14:47
@ShimmerGlass
Copy link
Collaborator

ShimmerGlass commented May 6, 2020 •
edited
Loading

I don't think hashing the password is needed here. b64 should only output chars compatible with the config file format and hashing doesn't add entropy to the generated string.
Otherwise LGTM.

@amelhusic
Copy link
Collaborator Author

amelhusic commented May 6, 2020 •
edited
Loading

If we remove hashing func, then we need to use insecure-password again. From the issue #12:

One possible simple solution would be to generate a strong crypto-random password for HAProxy on every startup so it's only in-memory in the consul-haproxy process and passed through to HAProxy. I guess the Config is written to disk so I'd suggest using the secure (hashed) password config to pass it through and hashing the random password in consul-haproxy. That way you can have high confidence that the only process able to configure the proxy is same consul-haproxy process that stated it.

My understanding from above is that generated password should be in memory and encrypted password saved to disk. What do you think?

EDIT: After rethinking this, I think also we don't need a password. Also, usage of password will affect a CPU usage according to docs. So, I removed hashing func.

@amelhusic amelhusic force-pushed the feat/password-encryption branch from 146eb1a to fd35223 Compare May 6, 2020 18:45
@amelhusic
FEATURE/MEDIUM: userList: generate random secure password
c9cfd4f 
This change previously hard coded password usage and instead use generated password.
So, on every start up a random password is generated and saved to HAProxy conf.
@amelhusic amelhusic force-pushed the feat/password-encryption branch from fd35223 to c9cfd4f Compare May 7, 2020 08:11
@aiharos aiharos requested a review from ShimmerGlass May 8, 2020 12:43
@ShimmerGlass
Copy link
Collaborator

LGTM

@aiharos aiharos merged commit f8963ae into haproxytech:master May 11, 2020
@aiharos aiharos linked an issue May 11, 2020 that may be closed by this pull request
Documenting the threat model/improving inter-process security. #12
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ShimmerGlass ShimmerGlass Awaiting requested review from ShimmerGlass

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Documenting the threat model/improving inter-process security.
3 participants
@amelhusic @ShimmerGlass @aiharos