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

Caddy does not support non-SNI clients anymore #2438

Closed
xombiemp opened this issue Jan 22, 2019 · 9 comments · Fixed by #2452
Closed

Caddy does not support non-SNI clients anymore #2438

xombiemp opened this issue Jan 22, 2019 · 9 comments · Fixed by #2452
Labels
duplicate 🖇️ This issue or pull request already exists

Comments

@xombiemp
Copy link

1. What version of Caddy are you using (caddy -version)?

Caddy (untracked dev build) (unofficial)

commit a7aeb97

2. What are you trying to do?

Request a https url using a non-SNI client. There is only one site defined in my Caddyfile, so Caddy should not be confused about which cert to serve.

This was caused by #2339. Caddy serves the TLS cert to non-SNI clients up to and including the commit right before this TLS change, 22dfb14

If there is only one TLS site defined in the Caddyfile, Caddy should serve that cert to non-SNI clients.
Additionally, maybe there could be an option to set a default cert when multiple TLS sites are defined in the Caddyfile.

3. What is your entire Caddyfile?

sub.mydomain.com

4. How did you run Caddy (give the full command and describe the execution environment)?

caddy

5. Please paste any relevant HTTP request(s) here.

The command I used to test is

echo | openssl s_client -connect sub.mydomain.com:443 | awk '/Certificate chain/,/---/'

which simulates a non-SNI client by not using the -servername argument.

6. What did you expect to see?

I would expect to see the certificate chain served from Caddy:

Certificate chain
 0 s:/CN=sub.mydomain.com
   i:/C=US/O=Let's Encrypt/CN=Let's Encrypt Authority X3
DONE
 1 s:/C=US/O=Let's Encrypt/CN=Let's Encrypt Authority X3
   i:/O=Digital Signature Trust Co./CN=DST Root CA X3
---

7. What did you see instead (give full error messages and/or log)?

I see this handshake failure:

4373775980:error:140040E5:SSL routines:CONNECT_CR_SRVR_HELLO:ssl handshake failure:/BuildRoot/Library/Caches/com.apple.xbs/Sources/libressl/libressl-22.230.1/libressl-2.6/ssl/ssl_pkt.c:585:

8. How can someone who is starting from scratch reproduce the bug as minimally as possible?

Create a Caddyfile with a single domain name.
Start Caddy and make sure it issued a TLS cert from Letsencrypt.
Use this command to test a non-SNI client:

echo | openssl s_client -connect sub.mydomain.com:443 | awk '/Certificate chain/,/---/'
@mholt
Copy link
Member

mholt commented Jan 23, 2019

Duplicate of #2414.

Perhaps some of the antagonists from #1303 would like to submit a PR that satisfies everyone, and if not, their complaints won't be justified, because their demands broke a lot of people unnecessarily. @ghoeffner @orenyomtov @Zenexer

If there is only one TLS site defined in the Caddyfile, Caddy should serve that cert to non-SNI clients.

This is where you're going to rub a certain crowd the wrong way because of "information leakage."

@mholt mholt closed this as completed Jan 23, 2019
@mholt mholt added the duplicate 🖇️ This issue or pull request already exists label Jan 23, 2019
@xombiemp
Copy link
Author

@mholt I don’t really see this as a duplicate. The other issue is about Caddy not serving a self signed cert that doesn’t contain the domain name as a Common Name or SANS.

This is is about Caddy not serving a signed certificate that matches the domain name to a non-SNI client when there is only one TLS site defined in the Caddyfile.

Besides, the issue you said mine is a duplicate of is closed too.

@mholt
Copy link
Member

mholt commented Jan 23, 2019

It's the same issue: Caddy won't serve a certificate unless the SNI value (or the listening IP address) matches a name (or IP) on the certificate. Who signed it doesn't matter.

The linked issue is closed because until better solutions are proposed, that's expected -- and by some, demanded -- behavior.

@magikstm
Copy link

I've been following issues related to HTTPS the last couple of weeks following #1303 and related PRs.

Would it be possible to revert these changes? Could this option possibly be considered?

It was mentioned as an:

Information disclosure vulnerability, as it allows enumeration of all SSL/TLS certificates. That exposes all websites on the host that have SSL enabled.

What was the level of risk that users or websites had before the proposed fix to this issue? What was the severity score of the CVE it fixes?

@xombiemp
Copy link
Author

