Skip to content

Require ClientAuth when verifying an X5cInsecure certificate#503

Merged
maraino merged 1 commit intomasterfrom
mariano/x5c-insecure
May 15, 2024
Merged

Require ClientAuth when verifying an X5cInsecure certificate#503
maraino merged 1 commit intomasterfrom
mariano/x5c-insecure

Conversation

@maraino
Copy link
Copy Markdown
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 thread jose/parse.go
Comment on lines +270 to +272
KeyUsages: []x509.ExtKeyUsage{
x509.ExtKeyUsageClientAuth,
},
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
@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