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]>

msampathkumar · 2024-09-30T12:20:18Z

This PR has been inactive for two months. If this inactivity continues for another two weeks, I will close the request.

hoexter · 2024-10-07T07:51:02Z

@msampathkumar I updated the PR long ago with the proposed changes by @pmansour. Thus it's just waiting for a review by you/your team. :)

glasnt · 2024-10-10T04:35:22Z

/gcbrun

* fix: add my-sub-pool to support #631 * update resource alias

iennae · 2024-10-14T19:19:43Z

/gcbrun

glasnt · 2024-10-14T21:16:03Z

The tests here now pass with #748 merged, but I am not knowledgeable enough in this specific area to review this PR. @iennae are you able to review?

hoexter · 2024-10-23T13:12:48Z

Can I do anything else to help you to review this PR and get it a state you can merge?

glasnt · 2024-10-23T21:30:14Z

/gcbrun

glasnt · 2024-11-04T04:24:29Z

I haven't been able to find a subject matter expert on this, but looking at the changes made, it seems reasonable. I'm not an expert on RFC5280, but removing some of the extra fields appears to match what modules do (e.g.. csa-certificate-authority-service)

glasnt · 2024-11-04T04:24:39Z

/gcbrun

…alues Some of the properties configured here are either wrong or at least not very sensible on root certificates / subordinates. A similar set of fixes got applied to terraform documentation samples in terraform-google-modules/terraform-docs-samples#631 Signed-off-by: Sven Hoexter <[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

hoexter requested a review from iennae October 7, 2024 07:49

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

572726a

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

f5acd83

glasnt added a commit that referenced this pull request Oct 10, 2024

fix: add my-sub-pool to support #631

ccf96f6

glasnt mentioned this pull request Oct 10, 2024

fix: add my-sub-pool to support #631 #748

Merged

2 tasks

iennae pushed a commit that referenced this pull request Oct 14, 2024

fix: add my-sub-pool to support #631 (#748)

0a8dfce

* fix: add my-sub-pool to support #631 * update resource alias

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

2e01f3b

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

5b144c9

glasnt enabled auto-merge (squash) November 4, 2024 04:19

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

a0147de

glasnt approved these changes Nov 4, 2024

View reviewed changes

glasnt merged commit 8925259 into terraform-google-modules:main Nov 4, 2024
5 checks passed

hoexter mentioned this pull request Nov 6, 2024

fix(privateca): configure pathLen: 0 constrain correctly #757

Merged

hoexter mentioned this pull request Nov 6, 2024

privateca: update certificate authority samples with more realistic values GoogleCloudPlatform/magic-modules#12259

Merged

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

msampathkumar commented Sep 30, 2024

hoexter commented Oct 7, 2024

glasnt commented Oct 10, 2024

iennae commented Oct 14, 2024

glasnt commented Oct 14, 2024

hoexter commented Oct 23, 2024

glasnt commented Oct 23, 2024

glasnt commented Nov 4, 2024

glasnt commented Nov 4, 2024

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

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

msampathkumar commented Sep 30, 2024

hoexter commented Oct 7, 2024

glasnt commented Oct 10, 2024

iennae commented Oct 14, 2024

glasnt commented Oct 14, 2024

hoexter commented Oct 23, 2024

glasnt commented Oct 23, 2024

glasnt commented Nov 4, 2024

glasnt commented Nov 4, 2024

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