Add support for setting `pathlen` on CA certificates and intermediate CA #135

socheatsok78 · 2022-01-13T14:21:06Z

Taken from RFC 5280, section 4.2.1.9:

A pathLenConstraint of zero indicates that no non-self-issued intermediate CA certificates may follow in a valid certification path. Where it appears, the pathLenConstraint field MUST be greater than or equal to zero. Where pathLenConstraint does not appear, no limit is imposed.

I.e. a pathLenConstraintof 0 does still allow the CA to issue certificates, but these certificates must be end-entity-certificates (the CA flag in BasicConstraints is false - these are the "normal" certificates that are issued to people or organizations).

It also implies that with this certificate, the CA must not issue intermediate CA certificates (where the CA flag is true again - these are certificates that could potentially issue further certificates, thereby increasing the pathLen by 1).

An absent pathLenConstraint on the other hand means that there is no limitation considering the length of certificate paths built from an end-entity certificate that would lead up to our example CA certificate. This implies that the CA could issue an intermediate certificate for a sub CA, this sub CA could again issue an intermediate certificate, this sub CA could again... until finally one sub CA would issue an end-entity certificate.

If the pathLenConstraintof a given CA certificate is > 0, then it expresses the number of possible intermediate CA certificates in a path built from an end-entity certificate up to the CA certificate. Let's say CA X has a pathLenConstraint of 2, the end-entity certificate is issued to EE. Then the following scenarios are valid (I denoting an intermediate CA certificate)

Example

Create CA key

$ certstrap init --common-name CA --exclude-path-length
[...]
$ openssl x509 -noout -text -in out/CA.crt
[...]
        X509v3 extensions:
            X509v3 Key Usage: critical
                Certificate Sign, CRL Sign
            X509v3 Basic Constraints: critical
                CA:TRUE
            X509v3 Subject Key Identifier:
                AE:1B:C3:B5:2C:29:05:3B:D2:94:D7:09:27:15:28:5A:04:D7:78:C7
[...]

Create intermediate CA 1 key

$ certstrap request-cert --common-name ICA1
[...]
$ certstrap sign ICA1 --CA CA --intermediate --path-length 1
[...]
$ openssl x509 -noout -text -in out/ICA1.crt
[...]
        X509v3 extensions:
            X509v3 Key Usage: critical
                Certificate Sign, CRL Sign
            X509v3 Basic Constraints: critical
                CA:TRUE, pathlen:1
            X509v3 Subject Key Identifier:
                1D:86:5B:08:0B:81:4A:12:02:0E:F0:B0:32:A1:F8:6D:88:F5:3A:4E
            X509v3 Authority Key Identifier:
                keyid:AE:1B:C3:B5:2C:29:05:3B:D2:94:D7:09:27:15:28:5A:04:D7:78:C7
[...]

Create intermediate CA 2 key

$ certstrap request-cert --common-name ICA2
[...]
$ certstrap sign ICA2 --CA ICA1 --intermediate
[...]
$ openssl x509 -noout -text -in out/ICA12crt
[...]
        X509v3 extensions:
            X509v3 Key Usage: critical
                Certificate Sign, CRL Sign
            X509v3 Basic Constraints: critical
                CA:TRUE, pathlen:0
            X509v3 Subject Key Identifier:
                47:0C:F5:1B:A8:72:64:D1:56:4B:D6:FB:AC:40:68:39:58:77:38:AA
            X509v3 Authority Key Identifier:
                keyid:1D:86:5B:08:0B:81:4A:12:02:0E:F0:B0:32:A1:F8:6D:88:F5:3A:4E
[...]

…aram

Add support for setting `pathlen` on CA certificates and intermediate CA

CLAassistant · 2022-01-13T14:21:11Z

All committers have signed the CLA.

socheatsok78 · 2022-01-13T14:22:21Z

Thanks to #112 for original idea.

MousyBusiness · 2022-01-13T20:21:12Z

I've recently hit this as an issue. A rootCA created with CreateCertificateAuthority used to create intermediaries with CreateIntermediateCertificateAuthority will create the certs, the intermediary even work in Go. But when using python or node (which require the full authority chain), it fails.

jdtw · 2022-05-10T16:07:39Z

pkix/cert_auth.go

+	if !excludePathlen {
+		if pathlen > 0 {
+			authTemplate.MaxPathLen = pathlen
+			authTemplate.MaxPathLenZero = false


MaxPathLenZero is only checked when MaxPathLen == 0. With that in mind, can we simplify this logic? Something like:

authTemplate.MaxPathLen = pathlen if excludePathLen { authTemplate.MaxPathLen = -1 }

Then MaxPathLenZero can remain true and we use the explicit MaxPathLen = -1 to unset it.

Sorry for late to response. I should have went with that in the first place xD. Thank you for pointing out.

Already made the change. Please give it a look.

To be honest, the change I've just made doesn't seem right. Let me see what I can do or if you have any suggestion for making it better. Please let me know.

Added a comment on the current iteration.

jdtw · 2022-05-10T16:25:29Z

pkix/cert_auth.go

 // CreateIntermediateCertificateAuthority creates an intermediate
 // CA certificate signed by the given authority.
-func CreateIntermediateCertificateAuthority(crtAuth *Certificate, keyAuth *Key, csr *CertificateSigningRequest, proposedExpiry time.Time) (*Certificate, error) {
+func CreateIntermediateCertificateAuthority(crtAuth *Certificate, keyAuth *Key, csr *CertificateSigningRequest, proposedExpiry time.Time, pathlen int) (*Certificate, error) {


Sorry, it's taken me so long to review these changes because I needed to be sure that this change to the public pkix package wouldn't break anyone. Unfortunately, looks like it will, for example https://cs.github.com/cloudfoundry/locket/blob/ad30c800960d38a0090acd7aa2acfc3f439b1f82/cmd/locket/certauthority/certauthority.go#L88.

Since we're not ready for a v2, I propose we add new functions (CreateIntermediateCertificateAuthorityWithOptions and CreateCertificateAuthorityWithOptions) to take opts ...Option to keep backwards compatibility:

type Option func(*x509.Certificate) func WithPathlen(pathlen int, excludePathlen bool) { return func(template *x509.Certificate) { // Set MaxPathLen appropriately.... } }

(Note that part of the v2 changes will be moving things into an internal package so that it's clear we won't guarantee backward compatibility of thee APIs -- this isn't the first time it's hampered contributions. Thanks again for your patience.)

Okay, let me revert the original function back. I kinda don't want other people code to break as well :D

For the init command, it is okay to add the extra flag right?

Yes, I think it's fine to have the exclude flag in init.

…thlen' param" This reverts commit f1e380b.

…ion`

…cateAuthorityWithOptions`

…thOptions`

…tions` before `x509.CreateCertificate`

socheatsok78 · 2022-06-18T14:49:03Z

Should we check the MaxPathLen when trying to sign an intermediate certificate?

if raw_crt.MaxPathLen == 0 {
	fmt.Fprintln(os.Stderr, "Selected CA certificate is not allowed to issue intermediate certificates.")
	os.Exit(1)
}

# Conflicts: # cmd/sign.go

jdtw · 2022-06-21T19:27:36Z

Should we check the MaxPathLen when trying to sign an intermediate certificate?

That might be a nice future improvement, but let's leave it out of this PR to keep the change minimal.

pkix/cert_auth.go

jdtw · 2022-06-21T19:26:15Z

pkix/cert_auth.go

+	if !excludePathlen {
+		if pathlen > 0 {
+			authTemplate.MaxPathLen = pathlen
+			authTemplate.MaxPathLenZero = false


Added a comment on the current iteration.

jdtw · 2022-06-21T19:26:35Z

pkix/cert_auth.go

 // CreateIntermediateCertificateAuthority creates an intermediate
 // CA certificate signed by the given authority.
-func CreateIntermediateCertificateAuthority(crtAuth *Certificate, keyAuth *Key, csr *CertificateSigningRequest, proposedExpiry time.Time) (*Certificate, error) {
+func CreateIntermediateCertificateAuthority(crtAuth *Certificate, keyAuth *Key, csr *CertificateSigningRequest, proposedExpiry time.Time, pathlen int) (*Certificate, error) {


Yes, I think it's fine to have the exclude flag in init.

…Options` internally

…CertificateAuthorityWithOptions` internally

…operty

jdtw

Thanks again for your patience on this one!

jdtw · 2022-06-24T19:05:29Z

pkix/cert_auth.go

 // CreateCertificateAuthority creates Certificate Authority using existing key.
 // CertificateAuthorityInfo returned is the extra infomation required by Certificate Authority.
 func CreateCertificateAuthority(key *Key, organizationalUnit string, expiry time.Time, organization string, country string, province string, locality string, commonName string, permitDomains []string) (*Certificate, error) {
+	// Passing all arguments to CreateCertificateAuthorityWithOptions


Nit: you can remove these comments (here and below).

Will do in the morning, it's 3am over here! And I'm commenting from my phone 😄

socheatsok78 · 2022-06-24T19:09:45Z

Thanks again for your patience on this one!

Thank you for reviewing the PR!

- Change pkix function to have WithPathLen options for intermediate certs Context: square/certstrap#135

* Bump to ginkgo/v2 & lager/v3 - Change pkix function to have WithPathLen options for intermediate certs Context: square/certstrap#135 * fix `go vet` failures

… CA (#135)

socheatsok78 and others added 7 commits January 13, 2022 21:00

Update 'CreateCertificateAuthority' to accept 'excludePathlen' param

3e21a32

Update 'CreateIntermediateCertificateAuthority' to accept 'pathlen' p…

f1e380b

…aram

Update cmd/revoke_test.go test

63b3315

Add 'path-length' and 'exclude-path-length' flags to 'init' command

b52c42e

Add 'path-length' flag to 'sign' command

2be8173

Update 'exclude-path-length' usage description

bf35827

Merge pull request #1 from socheatsok78/feature/add-ca-path-length

6e823d5

Add support for setting `pathlen` on CA certificates and intermediate CA

socheatsok78 changed the title ~~Add support for setting pathlen on CA certificates and intermediate CA~~ Add support for setting pathlen on CA certificates and intermediate CA Jan 13, 2022

VeitSanner mentioned this pull request Apr 9, 2022

Intermediate with OpenSSL: verify error:num=25:path length constraint exceeded #78

Open

jdtw suggested changes May 10, 2022

View reviewed changes

Fix excludePathlen check logic

4ce4781

socheatsok78 requested review from a team, eknapp-square, mmontgomery-square and sq-smits June 18, 2022 06:46

socheatsok78 added 12 commits June 18, 2022 13:50

Revert "Update 'CreateIntermediateCertificateAuthority' to accept 'pa…

8879b3e

…thlen' param" This reverts commit f1e380b.

Draft CreateCertificateAuthorityWithOption

292688a

Fix sign command since the has been reverted

52e75d7

Remove unused block

481991f

Draft init command to use CreateCertificateAuthorityWithOption

a22b32b

Update draft as @jdtw commented on `CreateCertificateAuthorityWithOpt…

39395b0

…ion`

Fix docs for CreateCertificateAuthorityWithOption

29766f0

Renamed func CreateCertificateAuthorityWithOption to `CreateCertifi…

1e828c7

…cateAuthorityWithOptions`

Draft CreateIntermediateCertificateAuthorityWithOptions func

e963a92

Draft sign command to use `CreateIntermediateCertificateAuthorityWi…

f021c62

…thOptions`

Fix 'path-length' flag

2176c12

Fix docs for WithPathlenOption func

8875015

socheatsok78 and others added 8 commits June 18, 2022 15:01

Fix tests that uses pkix.CreateCertificateAuthority

60a4f1c

Merge branch 'square:master' into master

043f716

Add applyOptions func instead of for_each loop

74dd9d4

Ensure CreateCertificateAuthority are back to it original settings

9809356

Add CreateCertificateAuthorityWithOptions test

9f20130

Fix CreateCertificateAuthorityWithOptions test name

a2a6467

Fix CreateIntermediateCertificateAuthorityWithOptions exec `applyOp…

3770ab4

…tions` before `x509.CreateCertificate`

Move authTemplate.MaxPathLenZero to WithPathlenOption

3b6d4be

Make certification creation options more accessible

3b8f332

# Conflicts: # cmd/sign.go

jdtw suggested changes Jun 21, 2022

View reviewed changes

socheatsok78 added 3 commits June 22, 2022 07:57

Let CreateCertificateAuthority call `CreateCertificateAuthorityWith…

c2b1399

…Options` internally

Let CreateIntermediateCertificateAuthority call `CreateIntermediate…

866b58f

…CertificateAuthorityWithOptions` internally

Fix WithPathlenOption func to avoid setting the MaxPathLenZero pr…

e2e8940

…operty

jdtw approved these changes Jun 24, 2022

View reviewed changes

jdtw merged commit 825fe42 into square:master Jun 24, 2022

winkingturtle-vmw pushed a commit to cloudfoundry/inigo that referenced this pull request Apr 5, 2023

Bump to ginkgo/v2 & lager/v3

c52bb13

- Change pkix function to have WithPathLen options for intermediate certs Context: square/certstrap#135

winkingturtle-vmw mentioned this pull request Apr 5, 2023

Bump to ginkgo/v2 & lager/v3 cloudfoundry/inigo#25

Merged

mbyczkowski pushed a commit that referenced this pull request May 12, 2023

Add support for setting pathlen on CA certificates and intermediate…

63ada5f

… CA (#135)

Add support for setting pathlen on CA certificates and intermediate CA #135

Add support for setting pathlen on CA certificates and intermediate CA #135

Uh oh!

Conversation

socheatsok78 commented Jan 13, 2022

Example

Uh oh!

CLAassistant commented Jan 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

socheatsok78 commented Jan 13, 2022

Uh oh!

MousyBusiness commented Jan 13, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

socheatsok78 Jun 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as resolved.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

socheatsok78 commented Jun 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jdtw commented Jun 21, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jdtw left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

socheatsok78 commented Jun 24, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Add support for setting `pathlen` on CA certificates and intermediate CA #135

Add support for setting `pathlen` on CA certificates and intermediate CA #135

CLAassistant commented Jan 13, 2022 •

edited

Loading

socheatsok78 Jun 18, 2022 •

edited

Loading

socheatsok78 commented Jun 18, 2022 •

edited

Loading