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

Add secure string generator for Password generator #149

Merged
merged 4 commits into from
Feb 28, 2024

Conversation

kanor1306
Copy link
Contributor

Hi,

I have had some occurrences where a service in different environments, newly bootstrapped, was getting the same password for both environments (dev and staging). I am not very proficient in Go, but I assume it has to do with the usage of the math random function without a seed. In any case I have been reading and it seems to be recommended the usage of crypto/rand for password generation (and also the usage of Seed is deprecated)

As I said I am not an expert Go programmer, but I have tried to add a new function following the same structure of the current one but with the usage of the crypto/rand library. I think it likely that this is not a totally secure implementation, as while investigating how to do this I have seen implementations that go in a lot more detail, but I thought that this was a good enough step in this direction.

One thing to note is that apparently the password generation can fail if the secure number generation has issues, so now when creating the password a new error has to be handled. I have chose for re queuing the instance, as it is done in other errors, though I have to say I am not certain if this would be a recoverable error.

Resources that I looked into for this implementation:
https://pkg.go.dev/crypto/rand
golang/go#56319
https://stackoverflow.com/a/32351471
https://stackoverflow.com/questions/22892120/how-to-generate-a-random-string-of-a-fixed-length-in-go
https://gist.github.com/denisbrodbeck/635a644089868a51eccd6ae22b2eb800

Copy link
Member

@hitman99 hitman99 left a comment

Choose a reason for hiding this comment

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

Nice feature!

@hitman99
Copy link
Member

@kanor1306 Tests are failing, could you check it out please?

@kanor1306
Copy link
Contributor Author

@kanor1306 Tests are failing, could you check it out please?

Will check it out tomorrow @hitman99, I thought that I had run the tests, but obviously I messed something up

@kanor1306
Copy link
Contributor Author

Should be fine now @hitman99, somehow I missed an import

@hitman99 hitman99 merged commit 9c05343 into movetokube:master Feb 28, 2024
2 checks passed
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.

2 participants