Harden root password class #1485
Conversation
chelnak
force-pushed
the
maint-harden_root_password_class
branch
3 times, most recently
from
b0e1f67 to
d9f03e6
Compare
|
ping @puppetlabs/security |
chelnak
force-pushed
the
maint-harden_root_password_class
branch
from
0c8be00 to
b60b76c
Compare
chelnak
force-pushed
the
maint-harden_root_password_class
branch
from
b60b76c to
1a0fe05
Compare
|
The test failures in this PR are present on main and part of another investigation. |
manifests/server/root_password.pp
Outdated
| "mysqladmin -u root --password=\$(grep -o '[^ ]\\+\$' ${secret_file}) password ''", | ||
| "rm -f ${secret_file}", | ||
| "mysqladmin -u root --password=\$(grep -o '[^ ]\\+\$' /.mysql_secret) password ''", | ||
| 'rm -f /.mysql_secret', | ||
| ], ' && ') |
There was a problem hiding this comment.
This should be chained with a ; not &&. If the mysqladmin command fails with the current command chaining it won't delete the .mysql_secret file
There was a problem hiding this comment.
I think that's desired though. If we remove the file without setting the password, then we've lost it.
There was a problem hiding this comment.
🤔 we don't want to leave a cleartext file sitting on the hard drive though
There was a problem hiding this comment.
Let's look at the workflow and fragilities. This is how I understand it, please correct me if I'm wrong.
- the mysql install runs and generates a randomized password
- this is saved in plaintext on disk. It allows you only the ability to set your own password--you can't use it for normal operations.
- the user is expected to specify a new password and delete the temporary password file, which is what this command string is doing.
If step 3 fails, I can only think of a handful of causes:
- mysql installation failed or the service didn't start. Manual intervention is required, and the plaintext password may or may not be valid.
- race condition -- the install succeeded, but an unknown actor already changed the root password. The plaintext password is invalid, but this should probably trigger an investigation.
- the process that generated the install password and file on disk failed. The plaintext password is invalid.
Are there other scenarios?
There was a problem hiding this comment.
yes, assume that something else went wrong with the mysqladmin password change command, in this case the password was not changed or removed, and it's still sitting on the disk, right? The safer option here is to just delete it regardless of success or failure
There was a problem hiding this comment.
I'd still call that scenario 1 and the proper response imho is to fail and ensure manual intervention. I'm not sure if I care about the password file existing. It could make troubleshooting easier, but I suspect that most users won't bother--they'll either burn the node or remove the installation and try again.
My concern with chaining with ; is that it will make that exec always succeed and so it won't bubble up errors.
There was a problem hiding this comment.
how about we do it like this
mysqladmin -u root --password=\$(grep -o '[^ ]\\+\$' /.mysql_secret) password \
&& (rm -f /.mysql_secret; exit 0) \
|| (rm -f /.mysql_secret; exit 1)Now we'll always delete the secret, and we'll have the error if something went wrong?
There was a problem hiding this comment.
Ha! I looked at the original code again and it wasn't surfacing errors to start with!
chelnak
force-pushed
the
maint-harden_root_password_class
branch
5 times, most recently
from
6e6075e to
0cb1c44
Compare
chelnak
force-pushed
the
maint-harden_root_password_class
branch
2 times, most recently
from
392e0d9 to
e00ebec
Compare
Prior to this commit there was a possibility that malformed strings could be passed in to the resource. This could lead to unsafe executions on a remote system. The parameters that are susceptible are `install_secret_file`. This commit fixes the above by adding validation to ensure the given values confirm to expectation. `secret_file` is validated with a regular expression that ensures the given value is a valid path.
This commit adds spec tests for the changes made in the previous commit.
chelnak
force-pushed
the
maint-harden_root_password_class
branch
from
e00ebec to
85956f5
Compare
chelnak
force-pushed
the
maint-harden_root_password_class
branch
from
3be226f to
85956f5
Compare
chelnak
force-pushed
the
maint-harden_root_password_class
branch
from
85956f5 to
f2b3bde
Compare
Prior to this PR there was a possibility that malformed strings could be passed in to the resource.
This could lead to unsafe executions on a remote system.
The parameters that are susceptible are
install_secret_file.This PR fixes the above by adding validation to ensure the given values confirm to expectation.
secret_fileis validated with a regular expression that ensures the given value is a valid path.