fix: provide a sensible example for a privateca Root CA example #631

hoexter · 2024-04-02T10:22:46Z

This one looks a lot like someone copied by accident the subordinate example out of certificate_authority_subordinate/main.tf as a root CA. Thus it contains a lot of values set which are outright invalid or not recommend for Root CA certficates if you consider RFC 5280 and CA/B Baseline Requirements as the standard to follow.

Also the subordinate example is a bit odd, e.g. configuring SAN on any kind of CA certificate doesn't make sense. And the resources examples there make use of the same pool name.

I tried to keep the lifetime setting, but set it to 99 years. That is probably a sensible value for a P(rivate)KI setup. For something public 10y or 15y are probably more sensible.

Description

Fixes #630

Note: If you are not associated with Google, open an issue for discussion before submitting a pull request.

Checklist

Readiness

[] Yes, merge this PR after it is approved
No, don't merge this PR after it is approved

Testing

I have performed tests described in the Contributing guide:
- Tests pass: terraform apply
- Lint pass: terraform fmt check

--> this should get a test run somewhere, right now I don't have a test setup at hand to validate it against the API of the CAS

iennae · 2024-04-16T04:29:35Z

privateca/certificate_authority_basic/main.tf

-          email_protection = true
-          code_signing     = true
-          time_stamping    = true
+          server_auth = false


I think for the basic case this should be true; matching up to documentation here https://registry.terraform.io/providers/hashicorp/google/5.24.0/docs/resources/privateca_certificate_authority.

For the subordinate it's false.

Plesae don't look at the provider configuration examples, those are equally wrong. Please take a look at either RFCs or the CA/B BR. E.g. here for the EKU values take a look at https://cabforum.org/uploads/CA-Browser-Forum-TLS-BRs-v2.0.2.pdf 7.1.2.1.2 Root CA Extensions -> It's "Extension extKeyUsage" "Presence MUST NOT".
For a subordinate the EKU values are a MUST if it's not empty aka unrestricted.

Just to mention the obvious I'm not a PKIX expert myself, but inside Google you also have wizards like Ryan Sleevi, though trivial stuff like this might be not a good target for a wizard. The people who developed the CAS product should be also fine to consult with for the details.

Agreed with @hoexter: by default, we probably don't want any EKUs here.

EKUs can appear on a CA, but they have a different meaning there -- specifically, they function to limit the allowed set of EKUs on all intermediate CAs and leaf certificates in that chain. That's probably not something most people want to do at their root-level CA.

Btw, in this case I'd suggest dropping the extended_key_usage { .. } block completely, rather than including it with only false values.

Currently the language server tells me that the provider expects the extended_key_usage {} block to be present, maybe it can be empty.

privateca/certificate_authority_basic/main.tf

privateca/certificate_authority_subordinate/main.tf

msampathkumar · 2024-07-04T11:10:31Z

/gcbrun

Sita04 · 2024-07-05T15:47:02Z

@msampathkumar Can you help add @pmansour to the list of reviewers?

pmansour

Great callouts, thanks for helping to fix this! The values in this sample were probably copied over from somewhere without much thought about reasonable defaults. I think the changes here go a long way towards making these more realistic.

- Peter from the CAS eng team

pmansour · 2024-07-08T22:39:57Z

privateca/certificate_authority_basic/main.tf

-          email_protection = true
-          code_signing     = true
-          time_stamping    = true
+          server_auth = false


Agreed with @hoexter: by default, we probably don't want any EKUs here.

EKUs can appear on a CA, but they have a different meaning there -- specifically, they function to limit the allowed set of EKUs on all intermediate CAs and leaf certificates in that chain. That's probably not something most people want to do at their root-level CA.

privateca/certificate_authority_basic/main.tf

pmansour · 2024-07-08T22:45:59Z

privateca/certificate_authority_basic/main.tf

-          email_protection = true
-          code_signing     = true
-          time_stamping    = true
+          server_auth = false


Btw, in this case I'd suggest dropping the extended_key_usage { .. } block completely, rather than including it with only false values.

pmansour · 2024-07-08T22:46:51Z

privateca/certificate_authority_subordinate/main.tf

@@ -27,9 +29,6 @@ resource "google_privateca_certificate_authority" "root_ca" {
        organization = "HashiCorp"
        common_name  = "my-certificate-authority"
      }
-      subject_alt_name {
-        dns_names = ["hashicorp.com"]
-      }
    }
    x509_config {


could you also remove the extended_key_usage { .. } block here as well?

privateca/certificate_authority_subordinate/main.tf

pmansour · 2024-07-08T22:51:48Z

privateca/certificate_authority_subordinate/main.tf

 }

 resource "google_privateca_certificate_authority" "default" {
  // This example assumes this pool already exists.
  // Pools cannot be deleted in normal test circumstances, so we depend on static pools
-  pool                     = "my-pool"
+  pool                     = "my-sub-pool"
  certificate_authority_id = "my-certificate-authority-sub"
  location                 = "us-central1"
  deletion_protection      = false # set to true to prevent destruction of the resource


(sorry for the misplaced comment -- I can't figure out how to comment on the actual lines I want to mention, which weren't modified here)

Could you also make the following changes for the sub CA:

Remove the subject_alt_name

Get rid of all base_key_usage values except for cert_sign and crl_sign.

Get rid of the extended_key_usage block entirely

Update the lifetime from 1 day (86400s) to 5 years ("{5 * 365 * 24 * 3600}")

Update the signing key algorithm to use RSA_PKCS1_2048_SHA256 (more efficient for use in a sub CA)

done, also took the freedom to replace HashiCorp with ACME
eku block is empty, someone has to check what the provider actually makes out of it

https://github.com/GoogleCloudPlatform/magic-modules/blob/main/mmv1/products/privateca/CertificateAuthority.yaml#L404 there it's requiring the EKU block

This one looks a lot like someone copied by accident the subordinate example out of `certificate_authority_subordinate/main.tf` as a root CA. Thus it contains a lot of values set which are outright invalid or not recommend for Root CA certficates if you consider RFC 5280 and CA/B Baseline Requirements as the standard to follow. Also the subordinate example is a bit odd, e.g. configuring SAN on any kind of CA certificate doesn't make sense. And the resources examples there make use of the same pool name. Align the lifetime to some practical values, 10years for a Root CA and 5years for a subordinate. Signed-off-by: Sven Höxter <[email protected]>

hoexter requested review from a team as code owners April 2, 2024 10:22

iennae reviewed Apr 16, 2024

View reviewed changes

privateca/certificate_authority_basic/main.tf Outdated Show resolved Hide resolved

iennae reviewed Apr 16, 2024

View reviewed changes

privateca/certificate_authority_subordinate/main.tf Show resolved Hide resolved

pmansour suggested changes Jul 8, 2024

View reviewed changes

hoexter force-pushed the fix-root-ca-sample branch from 1697025 to bdda738 Compare July 9, 2024 08:02

Merge branch 'main' into fix-root-ca-sample

ee34cf8

hoexter requested a review from pmansour July 9, 2024 11:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: provide a sensible example for a privateca Root CA example #631

fix: provide a sensible example for a privateca Root CA example #631

hoexter commented Apr 2, 2024 •

edited

Loading

iennae Apr 16, 2024

hoexter Apr 16, 2024

hoexter Apr 16, 2024

pmansour Jul 8, 2024 •

edited

Loading

pmansour Jul 8, 2024

hoexter Jul 9, 2024

msampathkumar commented Jul 4, 2024

Sita04 commented Jul 5, 2024

pmansour left a comment •

edited

Loading

pmansour Jul 8, 2024 •

edited

Loading

pmansour Jul 8, 2024

pmansour Jul 8, 2024

pmansour Jul 8, 2024

hoexter Jul 9, 2024

hoexter Jul 9, 2024

fix: provide a sensible example for a privateca Root CA example #631

Are you sure you want to change the base?

fix: provide a sensible example for a privateca Root CA example #631

Conversation

hoexter commented Apr 2, 2024 • edited Loading

Description

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pmansour Jul 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msampathkumar commented Jul 4, 2024

Sita04 commented Jul 5, 2024

pmansour left a comment • edited Loading

Choose a reason for hiding this comment

pmansour Jul 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hoexter commented Apr 2, 2024 •

edited

Loading

pmansour Jul 8, 2024 •

edited

Loading

pmansour left a comment •

edited

Loading

pmansour Jul 8, 2024 •

edited

Loading