-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
security: Stabilize AdvancedTlsX509TrustManager. #11216
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR @erm-g! Generally LG, just a few minor comments.
@@ -135,8 +144,7 @@ public void updateTrustCredentials(X509Certificate[] trustCerts) throws IOExcept | |||
keyStore.setCertificateEntry(alias, cert); | |||
i++; | |||
} | |||
X509ExtendedTrustManager newDelegateManager = createDelegateTrustManager(keyStore); | |||
this.delegateManager = newDelegateManager; | |||
this.delegateManager = createDelegateTrustManager(keyStore); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pre-existing readability nit: this.
is redundant here and can be removed. There seem to be a few other instances of this pattern in this file, feel free to clean them all up if you like.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, it's redundant here and the G style guide suggest to omit it.
I code it that way because it was the style here and at KeyManager, plus it also exists at
this.keyManagers = keyManagerList; |
Let's defer to Eric
@@ -43,30 +44,38 @@ | |||
/** | |||
* AdvancedTlsX509TrustManager is an {@code X509ExtendedTrustManager} that allows users to configure | |||
* advanced TLS features, such as root certificate reloading, peer cert custom verification, etc. | |||
* We expect only one of {@link AdvancedTlsX509TrustManager#useSystemDefaultTrustCerts()}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you elaborate on what happens if the user does not respect this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Theoretically speaking, a user can do the following trick:
- Some process (non gRPC) updates the cert file
- User spawns a thread to periodically call updateTrustCredentialsFromFile(file)
- User calls updateTrustCredentialsFromFile(File trustCertFile, long period, TimeUnit unit,
ScheduledExecutorService executor) multiple times without cancelling the result
As a result an outdated version of creds might be used till the next update. It's not truly a synchronization issue (that's why we don't guard this methods with locks) but a similar to it - so we decided to add a warning. Definitely the example is a made up one and we don't expect a user do anything like that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense, thanks for clarifying.
@@ -43,30 +44,38 @@ | |||
/** | |||
* AdvancedTlsX509TrustManager is an {@code X509ExtendedTrustManager} that allows users to configure | |||
* advanced TLS features, such as root certificate reloading, peer cert custom verification, etc. | |||
* We expect only one of {@link AdvancedTlsX509TrustManager#useSystemDefaultTrustCerts()}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Optional presentation suggestion: Consider structuring the Javadoc as:
AdvancedTlsX509TrustManager is an {@code X509ExtendedTrustManager} that allows users to configure
advanced TLS features, such as root certificate reloading, peer cert custom verification, etc.
For Android users: this class is only supported in API level 24 and above.
We expect only one of {@link AdvancedTlsX509TrustManager#useSystemDefaultTrustCerts()},
{@link AdvancedTlsX509TrustManager#updateTrustCredentials(X509Certificate[])},
{@link AdvancedTlsX509TrustManager#updateTrustCredentialsFromFile(File, long, TimeUnit,
ScheduledExecutorService)},
{@link AdvancedTlsX509TrustManager#updateTrustCredentialsFromFile(File)} methods to be called
after instantiation of this class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the idea of extra empty LOCs for separation (pushed). But I prefer the current order because of
- Overall description
- Impl notes (for all users, including Android)
- Extra note (for Android users only)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SG, just an optional suggestion. :)
@@ -211,12 +218,15 @@ private void checkTrusted(X509Certificate[] chain, String authType, SSLEngine ss | |||
|
|||
/** | |||
* Schedules a {@code ScheduledExecutorService} to read trust certificates from a local file path | |||
* periodically, and update the cached trust certs if there is an update. | |||
* periodically, and update the cached trust certs if there is an update. Please make sure to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
micro-nit: s/and update the/and updates the
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2 instances fixed, thank you!
@IgnoreJRERequirement | ||
public final class AdvancedTlsX509TrustManager extends X509ExtendedTrustManager { | ||
private static final Logger log = Logger.getLogger(AdvancedTlsX509TrustManager.class.getName()); | ||
|
||
// Minimum allowed period for refreshing files with credential information. | ||
private static final int MINIMUM_REFRESH_PERIOD = 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Prefer using Duration
here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same problem with the KeyManager - they use 'period' as a param name (plus it's long + TimeUnit) -
public Closeable updateTrustCredentialsFromFile(File trustCertFile, long period, TimeUnit unit, |
Let's defer to Eric if I need to refactor all of it.
@RunWith(JUnit4.class) | ||
public class AdvancedTlsX509TrustManagerTest { | ||
|
||
public static final String CA_PEM_FILE = "ca.pem"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Constants generally should be private unless we have a need for them to be visible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed - nothing in the codebase depends on this const. However this style is in AdvancedTlsTest -
public static final String CA_PEM_FILE = "ca.pem"; |
Let's defer to Eric, I can also refactor the AdvancedTlsTest to make it consistent
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed here and in AdvancedTlsTest (#11139)
} | ||
|
||
@Test | ||
public void credentialSetting() throws Exception { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same style nits here RE test structure as in the key manager PR, but we can defer to Eric if you prefer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The name here is pretty weak. updateTrustCredentials_replacesIssuers
would seem better. Although it seems updateTrustCredentialsFromFile(File)
should also be run again at the end if the point is to notice the replacement. Otherwise, I'd have expected three separate instances of AdvancedTlsX509TrustManager to be used (and three separate tests then seems fair).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM modulo readability concerns, for which I'll defer to @ejona86.
API review meeting notes:
Should document that these are dangerous to us, unless also specifying your own cert validation. Yes, one has INSECURELY in its name, but there should be some javadoc.
Mention that any loaded trust certs will be ignored. Yes, that's what it says, but just "those other methods that you use all the time stop doing anything" is helpful to point out.
"enforced" could mean several different things, including "causes an error." Probably want to tweak that to be more clear. |
Done - I also reworded few comments before 'bumping up' to javadoc level. PTAL |
util/src/main/java/io/grpc/util/AdvancedTlsX509TrustManager.java
Outdated
Show resolved
Hide resolved
@@ -117,7 +122,7 @@ public void useSystemDefaultTrustCerts() throws CertificateException, KeyStoreEx | |||
NoSuchAlgorithmException { | |||
// Passing a null value of KeyStore would make {@code TrustManagerFactory} attempt to use | |||
// system-default trust CA certs. | |||
this.delegateManager = createDelegateTrustManager(null); | |||
createDelegateTrustManager(null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
... doesn't this make this function a noop?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, thanks for catching this - I was debugging some tests and accidentally left it. No existing tests caught it, even
tm.useSystemDefaultTrustCerts(); |
I added a new small test to this PR to catch such situation.
util/src/main/java/io/grpc/util/AdvancedTlsX509TrustManager.java
Outdated
Show resolved
Hide resolved
util/src/main/java/io/grpc/util/AdvancedTlsX509TrustManager.java
Outdated
Show resolved
Hide resolved
util/src/main/java/io/grpc/util/AdvancedTlsX509TrustManager.java
Outdated
Show resolved
Hide resolved
util/src/main/java/io/grpc/util/AdvancedTlsX509TrustManager.java
Outdated
Show resolved
Hide resolved
} | ||
|
||
@Test | ||
public void credentialSetting() throws Exception { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The name here is pretty weak. updateTrustCredentials_replacesIssuers
would seem better. Although it seems updateTrustCredentialsFromFile(File)
should also be run again at the end if the point is to notice the replacement. Otherwise, I'd have expected three separate instances of AdvancedTlsX509TrustManager to be used (and three separate tests then seems fair).
util/src/test/java/io/grpc/util/AdvancedTlsX509TrustManagerTest.java
Outdated
Show resolved
Hide resolved
util/src/main/java/io/grpc/util/AdvancedTlsX509TrustManager.java
Outdated
Show resolved
Hide resolved
* (File, long, TimeUnit, ScheduledExecutorService)}, | ||
* {@link AdvancedTlsX509TrustManager#updateTrustCredentialsFromFile | ||
* (File, long, TimeUnit, ScheduledExecutorService)} | ||
* By default, {@link Verification#CERTIFICATE_AND_HOST_NAME_VERIFICATION} mode for authenticating |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This "newline before each sentence" is not great. Flow it together, or separate it more and put <p>
. I can understand it sometimes because you don't want to reflow the text, but here it is every sentence and that is quite different from what it will look like in the generated doc. The sentence just before this doesn't have a period, which would be more obvious if it was flowed together.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ejona86 I applied the changes we discussed - PTAL |
This PR is a part of 'Stabilize Advanced TLS' effort.
Clean up, improve javadoc, de-experimentalize of AdvancedTlsX509TrustManager, add a unit test (e2e already exists).