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

[Synthetics] Store SHA 256 hash of certificates in state instead of the actual cert #835

Merged
merged 3 commits into from
Jan 15, 2021

Conversation

romainberger
Copy link
Member

@romainberger romainberger commented Jan 13, 2021

Do not store the content of client certificate in the state file for privacy.

Note: When this is released, it will trigger a non empty plan for every user with certificates in their config

@romainberger romainberger requested review from a team as code owners January 13, 2021 15:29
Comment on lines +277 to +279
StateFunc: func(val interface{}) string {
return convertToSha256(val.(string))
},
Copy link
Contributor

Choose a reason for hiding this comment

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

Won't this make terraform hash the value twice ? once in the read function when you use getCertificateStateValue, and once by terraform automatically when you set the value in the state ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Well actually, from my testing, it seems this doesn't do anything at all, let's just remove it and manually set the hashed value directly.

Copy link
Member Author

@romainberger romainberger Jan 15, 2021

Choose a reason for hiding this comment

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

No this function never receives a hashed value (I've checked by printing it).

Comment on lines 1116 to +1117
if configCertificateContent, ok := d.GetOk("request_client_certificate.0.cert.0.content"); ok {
localCertificate["cert"][0]["content"] = configCertificateContent.(string)
localCertificate["cert"][0]["content"] = getCertificateStateValue(configCertificateContent.(string))
Copy link
Contributor

Choose a reason for hiding this comment

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

I think GetOk takes the value from the config, not the state, so it should never be hashed already. Unless I'm missing something, there should be no need to do this, and instead rely on the StateFunc you added above to let terraform store in the state no ?

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought so as well, but when I print the value to debug, we get a hashed value (even after commenting the StateFunc). I assumed it is when terraform reads the config to compare the state and the config.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah looks like depending on when you call it, it picks from state or from config...

@zippolyte
Copy link
Contributor

Well ignore all I said... terraform does too many weird things, what you have seems to be working fine, so let's go for it.

@zippolyte zippolyte changed the title [Synthetics] Do not store content of client certificate in state [Synthetics] Store SHA 256 hash of certificates in state instead of the actual cert Jan 15, 2021
@zippolyte
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@zippolyte zippolyte merged commit 313be09 into master Jan 15, 2021
@zippolyte zippolyte deleted the rberger/SYA-603/client-cert-state branch January 15, 2021 14:20
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