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

Hash MySQL passwords #13614

Closed
wants to merge 1 commit into from
Closed

Hash MySQL passwords #13614

wants to merge 1 commit into from

Conversation

joshuaspence
Copy link
Contributor

Instead of storing MySQL passwords in plaintext, hash them first. This implementation is largely based on #12128.

@joshuaspence
Copy link
Contributor Author

Is there any interest in this pull request?

@apparentlymart
Copy link
Contributor

Hi @joshuaspence! Sorry for the silence here.

Thanks for working on this!

The password is retained in cleartext with the idea that someone might want to interpolate it somewhere, e.g. to pass on the credentials for the created user to the application that will use it. In retrospect that is not a strong requirement in this particular case since the value is defined directly in config anyway, but to change this now would be a breaking change for anyone depending on doing that. (I have personally written configs like that at a previous company, so I know there's at least one user that would be broken!)

If we were writing this resource fresh again today we'd probably hash this value, but since we've already established the cleartext password as part of the interface here I think it's better to just put this under the heading of "use-cases for sensitive values in Terraform state" and address it holistically (along with other use-cases such as tls_private_key). There are already several issues open around this, such as #1421, #9556, #8076, and while this is not something we're immediately working on we do intend to address this eventually. For the time being we'd suggest using remote state backends that provide their own measures of security, such as S3 or Terraform Enterprise ("atlas").

If we can find an alternative way to address this without breaking backward compatibility then I think that would be acceptable, as a pragmatic short-term solution until we are able to address some of the larger concerns above.

Thanks again for working on this; I hope the reasoning above makes sense.

@joshuaspence
Copy link
Contributor Author

Thanks for the explanation. Would you accept a new parameter which hashes the password rather than storing it in plain text?

@apparentlymart
Copy link
Contributor

Hi @joshuaspence,

That seems fine in principle. I think in order for it to work we'd need to add a new attribute that ConflictsWith password, but I'm not sure what's best to call it. hashed_password makes it sound like you need to provide it already-hashed in the configuration for some reason, but password_that_will_be_hashed_in_state is of course far too long. If you have some ideas somewhere in the middle then otherwise this seems like a reasonable path!

Instead of storing MySQL passwords in plaintext, hash them first. This implementation is largely based on #12128.
@joshuaspence
Copy link
Contributor Author

@apparentlymart I've just updated the pull request. Let me know what you think.

@apparentlymart apparentlymart self-requested a review May 25, 2017 00:55
@apparentlymart
Copy link
Contributor

Hi @joshuaspence! Sorry for the silence here. I've had some 0.10 release work to get done so I've been unable to take a look at this yet, but it's still on my list and I hope to take a look at it soon.

@apparentlymart
Copy link
Contributor

This was merged into the now-separated MySQL provider in hashicorp/terraform-provider-mysql#9. Thanks, @joshuaspence!

@joshuaspence joshuaspence deleted the mysql_password branch February 4, 2018 09:05
@ghost
Copy link

ghost commented Apr 5, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@ghost ghost locked and limited conversation to collaborators Apr 5, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants