Implemented tls_x509_crl resource

[fllaca]

Hi, I added here a first version of a x509 CRL resource, resolving #20 . The way of using it is using a field certs_to_revoke, that contains the certificates (in PEM format) that will be revoked.

Resolves #20

PS: also updated Golang version to currently latest 1.14

[invidian]

Hey @fllaca, thanks for the PR. The code looks good to me as for the initial approach, great work!

I've added some suggestions of what I would improve + some styling picks.

[invidian]

While technically correct, it would be better, if we would update the list of certificates in the CRL, rather than create the new one every time, as then the expiry date will be updated too for example.

[fllaca]

Yeah... but I'm quite unsure on how to handle that lifecycle 🤔 , for instance the Id of the resource is calculated as a hash of the CRL PEM content (in the same way we do for locally signed certificates and other resources in this provider). When a CRL is "updated" actually what happens is that a new version of it is created, basically a new CRL itself. I was checking https://tools.ietf.org/html/rfc5280#section-5.1 trying to find a field that we can use to identify a CRL independently of the version, but I couldn't find any. But I can miss something, do you have any idea around this?

[invidian]

Hm, I'm not sure if I understand the problem. If you set the ID only in Create function and then don't touch it in Update, then what's the problem? I don't think there is a need to change the ID.

Aside from that, I think this is completely fine for the initial implementation. It can always be improved later 👍

[fllaca]

Hi @invidian , thanks a lot for hour review! I addressed most of your comments, please take a look again when you have a minute. For some other comments I had some doubts, responded with some thoughts about them.

[invidian]

Thanks for addressing my feedback @fllaca, the code looks very good for me now. I didn't check the tests thoroughly, but at a glace they look good to me too.

I followed up on the previous comments as well.

[invidian]

Hm, I'm not sure if I understand the problem. If you set the ID only in Create function and then don't touch it in Update, then what's the problem? I don't think there is a need to change the ID.

Aside from that, I think this is completely fine for the initial implementation. It can always be improved later 👍

[invidian]

So, I've made some tests on this code with the code posted below, which in my opinion roughly simulates real life use-case.

I've created few certificates using Terraform and then tried to revoke them. It seems that currently, it's difficult to revoke certificates created with Terraform, as if I taint the certificate for example, then run terraform apply, the old certificate is being removed from CRL and new one is added there. What I would expect is, once the certificate is tained, that the old certificate is being stored in the CRL and new one is not there.

The same would be needed for certificates, which are removed from Terraform code. Let's say I have an array of client certificates to create, I pass them to count. Ideally, if one of the clients is removed from the list, Terraform should destroy it's certificate and add it to the CRL.
I think this could be implemented in following way. One could provide all certificates which could ever be revoked to the CRL resource:

resource "tls_x509_crl" {
   certificates = [
      tls_self_signed_cert.client.*.cert_pem,
    ]
}

Then, in CustomizeDiff, we could check for changes on certificates field and when we would notice, that some certificate has been removed from this field, we would add it to the CRL (as CRL should be add-only, as far as I know). This way, if reference to the certificate is removed from Terraform in any way, then it will be added to the CRL. In addition, there could be another parameter, which handles "static" certificates for revocation, as certs_to_revoke currently does.

I think, if one wants to create CRL "manually" (providing hardcoded certificates e.g. via values), then this is sufficient, but to be actually able to control PKI lifecycle using Terraform, a bit more is needed. It would be good to get some more input from people interested in this PR, so we can ensure it's usable.

I also found 2 tiny nits @fllaca, I hope it's fine.

The code I used for testing, if someone wants to play around:

resource "tls_private_key" "root_ca" {
  algorithm = "RSA"
  rsa_bits  = 2048
}

resource "tls_self_signed_cert" "root_ca" {
  key_algorithm   = tls_private_key.root_ca.algorithm
  private_key_pem = tls_private_key.root_ca.private_key_pem

  subject {
    common_name  = "root-ca"
    organization = "foo"
  }

  is_ca_certificate     = true
  validity_period_hours = 24

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

resource "tls_private_key" "etcd_ca" {
  algorithm = "RSA"
  rsa_bits  = 2048
}

resource "tls_cert_request" "etcd_ca" {
  key_algorithm   = tls_private_key.etcd_ca.algorithm
  private_key_pem = tls_private_key.etcd_ca.private_key_pem

  subject {
    # This is CN recommended by K8s: https://kubernetes.io/docs/setup/best-practices/certificates/
    common_name  = "etcd-ca"
    organization = "foo"
  }
}

resource "tls_locally_signed_cert" "etcd_ca" {
  cert_request_pem = tls_cert_request.etcd_ca.cert_request_pem

  ca_key_algorithm   = "RSA"
  ca_private_key_pem = tls_private_key.root_ca.private_key_pem
  ca_cert_pem        = tls_self_signed_cert.root_ca.cert_pem

  is_ca_certificate = true

  # TODO make it configurable
  validity_period_hours = 8760

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

resource "tls_cert_request" "etcd_ca2" {
  key_algorithm   = tls_private_key.etcd_ca.algorithm
  private_key_pem = tls_private_key.etcd_ca.private_key_pem

  subject {
    # This is CN recommended by K8s: https://kubernetes.io/docs/setup/best-practices/certificates/
    common_name  = "etcd-ca"
    organization = "foo"
  }
}

resource "tls_locally_signed_cert" "etcd_ca2" {
  cert_request_pem = tls_cert_request.etcd_ca2.cert_request_pem

  ca_key_algorithm   = "RSA"
  ca_private_key_pem = tls_private_key.root_ca.private_key_pem
  ca_cert_pem        = tls_self_signed_cert.root_ca.cert_pem

  is_ca_certificate = true

  # TODO make it configurable
  validity_period_hours = 8760

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

resource "tls_x509_crl" "test" {
  certs_to_revoke = [
    tls_locally_signed_cert.etcd_ca2.cert_pem,
  ]
  validity_period_hours = 24
  early_renewal_hours   = 1
  ca_cert_pem           = tls_self_signed_cert.root_ca.cert_pem
  ca_key_algorithm      = "RSA"
  ca_private_key_pem    = tls_private_key.root_ca.private_key_pem
}

output "crl_pem" {
  value = tls_x509_crl.test.crl_pem
}

output "revoked_cert" {
  value = tls_locally_signed_cert.etcd_ca.cert_pem
}

[invidian]

RevocationTime field should be set here, otherwise revoked date in CRL is always: Revocation Date: Jan 1 00:00:00 1 GMT.

[invidian]

Ditto ${}.

[fllaca]

Hi @invidian , sorry for the delayed response, I was really busy the last two weeks.

Thank you so much for the thoughtful feedback! Yeah... I see the points in your argumentation: as you say, with the current approach in this PR if you revoke a certificate and add it to the CRL you need to keep it forever in the terraform code and state (either hardcoded PEM or keeping its certificate resource definition). If you taint a certificate, the old version of it is not going to be in the list of serial numbers of the newly regenerated CRL.

But on the other hand, if you taint it, from the crypto point of view it is going to be actually a completely new certificate different from the previous one you had in tf state. I'm not that sure that the Terraform resource should handle this usecase internally by maintaining an "history"� of revoked certificates in the state that don't appear in the resource definition in the terraform code. Normally, what I expect from terraform is that the code, the state, and the actual real resources are three views of the same thing, and Terraform's goal is to reconcile all three. Anyway I can be wrong here, I don't know if there's any other existing provider/resource that does something similar 🤔 , it could serve as inspiration to implement this as well.

These are just some thoughts, but I don't have a strong opinion... I agree with you, it would be good having some more input feedback on this. In any case I will give it a try to implement a version that keeps the history of revoked certs, maybe in another branch, at least just to see how it looks.

[hashicorp-cla]

Thank you for your submission! We require that all contributors sign our Contributor License Agreement ("CLA") before we can accept the contribution. Read and sign the agreement

Learn more about why HashiCorp requires a CLA and what the CLA includes

_{Have you signed the CLA already but the status is still pending? Recheck it.}

[mhmdio]

any update on this @fllaca ?

[swade1987]

any update on this?

[plsanchezfaure]

any update on this?

[sergio-toro]

Hey any update on this PR? would be great to manage revocation lists with this module 🙏🏽

[trebidav]

Would love to see this implemented! It's needed for years already!

[debu99]

any update??