Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Require ClientAuth when verifying an X5cInsecure certificate #503

Merged
merged 1 commit into from
May 15, 2024

Conversation

maraino
Copy link
Contributor

@maraino maraino commented May 14, 2024

The X5cInsecure certificate is used by step-ca to renew certificates without using mTLS, usually expired certificates. Certificate.Verify defaults to require ServerAuth if no KeyUsages is set as an option. Due to how these tokens are used, it makes more sense to require only ClientAuth.

Note that a few lines after this check is also made:

if leaf.KeyUsage&x509.KeyUsageDigitalSignature == 0 {
	return nil, nil, errors.New("certificate used to sign x5cInsecure token cannot be used for digital signature")
}

Related to smallstep/certificates#1843

The X5cInsecure certificate is used by step-ca to renew certificates
without using mTLS, usually expired certificates. Certificate.Verify
defaults to require ServerAuth if no KeyUsages is set as an option. But
due to how these tokens are used, it makes more sense to require only
ClientAuth.

Related to smallstep/certificates#1843
@maraino maraino requested a review from hslatman May 14, 2024 19:21
Comment on lines +270 to +272
KeyUsages: []x509.ExtKeyUsage{
x509.ExtKeyUsageClientAuth,
},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should ExtKeyUsageServerAuth be provided too, for backwards compatibility? I agree that it makes more sense to require just client auth, but there's a chance there's certs out there being used that don't have it set.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think so. This method is used as a replacement for mTLS when this cannot be performed, and the requirement for a client to do mTLS is to have x509.ExtKeyUsageClientAuth. See
https://github.com/golang/go/blob/1f6a983baf4b9a636e9e4bbd827fcb4d6ef4ebe0/src/crypto/tls/handshake_server.go#L892-L897

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine with merging it like this, but it still can be a breaking change in practice. Likely a fairly small chance, but still.

@maraino maraino requested a review from hslatman May 14, 2024 21:16
@maraino maraino merged commit 6a28ca4 into master May 15, 2024
13 checks passed
@maraino maraino deleted the mariano/x5c-insecure branch May 15, 2024 17:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants