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

tls_self_signed_cert is unintentionally replaced on 4.0.0 and later #284

Open
1 task done
ohookins opened this issue Oct 11, 2022 · 5 comments · May be fixed by #285
Open
1 task done

tls_self_signed_cert is unintentionally replaced on 4.0.0 and later #284

ohookins opened this issue Oct 11, 2022 · 5 comments · May be fixed by #285
Labels

Comments

@ohookins
Copy link

ohookins commented Oct 11, 2022

Terraform CLI and Provider Versions

Terraform 1.3.2
TLS provider 4.0.0 or higher

Terraform Configuration

resource "tls_self_signed_cert" "example" {
  private_key_pem   = tls_private_key.example.private_key_pem
  is_ca_certificate = true

  subject {
    organization   = "Example"
  }

  # 3 Years
  validity_period_hours = 24 * 365 * 3

  allowed_uses = [
    "cert_signing",
    "crl_signing",
  ]
}

Expected Behavior

tls_self_signed_cert resources are not replaced when updating the provider

Actual Behavior

Certificate is replaced, seemingly due to something in the subject block but it's unclear what:

      ~ subject { # forces replacement
            # (2 unchanged attributes hidden)
        }

I previously had the street_address attribute unspecified, in which case the output is this:

      ~ subject { # forces replacement
          - street_address = [] -> null # forces replacement
            # (1 unchanged attribute hidden)
        }

Specifying the street address as [] means the resource is still replaced, but we can't see why (if none of the attributes in the block are changing). We do use the organization attribute of the subject block already.

Maybe related to #175 ?

Steps to Reproduce

  1. terraform apply

How much impact is this issue causing?

High

Logs

No response

Additional Information

This is considered high impact as accepting this change means replacing some very critical certificates in our infrastructure that cannot be easily applied without some runtime impact. We don't want to have to replace these unless they are actually expiring. Alternatively, not upgrading this provider for another ~2.5 years is also not an acceptable trade-off.

Code of Conduct

  • I agree to follow this project's Code of Conduct
@ohookins ohookins added the bug label Oct 11, 2022
@bflad
Copy link
Contributor

bflad commented Oct 11, 2022

Different resource, but potentially related: #263

@bflad
Copy link
Contributor

bflad commented Oct 11, 2022

For anyone triaging this issue, there is likely chance that the CLI terminal plan output is missing useful information due to hashicorp/terraform#31887. A way to check for this CLI issue would be to apply the test resources on the older version, copy the state file, apply on the newer version, and perform a difference between the old state and new state.

Using this test configuration:

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

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

resource "tls_self_signed_cert" "example" {
  private_key_pem   = tls_private_key.example.private_key_pem
  is_ca_certificate = true

  subject {
    organization = "Example"
  }

  # 3 Years
  validity_period_hours = 24 * 365 * 3

  allowed_uses = [
    "cert_signing",
    "crl_signing",
  ]
}

The original state shows as:

          "attributes": {
            "allowed_uses": [
              "cert_signing",
              "crl_signing"
            ],
            "cert_pem": "-----BEGIN CERTIFICATE-----\nMIIBUzCCAQCgAwIBAgIRAIA5DDxtNWDpLpkXpeOlCjEwCgYIKoZIzj0EAwIwEjEQ\nMA4GA1UEChMHRXhhbXBsZTAeFw0yMjEwMTEyMzI1MzRaFw0yNTEwMTAyMzI1MzRa\nMBIxEDAOBgNVBAoTB0V4YW1wbGUwTjAQBgcqhkjOPQIBBgUrgQQAIQM6AARRoymj\ni7yr2542ZS3pkuZEbYrEjuCNwZeq7nQqmRKJyBhk6hLzPr4+N8ddjyZ8aeTasSXM\ny2QhVKNCMEAwDgYDVR0PAQH/BAQDAgEGMA8GA1UdEwEB/wQFMAMBAf8wHQYDVR0O\nBBYEFOystxSMDSQgyz89FNjH5lfjw25OMAoGCCqGSM49BAMCA0EAMD4CHQDAnw8J\nESI5DPtKsSHAe5T/mUAu9yZhNeq+0dymAh0A67gYB4t6ETItJ2AIbvPSC05Tf25U\nYemv5fLTRw==\n-----END CERTIFICATE-----\n",
            "dns_names": null,
            "early_renewal_hours": 0,
            "id": "170437392557809108370016538482266409521",
            "ip_addresses": null,
            "is_ca_certificate": true,
            "key_algorithm": "ECDSA",
            "private_key_pem": "...",
            "ready_for_renewal": false,
            "set_authority_key_id": null,
            "set_subject_key_id": null,
            "subject": [
              {
                "common_name": "",
                "country": "",
                "locality": "",
                "organization": "Example",
                "organizational_unit": "",
                "postal_code": "",
                "province": "",
                "serial_number": "",
                "street_address": null
              }
            ],
            "uris": null,
            "validity_end_time": "2025-10-10T16:25:34.216702-07:00",
            "validity_period_hours": 26280,
            "validity_start_time": "2022-10-11T16:25:34.216702-07:00"
          },

The new state shows as:

          "attributes": {
            "allowed_uses": [
              "cert_signing",
              "crl_signing"
            ],
            "cert_pem": "-----BEGIN CERTIFICATE-----\nMIIBUTCCAQCgAwIBAgIRAKMsfuydhv02B4nZEMDIaREwCgYIKoZIzj0EAwIwEjEQ\nMA4GA1UEChMHRXhhbXBsZTAeFw0yMjEwMTEyMzMwMTlaFw0yNTEwMTAyMzMwMTla\nMBIxEDAOBgNVBAoTB0V4YW1wbGUwTjAQBgcqhkjOPQIBBgUrgQQAIQM6AARRoymj\ni7yr2542ZS3pkuZEbYrEjuCNwZeq7nQqmRKJyBhk6hLzPr4+N8ddjyZ8aeTasSXM\ny2QhVKNCMEAwDgYDVR0PAQH/BAQDAgEGMA8GA1UdEwEB/wQFMAMBAf8wHQYDVR0O\nBBYEFOystxSMDSQgyz89FNjH5lfjw25OMAoGCCqGSM49BAMCAz8AMDwCHAhBUqw6\niP7gOnIPmGlhTWPyNHFllweKrpOmhhUCHGvjC+4RRc6p7sqBMUZoshyuuxvs+PIp\nDSLZCCA=\n-----END CERTIFICATE-----\n",
            "dns_names": null,
            "early_renewal_hours": 0,
            "id": "216895198704925571533000068090024192273",
            "ip_addresses": null,
            "is_ca_certificate": true,
            "key_algorithm": "ECDSA",
            "private_key_pem": "... test private key contents ...",
            "ready_for_renewal": false,
            "set_authority_key_id": false,
            "set_subject_key_id": false,
            "subject": [
              {
                "common_name": null,
                "country": null,
                "locality": null,
                "organization": "Example",
                "organizational_unit": null,
                "postal_code": null,
                "province": null,
                "serial_number": null,
                "street_address": null
              }
            ],
            "uris": null,
            "validity_end_time": "2025-10-10T16:30:19.333401-07:00",
            "validity_period_hours": 26280,
            "validity_start_time": "2022-10-11T16:30:19.333401-07:00"
          },

With a difference of (amongst others due to the recreation):

59,61c59,61
<                 "common_name": "",
<                 "country": "",
<                 "locality": "",
---
>                 "common_name": null,
>                 "country": null,
>                 "locality": null,
63,66c63,66
<                 "organizational_unit": "",
<                 "postal_code": "",
<                 "province": "",
<                 "serial_number": "",
---
>                 "organizational_unit": null,
>                 "postal_code": null,
>                 "province": null,
>                 "serial_number": null,

Unfortunately, we cannot just introduce a state upgrade to unilaterally fix these "" to null differences as configurations may actually be setting "" explicitly (which we'd then be forcing those resources to recreate) and state upgrades do not have access to configuration information to conditionally perform the state upgrade in these cases.

bendbennett added a commit that referenced this issue Oct 12, 2022
… plan modifier which examines state and plan for string and list types (#284)
bendbennett added a commit that referenced this issue Oct 12, 2022
… plan modifier which examines state and plan for string and list types (#284)
bendbennett added a commit that referenced this issue Oct 12, 2022
… plan modifier which examines state and plan for string and list types (#284)
bendbennett added a commit that referenced this issue Oct 13, 2022
bendbennett added a commit that referenced this issue Oct 13, 2022
@hargut
Copy link

hargut commented Nov 2, 2022

As a workaround adding the fields with empty strings/lists works:

initial 3.x:

subject {
    common_name  = "Your CN"
    organization = "Your Org"
    organizational_unit = "Your OU"
}

avoid replace on 4.x:

subject {
    common_name  = "Your CN"
    organization = "Your Org"
    organizational_unit = "Your OU"
    # required on tls provider upgrade to 4.0.x to avoid replacement of the resource
    # FIXME: https://github.com/hashicorp/terraform-provider-tls/issues/284
    country = ""
    locality = ""
    postal_code = ""
    province = ""
    serial_number = ""
    street_address = []
}

Reminder: Always use lifecycle / prevent_destroy on critical resources.

@frimik
Copy link

frimik commented Nov 14, 2022

As per @hargut suggestion, note that values can vary a bit (see null in street_address). And this issue applies also to tls_cert_request.

There's also #263 but it's a no-op if the subjects is hardcoded to match current state as described in this issue.

terraform state pull | jq '.resources[] | select(.type == ("tls_self_signed_cert", "tls_cert_request")).instances[].attributes.subject'
[
  {
    "common_name": "identity-ca.linkerd.cluster.local",
    "country": "",
    "locality": "",
    "organization": "",
    "organizational_unit": "",
    "postal_code": "",
    "province": "",
    "serial_number": "",
    "street_address": null
  }
]
[
  {
    "common_name": "webhook.linkerd.cluster.local",
    "country": "",
    "locality": "",
    "organization": "",
    "organizational_unit": "",
    "postal_code": "",
    "province": "",
    "serial_number": "",
    "street_address": null
  }
]

@BornToBeRoot
Copy link

Any update on this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants