Unexpected TLS alert for IP-only certificate

[jung-kurt]

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

Caddy (untracked dev build) (unofficial)

commit f6e5089

2. What are you trying to do?

Use TLS with IP addresses (not DNS) for local development. Everything has worked fine up to and including commit 22dfb14 right before the TLS alert commit. I am not sure if this is a problem with (1) Caddy correctly handling invalid certificates or (2) Caddy incorrectly handling valid certificates. Nothing jumped out at me when examining the commit diffs so I will not be surprised if the fault is with the certificates. Any insights are appreciated.

3. What is your entire Caddyfile?

https://192.168.1.2 {
        tls cert/pki/issued/192.168.1.2.bundle.crt cert/pki/private/192.168.1.2.key
}

More details on the certificates can be found below.

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

caddy -conf ./Caddyfile

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

curl --cacert cert/pki/ca.crt https://192.168.1.2

6. What did you expect to see?

<h1>Hello from Caddy</h1>

This is what I get with versions of Caddy before the TLS alert commit.

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

curl: (35) error:14094438:SSL routines:ssl3_read_bytes:tlsv1 alert internal error

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

Here are the steps in the form of a shell script. Sorry for the involved sequence, but nothing is easy when it comes to TLS certificates.

# --- Obtain, install and initialize easyrsa ---

wget https://github.com/OpenVPN/easy-rsa/releases/download/v3.0.5/EasyRSA-nix-3.0.5.tgz
mkdir -p ~/tmp/caddy
tar -xzf EasyRSA-nix-3.0.5.tgz -C ~/tmp/caddy
mv ~/tmp/caddy/EasyRSA-3.0.5 ~/tmp/caddy/cert
cd ~/tmp/caddy/cert
cat > vars  <<end-of-vars
set_var EASYRSA_PKI		"$PWD/pki"
set_var EASYRSA_DN	"cn_only"
set_var EASYRSA_KEY_SIZE	2048
set_var EASYRSA_ALGO		rsa
set_var EASYRSA_CA_EXPIRE	3650
set_var EASYRSA_CERT_EXPIRE	3650
set_var EASYRSA_CRL_DAYS	180
set_var EASYRSA_NS_SUPPORT	"no"
set_var EASYRSA_NS_COMMENT	"Easy-RSA Generated Certificate"
set_var EASYRSA_TEMP_FILE	"$EASYRSA_PKI/extensions.temp"
end-of-vars
./easyrsa init-pki

# --- Generate certificate authority, specify CA name interactively ---

./easyrsa build-ca nopass

# --- Generate self-signed server certificate with IP SAN ---

./easyrsa --subject-alt-name="IP:192.168.1.2" build-server-full 192.168.1.2 nopass
cat pki/issued/192.168.1.2.crt pki/ca.crt > pki/issued/192.168.1.2.bundle.crt

#--- Fire up caddy with minimal configuration

cd ..
cat > index.html <<end-of-index
<h1>Hello from Caddy</h1>
end-of-index
cat > Caddyfile <<end-of-cfg
https://192.168.1.2 {
	tls cert/pki/issued/192.168.1.2.bundle.crt cert/pki/private/192.168.1.2.key
}
end-of-cfg
caddy -conf ./Caddyfile &

# --- Test with curl

curl --cacert cert/pki/ca.crt https://192.168.1.2
# With caddy before TLS alert commit: <h1>Hello from Caddy</h1>
# With caddy after commit: curl: (35) error:14094438:SSL routines:ssl3_read_bytes:tlsv1 alert internal error

[mholt]

Sigh. I knew this would happen. It's a consequence of #2339.

I would happily accept a pull request from @Zenexer / @ghoeffner / @orenyomtov / @mxlje to fix this. But I'm on holiday for a while.

[Zenexer]

@mholt I'm totally BSing my way through this because I have almost no experience with Go, but from poking around with breakpoints in GoLand/IntelliJ, I think I found the issue. It looks like you're choosing a config based exclusively on the server name passed through SNI in the TLS handshake. Doesn't that mean raw IP addresses never really worked? It would work if there was only one config, but otherwise they'd just get a random certificate, no?

Specifically, here's what I'm seeing: in handshake.go -> GetConfigForClient, clientHello.ServerName is an empty string when a raw IP address is requested with curl (or Chrome, for that matter).

There's no easy fix for this; it would require multiple significant changes:

