Support issuance of ACM private certificate #6666

micbase · 2018-11-30T18:11:38Z

Fixes partial of #5550
For certificates issued by PCA, need a separate resource, and not covered in this PR

Changes proposed in this pull request:

Support issuance of ACM private certificate

Output from acceptance testing:

$ make testacc TESTARGS='-run=TestAccAWSAcmCertificate_privateCert'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./... -v -parallel 20 -run=TestAccAWSAcmCertificate_privateCert -timeout 120m
?   	github.com/terraform-providers/terraform-provider-aws	[no test files]
=== RUN   TestAccAWSAcmCertificate_privateCert
=== PAUSE TestAccAWSAcmCertificate_privateCert
=== CONT  TestAccAWSAcmCertificate_privateCert
--- PASS: TestAccAWSAcmCertificate_privateCert (12.92s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	12.965s

slaunay · 2019-01-23T23:55:57Z

aws/resource_aws_acm_certificate.go

 	acmconn := meta.(*AWSClient).acmconn
 	params := &acm.RequestCertificateInput{
 		DomainName:       aws.String(d.Get("domain_name").(string)),
-		ValidationMethod: aws.String(d.Get("validation_method").(string)),
+		IdempotencyToken: aws.String(strconv.Itoa(rand.Int())),


I'm wondering if using some sort of hash of the domain_name (that fits in 32 characters or less) would be more suitable.
That is, one might not want to generate multiple certificates for the same domain if somehow the API request fails and has to retry multiple times within the hour.

From the RequestCertificate API documentation:

Customer chosen string that can be used to distinguish between calls to RequestCertificate. Idempotency tokens time out after one hour. Therefore, if you call RequestCertificate multiple times with the same idempotency token within one hour, ACM recognizes that you are requesting only one certificate and will issue only one. If you change the idempotency token for each call, ACM recognizes that you are requesting multiple certificates.

I think we may want to hash domain name + SANs + private CA ARN if it's a private cert.

@micbase Good question.

I was wondering if the goal of the token is to distinguish two (or possibly more) of the same calls to the RequestCertificate API (i.e. with the exact same parameters) so that the same certificate ARN is returned to prevent creating a new certificate if the client was somehow not able to read the response (e.g. network issues).
That is whether changing one parameter (e.g. adding a SAN) is considered to a different "request" from AWS perspective even if you provide the same idempotency token.

So, I run the following command against two PCAs in a single region from a single account during the same hour and got some interesting results.

Requesting a certificate multiple times with the same token:

$ date && aws acm request-certificate --domain-name pr-6666.com --certificate-authority-arn arn:aws:acm-pca:region:1234:certificate-authority/1 --idempotency-token PR6666 Tue 5 Feb 2019 12:32:55 PST { "CertificateArn": "arn:aws:acm:region:1234:certificate/78d5b732-88a8-49af-8929-f28359eb8b75" } $ date && aws acm request-certificate --domain-name pr-6666.com --certificate-authority-arn arn:aws:acm-pca:region:1234:certificate-authority/1 --idempotency-token PR6666 Tue 5 Feb 2019 12:33:01 PST { "CertificateArn": "arn:aws:acm:region:1234:certificate/78d5b732-88a8-49af-8929-f28359eb8b75" }

Then adding a SAN yet keeping the same token will generate a new certificate:

$ date && aws acm request-certificate --domain-name pr-6666.com --subject-alternative-names pr-6666.org --certificate-authority-arn arn:aws:acm-pca:region:1234:certificate-authority/1 --idempotency-token PR6666 Tue 5 Feb 2019 12:35:16 PST { "CertificateArn": "arn:aws:acm:region:1234:certificate/b75395d9-d9fe-4249-8b30-c7ac4d1fbfd0" } $ date && aws acm request-certificate --domain-name pr-6666.com --subject-alternative-names pr-6666.org --certificate-authority-arn arn:aws:acm-pca:region:1234:certificate-authority/1 --idempotency-token PR6666 Tue 5 Feb 2019 12:35:32 PST { "CertificateArn": "arn:aws:acm:region:1234:certificate/b75395d9-d9fe-4249-8b30-c7ac4d1fbfd0" }

Then changing the certificate authority (same account, same region) somehow returns the same certificate (I consider that behaviour a bug):

$ date && aws acm request-certificate --domain-name pr-6666.com --subject-alternative-names pr-6666.org --certificate-authority-arn arn:aws:acm-pca:region:1234:certificate-authority/2 --idempotency-token PR6666 Tue 5 Feb 2019 12:36:05 PST { "CertificateArn": "arn:aws:acm:region:1234:certificate/b75395d9-d9fe-4249-8b30-c7ac4d1fbfd0" }

Then adding one more SAN generates a new certificate:

$ date && aws acm request-certificate --domain-name pr-6666.com --subject-alternative-names pr-6666.org pr-6666.net --certificate-authority-arn arn:aws:acm-pca:region:1234:certificate-authority/2 --idempotency-token PR6666 Tue 5 Feb 2019 12:39:03 PST { "CertificateArn": "arn:aws:acm:region:1234:certificate/0d41f00a-25ce-492e-b6c4-1bb34b64da60" } $ date && aws acm request-certificate --domain-name pr-6666.com --subject-alternative-names pr-6666.org pr-6666.net --certificate-authority-arn arn:aws:acm-pca:region:1234:certificate-authority/2 --idempotency-token PR6666 Tue 5 Feb 2019 12:39:10 PST { "CertificateArn": "arn:aws:acm:region:1234:certificate/0d41f00a-25ce-492e-b6c4-1bb34b64da60" }

And switching back to the original certificate authority do keep the same certificate (again kind of unexpected behaviour IMHO):

$ date && aws acm request-certificate --domain-name pr-6666.com --subject-alternative-names pr-6666.org pr-6666.net --certificate-authority-arn arn:aws:acm-pca:region:1234:certificate-authority/1 --idempotency-token PR6666 Tue 5 Feb 2019 12:39:30 PST { "CertificateArn": "arn:aws:acm:region:1234:certificate/0d41f00a-25ce-492e-b6c4-1bb34b64da60" }

I would be keen to only have the domain name as part of the hash but it seems that adding the PCA ARN could work around that bug of getting the same certificate.
However this might be fixed eventually (if it is a bug and not a feature).

What do you think?

That is the exact problem I encountered, apologize for not expressing it clearly in my initial PR.
IMO, using two different PCAs should generate different certs, so I used IdempotencyToken to work it around. I'm not sure if that's a bug or feature, more like a bug to me.

slaunay · 2019-01-24T00:03:35Z

aws/resource_aws_acm_certificate.go

+	if ok {
+		params.CertificateAuthorityArn = aws.String(caARN.(string))
+	} else {
+		params.ValidationMethod = aws.String(d.Get("validation_method").(string))


The changes made to move the validation_method argument from required to optional might require some extra validation to prevent calling the API for public certificate with the unsupported value NONE (if such an argument is not set).

Addressed here https://github.com/micbase/terraform-provider-aws/blob/72d1fc49d8e2e5ad7b415f16e47c712608b70328/aws/resource_aws_acm_certificate.go#L123-L125

slaunay · 2019-01-31T16:34:39Z

@micbase Any chance you can resolve the conflicts and look at my comments?

If not I would be happy to take over by creating a separate PR based on your commits.
I'm really interested in that feature and your changes make a lot of sense to me.

micbase · 2019-01-31T16:49:52Z

@slaunay Sorry about the delay, will take a look this weekend.

micbase · 2019-02-06T16:54:23Z

@slaunay I updated the branch, please take a look when you have time.

slaunay · 2019-02-07T16:59:39Z

I have reach to AWS about that token behaviour and they were able to reproduce it including with an invalid certificate authority ARN:

Additionally, if an idempotency-token is included then certificate manager is not verifying the certificate authority provided in the request if same idempotency token used in the same hour.

I have requested a private certificate and provided invalid certificate-authority ARN along with the same idempotency-token and I was issued the same certificate ARN without any error. This proves that certificate Manager is not verifying the certificate authority when same idempotency-token is used again in the same hour. In order to get clarity on this behavior, I have logged a request with my internal team and I will update you as soon as I hear from them.

They also mentioned the same workaround of using a different token when the certificate authority changes, I'll provide an update once they get back to me.

It would also be nice to have somebody with write access to the repo look at this PR now that is has no merge conflicts.

bflad · 2019-02-08T03:26:45Z

Hey folks 👋 We typically use something like resource.UniqueId() or some form of random string for ClientToken/IdempotencyToken to ensure we don't have length issues (although probably not an issue with the hashing) or cause unexpected behavior with operators explicitly calling terraform taint shortly after creating a resource. For the last point, does ACM correctly recreate the certificate if you create with an IdempotencyToken, delete it, then try to recreate with the same IdempotencyToken?

I guess more generally my question is whether its worth all the hassle trying to come up with the more complicated implementation (that may be potentially incorrect/buggy) for an atypical situation that we won't be able to easily test. For general networking issues, the AWS Go SDK should be automatically retrying with the provided IdempotencyToken in the original request.

slaunay · 2019-02-15T20:32:59Z

Does ACM correctly recreate the certificate if you create with an IdempotencyToken, delete it, then try to recreate with the same IdempotencyToken?

It seems that the token is tied to the life cycle of the generated certificate as seen with the following test:

Issuing a new certificate twice using the same token:

$date && aws acm request-certificate --domain-name pr-6666.com --certificate-authority-arn arn:aws:acm-pca:region:1234:certificate-authority/1 --idempotency-token PR6666
Fri 15 Feb 2019 09:10:24 PST
{
    "CertificateArn": "arn:aws:acm:region:1234:certificate/cc030479-17a7-45a3-bdb5-bfcd454aa24f"
}
$ date && aws acm request-certificate --domain-name pr-6666.com --certificate-authority-arn arn:aws:acm-pca:region:1234:certificate-authority/1 --idempotency-token PR6666
Fri 15 Feb 2019 09:10:30 PST
{
    "CertificateArn": "arn:aws:acm:region:1234:certificate/cc030479-17a7-45a3-bdb5-bfcd454aa24f"
}

Deleting that certificate:

$ date && aws acm delete-certificate --certificate-arn arn:aws:acm:region:1234:certificate/cc030479-17a7-45a3-bdb5-bfcd454aa24f
Fri 15 Feb 2019 09:11:21 PST

Reissuing the "same" certificate within the same hour using the same token result in a different ARN:

$ date && aws acm request-certificate --domain-name pr-6666.com --certificate-authority-arn arn:aws:acm-pca:region:1234:certificate-authority/1 --idempotency-token PR6666
Fri 15 Feb 2019 09:11:28 PST
{
    "CertificateArn": "arn:aws:acm:region:1234:certificate/70f98a7d-c799-4c43-bdc8-4928300a3d53"
}
$ date && aws acm request-certificate --domain-name pr-6666.com --certificate-authority-arn arn:aws:acm-pca:region:1234:certificate-authority/1 --idempotency-token PR6666
Fri 15 Feb 2019 09:11:32 PST
{
    "CertificateArn": "arn:aws:acm:region:1234:certificate/70f98a7d-c799-4c43-bdc8-4928300a3d53"
}

AWS support got back to me and here is their response that confirm the bug mentioned above:

Firstly, I appreciate your feed back as it helped us to identify an issue in validation while requesting a private certificate using ACM PCA. Our team has confirmed that this isn't the expected behavior while making 'RequestCertificate' call for private certificates using idempotency-token.

Since our Internal team noticed the issue, this issue will be fixed by our service team. Being a support engineer I don't have visibility over the progress of this issue. Hence I will not be able to provide you an ETA regarding this issue.

Till this issue is fixed, please consider using the different idempotency tokens while making a request to issue private certificate from different private certificate authorities and I suggest you to keep an eye on our What's New page, Blog and RSS Feed for latest updates on AWS where we announce all new features when we release them.

For general networking issues, the AWS Go SDK should be automatically retrying with the provided IdempotencyToken in the original request.

True, the internal retry is a good first line of defence to avoid generating two certificates for the same domain during a single terraform apply.
Now, if somehow such retries would fail (e.g. loss of connectivity for a short period of time, terraform crashing or maybe double Ctrl+C by user) then it could be useful to make sure that the new terraform apply do not leave an "orphan" certificate in ACM behind.

I would personally lean towards using only the domain name to compute the idempotency token and explicitly documenting that, knowing that it would currently return the same certificate ARN when changing the PCA.

We could also expose the token to the end user as an attribute to set, maybe with a default to such hash for more flexibility but that seems convoluted to me.

What do you guys think make sense?

micbase · 2019-02-21T22:10:09Z

So when you using the same IdempotencyToken with different domain name, ACM will create new cert right? Why do we want to compute the token based on domain name?

slaunay · 2019-02-22T16:42:05Z

I believe the crux of the idempotency token is to deduplicate the exact same request (i.e. same domain name requested plus other properties) in case the client is retrying to prevent issuing a new certificate.
So it makes sense to me that deduplication does not happen if you use a different domain but the same token as seen above.

Now for what to put in the token, I think it really depends on how bad you want to prevent a new certificate from getting issued:

when a retry is done by AWS SDK (will send the same request payload and therefore the same token you computed whatever that is)
when a retry is done submitting a new HTTP request from a new terraform apply process during the same hour (will send a new token you computed, likely different if you are using resource.UniqueId() but would be the same if you hash the domain name)

slaunay · 2019-03-06T22:54:11Z

@bflad What approach would you recommend?

slaunay · 2019-04-01T20:20:04Z

@bflad Do you have any suggestions on how to get this feature integrated into the AWS provider?

mattburgess · 2019-07-15T19:22:46Z

I have a very similar looking PR at #9288 which I obviously raised because my poor searching didn't find this one somehow! Interestingly, you don't appear to have hit the issue I did whereby I need to wait for the cert to be in ISSUED state, otherwise things would die horribly due to various fields not being populated until that happens. I wonder if ACM PCA has different performance in different regions such that your tests never hit that condition? Anyway, would be really good if someone could review this and mine please so that we can get support upstream.

bflad

This looks good, thanks, @micbase 🚀 The only minor changes on merge are changing IdempotencyToken to use resource.PrefixedUniqueId() which uses a monotonic counter to follow the conventions with the rest of the provider and having these new private certificates automatically retry while they sit in PENDING_VALIDATION where certificate details such as domain name are filled in by the API.

Output from acceptance testing:

--- PASS: TestAccAWSAcmCertificate_imported_IpAddress (10.34s)
--- PASS: TestAccAWSAcmCertificate_wildcard (13.56s)
--- PASS: TestAccAWSAcmCertificate_dnsValidation (15.16s)
--- PASS: TestAccAWSAcmCertificate_privateCert (15.19s)
--- PASS: TestAccAWSAcmCertificate_imported_DomainName (15.45s)
--- PASS: TestAccAWSAcmCertificate_rootAndWildcardSan (16.29s)
--- PASS: TestAccAWSAcmCertificate_root_TrailingPeriod (16.29s)
--- PASS: TestAccAWSAcmCertificate_root (16.31s)
--- PASS: TestAccAWSAcmCertificate_san_TrailingPeriod (17.01s)
--- PASS: TestAccAWSAcmCertificate_san_multiple (18.08s)
--- PASS: TestAccAWSAcmCertificate_disableCTLogging (18.53s)
--- PASS: TestAccAWSAcmCertificate_wildcardAndRootSan (18.66s)
--- PASS: TestAccAWSAcmCertificate_san_single (21.37s)
--- PASS: TestAccAWSAcmCertificate_emailValidation (23.08s)
--- PASS: TestAccAWSAcmCertificate_tags (33.48s)

ghost · 2019-08-07T17:38:42Z

This has been released in version 2.23.0 of the Terraform AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template for triage. Thanks!

ghost · 2019-11-02T14:53:23Z

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 feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks!

bflad added the enhancement Requests to existing resources that expand the functionality or scope. label Nov 30, 2018

slaunay reviewed Jan 24, 2019

View reviewed changes

aeschright requested a review from a team June 25, 2019 21:47

Michael Chang added 2 commits July 9, 2019 16:56

Support issuance of ACM private certificate

68f0da0

Add IdempotencyToken

d96d866

mattburgess mentioned this pull request Jul 21, 2019

Acm cert transparency logging #9413

Merged

bflad approved these changes Aug 6, 2019

View reviewed changes

bflad added this to the v2.23.0 milestone Aug 6, 2019

bflad merged commit d96d866 into hashicorp:master Aug 6, 2019

bflad added a commit that referenced this pull request Aug 6, 2019

Update CHANGELOG for #6666

947c5b4

bflad mentioned this pull request Aug 6, 2019

Support creation of private ACM certificates #9288

Closed

ghost locked and limited conversation to collaborators Nov 2, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support issuance of ACM private certificate #6666

Support issuance of ACM private certificate #6666

micbase commented Nov 30, 2018

slaunay Jan 23, 2019

micbase Feb 4, 2019

slaunay Feb 5, 2019

micbase Feb 5, 2019

slaunay Jan 24, 2019

micbase Feb 6, 2019

slaunay commented Jan 31, 2019

micbase commented Jan 31, 2019

micbase commented Feb 6, 2019

slaunay commented Feb 7, 2019

bflad commented Feb 8, 2019 •

edited

Loading

slaunay commented Feb 15, 2019

micbase commented Feb 21, 2019

slaunay commented Feb 22, 2019

slaunay commented Mar 6, 2019

slaunay commented Apr 1, 2019

mattburgess commented Jul 15, 2019

bflad left a comment

ghost commented Aug 7, 2019

ghost commented Nov 2, 2019

Support issuance of ACM private certificate #6666

Support issuance of ACM private certificate #6666

Conversation

micbase commented Nov 30, 2018

slaunay Jan 23, 2019

Choose a reason for hiding this comment

micbase Feb 4, 2019

Choose a reason for hiding this comment

slaunay Feb 5, 2019

Choose a reason for hiding this comment

micbase Feb 5, 2019

Choose a reason for hiding this comment

slaunay Jan 24, 2019

Choose a reason for hiding this comment

micbase Feb 6, 2019

Choose a reason for hiding this comment

slaunay commented Jan 31, 2019

micbase commented Jan 31, 2019

micbase commented Feb 6, 2019

slaunay commented Feb 7, 2019

bflad commented Feb 8, 2019 • edited Loading

slaunay commented Feb 15, 2019

micbase commented Feb 21, 2019

slaunay commented Feb 22, 2019

slaunay commented Mar 6, 2019

slaunay commented Apr 1, 2019

mattburgess commented Jul 15, 2019

bflad left a comment

Choose a reason for hiding this comment

ghost commented Aug 7, 2019

ghost commented Nov 2, 2019

bflad commented Feb 8, 2019 •

edited

Loading