What about a configuration for fallback to a default cert?
Then it is more “secure” by default, but the admin can configure a fallback cert that would be served if the standard requirements to serve a cert aren’t met (lack of SNI or non-matching SNI/listening IP).

@mholt
Copy link
Member

mholt commented Jan 23, 2019

Possible -- how should that be done?

@Zenexer
Copy link

Zenexer commented Jan 24, 2019

Would it be possible to revert these changes? Could this option possibly be considered?

Of course, but it's a vulnerability, like it or not. It's also entirely unnecessary behavior: there is zero benefit to serving a random certificate. Serving the first available certificate would work just as well and wouldn't be considered an information disclosure vulnerability, at least on Caddy's part. It's the randomization that's the problem.

Perhaps some of the antagonists from #1303 would like to submit a PR that satisfies everyone, and if not, their complaints won't be justified, because their demands broke a lot of people unnecessarily.

I'm doing my best, @mholt. I spent about 20 hours writing the initial PR with almost no knowledge of Go; it was mostly trial and error. I took a day off from work to do it. My understanding is that although the fix worked, you were already in the process of moving away from the code that I patched, which I wasn't aware of (and had no way of knowing--it would've been nice if you had told me before asking me to fix it).

Given the backlog of work I currently have, I can't commit to spending that much time on this anytime soon. I'm afraid you're on your own from here.

I reported the vulnerability and mostly ignored the matter when you chose not to fix it. When you complained after security experts started commenting on the matter, I did my best to provide a patch, despite your continuous retaliations toward myself and other security researchers. I tried very hard to ignore your poor choice of wording and aggressive attitude for the sake of the internet community, and I did my best to convince security researchers behind-the-scenes to lay low, but I'm still seeing aggressive outbursts like this.

Sorry, but you're on your own from here. You can revert the changes if you so desire, but it will continue to be treated as a significant information disclosure vulnerability by the security community.

@whitestrake
Copy link
Collaborator

whitestrake commented Jan 25, 2019

Both sides could do with less sniping at each other, but I wouldn't try to tell the developers that they shouldn't simply point to the off-topic latter half of #1303 every time a valid use case is broken because of that fix. The code is what it is, and it is that way because you demanded it. Resultingly, we now see users with legitimate use cases experiencing unique usability issues.

If you don't have the time to - or even if you just don't want to - contribute further, that's absolutely fine, and the Caddy project is not entitled to your time or work. But to leverage very vocal demands against an open source project (while repeatedly misconstruing the facts of the situation, and without providing any kind of implementation for months, despite the fact that nobody knew how to satisfy all of the various demands on Caddy's functionality), and then wash your hands of the matter afterwards is rude in the extreme.

Let's not presume that either side were without aggression or retaliation, or that either side is without responsibility for the current state. The final comment of #1303 sums up very well a number of misconceptions you still seem to hold (that the issue was ignored, that the developer refused to fix the problem, that the developer was retaliating against you without cause). I'd caution against accusing the developer of holding an "aggressive attitude" in light of the tone and manner in which you prosecuted your demands in that issue thread. His "aggressive outburst" in this thread is equally as accurate as you found it to be antagonistic.

Quibbling about it further is not useful to the Caddy project. Ideally this thread should remain focused on reaching a solution for this issue, rather than continuing off-topic. I appreciate that you're busy, so thank you for advising us as much.


The topic at hand is Caddy's ability to support non-SNI clients.

The direction of the conversation is whether this can be provided as an option for users to configure.

The question currently posed in this thread: how should this be done?

@mholt, are you asking what it should look like in the Caddyfile, or are you seeking proposals on how it should be implemented in code?

@xombiemp
Copy link
Author

Possible -- how should that be done?

What do you think of this?

tls {
  default_cert host
}

where host is the domain or IP that matches a Common Name or SAN on the certificate you want served by default when the standard conditions aren't met.

mholt added a commit that referenced this issue Feb 2, 2019
vendor: update certmagic, needed to support this

Hopefully fixes #2451, fixes #2438, and fixes #2414
mholt added a commit that referenced this issue Feb 5, 2019
#2452)

* caddytls: Fix empty SNI handling (new -default-sni flag)

vendor: update certmagic, needed to support this

Hopefully fixes #2451, fixes #2438, and fixes #2414

* caddytls: Don't overwrite certmagic Manager (fixes #2407)

Supersedes #2447

* vendor: Update certmagic to fix nil pointer deref and TLS-ALPN cleanup

* Improve -default-sni flag help text
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
duplicate 🖇️ This issue or pull request already exists
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants