Skip to content

tls/client auth: verify first certificate in client request #3344

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

Merged
merged 1 commit into from
May 6, 2020

Conversation

KarolBedkowski
Copy link
Contributor

Hi,
I have problem with certificate-based authentication. In my env Caddy can't authorized connection despite correct certificates and configuration. After some debugging I notice that Caddy check only last certificate in request but client certificate is fist and second is CA cert.
This patch change this behavior - all certs from request are verified.


When client certificate is enabled Caddy check only last certificate from
request. When this cert is not in list of trusted leaf certificates,
connection is rejected. But in some cases client certificate is not
last certificate in list; sometimes is first, before certificates of CA.

This patch add checking all sent certificates - if any match with trusted
certificates connection is accepted.

@CLAassistant
Copy link

CLAassistant commented May 4, 2020

CLA assistant check
All committers have signed the CLA.

@francislavoie francislavoie added this to the 2.1 milestone May 4, 2020
@francislavoie
Copy link
Member

francislavoie commented May 4, 2020

Thanks! Looks pretty simple. We definitely want to be careful here because it affects security.

Don't forget to sign the CLA! It looks like there might be a mismatch between the email address you made your commit with and the email address in your Github account. You might need to resolve that before the CLA bot recognizes that you signed it.

@mholt
Copy link
Member

mholt commented May 4, 2020

It does look like we got it backwards; the spec (RFC 5246) requires:

   certificate_list
      This is a sequence (chain) of certificates.  The sender's
      certificate MUST come first in the list.  Each following
      certificate MUST directly certify the one preceding it.  Because
      certificate validation requires that root keys be distributed
      independently, the self-signed certificate that specifies the root
      certificate authority MAY be omitted from the chain, under the
      assumption that the remote end must already possess it in order to
      validate it in any case.

In my own testing I've only used one-cert chains, so I never encountered this.

I'm also told that TLS 1.3 abandoned the strict ordering of certificates other than the first one.

I will review this soon!

In the meantime please sign the CLA @KarolBedkowski . Thanks!

@mholt mholt added the under review 🧐 Review is pending before merging label May 4, 2020
Copy link
Member

@mholt mholt left a comment

Choose a reason for hiding this comment

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

I agree we need to change our current logic, but not quite this way.

The config field is "trustedLeafCerts" which is the list of leaf certificates to trust, not intermediates. The RFC specs for TLS 1.2 and 1.3 do require that the end-entity (leaf) certificates must be first.

So while we could extend our current trust logic to include any certs in the chain, for now I'd rather be conservative and simply limit our checking to the leaf certificate in the first position.

Basically, just change [len(rawCerts)-1] to [0].

@KarolBedkowski KarolBedkowski requested a review from mholt May 6, 2020 08:04
When client certificate is enabled Caddy check only last certificate from
request. When this cert is not in list of trusted leaf certificates,
connection is rejected. According to RFC TLS1.x the sender's certificate
must come first in the list.  Each following certificate must directly
certify the one preceding it.

This patch fix this problem - first certificate is checked instead of last.
@KarolBedkowski
Copy link
Contributor Author

Yes, according to specification checking first certificate should be enough.
Thanks.

Copy link
Member

@mholt mholt left a comment

Choose a reason for hiding this comment

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

Awesome, that's great -- thank you for the fix!

@mholt mholt merged commit b814c0a into caddyserver:master May 6, 2020
@mholt mholt removed the under review 🧐 Review is pending before merging label May 6, 2020
@mholt mholt changed the title tls/client auth: verify all certificates in client request tls/client auth: verify first certificate in client request May 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants