Skip to content

Conversation

@ghost
Copy link

@ghost ghost commented Mar 2, 2020

The current password generation algorithm is extremely deterministic, due to being based on the standard random number generator with a deterministic seed based on the current Unix timestamp (in seconds).

This can lead to a number of security issues, including:

  • The same passwords being used in different Kubernetes clusters if the operator is deployed in parallel. (This issue was discovered because of four deployments having the same generated passwords due to automatically being deployed in parallel.)
  • The passwords being easily guessable based on the time the operator pod started when the database was created. (This would typically be present in logs, metrics, etc., that may typically be accessible to more people than should have database access.)

This PR attempts to fix this issue by replacing the current randomness source with crypto/rand, which should produce cryptographically secure random data that is virtually unguessable. This will avoid both of the above problems as each deployment will be guaranteed to have unique, indeterministic passwords.

@erthalion
Copy link
Contributor

Thank you for the PR, it looks reasonable and after a short testing I don't see any issues. The only commentary from me is that operator will obviously not synchronize any user on already existing cluster with a new stronger password, so if it's necessary to apply it in this case one have to rotate secrets. We probably need to mention it somewhere in change log or such.

@erthalion
Copy link
Contributor

👍

1 similar comment
@FxKu
Copy link
Member

FxKu commented Mar 18, 2020

👍

@erthalion erthalion merged commit 9ddee8f into zalando:master Mar 18, 2020
@FxKu FxKu added this to the 1.5 milestone Mar 18, 2020
@ghost ghost deleted the secure-password-generation branch April 1, 2020 11:47
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.

3 participants