From 68296911b7ee58a9853b1baadbba549fb89f3904 Mon Sep 17 00:00:00 2001 From: Simone Bordet Date: Thu, 6 May 2021 22:08:11 +0200 Subject: [PATCH] Fixes #6099 - Cipher preference may break SNI if certificates have different key types. Updated the logic in SslContextFactory.Server.sniSelect(...) to check if there is any certificate that matches, and if so return a null alias in the hope to be called again and pick the right alias for the SNI. Signed-off-by: Simone Bordet --- .../ssl/SniSslConnectionFactoryTest.java | 31 +++++++++-- .../test/resources/keystore_sni_key_types.p12 | Bin 0 -> 3348 bytes .../util/ssl/SniX509ExtendedKeyManager.java | 2 +- .../jetty/util/ssl/SslContextFactory.java | 50 ++++++++++++------ 4 files changed, 61 insertions(+), 22 deletions(-) create mode 100644 jetty-server/src/test/resources/keystore_sni_key_types.p12 diff --git a/jetty-server/src/test/java/org/eclipse/jetty/server/ssl/SniSslConnectionFactoryTest.java b/jetty-server/src/test/java/org/eclipse/jetty/server/ssl/SniSslConnectionFactoryTest.java index 5ca56f4a60c..8e11b802bc2 100644 --- a/jetty-server/src/test/java/org/eclipse/jetty/server/ssl/SniSslConnectionFactoryTest.java +++ b/jetty-server/src/test/java/org/eclipse/jetty/server/ssl/SniSslConnectionFactoryTest.java @@ -44,6 +44,7 @@ import javax.net.ssl.SSLSocketFactory; import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; +import org.eclipse.jetty.http.HttpStatus; import org.eclipse.jetty.http.HttpTester; import org.eclipse.jetty.http.HttpVersion; import org.eclipse.jetty.io.Connection; @@ -123,21 +124,23 @@ public class SniSslConnectionFactoryTest protected void start(String keystorePath) throws Exception { - start(ssl -> ssl.setKeyStorePath(keystorePath)); + start(ssl -> + { + ssl.setKeyStorePath(keystorePath); + ssl.setKeyManagerPassword("keypwd"); + }); } protected void start(Consumer sslConfig) throws Exception { SslContextFactory.Server sslContextFactory = new SslContextFactory.Server(); + sslContextFactory.setKeyStorePassword("storepwd"); sslConfig.accept(sslContextFactory); File keystoreFile = sslContextFactory.getKeyStoreResource().getFile(); if (!keystoreFile.exists()) throw new FileNotFoundException(keystoreFile.getAbsolutePath()); - sslContextFactory.setKeyStorePassword("OBF:1vny1zlo1x8e1vnw1vn61x8g1zlu1vn4"); - sslContextFactory.setKeyManagerPassword("OBF:1u2u1wml1z7s1z7a1wnl1u2g"); - ServerConnector https = _connector = new ServerConnector(_server, new SslConnectionFactory(sslContextFactory, HttpVersion.HTTP_1_1.asString()), new HttpConnectionFactory(_httpsConfiguration)); @@ -194,6 +197,7 @@ public class SniSslConnectionFactoryTest start(ssl -> { ssl.setKeyStorePath("src/test/resources/keystore_sni.p12"); + ssl.setKeyManagerPassword("OBF:1u2u1wml1z7s1z7a1wnl1u2g"); ssl.setSNISelector((keyType, issuers, session, sniHost, certificates) -> { // Make sure the *.domain.com comes before sub.domain.com @@ -264,6 +268,7 @@ public class SniSslConnectionFactoryTest start(ssl -> { ssl.setKeyStorePath("src/test/resources/keystore_sni.p12"); + ssl.setKeyManagerPassword("OBF:1u2u1wml1z7s1z7a1wnl1u2g"); // Do not allow unmatched SNI. ssl.setSniRequired(true); }); @@ -281,6 +286,7 @@ public class SniSslConnectionFactoryTest start(ssl -> { ssl.setKeyStorePath("src/test/resources/keystore_sni.p12"); + ssl.setKeyManagerPassword("OBF:1u2u1wml1z7s1z7a1wnl1u2g"); // Do not allow unmatched SNI. ssl.setSniRequired(false); _httpsConfiguration.getCustomizers().stream() @@ -307,6 +313,7 @@ public class SniSslConnectionFactoryTest start(ssl -> { ssl.setKeyStorePath("src/test/resources/keystore_sni.p12"); + ssl.setKeyManagerPassword("OBF:1u2u1wml1z7s1z7a1wnl1u2g"); // Do not allow unmatched SNI. ssl.setSniRequired(true); ssl.setSNISelector((keyType, issuers, session, sniHost, certificates) -> @@ -338,6 +345,7 @@ public class SniSslConnectionFactoryTest { // Keystore has only one certificate, but we want to enforce SNI. ssl.setKeyStorePath("src/test/resources/keystore"); + ssl.setKeyManagerPassword("OBF:1u2u1wml1z7s1z7a1wnl1u2g"); ssl.setSniRequired(true); }); @@ -519,6 +527,21 @@ public class SniSslConnectionFactoryTest assertEquals(0, history.size()); } + @Test + public void testSNIWithDifferentKeyTypes() throws Exception + { + // This KeyStore contains 2 certificates, one with keyAlg=EC, one with keyAlg=RSA. + start(ssl -> ssl.setKeyStorePath("src/test/resources/keystore_sni_key_types.p12")); + + // Make a request with SNI = rsa.domain.com, the RSA certificate should be chosen. + HttpTester.Response response1 = HttpTester.parseResponse(getResponse("rsa.domain.com", "rsa.domain.com")); + assertEquals(HttpStatus.OK_200, response1.getStatus()); + + // Make a request with SNI = ec.domain.com, the EC certificate should be chosen. + HttpTester.Response response2 = HttpTester.parseResponse(getResponse("ec.domain.com", "ec.domain.com")); + assertEquals(HttpStatus.OK_200, response2.getStatus()); + } + private String getResponse(String host, String cn) throws Exception { String response = getResponse(host, host, cn); diff --git a/jetty-server/src/test/resources/keystore_sni_key_types.p12 b/jetty-server/src/test/resources/keystore_sni_key_types.p12 new file mode 100644 index 0000000000000000000000000000000000000000..b000fbeff57371976aa04a8c0961db5420ece8cb GIT binary patch literal 3348 zcmaKtcQhLc`^SZd+FKDT#4Zvgh*301mG;_75v^6D)-`L!Y7w+*6|r}z8Ld5Pw>3L# ztx|i%j8@*hzj5!m=RN2B>pAE1J>Ta%-+#Uj49mzu37~>u8HPZ#QnBdR6Gi|HARo)n z0>m;9e&c!=79{o$33M5V1+o9e%&^EV5dHs|80Y|$`2jFLm?Vhq|IzQgU?9f|f$i38 zIT)jp2exu)sp}mj6-DnKK=gNa+w|hH#z=>PRb(d_J?8gD_k2@((*D|sLCl-@upO!nG+NqTh3hR{Dltdoe%~$|Ljocn+Fj-SeG?4!MEOa4nSI?;^ zN2OxHrH#}knrd2U`1VKIxx5IhZY6_qniXiHrv!altkD$9H+^ttN7DGSI9nzNpl&494{P0E z(`C83RNM}%?jUmA{ZOTGdaV~ZXsN1@+Iec<{tWWy%t4Nt8q{VM(P?JrbX;$?QtGsE zvB-8DNZT9Wt=%wxYWj)xZW#Ri+Q^B;0e`jIrOyn8AFV?urPQw%2t>!GFgj*qCri`& zz2FMZ6+Ss4^ShOJrHO5F=_xOx>8jK&)iu&>=oU`7PSW%xtVjl^CU$JSuyAOVBv_(qd( z5yhSNT_YxDAP5T(v=S2}qSE+614@a#S++~^{y5~umoA6;B)+jh<6_5(^D04lC31iE zQfgQfU$wyUsG=S6%Qa!AR$(1Mey5>@W*zL{xO2D{Uib>DbZc*00^#kocn3e?{vp|l zJN^xxa=z}1Qr1JwGtXIx6p^HSf&pk;DH95fbeq4-918%N)v%n_zDAoUm;NNjZhnt= zQUvJsAt#ax)djzPf*e1Phh6%(a65+Bgq~0cwZE9HyK~8K3kR8bqOZ zq-4H@h7vVpwkau~JVhz?xdD*QtTBt~Z+`D85SWNc3zQq5Nm92&_3{p-kBJc+3FG{` zG&$gx1V9u%6uuPp|Fuh%l;H}B@ZT>X`Y$gHC?5;*`;ELQ0TjQ7>)#6CAKeQ2_m8+9 z@|kJ1E7Y4XTVfxg`)(!DZoue&x-|j|f`MX$?~d&9){$pL5zCs|VNv`qa(-m{s!||6 zqlA2tDyi70;C1N}bdE;b#v5Ar2UpP?7D$wbQH?lF z%C@K^ASUKc>_`qHa=ZVs+-hrS=S_s?Nx3Gx#ZV$*8xiQl`Lk!P+`tBE`jm~GyDY}K zKsmH)I^_8udZ2mJIg8-VL!FblEUv)`yzGJGy{2aa+O)v)iv@ zl8VjIIuTqe8(Q1OBihNc{F_N;OqS{6HZJf`tzh`wDD6R$jGI)W21a$SRkHIa`!Wi^ zTK$Sx)}T1`Gbn#cNeO2nYFl$nh!AXv*76uhp`KXhvu#Oot-aJ{FBk!9+R-{cQ;$dE z`^F=^O$Lp?ZquelyVNTDi9LKLL6=ty{bs<6rn@5xE?~)<9NCr|{RIAaALH9smGK@>5wt_OUt%_VItyZh9871LiF{U=%7EwNU;b4W5 z%+@k@H<#(uBSrfwJ#N~y;VHPiX8&?Fr3SZAeVQU71zlG{;B1(&W$nh3TD{|OQY-&5^90X`9b;@Y?)ilFX#anU^HyOV8Q44GJ zjJwHA1(lSf^oq9L)9j=vvd{mjg|Tf4{*|MFIIFYz3anpjLr{cJpTa>jw%%bTfaf%d zf>s|LBb3xsKl*kWIwBiLEvFDBsH(@;!pe6CgzFD@ARB%AXe4RA@7@PZq@Sm)eU`cS z7y-f$_g=<&%VzV$N&4=VME2^?w4igCWDYqm+Q0U%9fB_;c(%QBaa*C_A|b|<1Xm8n zKl8g@?dKWL_E+{x-#8Z4a82JWc(gZq$f(C-R|&fSyWtwY-l^J1c@!apxmC&2vf%4Q zl@%n0cJ^8mxvxCCYSa#070#{w(KkqZWeV?$lSGgoJ}XtItO1o4s3Axz*%dX2gO?DL zvQNpr1LN^$)r~}I52=rY$1j7esRIVN8%ne=_2PLYt;wHmWPGDuJY9R;v0Oc4o6AL_ zrdRfr)rI+;m{qsgXuHLDoMeHupOyqpqG7Q~cg`n!xU*yYv}oz3gPxS-xBXeNUUi7E zCSNCUmcN85^kTNfhM^pNcsyT&V^zr`yUZ7ru2wN0s4OgXm5gFZ%1Xs7qVKnWSdz;^ zLYdM)WRTR%2M?QL~NTZK6iaHgIU88{88a$dS1@p_CtZq z#~GUYMJ&;~LlOqQXSow8`iM2YMmlMrmBX};P@GU`&$$zkHFlwSrEB;0wyb^F((bLk zj0o8-T+Fq{VRLl7;k)}8&V85Ces&?1>{tYq(}-u+@2cM|9TZ(h&~IhR)Ti{hM^^uY z3O$x|<(p)FzT8@$amPrWc|4^Qg=>~V?@+wCe)|ZfHn%KK>1Selects a certificate based on SNI information.

*

This method may be invoked multiple times during the TLS handshake, with different parameters. * For example, the {@code keyType} could be different, and subsequently the collection of certificates - * (because they need to match the {@code keyType}.

+ * (because they need to match the {@code keyType}).

* * @param keyType the key algorithm type name * @param issuers the list of acceptable CA issuer subject names or null if it does not matter which issuers are used diff --git a/jetty-util/src/main/java/org/eclipse/jetty/util/ssl/SslContextFactory.java b/jetty-util/src/main/java/org/eclipse/jetty/util/ssl/SslContextFactory.java index 209d69d3988..0da182e7cbf 100644 --- a/jetty-util/src/main/java/org/eclipse/jetty/util/ssl/SslContextFactory.java +++ b/jetty-util/src/main/java/org/eclipse/jetty/util/ssl/SslContextFactory.java @@ -479,6 +479,11 @@ public class SslContextFactory extends AbstractLifeCycle implements Dumpable _certWilds.clear(); } + Map aliasCerts() + { + return _aliasX509; + } + @ManagedAttribute(value = "The selected TLS protocol versions", readonly = true) public String[] getSelectedProtocols() { @@ -2118,7 +2123,7 @@ public class SslContextFactory extends AbstractLifeCycle implements Dumpable _trustStoreResource); } - class Factory + private static class Factory { private final KeyStore _keyStore; private final KeyStore _trustStore; @@ -2133,7 +2138,7 @@ public class SslContextFactory extends AbstractLifeCycle implements Dumpable } } - class AliasSNIMatcher extends SNIMatcher + static class AliasSNIMatcher extends SNIMatcher { private String _host; @@ -2235,11 +2240,17 @@ public class SslContextFactory extends AbstractLifeCycle implements Dumpable } /** - * Does the default {@link #sniSelect(String, Principal[], SSLSession, String, Collection)} implementation - * require an SNI match? Note that if a non SNI handshake is accepted, requests may still be rejected - * at the HTTP level for incorrect SNI (see SecureRequestCustomizer). - * @return true if no SNI match is handled as no certificate match, false if no SNI match is handled by - * delegation to the non SNI matching methods. + *

Returns whether an SNI match is required when choosing the alias that + * identifies the certificate to send to the client.

+ *

The exact logic to choose an alias given the SNI is configurable via + * {@link #setSNISelector(SniX509ExtendedKeyManager.SniSelector)}.

+ *

The default implementation is {@link #sniSelect(String, Principal[], SSLSession, String, Collection)} + * and if SNI is not required it will delegate the TLS implementation to + * choose an alias (typically the first alias in the KeyStore).

+ *

Note that if a non SNI handshake is accepted, requests may still be rejected + * at the HTTP level for incorrect SNI (see SecureRequestCustomizer).

+ * + * @return whether an SNI match is required when choosing the alias that identifies the certificate */ @ManagedAttribute("Whether the TLS handshake is rejected if there is no SNI host match") public boolean isSniRequired() @@ -2248,13 +2259,12 @@ public class SslContextFactory extends AbstractLifeCycle implements Dumpable } /** - * Set if the default {@link #sniSelect(String, Principal[], SSLSession, String, Collection)} implementation - * require an SNI match? Note that if a non SNI handshake is accepted, requests may still be rejected - * at the HTTP level for incorrect SNI (see SecureRequestCustomizer). - * This setting may have no effect if {@link #sniSelect(String, Principal[], SSLSession, String, Collection)} is - * overridden or a non null function is passed to {@link #setSNISelector(SniX509ExtendedKeyManager.SniSelector)}. - * @param sniRequired true if no SNI match is handled as no certificate match, false if no SNI match is handled by - * delegation to the non SNI matching methods. + *

Sets whether an SNI match is required when choosing the alias that + * identifies the certificate to send to the client.

+ *

This setting may have no effect if {@link #sniSelect(String, Principal[], SSLSession, String, Collection)} is + * overridden or a custom function is passed to {@link #setSNISelector(SniX509ExtendedKeyManager.SniSelector)}.

+ * + * @param sniRequired whether an SNI match is required when choosing the alias that identifies the certificate */ public void setSniRequired(boolean sniRequired) { @@ -2297,7 +2307,7 @@ public class SslContextFactory extends AbstractLifeCycle implements Dumpable if (sniHost == null) { // No SNI, so reject or delegate. - return _sniRequired ? null : SniX509ExtendedKeyManager.SniSelector.DELEGATE; + return isSniRequired() ? null : SniX509ExtendedKeyManager.SniSelector.DELEGATE; } else { @@ -2306,9 +2316,15 @@ public class SslContextFactory extends AbstractLifeCycle implements Dumpable .filter(x509 -> x509.matches(sniHost)) .collect(Collectors.toList()); - // No match, let the JDK decide unless unmatched SNIs are rejected. if (matching.isEmpty()) - return isSniRequired() ? null : SniX509ExtendedKeyManager.SniSelector.DELEGATE; + { + // There is no match for this SNI among the certificates valid for + // this keyType; check if there is any certificate that matches this + // SNI, as we will likely be called again with a different keyType. + boolean anyMatching = aliasCerts().values().stream() + .anyMatch(x509 -> x509.matches(sniHost)); + return isSniRequired() || anyMatching ? null : SniX509ExtendedKeyManager.SniSelector.DELEGATE; + } String alias = matching.get(0).getAlias(); if (matching.size() == 1)