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

Conscrypt does not support server-side SNI #11413

Closed
cowwoc opened this issue Feb 15, 2024 · 15 comments · Fixed by #12549 or #12632
Closed

Conscrypt does not support server-side SNI #11413

cowwoc opened this issue Feb 15, 2024 · 15 comments · Fixed by #12549 or #12632
Assignees
Labels
Bug For general bugs on Jetty side

Comments

@cowwoc
Copy link
Contributor

cowwoc commented Feb 15, 2024

Jetty version(s)
12.0.7

Jetty Environment
core

Java version/vendor (use: java -version)
openjdk version "21.0.1" 2023-10-17 LTS
OpenJDK Runtime Environment Zulu21.30+15-CA (build 21.0.1+12-LTS)
OpenJDK 64-Bit Server VM Zulu21.30+15-CA (build 21.0.1+12-LTS, mixed mode, sharing)

OS type/version
Microsoft Windows [Version 10.0.19045.4046]

Description
https://eclipse.dev/jetty/documentation/jetty-12/operations-guide/index.html#og-protocols-ssl-sni states that when sniRequired=false the SNI checks should always pass.

If you take a look at the attached DEBUG log, you'll notice that sniRequired=false (the default setting. I didn't alter it) but the server still responds with org.eclipse.jetty.http.BadMessageException: 400: Invalid SNI.

debug.log

I suspect I know what is going wrong in my particular case: the browser is hitting the server by IP address, and that IP address happens to be my own IP address. I'm not sure if that ends up getting rerouted to localhost but it could be the case.

Per https://stackoverflow.com/a/69945374/14731:

Know that SNI itself has restrictions:

  1. localhost is not allowed
  2. IP Address Literals are not allowed (eg: 192.168.1.215 or fe80::3831:400c:dc07:7c40)

If the failure I am seeing is expected behavior (working as designed) then I'd suggest making the following changes:

  1. Update the documentation to mention these SNI restrictions.
  2. Add more information to the exception message, such as Invalid SNI. localhost is not allowed or Invalid SNI IP address literals are not allowed, etc.

Out of curiosity, is there any way to bypass these restrictions? Or do I have to hit the server by name instead of by IP?

@cowwoc cowwoc added the Bug For general bugs on Jetty side label Feb 15, 2024
@cowwoc
Copy link
Contributor Author

cowwoc commented Feb 15, 2024

More clues...

  • The server is configured with an SSL certificate for a domain called licensed.app.
  • Instead of hitting the server through that domain, I tried hitting it through a DDNS provider... so I was hitting the server by hostname instead of by IP address, I still got the same Invalid SNI exception.

I eventually redirected pointing licensed.app to my local PC and hit the server through it. This time, the request was accepted without an error.

So it seems that the only way to hit the server is through the hostname that matches the SSL certificate.

@cowwoc
Copy link
Contributor Author

cowwoc commented Feb 16, 2024

Okay, I feel like an idiot :) My code contains new SecureRequestCustomizer(). While it's true that sniRequired=false, this constructor sets sniHostCheck=true which is why I was getting this problem. Setting this property to false allowed me to hit the server by IP address and the DDNS provider.

I still suggest improving ease of troubleshooting for this case. The DEBUG logs don't seem to contain any hint that this is what went wrong. sniHostCheck isn't mentioned at all.

@joakime
Copy link
Contributor

joakime commented Feb 16, 2024

2. Add more information to the exception message, such as Invalid SNI. localhost is not allowed or Invalid SNI IP address literals are not allowed, etc.

This is the code that does this check ...

protected void checkSni(Request request, SSLSession session)
{
if (isSniRequired() || isSniHostCheck())
{
String sniHost = (String)session.getValue(SslContextFactory.Server.SNI_HOST);
X509 x509 = getX509(session);
if (x509 == null)
throw new BadMessageException(400, "Invalid SNI");
String serverName = Request.getServerName(request);
if (LOG.isDebugEnabled())
LOG.debug("Host={}, SNI={}, SNI Certificate={}", serverName, sniHost, x509);
if (isSniRequired() && (sniHost == null || !x509.matches(sniHost)))
throw new BadMessageException(400, "Invalid SNI");
if (isSniHostCheck() && !x509.matches(serverName))
throw new BadMessageException(400, "Invalid SNI");
}
}

