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

X509Certificate array used for trust managers is not a chain #4973

Closed
wants to merge 1 commit into from

Conversation

@harbulot
Copy link
Contributor

harbulot commented Mar 13, 2016

In multiple parts of the code related to SSLContext, the X509Certificate[] array is called trustCertChain and is referred to as a "chain".

However, trust anchors are generally not a chain at all (unlike keyCertChain, which is a chain). There can indeed be multiple completely unrelated certificates in that collection of trusted certificates.

This simple PR comprises two commits, addressing two sub-problems on this topic:

  • The code in SSLContext was using the Subject DN (Subject Principal) of each certificate in this array as the alias to build the trust store. However, it is perfectly possible to have multiple distinct certificates with the same Subject DN in that array. Using the Subject DN effectively discards some of those certificates with overlapping names.
    • The fix here is to use the SHA-1 fingerprint of the certificate (nothing to do with the signature). This identifier is used by a number of other tools, most notably, by the Windows API.
    • If this was to become a performance problem, since the identifiers themselves don't matter, another way could be to use an incremented identifier.
  • The second commit is more cosmetic: it renames all occurrences of trustCertChain into trustCertCollection, and modifies the Javadoc accordingly. This is mostly to prevent confusion based on the name.
@normanmaurer

This comment has been minimized.

Copy link
Member

normanmaurer commented Mar 13, 2016

Could you add a unit test ?

Also please sign our icla and adjust the commit message to match our template:
http://netty.io/s /icla
http://netty.io/wiki/writing-a-commit-message.html

