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

set a GetCertificate callback in the tls.Config #2404

Merged
merged 1 commit into from Jan 14, 2019

Conversation

Projects
None yet
3 participants
@marten-seemann
Copy link
Contributor

marten-seemann commented Dec 21, 2018

1. What does this change do, exactly?

It sets a fake tls.Config.GetCertificate callback in the tls.Config. According to the Go standard library, a tls.Config must have Certificates or GetCertificate set, otherwise it will be rejected by tls.Listen. quic-go recently implemented the same check, and rejects the tls.Config as currently constructed by Caddy.
I think it should be permissible to have a tls.Config that only sets the GetConfigForClient callback, and I opened golang/go#29139, but it looks like this was postponed to Go 1.13.

2. Please link to the relevant issues.

#2346, especially #2346 (comment)

3. Which documentation changes (if any) need to be made because of this PR?

4. Checklist

  • I have written tests and verified that they fail without my change
  • I have squashed any insignificant commits
  • This change has comments for package types, values, functions, and non-obvious lines of code
  • I am willing to help maintain this change if there are issues with it later
@mholt
Copy link
Owner

mholt left a comment

Thanks for the change! I also agree with you that a GetConfigForClient should be sufficient. (That is the point of the field, after all.)

quic-go recently implemented the same check

I do wonder, why did you implement the same check, if 1) the standard library already does it, and 2) you disagree with it?

// A tls.Config must have Certificates or GetCertificate
// set, in order to be accepted by tls.Listen and quic.Listen.
GetCertificate: func(*tls.ClientHelloInfo) (*tls.Certificate, error) {
return nil, errors.New("all certificates configured via GetConfigForClient")

This comment has been minimized.

@mholt

mholt Dec 21, 2018

Owner

Just a little nit: we prefer fmt.Errorf in this project (as opposed to importing a whole nother package)

Show resolved Hide resolved caddytls/config.go
@francislavoie

This comment has been minimized.

Copy link
Collaborator

francislavoie commented Jan 13, 2019

@marten-seemann just wondering, what's the status on this? Could you address @mholt's feedback?

set a GetCertificate callback in the tls.Config
A tls.Config must have Certificates or GetCertificate set, in order to
be accepted by tls.Listen and quic.Listen.

@marten-seemann marten-seemann force-pushed the marten-seemann:fake-get-certificate branch from 9813920 to a659643 Jan 14, 2019

@marten-seemann

This comment has been minimized.

Copy link
Contributor Author

marten-seemann commented Jan 14, 2019

quic-go recently implemented the same check

I do wonder, why did you implement the same check, if 1) the standard library already does it, and 2) you disagree with it?

In the development version of QUIC (which has preliminary IETF QUIC support), we're using tls-tris, which is based on standard library code. The change I'm proposing requires changes to way the standard library retrieves the applicable tls.Config. While technically not necessary, since the gQUIC branch of quic-go we're using in Caddy doesn't use tris, I prefer to be consistent.

I updated the PR according to your review.

@mholt

mholt approved these changes Jan 14, 2019

@mholt

This comment has been minimized.

Copy link
Owner

mholt commented Jan 14, 2019

Great, thanks for the explanation! Merging now. Thanks again for the patch.

@mholt mholt merged commit e14328b into mholt:master Jan 14, 2019

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment