feature: added name constraints in certs with isCA enabled

[tanujd11]

Pull Request Motivation

Add name constraints feature in certificates with isCA enabled. Check #3655 for more details. The PR implements RFC: https://datatracker.ietf.org/doc/html/rfc5280#section-4.2.1.10

Following is an example of the certificate using the nameConstraint:

apiVersion: cert-manager.io/v1
kind: Certificate
metadata:
  name: ca-cert-example
spec:
  secretName: example-ca-key-pair
  isCA: true
  issuerRef:
    name: selfsigned
    kind: ClusterIssuer
  commonName: "example1.com"
  dnsNames:
  - example1.com
  nameConstraints:
    critical: true
    permitted:
      dnsDomains: ["example1.com", "example2.com"]
      ipRanges: ["10.10.0.0/16"]
      emailAddress: ["example@example.org"]
    excluded:
      ipRanges: ["10.10.0.0/24"]

Verification: Checkout the nameConstraint section of the CA cert generated by above yaml.

openssl x509 -in cert.crt -text
Certificate:
    Data:
        Version: 3 (0x2)
        Serial Number:
            51:81:28:14:0f:9b:fc:41:06:59:95:60:ff:2c:53:ed
        Signature Algorithm: sha256WithRSAEncryption
        Issuer: CN = example1.com
        Validity
            Not Before: Nov 23 06:11:27 2023 GMT
            Not After : Feb 21 06:11:27 2024 GMT
        Subject: CN = example1.com
        Subject Public Key Info:
            Public Key Algorithm: rsaEncryption
                Public-Key: (2048 bit)
                Modulus:
                    00:b7:fd:2c:bc:48:53:c2:f8:b8:2c:1d:54:62:ef:
                    04:d3:73:48:6d:24:2b:c9:37:cb:ba:7d:f3:b8:56:
                    f2:c7:59:26:70:a9:fd:7d:e2:34:3d:52:f9:ad:3d:
                    c9:54:17:1f:6e:92:56:cd:5f:4a:f2:fc:32:20:48:
                    94:b5:cf:30:8e:90:76:91:a5:34:bc:de:74:ab:8a:
                    a6:03:0b:41:74:f4:b8:c7:fd:93:19:e4:ed:d6:5f:
                    35:7f:2a:27:31:1c:9b:f0:92:05:c1:2f:19:73:de:
                    e1:b3:6b:6f:59:bd:89:5b:42:63:9c:82:6e:00:4c:
                    00:24:61:7f:4e:8d:95:0d:7b:b1:8b:2f:55:f8:d6:
                    a1:2e:0c:84:a4:64:38:a6:15:a5:1e:8d:8d:89:20:
                    55:52:15:ad:7b:ce:ac:b5:b8:f7:6b:18:24:54:0c:
                    63:fe:de:e4:1e:40:26:92:e8:e5:87:cf:23:0c:49:
                    f2:85:bb:94:ae:2f:4b:97:fb:97:ab:b5:b0:06:45:
                    51:69:95:d3:90:41:75:b1:4a:ed:6b:2d:fd:25:73:
                    51:83:8a:44:8c:f8:a0:20:dd:41:a4:d4:f5:4b:ef:
                    d8:6c:df:10:e6:51:67:15:c7:0d:ea:c3:d2:b1:64:
                    a5:57:04:00:5b:2e:91:31:97:ef:40:85:c5:9c:64:
                    74:67
                Exponent: 65537 (0x10001)
        X509v3 extensions:
            X509v3 Key Usage: critical
                Digital Signature, Key Encipherment, Certificate Sign
            X509v3 Basic Constraints: critical
                CA:TRUE
            X509v3 Subject Key Identifier:
                8B:00:98:38:52:1F:A1:03:83:12:B0:0A:D9:35:14:27:E2:FB:69:D4
            X509v3 Subject Alternative Name:
                DNS:example1.com
            X509v3 Name Constraints: critical
                Permitted:
                  DNS:example1.com
                  DNS:example2.com
                  IP:10.10.0.0/255.255.0.0
                  email:example@example.org
                Excluded:
                  IP:10.10.0.0/255.255.255.0
    Signature Algorithm: sha256WithRSAEncryption
    Signature Value:
        25:0f:c4:ea:e3:1b:29:c7:04:6a:ad:e1:78:bc:39:21:72:e3:
        a5:fc:4e:57:33:e1:92:c8:c3:41:b2:2b:22:ec:0f:2b:a8:3d:
        80:b3:3d:4b:c9:29:8e:fb:e9:0b:99:f5:2c:6c:02:b5:78:06:
        19:3b:55:f4:9e:1d:6a:b1:7d:3a:c8:07:47:50:68:d5:7d:c8:
        f6:83:45:6d:12:12:b5:15:ce:b9:e8:be:1c:81:ef:8a:01:08:
        66:e7:66:5c:02:fe:25:c7:f5:83:4e:9c:63:d2:3c:a3:eb:3a:
        77:8a:ef:8b:cc:fe:67:33:02:51:4f:81:d4:ff:8e:8b:e9:d0:
        7f:2a:f1:2e:51:5e:90:ea:ef:fd:62:54:9c:a0:82:1c:f5:ed:
        05:1a:a4:ee:aa:45:47:77:80:75:79:0c:04:d7:ee:4f:6a:1c:
        7e:f4:f7:83:d4:9a:e3:eb:28:13:88:fc:6c:70:df:c1:89:ab:
        76:b3:b7:e0:7f:63:53:bf:cc:92:c4:d2:4c:cb:ce:89:a3:dc:
        a3:09:13:14:88:aa:42:0a:87:df:86:3d:64:fe:36:cc:54:94:
        2e:51:77:2a:6c:98:ab:64:db:3b:cf:72:c9:9f:4f:48:bb:c3:
        f2:e2:cd:19:b7:f7:ea:29:0f:97:6c:65:92:ba:33:08:96:95:
        17:44:bc:50