1. Caddy would have to bind to specific IP addresses based on configs. In this case, it would need to bind explicitly to 192.168.1.2:443 rather than 0.0.0.0:443.
2. When possible, SNI should be used, and if there's an SNI mismatch, a TLS alert should be raised--no change there. This differs from Nginx (which will just use the first certificate declared for an endpoint every time), but I think a TLS alert is a better choice. This assumes that Caddy is looking at all the SANs in each cert rather than relying on the hostnames declared in configs, but I get the impression it already does that.
3. When SNI can't be used, if there's a single certificate for an endpoint, as would be the case here, that certificate should be used.
4. When SNI can't be used and there are multiple certificates for an endpoint, and one of those certificates contains the raw IP address, that certificate should be used.
5. When SNI can't be used, there are multiple certificates for an endpoint, and none or multiple of those certificates contain(s) the raw IP address, a TLS alert should be raised.

This won't really be intuitive to end users--we can guess what they're trying to do, but it's likely that people will end up with conflicting certificates or endpoints. Perhaps it would make more sense to just say Caddy requires SNI, which means raw IP addresses can't be used--they never worked reliably anyway, since a Caddyfile with multiple configs would inevitably have served the wrong certificate a good portion of the time. The only situation in which it would've reliably worked in old versions is when there was a single config in the Caddyfile.

Ultimately, this seems like a shortcoming in SNI; I don't really see why IP addresses wouldn't be passed. With ESNI on the horizon, perhaps now is a good time to campaign for this behavior to be changed.

@jung-kurt, from what I've gathered, you're basically just getting lucky with it working in old versions. Caddy doesn't actually know which certificate to use, so it picks a random certificate--but you only have one certificate in your entire Caddyfile, so it always chooses the right one. This random behavior was removed in #2339 because several security researchers (including myself) pushed for such a change on the grounds that it could lead to information disclosure, though the severity and classification of this issue was disputed.

Disclaimer: Again, I don't really know what I'm doing here, and I've made numerous assumptions, any number of which could be inaccurate.

[jung-kurt]

Excellent diagnostics and summary, @Zenexer. Your conclusion and recommendation make complete sense to me and explain why I needed to jump through some hoops with my working configuration. In practice, my Caddyfile has multiple interfaces. Through trial-and-error I found that I needed to set up network interface aliases (for example, eth0:0) for the addresses I needed to reach by IP. In addition, I used the bind directive in the Caddyfile block to restrict the listener to a particular interface.

I think your proposal makes very good sense for the general case. My particular use case, in which the traffic of interest comes to a bound interface, could be met with a TLS subdirective that means "use this certificate, don't issue a TLS alert, don't ask questions". However, I'm the first to acknowledge that that would be sweeping the real problem under the rug.

[mholt]

Doesn't that mean raw IP addresses never really worked?

They worked before that change, because we allowed serving certificates without SNI.

There's no easy fix for this

I agree, at least for a fix that would satisfy everyone.

Ultimately, this seems like a shortcoming in SNI; I don't really see why IP addresses wouldn't be passed.

No, this is definitely working as intended. SNI must not have IP addresses in it.

@jung-kurt, from what I've gathered, you're basically just getting lucky with it working in old versions.

Again, it "working" in old versions was intended, but not to cough everyone's satisfaction.

pushed for such a change on the grounds that it could lead to information disclosure, though the severity and classification of this issue was disputed.

Yes, rather than being productive and contributing a proper fix; but the squeaky wheel gets the grease.

So, @jung-kurt -- I'd recommend simply using the bind directive so that that site is the only one listening on the interface given by that IP address. Since it sounds like you've got this working, the issue is closed.

[jung-kurt]

So, @jung-kurt -- I'd recommend simply using the bind directive so that that site is the only one listening on the interface given by that IP address. Since it sounds like you've got this working, the issue is closed.

Unfortunately, only for versions of Caddy before the TLS alert commit (f6e5089).

[Zenexer]

@jung-kurt, even with bind?

@mholt, I really don’t think it worked previously if a Caddyfile had multiple configs. How would it know to serve the cert that matched the IP address? It doesn’t seem to have code to support that. @jung-kurt, do you think you could test this? If it works reliably even with multiple different certs in the same Caddyfile, I’m probably missing something important. For example, have a cert for 127.0.0.1 and a separate cert for 127.0.0.2. When you visit https://127.0.0.1/, sometimes you should get the cert for 127.0.0.2. It sounds to me like you’ve already tested this scenario and confirmed my suspicion that it never quite worked; however, @mholt seems to think otherwise.

[Zenexer]

I think an intermediary solution would be to serve an IP cert when SNI isn’t used if there’s only one such cert for an endpoint. This would at least allow Caddyfiles that worked reliably in previous versions to continue working reliably. Caddyfiles that would’ve served a random cert will start working reliably if there is only one IP cert; if there are multiple, they will remain unreliable. This eliminates any regressions related to raw IP addresses, although it’s not as intuitive as it could ultimately be.

This would not restore compatibility with browsers that don’t support SNI, though those only ever worked if there was a single cert in the entire Caddyfile anyway. To eliminate the regression here, we could simply say that if there’s only a single known cert, always use that. This would still be less than ideal, since it requires that crazy interface/bind hack if you want multiple cents, but that’s really no different from any other web server. It would be possible to do a better job of guessing in some cases, but no matter what we do, it will be possible to end up in an indeterminate situation. There’s no way around that; pre-fix, it would’ve just resulted in random certs, which doesn’t make for a usable website.

[jung-kurt]

@Zenexer, yes, with the current version of Caddy I get a TLS alert even when binding to an IP address. This makes sense based on your explanation -- no SNI match, no certificate.

I think an intermediary solution would be to serve an IP cert when SNI isn’t used if there’s only one such cert for an endpoint.

I agree.

[Zenexer]

Great, I’ll see if I can cook up an intermediary PR. I’m not experienced with Go, though, so someone else will need to very thoroughly review it.

[jung-kurt]

Many thanks, @Zenexer

[francislavoie]

Thanks for getting involved on this @Zenexer, it's appreciated!

[mholt]

@Zenexer

I think an intermediary solution would be to serve an IP cert when SNI isn’t used if there’s only one such cert for an endpoint.

That's what I'm thinking as well. Would be happy to review a PR for this.

[Zenexer]

@jung-kurt, if you're comfortable compiling Caddy yourself, give #2356 a try; I'm pretty sure it will resolve the problem. It should also eliminate the need to do that crazy bind/interface dance. The only case in which it won't work is if the IP address on your certificate doesn't match the IP address that Caddy sees as its local endpoint. I attempted to fix that, as well, but I had to abandon the effort because it was just too complicated for someone who's unfamiliar with the codebase; I couldn't be sure my changes wouldn't have unintended side effects that could impact security.

[jung-kurt]

Thanks, @Zenexer, your changes work perfectly for both my simple example shown above and for my more involved development setup. Great work!

[xombiemp]

I noticed that Caddy does not support non-SNI clients anymore, even if there is only one certificate in the Caddyfile.
@Zenexer, sounds like you have an idea of how to restore this functionality

This would not restore compatibility with browsers that don’t support SNI, though those only ever worked if there was a single cert in the entire Caddyfile anyway. To eliminate the regression here, we could simply say that if there’s only a single known cert, always use that.

I tired your pull request #2367 but it doesn't seem to address this part of the issue. @Zenexer, I was wondering if you could add this functionality of "if there's only a single known cert, always use that" in your pull request?

[Zenexer]

@xombiemp I have to rework the existing PR for the portion of Caddy that split off before I make any more changes. I don't normally work with Go; since this code has a significant security impact, I have to very carefully check various docs and specs for each change I make, so it's a slow process.

Can you double-check that you were using the code from the PR? I'm pretty sure I covered your scenario, but I haven't looked at the code since before the holidays, so I could be mistaken. (I may be conflating it with the issue of certs that contain a raw IP address.)

[mholt]

I appreciate your attention to detail on this one, @Zenexer. Sorry the code base got moved in the process -- but fortunately it was mostly a move, the basic structure of the methods and types is still very similar over in CertMagic.

[xombiemp]

@Zenexer First I checked out and built 22dfb14 which is the commit before the change. My Caddyfile simply has my domain name in it
sub.mydomain.com
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.
The result using this commit was successful and I saw this output:

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
---

I then checked out and built your pull request #2367
When I ran the test command this time I got 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:

I also built the latest master just to be sure and I got the same error when testing.

Unfortunately it looks like your pull request did not fix this issue. I appreciate you looking into this and would be grateful if you could address this issue as you are reworking the PR.

[mholt]

@xombiemp Your certificate is not an IP-only certificate; and @Zenexer's fix is specific to IP addresses. For now, your clients will have to use SNI to request that name.

@Zenexer Since we couldn't wait on this, I've pushed a commit which effectively implements your PR. I also updated CertMagic. I thought about the problem and solution independently from your PR and arrived at a similar change as yours, so that's a good sign. However, I didn't take the time to write new tests for it, I would appreciate being able to merge a PR with your test case(s).

Please please help test to verify a correct solution here. 🙏 I will do a new release any day now.

[xombiemp]

OK, I opened a new issue #2438