@normanmaurer normanmaurer self-assigned this Mar 13, 2016
@@ -165,7 +166,7 @@ public OpenSslClientContext(File certChainFile, TrustManagerFactory trustManager
* @deprecated use {@link SslContextBuilder}
*/
@Deprecated
public OpenSslClientContext(File trustCertChainFile, TrustManagerFactory trustManagerFactory,
public OpenSslClientContext(File trustCertCollectionFile, TrustManagerFactory trustManagerFactory,

This comment has been minimized.

Copy link
@netkins

netkins Mar 13, 2016

MAJOR Constructor has 11 parameters, which is greater than 7 authorized. rule

@@ -256,22 +258,22 @@ public JdkSslClientContext(File trustCertChainFile, TrustManagerFactory trustMan
}
}

JdkSslClientContext(X509Certificate[] trustCertChain, TrustManagerFactory trustManagerFactory,
JdkSslClientContext(X509Certificate[] trustCertCollection, TrustManagerFactory trustManagerFactory,

This comment has been minimized.

Copy link
@netkins

netkins Mar 13, 2016

MAJOR Constructor has 11 parameters, which is greater than 7 authorized. rule

X509Certificate[] keyCertChain, PrivateKey key, String keyPassword,
KeyManagerFactory keyManagerFactory, long sessionCacheSize,
long sessionTimeout) throws SSLException {
private static SSLContext newSSLContext(X509Certificate[] trustCertCollection,

This comment has been minimized.

Copy link
@netkins

netkins Mar 13, 2016

MAJOR Method has 8 parameters, which is greater than 7 authorized. rule

private static SSLContext newSSLContext(X509Certificate[] trustCertChain, TrustManagerFactory trustManagerFactory,
X509Certificate[] keyCertChain, PrivateKey key, String keyPassword,
KeyManagerFactory keyManagerFactory, long sessionCacheSize, long sessionTimeout)
private static SSLContext newSSLContext(X509Certificate[] trustCertCollection,

This comment has been minimized.

Copy link
@netkins

netkins Mar 13, 2016

MAJOR Method has 8 parameters, which is greater than 7 authorized. rule

@@ -221,27 +223,28 @@ public JdkSslServerContext(File trustCertChainFile, TrustManagerFactory trustMan
}
}

JdkSslServerContext(X509Certificate[] trustCertChain, TrustManagerFactory trustManagerFactory,
JdkSslServerContext(X509Certificate[] trustCertCollection, TrustManagerFactory trustManagerFactory,

This comment has been minimized.

Copy link
@netkins

netkins Mar 13, 2016

MAJOR Constructor has 12 parameters, which is greater than 7 authorized. rule

@harbulot

This comment has been minimized.

Copy link
Contributor Author

harbulot commented Mar 14, 2016

@normanmaurer

Sure, I can try to write unit tests (although there weren't any before anyway).

Could you tell me what's wrong with the commit messages I've submitted? I thought I'd followed the required format.

@normanmaurer

This comment has been minimized.

Copy link
Member

normanmaurer commented Mar 14, 2016

@harbulot actually the commit message is correct, I somehow missed this.

It is correct that there were no unit tests for this before and this is why it had a bug ;) So I would say please submit a unit-test with this as well so we can be sure it is fixed and stay fixed.

@Scottmitch Scottmitch added the defect label Mar 14, 2016
@Scottmitch

This comment has been minimized.

Copy link
Member

Scottmitch commented Mar 14, 2016

@harbulot - Thanks for the patch! Unit test would be much appreciated :)

@normanmaurer

This comment has been minimized.

Copy link
Member

normanmaurer commented Mar 15, 2016

@harbulot let me know once you added the test-case an I will review :) Thanks!

@harbulot

This comment has been minimized.

Copy link
Contributor Author

harbulot commented Mar 15, 2016

@normanmaurer Sure, I'll try to do it within the next few days, as I'll need to spend a bit more time on it to have good test cases (and create test certs).

@normanmaurer

This comment has been minimized.

Copy link
Member

normanmaurer commented Mar 15, 2016

@harbulot awesome!

@normanmaurer

This comment has been minimized.

Copy link
Member

normanmaurer commented Mar 22, 2016

@harbulot any news ?

@harbulot harbulot force-pushed the harbulot:ssl_trust_anchors branch 3 times, most recently from 823f41b to 5f85917 Mar 22, 2016
@harbulot

This comment has been minimized.

Copy link
Contributor Author

harbulot commented Mar 22, 2016

@normanmaurer Just updated (and rebased against the latest 4.1 branch):

  • I've actually changed the way to name each cert. Instead of computing the SHA-1 fingerprint for each certificate to use as alias name, I'm just using a counter (and converting it into a String). Since the names are never used to get access to the certificate later, there doesn't seem to be any point spending time calculating a digest.
  • I've added a test case, with two CAs with the same name and one CA with a different name. Each of these three CAs has a test server certificate.
TrustManagerFactory tmf = SslContext.buildTrustManagerFactory(
certCollection, null);

X509TrustManager x509Tm = null;

This comment has been minimized.

Copy link
@normanmaurer

normanmaurer Mar 22, 2016

Member

remove redundant null assignment.

This comment has been minimized.

Copy link
@harbulot

harbulot Mar 22, 2016

Author Contributor

This one is not redundant (it won't compile without), unless we return the value directly in the loop (instead of breaking) and return null after the loop. Would that be the preferred style?

This comment has been minimized.

Copy link
@normanmaurer

normanmaurer Mar 22, 2016

Member

just return directly. and throw an exception if the loop not return early.

X509Certificate[] certCollection = new X509Certificate[resourceNames.length];
for (int i = 0; i < resourceNames.length; i++) {
String resourceName = resourceNames[i];
InputStream is = null;

This comment has been minimized.

Copy link
@normanmaurer

normanmaurer Mar 22, 2016

Member

remove redundant null assignment.

This comment has been minimized.

Copy link
@harbulot

harbulot Mar 22, 2016

Author Contributor

This null assignment is not redundant with the try-finally pattern.

This comment has been minimized.

Copy link
@normanmaurer

normanmaurer Mar 22, 2016

Member

oh sorry missed this.

@normanmaurer

This comment has been minimized.

Copy link
Member

normanmaurer commented Mar 22, 2016

@harbulot after addressing my comment can you also please squash your commits into one ?

Also please sign our ICLA: http://netty.io/s/icla

@@ -239,13 +241,13 @@ public JdkSslClientContext(File trustCertChainFile, TrustManagerFactory trustMan
* @deprecated use {@link SslContextBuilder}
*/
@Deprecated
public JdkSslClientContext(File trustCertChainFile, TrustManagerFactory trustManagerFactory,
public JdkSslClientContext(File trustCertCollectionFile, TrustManagerFactory trustManagerFactory,

This comment has been minimized.

Copy link
@netkins

netkins Mar 22, 2016

MAJOR Constructor has 11 parameters, which is greater than 7 authorized. rule

@@ -204,13 +206,13 @@ public JdkSslServerContext(File trustCertChainFile, TrustManagerFactory trustMan
* @deprecated use {@link SslContextBuilder}
*/
@Deprecated
public JdkSslServerContext(File trustCertChainFile, TrustManagerFactory trustManagerFactory,
public JdkSslServerContext(File trustCertCollectionFile, TrustManagerFactory trustManagerFactory,

This comment has been minimized.

Copy link
@netkins

netkins Mar 22, 2016

MAJOR Constructor has 11 parameters, which is greater than 7 authorized. rule

toX509CertificatesInternal(keyCertChainFile), toPrivateKeyInternal(keyFile, keyPassword),
keyPassword, keyManagerFactory, ciphers, cipherFilter, apn, sessionCacheSize, sessionTimeout);
}

@SuppressWarnings("deprecation")
OpenSslClientContext(X509Certificate[] trustCertChain, TrustManagerFactory trustManagerFactory,
OpenSslClientContext(X509Certificate[] trustCertCollection, TrustManagerFactory trustManagerFactory,

This comment has been minimized.

Copy link
@netkins

netkins Mar 22, 2016

MAJOR Constructor has 11 parameters, which is greater than 7 authorized. rule
MAJOR The Cyclomatic Complexity of this method "OpenSslClientContext" is 28 which is greater than 25 authorized. rule

@harbulot harbulot force-pushed the harbulot:ssl_trust_anchors branch from 5f85917 to 9863fba Mar 22, 2016
renamed trustCertChain into trustCertCollection.

Motivation:

SSLContext.buildTrustManagerFactory(...) builds a KeyStore to
initialize the TrustManagerFactory from an array of X509Certificates,
assuming that array is a chain and that each certificate will have a
unique Subject Distinguised Name.
However, the collection of certificates used as trust anchors is generally
not a chain (it is an unordered collection), and it is legitimate for it
to contain multiple certificates with the same Subject DN.
The existing code uses the Subject DN as the alias name when filling in
the `KeyStore`, thereby overwriting other certificates with the same
Subject DN in this collection, so some certificates may be discarded.
In addition, the code related to building trust managers can take an array of
X509Certificate instances to use as trust anchors. The variable name is
usually trustCertChain, and the documentation refers to them as a "chain".
However, while it makes sense to talk about a "chain" from a keymanager
point of view, these certificates are just an unordered collection in a
trust manager. (There is no chaining requirement, having the Subject DN
matching its predecessor's Issuer DN.)
This can create confusion to for users not used with PKI concepts.

Modifications:

SSLContext.buildTrustManagerFactory(...) now uses a distinct alias for each
array (simply using a counter, since this name is never used for reference
later). This patch also includes a unit test with CA certificates using the
same Subject DN.
Also renamed trustCertChain into trustCertCollection, and changed the
references to "chain" in the Javadoc.

Result:

Each loaded certificate now has a unique identifier when loaded, so it is
now possible to use multiple certificates with the same Subject DN as
trust anchors.
Hopefully, renaming the parameter should also reduce confusion around PKI
concepts.
@harbulot harbulot force-pushed the harbulot:ssl_trust_anchors branch from 9863fba to 502644f Mar 22, 2016
@@ -239,13 +241,13 @@ public JdkSslClientContext(File trustCertChainFile, TrustManagerFactory trustMan
* @deprecated use {@link SslContextBuilder}
*/
@Deprecated
public JdkSslClientContext(File trustCertChainFile, TrustManagerFactory trustManagerFactory,
public JdkSslClientContext(File trustCertCollectionFile, TrustManagerFactory trustManagerFactory,

This comment has been minimized.

Copy link
@netkins

netkins Mar 22, 2016

MAJOR Constructor has 11 parameters, which is greater than 7 authorized. rule

@@ -204,13 +206,13 @@ public JdkSslServerContext(File trustCertChainFile, TrustManagerFactory trustMan
* @deprecated use {@link SslContextBuilder}
*/
@Deprecated
public JdkSslServerContext(File trustCertChainFile, TrustManagerFactory trustManagerFactory,
public JdkSslServerContext(File trustCertCollectionFile, TrustManagerFactory trustManagerFactory,

This comment has been minimized.

Copy link
@netkins

netkins Mar 22, 2016

MAJOR Constructor has 11 parameters, which is greater than 7 authorized. rule

@normanmaurer

This comment has been minimized.

Copy link
Member

normanmaurer commented Mar 22, 2016

Cherry-picked into 4.0 (f5fab38) and 4.1 (9ebb4b7).

@harbulot thanks a lot!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.