[Bug]: Certificates with UnknownCriticalExtensions cannot be renewed

unreality commented 2 years ago

Steps to Reproduce

Create a new provisioner with a template that has an unknown critical extension, eg:

{
"subject": {{ toJson .Subject }},
"sans": {{ toJson .SANs }},
{{- if typeIs "*rsa.PublicKey" .Insecure.CR.PublicKey }}
"keyUsage": ["keyEncipherment", "digitalSignature"],
{{- else }}
"keyUsage": ["keyAgreement", "digitalSignature"],
{{- end }}
"extKeyUsage": ["serverAuth", "clientAuth"],
"extensions": [
    {"id": "1.3.6.1.4.1.311.20.2", "critical": true, "value": "HiIARABvAG0AYQBpAG4AQwBvAG4AdAByAG8AbABsAGUAcgAA"}
]
}

Obtain a certificate normally (eg step ca certificate TestCert testcert.crt testcert.key)
Attempt to renew certificate with step (eg step ca renew --force testcert.crt testcert.key)

step-cli will report an error about invalid certificate, step-ca will report:

/usr/local/go/src/net/http/server.go:3228: http: TLS handshake error from 1.2.3.4:48050: tls: failed to verify client certificate: x509: unhandled critical extension

Your Environment

OS - Linux
step-ca Version - Smallstep CA/0.23.0-rc.1 (linux/amd64)

Expected Behavior

Certificate should be renewed

Actual Behavior

Certificate fails TLS handshake

Additional Context

No response

Contributing

Vote on this issue by adding a 👍 reaction. To contribute a fix for this issue, leave a comment (and link to your pull request, if you've opened one already).

hslatman commented 2 years ago

This is happening due to certificate validation after establishing a mutual TLS connection while trying to build a valid certificate chain. The error comes from here: https://github.com/golang/go/blob/4274ffd4b8bcef4e07cfdef9405a2e33f935d079/src/crypto/x509/verify.go#L556-L561. It is possible to remove critical extensions not handled by the standard library by removing them, but I think we should not default to doing that.

There exists a workaround for cases in which authentication with mTLS can be problematic, primarily with proxies, but it can also help in this case. With step ca renew --mtls=false --force testcert.crt testcert.key, the CLI won't try to establish a mutual TLS connection, but will use a token instead.

maraino commented 2 years ago

@hslatman the CA will also call x509.Verify with the certificate present in the token.

hslatman commented 2 years ago

Hmmm, too bad 😔

So we need to look into ignoring the unknown extensions, I suppose.

maraino commented 2 years ago

Hi @unreality, the root of the problem is that by RFC 5280 any system MUST reject a certificate with unknown critical extensions. Go doesn't know about them, and the verification fails appropriately.

We don't want to change the TLS verification, but we can change the X5C verification for a subset of extensions. The list of OIDS should be configurable, although there might be a reason to include some default ones used by Microsoft. See https://github.com/PeculiarVentures/PKI.js/issues/223 for a list.

Can you tell us how those extensions are used? And why do they need to be critical?

unreality commented 2 years ago

Can you tell us how those extensions are used? And why do they need to be critical?

Turns out this particular one doesn't need to be critical, I just made an error reading some MS documentation :)

1.3.6.1.4.1.311.20.2 is used to mark which Microsoft certificate template was used when creating the certificate. The BMP value i specified (DomainController) is required for Domain Controller certificates to be recognised correctly (per https://learn.microsoft.com/en-us/troubleshoot/windows-server/windows-security/requirements-domain-controller)

maraino commented 2 years ago

Thanks @unreality, I'm closing this issue for the moment, but if we need to allow critical extensions at some point, we can add a list. However, the need for a critical extension in this context would be very rare.

smallstep / certificates