d/acm_certificate Explicitly define key types requested

[pselle]

AWS doesn't give all key types by default without this filter, so explicitly request the key types documented as supported.

Community Note

• Please vote on this pull request by adding a 👍 reaction to the original pull request comment to help the community and maintainers prioritize this request
• Please do not leave "+1" comments, they generate extra noise for pull request followers and do not help prioritize the request

Fixes #7553

Release note for CHANGELOG:

Exposes non-2048 certificates when reading for the acm_certificate data source

Output from acceptance testing:

Where tf-4096.example.com has been created with a 4096 RSA key, and tf-2048 has been created with a 2048 RSA key:

$ AWS_PROFILE=default ACM_CERTIFICATE_SINGLE_ISSUED_DOMAIN=tf-4096.example.com ACM_CERTIFICATE_ROOT_DOMAIN=example.com TF_LOG_PATH=log.txt TF_LOG=debug TF_ACC=1 go test ./aws -v -timeout 120m -parallel 20 -run='TestAccAWSAcmCertificateDataSource_single'
=== RUN   TestAccAWSAcmCertificateDataSource_singleIssued
=== PAUSE TestAccAWSAcmCertificateDataSource_singleIssued
=== CONT  TestAccAWSAcmCertificateDataSource_singleIssued
--- PASS: TestAccAWSAcmCertificateDataSource_singleIssued (11.01s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	13.007s

$ AWS_PROFILE=default ACM_CERTIFICATE_SINGLE_ISSUED_DOMAIN=tf-2048.example.com ACM_CERTIFICATE_ROOT_DOMAIN=example.com TF_LOG_PATH=log.txt TF_LOG=debug TF_ACC=1 go test ./aws -v -timeout 120m -parallel 20 -run='TestAccAWSAcmCertificateDataSource_single'
=== RUN   TestAccAWSAcmCertificateDataSource_singleIssued
=== PAUSE TestAccAWSAcmCertificateDataSource_singleIssued
=== CONT  TestAccAWSAcmCertificateDataSource_singleIssued
--- PASS: TestAccAWSAcmCertificateDataSource_singleIssued (21.62s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	22.395s

[bflad]

Hi @pselle 👋 Thanks for submitting this! I left two inline feedback items but it might be worth discussing some larger items outside that feedback. Namely:

• Adding covering acceptance testing
• Allowing operators to opt out searching all key algorithms by providing a new data source argument

With both these addressed/discussed, we should be able to get this in. 🚀

---

For the acceptance testing this functionality, we can use the newer aws_acm_certificate resource support for importing the certificate body and private key to generate a certificate that lies outside the existing ListCertificates key algorithms. The tls_private_key resource looks like it supports configuring the amount of RSA bits or using EDCSA.

A test configuration like the below should get this started with a RSA 4096 certificate in ACM:

resource "tls_private_key" "test" {
  algorithm = "RSA"
  rsa_bits  = 4096
}

resource "tls_self_signed_cert" "test" {
  allowed_uses = [
    "key_encipherment",
    "digital_signature",
    "server_auth",
  ]

  key_algorithm         = "RSA"
  private_key_pem       = "${tls_private_key.test.private_key_pem}"
  validity_period_hours = 12

  subject {
    common_name  = "example.com"
    organization = "ACME Examples, Inc"
  }
}

resource "aws_acm_certificate" "test" {
  certificate_body = "${tls_self_signed_cert.test.cert_pem}"
  private_key      = "${tls_private_key.test.private_key_pem}"
}

data "aws_acm_certificate" "test" {
  domain = "${aws_acm_certificate.test.domain_name}"
}

Taking that as a starting point, we can setup the acceptance test similar to the following:

func TestAccAWSAcmCertificateDataSource_Rsa4096(t *testing.T) {
	resourceName := "aws_acm_certificate.test"
	dataSourceName := "data.aws_acm_certificate.test"

	resource.ParallelTest(t, resource.TestCase{
		PreCheck:     func() { testAccPreCheck(t) },
		Providers:    testAccProvidersWithTLS,
		CheckDestroy: testAccCheckAcmCertificateDestroy,
		Steps: []resource.TestStep{
			{
				Config: testAccAwsAcmCertificateDataSourceConfigRsa4096(),
				Check: resource.ComposeTestCheckFunc(
					resource.TestCheckResourceAttrPair(resourceName, "arn", dataSourceName, "arn"),
				),
			},
		},
	})
}

func testAccAwsAcmCertificateDataSourceConfigRsa4096() string {
	return fmt.Sprintf(`
resource "tls_private_key" "test" {
  algorithm = "RSA"
  rsa_bits  = 4096
}

resource "tls_self_signed_cert" "test" {
  allowed_uses = [
    "key_encipherment",
    "digital_signature",
    "server_auth",
  ]

  key_algorithm         = "RSA"
  private_key_pem       = "${tls_private_key.test.private_key_pem}"
  validity_period_hours = 12

  subject {
    common_name  = "example.com"
    organization = "ACME Examples, Inc"
  }
}

resource "aws_acm_certificate" "test" {
  certificate_body = "${tls_self_signed_cert.test.cert_pem}"
  private_key      = "${tls_private_key.test.private_key_pem}"
}

data "aws_acm_certificate" "test" {
  domain = "${aws_acm_certificate.test.domain_name}"
}
`)
}

Which should fail without this change and should pass with this change. 👍

---

Relating to the second item, I'm a little worried operators may not want to opt out of searching all key algorithms for some of the following reasons:

• Their environments may have overlapping ACM certificates with different key algorithms, in which this update could be a breaking change
• Their environments may have a large number of ACM certificates, in which searching all key algorithms could further exasperate any throttling issues
• Their compliance regulations may require only using certain key algorithms, in which a Sentinel policy could be used to limit searches

Given the above, we may want to consider adding a new and optional key_algorithm argument for the data source. I think the data source can still default to searching all algorithms as changed by this pull request (for operator simplicity), but providing an escape hatch that overrides this behavior would be helpful.

If we do go down this route, we should include an acceptance test (or test step) that verifies the new argument prevents finding a certificate of a different key algorithm. 👍

[pselle]

@bflad Thanks so much for the feedback! I've implemented suggested changes, and am trying out the suggested acceptance test, although I'll take a little time to make sure I understand how it works/what it's doing.

As to "searching all algorithms might be a bad idea for some folks" I think this is very valid, since the change as is essentially is covering up AWS's current list behavior and modifying it, which could indeed be breaking to people expecting it to work as it has. So the suggestion here:

Allowing operators to opt out searching all key algorithms by providing a new data source argument

Makes me almost think that "do what AWS does, but give people an option to modify that behavior" might be a more palatable path actually? Basically, should we come at the problem from the other direction? I like the addition of an optional key_algorithms [updated to plural] in general, but seeing the possible failure modes makes it less palatable to change current behavior.

So this suggestion would be:

• Without key_algorithms, no Filters are added
• With key_algorithms, apply those filters
• ❓ Maybe call it key_types to match the API?

[bflad]

Hi again @pselle 👋 Sorry for the delayed response. I'm 👍 a new attribute on the data source that does as you say, optionally adding the Filters if defined with elements.

A schema definition like this should get you close:

"key_types": {
	Type: schema.TypeSet,
	Optional: true,
	Elem: &schema.Schema{Type: schema.TypeString},
	ValidateFunc: validation.StringInSlice([]string{
		acm.KeyAlgorithmEcPrime256v1,
		acm.KeyAlgorithmEcSecp384r1,
		acm.KeyAlgorithmEcSecp521r1,
		acm.KeyAlgorithmRsa1024,
		acm.KeyAlgorithmRsa2048,
		acm.KeyAlgorithmRsa4096,
	}, false),
},

Then in the code we can optionally use it with something like the below. 😄

params := &acm.ListCertificatesInput{}

if v := d.Get("key_types").(*schema.Set); v.Len() > 0 {
	params.Includes = &acm.Filters{
		KeyTypes: expandStringSet(v),
	},
}

Add the new argument to the documentation in website/docs/d/acm_certificate.html.markdown and hopefully this should be good to go! Thanks again!

[bflad]

Hey again @pselle 👋 Since I know you're busy in your new role and we would like to get this released today, I have performed the followup activities described in #8553 (comment) in a followup commit c160a98. Thanks so much for your work and discussion here! 🚀

[bflad]

This has been released in version 2.17.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]

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!