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

utils: use SystemRandom when generating random password. #204

Merged
merged 2 commits into from Feb 18, 2020

Conversation

xnox
Copy link
Contributor

@xnox xnox commented Feb 5, 2020

As noticed by Seth Arnold, non-deterministic SystemRandom should be
used when creating security-sensitive random strings.

LP: #1860795

As noticed by Seth Arnold, non-deterministic SystemRandom should be
used when creating security sensitive random strings.

LP: #1860795
Copy link
Collaborator

@smoser smoser left a comment

Choose a reason for hiding this comment

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

just as a comment... there is a long string of issues using random in early boot.

summary: be careful with random. Especially if you're attempting to get "Better random".

@xnox
Copy link
Contributor Author

xnox commented Feb 5, 2020

just as a comment... there is a long string of issues using random in early boot.

summary: be careful with random. Especially if you're attempting to get "Better random".

Yes this will cause to block and wait on randomness. However, RANDOM in setting user passwords is not used by default in most cloud types. And recent kernels have more realiable earlier access to devrandom pools, and we try to push our cloud partners to provide hwrng exposed to the virtual machines too.

At the moment, I'd rather boot slowly / fail to boot, than to provision machines with quickly crackable passwords or those that provide a side-channel to establish the machine seed remotely.

@igalic
Copy link
Collaborator

igalic commented Feb 5, 2020

perhaps we should also add a warning to the RANDOM option.

Copy link
Collaborator

@OddBloke OddBloke left a comment

Choose a reason for hiding this comment

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

Should we be using the secrets module when it's available?

However, RANDOM in setting user passwords is not used by default in most cloud types.

Azure doesn't do it by default, but password auth is a first-class option in their UI. I'd like us to be sure that we aren't going to see enormous differences in boot time there before we release this.

@@ -397,9 +397,10 @@ def translate_bool(val, addons=None):


def rand_str(strlen=32, select_from=None):
r = random.SystemRandom()
Copy link
Collaborator

Choose a reason for hiding this comment

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

The SystemRandom docs say that it is "Not available on all systems." Specifically, it is not available on systems where os.urandom would raise NotImplementedError:

On a Unix-like system, random bytes are read from the /dev/urandom device. If the /dev/urandom device is not available or not readable, the NotImplementedError exception is raised.

cloud-init is used on BSD systems, so we cannot assume that we are running on top of the Linux kernel. I don't know the state of urandom on BSDs, but I would like to know that we won't be breaking behaviour there.

@setharnold
Copy link

Use CVE-2020-8631. Thanks

@igalic
Copy link
Collaborator

igalic commented Feb 5, 2020

cloud-init is used on BSD systems, so we cannot assume that we are running on top of the Linux kernel. I don't know the state of urandom on BSDs, but I would like to know that we won't be breaking behaviour there.

most Unix systems these days symlink /dev/random to /dev/urandom
so if I'm reading this code right, it should just work™, on all the platforms we support
https://github.com/python/cpython/blob/c4cacc8c5eab50db8da3140353596f38a01115ca/Python/bootstrap_hash.c#L434
(this is the code eventually called by os.urandom

Copy link
Collaborator

@OddBloke OddBloke left a comment

Choose a reason for hiding this comment

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

We're going to land this and perform testing on (a) a KVM instance with user-data that uses this code path, and (b) an Azure instance with a fabric-generated password. If (a) indicates issues, we'll update the docs to indicate that people may see blocking if they use this path. If (b) indicates issues, we'll dig into the specifics.

@OddBloke OddBloke merged commit 3e2f735 into canonical:master Feb 18, 2020
xiaofengw-vmware pushed a commit to xiaofengw-vmware/cloud-init that referenced this pull request Feb 25, 2020
As noticed by Seth Arnold, non-deterministic SystemRandom should be
used when creating security sensitive random strings.
This was referenced May 12, 2023
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.

None yet

5 participants