When you run your DEBUG logs with your failing scenario do you get the DEBUG log output seen on line 223?
If so, what does it say?

Having the BadMessageException say something different and specific can be viewed as a security issue.
Instead, having meaningful DEBUG logging is a more likely solution.

@cowwoc
Copy link
Contributor Author

cowwoc commented Feb 16, 2024

@joakime

Having the BadMessageException say something different and specific can be viewed as a security issue.
Instead, having meaningful DEBUG logging is a more likely solution.

Got it. Here is where I'm at now:

  1. I updated the code to require both sniRequired=true and sniHostCheck=true.
  2. I generated and installed an SSL certificate for licensed.app
  3. I configured the DNS to redirect requests from that domain to my home computer
  4. When I hit licensed.app with my browser I get the following DEBUG output:

DEBUG o.e.j.server.SecureRequestCustomizer.checkSni() - Host=licensed.app, SNI=null, SNI Certificate=X509@32673d16(null,h=[licensed.app, www.licensed.app],a=[],w=[])

My understanding is that this means that the client is not sending a SNI hostname as part of the request, but that doesn't make sense since all browsers are supposed to support this by now. Here are some clues that might help:

  • The domain is hosted behind Cloudflare with proxy=true on their end.
  • I am using an abnormal port. I am hitting https://licensed.app:8443/ though I doubt that is a problem.
  • My browser is Brave [Version 1.62.165 Chromium: 121.0.6167.184 (Official Build) (64-bit)](https://brave.com/latest/) which should be equivalent to the latest stable version of Chrome.

The documentation mentions the SNI=null scenario, but it doesn't explain when that could happen. In other words, I don't know how to debug this further. Any ideas?

@sbordet
Copy link
Contributor

sbordet commented Feb 16, 2024

@cowwoc remember that DEBUG log statements are typically for Jetty developers to have a clearer understanding of what's going on.

In that particular log statement SNI=null means that there was no SNI in the SSLSession, from there it is supposed to be retrieved.

As a side note, remember that the SNI is sent per connection, not per request.

It really seems from your logs that the SNI is not sent.
Can you capture the network trace via tcpdump or wireshark?

Also, you are using Conscrypt. Does it work if you use the JDK TLS implementation?
It might be a Conscrypt bug, see for example: https://groups.google.com/g/conscrypt/c/vHXDvjssGZI (not sure how up-to-date that is though).

@cowwoc
Copy link
Contributor Author

cowwoc commented Feb 16, 2024

@sbordet Sorry for the delayed response. Yes, it seems to be a bug in Conscrypt. If I replace:

<dependency>
	<groupId>org.eclipse.jetty</groupId>
	<artifactId>jetty-alpn-conscrypt-server</artifactId>
	<version>${jetty.version}</version>
	<exclusions>
		<exclusion>
			<!-- Replaced by OS-specific artifact -->
			<groupId>org.conscrypt</groupId>
			<artifactId>conscrypt-openjdk-uber</artifactId>
		</exclusion>
	</exclusions>
</dependency>
<dependency>
	<groupId>org.eclipse.jetty</groupId>
	<artifactId>jetty-alpn-conscrypt-client</artifactId>
	<version>${jetty.version}</version>
	<exclusions>
		<exclusion>
			<!-- Replaced by OS-specific artifact -->
			<groupId>org.conscrypt</groupId>
			<artifactId>conscrypt-openjdk-uber</artifactId>
		</exclusion>
	</exclusions>
</dependency>
<dependency>
	<groupId>org.conscrypt</groupId>
	<artifactId>conscrypt-openjdk</artifactId>
	<version>${conscrypt.version}</version>
	<classifier>${os.detected.classifier}</classifier>
</dependency>

with:

<dependency>
	<groupId>org.eclipse.jetty</groupId>
	<artifactId>jetty-alpn-java-server</artifactId>
	<version>${jetty.version}</version>
</dependency>
<dependency>
	<groupId>org.eclipse.jetty</groupId>
	<artifactId>jetty-alpn-java-client</artifactId>
	<version>${jetty.version}</version>
</dependency>

and remove:

Security.addProvider(new OpenSSLProvider());
sslContextFactory.setProvider("Conscrypt");

from my code, then the SNI error goes away and the debug log show the correct SNI value.

Can you please confirm that I was using Conscrypt correctly? Is it sufficient to include the dependencies I mentioned and invoking the two lines to configure sslContextFactory to use it?

I tried removing the exclusions and using the uber-jar version of Conscrypt that Jetty links to, but I got the same "Invalid SNI" error.

@cowwoc
Copy link
Contributor Author

cowwoc commented Feb 16, 2024

I'm digging further into this and it smells of user error. SslContextFactory picks up the correct SNI but when SecureRequestCustomizer invokes String sniHost = (String)session.getValue(SslContextFactory.Server.SNI_HOST); it gets back null. This seems to imply that I am not doing something to cause SslContextFactory to pass the value to SecureRequestCustomizer.

Any ideas?

@sbordet
Copy link
Contributor

sbordet commented Feb 16, 2024

The SNI host value is put into the SSLSession by SniX509ExtendedKeyManager, which is automatically configured if you have a certificate with multiple SAN, which is your case.

Your logs shows that SniX509ExtendedKeyManager is in use, so all good on your part.

I really think that Conscrypt does not support SNI on the server, which I think it's confirmed by the fact that if you switch to OpenJDK all works as it should.

@cowwoc
Copy link
Contributor Author

cowwoc commented Feb 16, 2024

I think you're right, but then... what did google/conscrypt#712 add? I thought it was adding SNI support on the server.

Okay, so what are the next steps here?

Will you guys pick up the torch and follow up with the Conscrypt team? If so, google/conscrypt#644 might be a good place to start.

I guess another thing to do is add an automated test for this scenario.

In light of the current restriction, what is the recommended behavior for production? Use Conscript with SNI disabled or use the slower Java implementation? It's not clear how much of a performance difference this would make, nor how big a security risk it would be to disable SNI.

@cowwoc cowwoc changed the title Invalid SNI even when sniRequired=false Conscrypt does not support server-side SNI Feb 22, 2024
@cowwoc
Copy link
Contributor Author

cowwoc commented May 21, 2024

@sbordet friendly bump

@cowwoc
Copy link
Contributor Author

cowwoc commented Nov 12, 2024

I dug into this scenario again now that I have a better understanding of SNI and reverse-proxy configurations, and I'm seeing something weird...

  1. I had to explicitly set SslContextFactory.setSNiRequired(true) because this time around my certificate only contains a single alias. (Yes, the entry is called [licensed.app, fairuse.org] but it's only a single entry. I need to fix a bug in the way the alias name is generated.)
  2. I am now using a ECDSA key instead of RSA.
  3. SniX509ExtendedKeyManager.chooseServerAlias() is getting invoked with the correct SNI and it invokes session.putValue().
  4. However, when SecureRequestCustomizer.checkSni() gets invoked, session.getValue(SslContextFactory.Server.SNI_HOST) returns null because the session instance is different from the one passed into SniX509ExtendedKeyManager.
  5. I am running behind a nginx reverse proxy now. I used to use httpd in the past. In any case, I configured nginx to pass the SNI value through to the origin server as follows:
    proxy_ssl_server_name on;
    proxy_ssl_name $host;
  1. Per nginx's logs, it is only forwarding a single request to the origin server and its SNI value is set correctly.

Here are the server logs: server.log

This makes me wonder whether the bug lies in conscrypt, jetty itself or a configuration mistaken on my part. What should I try next?

Then again, maybe I'm beating a dead horse and #11413 (comment) proves that the problem lies in Conscrypt...

EDIT: I just updated the server logs. The previous ones suffered from an nginx configuration problem (the reverse proxy wasn't configured to handle http2). Also, I just found out that nginx for Windows does not support http3, so that's guaranteed not to work. Is that a problem in our case? Is Conscrypt only used for HTTP/3 support?

@sbordet
Copy link
Contributor

sbordet commented Nov 19, 2024

I think it's a Conscrypt bug.

The SSLSession is wrapped in every call (!) in ConscryptEngine.handshakeSession() -- stating in the comments that this is necessary for old Android versions.

The wrapping goes Java8ExtendedSSLSession(ExternalSession(Provider(ActiveSession))), where ActiveSession is supposedly the object that does not change (only the wrappers are new instances every time).

Unfortunately, calling Java8ExtendedSSLSession.putValue() delegates to ExternalSession.putValue(), which stores the value in a local Map. The putValue() call is not delegated all the way to the ActiveSession, so the value is lost in one of the ExternalSession wrappers.

What we can do is to try to retrieve the SNI via ExtendedSSLSession in SecureRequestCustomizer first, and then using SSLSession attributes.

Stand by for a PR, but I would need you to test it 😃

sbordet added a commit that referenced this issue Nov 19, 2024
Modified `SecureRequestCustomizer` to use `ExtendedSSLSession` APIs instead of relying on SSLSession attributes, that do not work in Conscrypt.

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
@sbordet sbordet linked a pull request Nov 19, 2024 that will close this issue
@sbordet sbordet moved this to 🏗 In progress in Jetty 12.0.16 FROZEN Nov 19, 2024
@sbordet sbordet self-assigned this Nov 19, 2024
@sbordet
Copy link
Contributor

sbordet commented Nov 19, 2024

@cowwoc can you please test #12549 and report back if it works for you?

@sbordet
Copy link
Contributor

sbordet commented Nov 20, 2024

@cowwoc friendly bump 😄

@cowwoc
Copy link
Contributor Author

cowwoc commented Nov 20, 2024

Sorry @sbordet. I saw your earlier message. Unfortunately, my project is in an akward state at the moment which prevents it from running. I'll try to get this sorted out and test your fix in the upcoming week.

sbordet added a commit that referenced this issue Nov 22, 2024
Modified `SecureRequestCustomizer` to use `ExtendedSSLSession` APIs instead of relying on SSLSession attributes, that do not work in Conscrypt.

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
@github-project-automation github-project-automation bot moved this from 🏗 In progress to ✅ Done in Jetty 12.0.16 FROZEN Nov 22, 2024
jbertram added a commit to jbertram/activemq-artemis that referenced this issue Dec 11, 2024
The change to o.a.a.c.t.WebServerComponentTest#testSimpleSecureServerWithSniRequiredEnabled
was due to jetty/jetty.project#11413. The test
needed to be updated to deal with the new behavior from Jetty.
sbordet added a commit that referenced this issue Dec 12, 2024
Addendum to #12549 to fix the silly mistake of returning null rather than the SNI host.

Thanks @brusdev for pointing that out.

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
@sbordet sbordet linked a pull request Dec 12, 2024 that will close this issue
jbertram added a commit to jbertram/activemq-artemis that referenced this issue Dec 12, 2024
The change to o.a.a.c.t.WebServerComponentTest#testSimpleSecureServerWithSniRequiredEnabled
was due to jetty/jetty.project#11413. The test
needed to be updated to deal with the new behavior from Jetty.
sbordet added a commit that referenced this issue Dec 12, 2024
Addendum to #12549 to fix the silly mistake of returning null rather than the SNI host.

Thanks @brusdev for pointing that out.

Signed-off-by: Simone Bordet <simone.bordet@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug For general bugs on Jetty side
Projects
No open projects
Status: ✅ Done
3 participants