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.

Sign up for GitHub

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
+276 −228
Merged

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
1f9687c
[Synthetics] Do not store content of client certificate in state
romainberger Jan 13, 2021
375f857
[Synthetics] Store hashed content for client certificate
romainberger Jan 15, 2021
e5c3c6a
Merge branch 'master' into rberger/SYA-603/client-cert-state
zippolyte Jan 15, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion datadog/cassettes/TestAccDatadogSyntheticsAPITest_BasicNewAssertionsOptions.freeze
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
2021-01-06T10:49:33.846702+01:00
2021-01-15T11:28:33.525482+01:00
158 changes: 79 additions & 79 deletions datadog/cassettes/TestAccDatadogSyntheticsAPITest_BasicNewAssertionsOptions.yaml
View file
Open in desktop

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion datadog/cassettes/TestAccDatadogSyntheticsAPITest_UpdatedNewAssertionsOptions.freeze
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
2021-01-06T10:58:05.577416+01:00
2021-01-15T11:42:09.54007+01:00
285 changes: 145 additions & 140 deletions datadog/cassettes/TestAccDatadogSyntheticsAPITest_UpdatedNewAssertionsOptions.yaml
View file
Open in desktop

Large diffs are not rendered by default.

35 changes: 30 additions & 5 deletions datadog/resource_datadog_synthetics_test_.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
package datadog

import (
"crypto/sha256"
"encoding/json"
"fmt"
"log"
Expand Down Expand Up @@ -273,6 +274,9 @@ func syntheticsTestRequestClientCertificateItem() *schema.Schema {
Type: schema.TypeString,
Required: true,
Sensitive: true,
StateFunc: func(val interface{}) string {
return convertToSha256(val.(string))
},
Comment on lines +277 to +279
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 •
edited
Loading

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).

},
"filename": {
Description: "File name for the certificate.",
Expand Down Expand Up @@ -1106,14 +1110,14 @@ func updateSyntheticsTestLocalState(d *schema.ResourceData, syntheticsTest *data
key := clientCertificate.GetKey()
localCertificate["key"][0]["filename"] = key.GetFilename()

// the content of the certificate and the key are write-only
// so we need to get them from the config since they will
// not be in the api response
// the content of client certificate is write-only so it will not be returned by the API.
// To avoid useless diff but also prevent storing the value in clear in the state
// we store a hash of the value.
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))
Comment on lines 1116 to +1117
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...

}
if configKeyContent, ok := d.GetOk("request_client_certificate.0.key.0.content"); ok {
localCertificate["key"][0]["content"] = configKeyContent.(string)
localCertificate["key"][0]["content"] = getCertificateStateValue(configKeyContent.(string))
}

d.Set("request_client_certificate", []map[string][]map[string]string{localCertificate})
Expand Down Expand Up @@ -1371,3 +1375,24 @@ func validateSyntheticsAssertionOperator(val interface{}, key string) (warns []s
}
return
}

func convertToSha256(content string) string {
data := []byte(content)
hash := sha256.Sum256(data)
return fmt.Sprintf("%x", hash[:])
}

// get the sha256 of a client certificate content
// in some case where Terraform compares the state value
// we already get the hashed value so we don't need to
// hash it again
func getCertificateStateValue(content string) string {
contentBytes := []byte(content)

// hacky way to detect if the value is already a sha256 hash
if len(contentBytes) == 64 {
return content
}

return convertToSha256(content)
}
22 changes: 20 additions & 2 deletions datadog/resource_datadog_synthetics_test_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -551,11 +551,11 @@ func createSyntheticsAPITestStepNewAssertionsOptions(accProvider *schema.Provide
resource.TestCheckResourceAttr(
"datadog_synthetics_test.bar", "request_basicauth.0.password", "secret"),
resource.TestCheckResourceAttr(
"datadog_synthetics_test.bar", "request_client_certificate.0.cert.0.content", "content-certificate"),
"datadog_synthetics_test.bar", "request_client_certificate.0.cert.0.content", convertToSha256("content-certificate")),
resource.TestCheckResourceAttr(
"datadog_synthetics_test.bar", "request_client_certificate.0.cert.0.filename", "Provided in Terraform config"),
resource.TestCheckResourceAttr(
"datadog_synthetics_test.bar", "request_client_certificate.0.key.0.content", "content-key"),
"datadog_synthetics_test.bar", "request_client_certificate.0.key.0.content", convertToSha256("content-key")),
resource.TestCheckResourceAttr(
"datadog_synthetics_test.bar", "request_client_certificate.0.key.0.filename", "key"),
resource.TestCheckResourceAttr(
Expand Down Expand Up @@ -872,6 +872,14 @@ func updateSyntheticsAPITestStepNewAssertionsOptions(accProvider *schema.Provide
"datadog_synthetics_test.bar", "request.url", "https://docs.datadoghq.com"),
resource.TestCheckResourceAttr(
"datadog_synthetics_test.bar", "request.timeout", "60"),
resource.TestCheckResourceAttr(
"datadog_synthetics_test.bar", "request_client_certificate.0.cert.0.content", convertToSha256("content-certificate-updated")),
resource.TestCheckResourceAttr(
"datadog_synthetics_test.bar", "request_client_certificate.0.cert.0.filename", "Provided in Terraform config"),
resource.TestCheckResourceAttr(
"datadog_synthetics_test.bar", "request_client_certificate.0.key.0.content", convertToSha256("content-key-updated")),
resource.TestCheckResourceAttr(
"datadog_synthetics_test.bar", "request_client_certificate.0.key.0.filename", "key-updated"),
resource.TestCheckResourceAttr(
"datadog_synthetics_test.bar", "assertion.#", "1"),
resource.TestCheckResourceAttr(
Expand Down Expand Up @@ -932,6 +940,16 @@ resource "datadog_synthetics_test" "bar" {
timeout = 60
}

request_client_certificate {
cert {
content = "content-certificate-updated"
}
key {
content = "content-key-updated"
filename = "key-updated"
}
}

assertion {
type = "body"
operator = "validatesJSONPath"
Expand Down