-----BEGIN CERTIFICATE-----
MIIDdjCCAl6gAwIBAgIQUYEoFA+b/EEGWZVg/yxT7TANBgkqhkiG9w0BAQsFADAX
MRUwEwYDVQQDEwxleGFtcGxlMS5jb20wHhcNMjMxMTIzMDYxMTI3WhcNMjQwMjIx
MDYxMTI3WjAXMRUwEwYDVQQDEwxleGFtcGxlMS5jb20wggEiMA0GCSqGSIb3DQEB
AQUAA4IBDwAwggEKAoIBAQC3/Sy8SFPC+LgsHVRi7wTTc0htJCvJN8u6ffO4VvLH
WSZwqf194jQ9UvmtPclUFx9uklbNX0ry/DIgSJS1zzCOkHaRpTS83nSriqYDC0F0
9LjH/ZMZ5O3WXzV/KicxHJvwkgXBLxlz3uGza29ZvYlbQmOcgm4ATAAkYX9OjZUN
e7GLL1X41qEuDISkZDimFaUejY2JIFVSFa17zqy1uPdrGCRUDGP+3uQeQCaS6OWH
zyMMSfKFu5SuL0uX+5ertbAGRVFpldOQQXWxSu1rLf0lc1GDikSM+KAg3UGk1PVL
79hs3xDmUWcVxw3qw9KxZKVXBABbLpExl+9AhcWcZHRnAgMBAAGjgb0wgbowDgYD
VR0PAQH/BAQDAgKkMA8GA1UdEwEB/wQFMAMBAf8wHQYDVR0OBBYEFIsAmDhSH6ED
gxKwCtk1FCfi+2nUMBcGA1UdEQQQMA6CDGV4YW1wbGUxLmNvbTBfBgNVHR4BAf8E
VTBToEMwDoIMZXhhbXBsZTEuY29tMA6CDGV4YW1wbGUyLmNvbTAKhwgKCgAA//8A
ADAVgRNleGFtcGxlQGV4YW1wbGUub3JnoQwwCocICgoAAP///wAwDQYJKoZIhvcN
AQELBQADggEBACUPxOrjGynHBGqt4Xi8OSFy46X8Tlcz4ZLIw0GyKyLsDyuoPYCz
PUvJKY776QuZ9SxsArV4Bhk7VfSeHWqxfTrIB0dQaNV9yPaDRW0SErUVzrnovhyB
74oBCGbnZlwC/iXH9YNOnGPSPKPrOneK74vM/mczAlFPgdT/jovp0H8q8S5RXpDq
7/1iVJygghz17QUapO6qRUd3gHV5DATX7k9qHH7094PUmuPrKBOI/Gxw38GJq3az
t+B/Y1O/zJLE0kzLzomj3KMJExSIqkIKh9+GPWT+NsxUlC5RdypsmKtk2zvPcsmf
T0i7w/LizRm39+opD5dsZZK6MwiWlRdEvFA=
-----END CERTIFICATE-----

Kind

feature

Release Note

Users can now use nameConstraints in CA certificates. To know more details on name constraints check out RFC section https://datatracker.ietf.org/doc/html/rfc5280#section-4.2.1.10

[jetstack-bot]

Hi @tanujd11. Thanks for your PR.

I'm waiting for a cert-manager member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[maelvls]

Hi! I'll turn on testing for you.

