Adding support for ED25519 to all the *_cert resources

[detro]

Context: #150

By moving all the utilities related to *_cert in common_cert.go, and doing the same for *.key, it is now easy to have the same support for Key Algorithm across the board.

With this, all the certs support ED25519 private keys, without any code specific for this in their implementation.

Additionally, the fields *_key_algorithm used in the *_cert resources are now all marked as Optional and Deprecated: the plan is to release 3.2.0 first, then change those fields to Computed (i.e. exposing the inferred algorithm), and then release a 4.0.0. This (should be) in accordance with our guidelines: https://www.terraform.io/plugin/sdkv2/best-practices/deprecations.

How the "deprecation" of *_key_algorithm looks like

Today

"key_algorithm": &schema.Schema{
  Type:     schema.TypeString,
  Required: true,
  ForceNew: true,

3.2.0 (i.e. this PR)

"key_algorithm": &schema.Schema{
  Type:       schema.TypeString,
  Optional:   true,
  ForceNew:   true,
  Deprecated: "DEPRECATION MESSAGE"

This will:

1. inform user with a warning that the field should not be used anymore
2. if the user complies, the state gets updated with the removal of the string from it

At this point, we release 3.2.0

4.0.0 (future PR)

"key_algorithm": &schema.Schema{
  Type:       schema.TypeString,
  Computed:   true,

This will:

1. allow the provider to .Set("key_algorithm")
2. the state is updated with the computed value

[detro]

ping @bflad :)

[bflad]

I think this is generally moving in an okay direction, but I have left some testing and security considerations. It may be good to reach out to additional security folks in this situation to more authoritatively determine the safety of this change (where still considering an "expected" algorithm may be a decent compromise).

[bflad]

This deprecation seems to imply that this attribute should be available as read-only today too, so folks can update their configurations in preparation of the change. However, without calling (ResourceData).Set() on the attribute, the only time it would be available for reference today is if it is configured and relying on the SDK's behavior of copying the attribute's planned value (from the configuration) to the state.

My suggestion for these is to mark this attribute as Computed: true and call d.Set("key_algorithm", /*...*./) as appropriate. This removes any confusion about when the attribute value may be present, rather than it being dependent on the configuration. This will also ensure practitioners that remove the attribute from their configuration do not see an unexpected "whatever" -> null plan when removing the configuration, since the attribute will remain in state in that case.

[detro]

In my testing I concluded that a 2 step approach (first make it Optional, release, then make it Computed) was necessary to avoid causing unnecessary re-creation of the resources.

I can remove the reference to "future behaviour" as what matters is that ppl stop using it.

[bflad]

I should've clarified better, sorry. I meant Optional and Computed, with calling d.Set(). Optional + Computed + ForceNew should not trigger recreation when removing the attribute from the configuration and if it does, that would be unexpected behavior we should investigate further.

[detro]

So, here is the issue with adding Computer to Optional now: it will be a breaking change.

I think we are expected to cross that bridge only once we release a major version, after a minor version with the deprecation - no?

[detro]

To be clear, I tried yesterday all the combinations, and just changing from Required to Optional for now, seemed like the only path forward to allow for an opt-in migration.

All other options would end up with a breaking change.

[bflad]

I have some concerns over unilaterally accepting the input PEM as a source of truth in this situation, especially reading through RFC 7468. In particular:

Section 2 says:

The label type implies that the encoded data follows the specified
syntax. Parsers MUST handle non-conforming data gracefully.
However, not all parsers or generators prior to this document behave
consistently. A conforming parser MAY interpret the contents as
another label type but ought to be aware of the security implications
discussed in the Security Considerations section. The labels
described in this document identify container formats that are not
specific to any particular cryptographic algorithm, a property
consistent with algorithm agility.

Section 14 says:

Data in this format often originates from untrusted sources, thus
parsers must be prepared to handle unexpected data without causing
security vulnerabilities.

It makes me wonder if we should still allow practitioners to still optionally set the attribute for an additional security posture. The function here can take the passed in string parameter and still automatically fallback to algorithm lookup if it is set to "".

[detro]

OK so, I have dug into the code of crypto/ssh and crypto.x509 and the Preamble (i.e. the pem.Block.Type field) are indeed what drives the parsing decision in all the "switches".

Given the ambiguity in the RFC that I highlighted in our private chat, I feel like the best course of action is to imitate what Golang libraries already do.

I will of course integrate any feedback I might get back from the security team, so far nothing.

Additional to all of this, by poking me on this it made me realise that there was a corner case currently unhandled: the Preamble BEGING PRIVATE KEY can be used for more than just ED25519, but it can instead be used for RSA and ECDSA too.

Here is the signature of the method designated to parse it:

// ParsePKCS8PrivateKey parses an unencrypted private key in PKCS #8, ASN.1 DER form.
//
// It returns a *rsa.PrivateKey, a *ecdsa.PrivateKey, or a ed25519.PrivateKey.
// More types might be supported in the future.
//
// This kind of key is commonly encoded in PEM blocks of type "PRIVATE KEY".
func ParsePKCS8PrivateKey(der []byte) (key interface{}, err error) {
  // ...
}

So, I have restructured things to handle the key identification AFTER the key parsing.

[bflad]

Here's the full reproduction of how I'm coming to the conclusion about Computed: true and calling d.Set(). Please reach out if you need help getting this going in your local development environment.

Starting configuration (e.g. create a directory with this as main.tf):

terraform {
  required_providers {
    tls = {
      source  = "hashicorp/tls"
      version = "3.1.0"
    }
  }

  required_version = "1.1.7"
}

resource "tls_private_key" "example" {
  algorithm   = "ECDSA"
  ecdsa_curve = "P384"
}

resource "tls_self_signed_cert" "example" {
  key_algorithm   = "ECDSA"
  private_key_pem = tls_private_key.example.private_key_pem

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

  validity_period_hours = 12

  allowed_uses = [
    "key_encipherment",
    "digital_signature",
    "server_auth",
  ]
}

output "algorithm" {
  value = tls_self_signed_cert.example.key_algorithm
}

Install and use the production 3.1.0 provider to create a starting state that represents what a practitioner may have today:

terraform init
terraform apply -auto-approve

With that starting state in place, we can experiment with the updated provider locally outside the testing framework. Via this example .terraformrc file (e.g. I just created it in the configuration directory as .terraformrc; it should be updated for your GOBIN):

provider_installation {
  dev_overrides {
    "hashicorp/tls" = "/Users/bflad/go/bin"
  }

  direct {}
}

In the provider code directory, run go install . from this PR branch to install the provider into the GOBIN.

Running TF_CLI_CONFIG_FILE=./.terraformrc terraform apply without configuration changes yields no relevant changes and a deprecation warning, which is great and what we expect.

If I then comment key_algorithm = "ECDSA"in tls_self_signed_cert to try and fix the deprecation, running TF_CLI_CONFIG_FILE=./.terraformrc terraform apply yields this unexpected plan:

      - key_algorithm         = "ECDSA" -> null # forces replacement

That is not what we are looking for here. I should be able to fix the deprecation in my configuration without other effects. If I update the provider schema to instead include Computed: true here:

	s["key_algorithm"] = &schema.Schema{
		Type:       schema.TypeString,
		Optional:   true,
		Computed:   true,
		ForceNew:   true,
		Deprecated: "This is now ignored, as the key algorithm is inferred from the `private_key_pem`.",
		Description: "Name of the algorithm used when generating the private key provided in `private_key_pem`. " +
			"**NOTE**: this is deprecated and ignored, as the key algorithm is now inferred from the key. ",
	}

And update the CreateSelfSignedCert function to return the algorithm and call d.Set():

key, keyAlgorithm, err := parsePrivateKeyPEM([]byte(d.Get("private_key_pem").(string)))

if err != nil {
	return err
}

// Ensure key_algorithm is always set, even when not in the configuration
if err := d.Set("key_algorithm", keyAlgorithm); err != nil {
	return fmt.Errorf("unable to set key_algorithm: %w", err)
}
// ... rest of logic ...

With the schema and create function updates done, we can now call go install . to reinstall the local provider. Running TF_CLI_CONFIG_FILE=./.terraformrc terraform apply now yields no changes, which is what I'd expect.

Using this updated provider, we can also verify that it works as expected on new resource creation without configuring key_algorithm in the configuration:

rm -rf terraform.tfstate*
TF_CLI_CONFIG_FILE=./.terraformrc terraform apply -auto-approve

This also works as expected. We can use this same pattern for other similar resources.

---

Aside: Typically, we'd also update readCertificate to do the same so the algorithm is always refreshed (in case of import), however this schema implements StateFunc in a way we typically do not recommend so we cannot d.Get() the private_key_pem like normal from the Read function. The [sensitive state page](https://www.terraform.io/plugin/sdkv2/best-practices/sensitive-state) used to have a note about not hashing values, but I forget why it was removed. If we did want to support import in the future, the import function itself could extract and set the key_algorithm attribute appropriately though.

[bflad]

Rather than implementing this logic in Go it could also be done in the configuration itself with conditional logic, e.g.

ca_key_algorithm = %t ? "RSA" : null

Although admittedly, most providers prefer to create wholly separate configurations for clarity following the Go practice of preferring some duplication for clarity over being purely DRY. (It makes much more sense when you're working with dozens or hundreds of tests so you don't need to figure out any special testing configuration logic.)

[detro]

Given those tests are only temporarily there, is it ok if I punt on it? I intend to release 4.x soon after this.

[bflad]

Totally! Not a big deal, just wanted to let you know. 😄

[detro]

OK, one thing is clear: what the tests can't reproduce (easily) is the behaviour of such changes when there is a pre-existing state.

Not having Computed: true and the practitioner removing key_algorithm, triggers the change from value being set to null:

- key_algorithm         = "ECDSA" -> null # forces replacement

The same does NOT happen when Computed: true is indeed set: the output in your example can STILL return the value ECDSA, even though in configuration is gone and there is no d.Set() in my code yet.

I have to presume that terraform, seeing that it's computed, it initializes the resource during the plan from the state, so we carry that forward.

(cont...)

[detro]

OK @bflad , this should be ready for another review: I have set the fields to Computed + Optional + of course added calls to d.Set("key_algorithm") as expected.

[bflad]

Thank you for sticking through me with this and it looks good to go! 🚀

[github-actions]

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.