/ok-to-test
/kind feature

[maelvls]

Hi! Great work on this, I'm really glad you were able to take a stab at this!!

Regarding the choice of going with a feature flag, I imagine that the point of gating this feature is to refine the API surface? Or should we have to worry about the consequence of turning on this feature for everyone for some unexpected reason?

Could you add an example of what the API would look like in the PR description too? Thanks!

[tanujd11]

Hi @maelvls I have added the API in the PR description. Also added validating name constraint step at issuer before signing the certificate. Please take a look. Thanks

[maelvls]

The test "certificates: TestGeneratesNewPrivateKeyPerRequest" fails because one of the HTTP calls in the test code isn't being retried and thus fails when a benign optimistic locking error shows up:

generates_new_private_key_per_request_test.go:273: failed to update certificate: Operation cannot be fulfilled on certificates.cert-manager.io "testcrt": the object has been modified; please apply your changes to the latest version and try again

I am surprised that we still get this kind of problem, I tried to fix most of them a year ago 😅 Let's re-run the test, the error should go away.

[maelvls]

For context for anyone reading this, this PR is in three folds:

• A new feature gate UseCertificateRequestNameConstraints introduced in alpha. I am still unsure whether this is needed. Previously, we used the feature gate UseCertificateRequestBasicConstraints because it would suddenly change the contents of CertificateRequests if they had isCA: true. I am unsure whether it also applies to this name constraints PR.
• A pre-signing check specific to the CA issuer: when the CA issuer is about to sign a certificate, the CA issuer performs an ad-hoc check of the nameConstraints with respect to the DNS names, URIs, emails, and IP addresses. I think I need help in knowing whether having an ad-hoc func is a good idea here.
• An addition to the Certificate API: the nameConstraints field is added. I think this change in not controversial, the API seems OK to me.

About the pre-signing check in the CA issuer (func validateNameConstraints):

• The reason we want to add this pre-signing check is because Tanuj found out that it is easy to mistakenly to have the CA issuer issue a leaf certificate that doesn't comply with the name constraints. That's because the signed cert isn't validated after signing it in the CA issuer.
• For context, the name constraints are meant to be validated by the client during the TLS handshake. The client validates that the name constraints of the intermediate CAs and root CA match its subject DN and SANs.
• After looking at the complexity of the Name Constraints section in the RFC, I worry that this ad-hoc pre-signing check won't match the actual rules. The risk is minimal since it is the client's responsibility to check these constraints, not ours. The worst case scenario is that the pre-signing check is too tight, in which case we can always patch cert-manager.
• Go's x509 package has an internal func checkNameConstraints that does this correctly, but it is much more involved and requires knowing about the whole chain, not just the (possibly) intermediate CA cert. The CA issuer doesn't have the information about the whole chain which prevents us from vendoring the Go lib's func.
• To conclude, although validateNameConstraints is imperfect, I am happy with keeping it as-is and not trying to re-implement the whole name constraints validation.

@SgtCoDFish Am I correct in thinking that the CA issuer can't validate its own issued certificates when the CA issuer is given an intermediate CA certificate and private key?

[tanujd11]

/retest

[tanujd11]

Thanks @inteon for pointing this one out. Somehow I thought I fixed the typo but it was not the case. Have done it now, Also please let me know of your thoughts on the other comment

[maelvls]

Hey! It seems like all the remarks from @inteon have been resolved. I'd be happy to have this merged once we have a documentation PR. Since this new feature applies to the next version of cert-manager (1.14), the changes need to be done to the docs/ folder and branched from the release-next branch. Let me know if you need help!

[inteon]

/approve
/lgtm
/hold in case someone else wants to review

[maelvls]

The documentation PR is ready: cert-manager/website#1358. Thanks Tanuj!

/lgtm

[jetstack-bot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: inteon, maelvls

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [inteon,maelvls]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[inteon]

/unhold

[inteon]

I missed this in the review, but this is not correct, this is not what is described in the x509 spec.
/cc @maelvls

[inteon]

@tanujd11 could you create a new PR to fix this?

[tanujd11]

Hey @inteon , what exactly are you referring here?

[inteon]

The marshalled NameConstraints extension in the CSR has to match the format defined in the RFC 5280, 4.2.1.10 (see https://www.alvestrand.no/objectid/2.5.29.30.html).
The encoding you specified seems to be custom.
We can use the logic the x509 library uses: https://cs.opensource.google/go/go/+/refs/tags/go1.21.5:src/crypto/x509/x509.go;l=1189-1285;drc=82c713feb05da594567631972082af2fcba0ee4f

[inteon]

NOTE: we found that we missed something in this PR and fixed the issue in #6542.