advancedtls: adding AdvancedTlsX509TrustManager and AdvancedTlsX509KeyManager

[ZhenLian]

This pull request adds the following classes to io.grpc.util:

1. an AdvancedTlsX509TrustManager that supports
   • reloading root certificates from the file system or memory
   • disabling host name verification check, and applying any custom-implemented in-handshake verification checks
2. an AdvancedTlsX509KeyManager that supports
   • reloading identity credentials(private key and certificate chain) from the file system or memory

This also adds the integration tests to io.grpc.netty.

[ZhenLian]

Hi @ejona86 , this is the rest of the parts for the advancedtls code. Could you please take a look at it please?
The Travis CI passes, while I still got a few other pre-submit failures that I will need to solve. But I think the majority of the code is there. Feel free to let me know if you have any questions/concerns about this. Thank you in advance for your review!

[cfredri4]

Some questions/comments:

• This should fix Server cert hot reloading #5335?

• I would like support for PKCS1 keys (generated by e.g. Istio), would you accept a small merge to CertificateUtils?

• Can OpenSslCachingX509KeyManagerFactory cause issues by caching the key material? (have not looked into it deeper)

• Any reason for using a scheduled executor and not a file watcher, similar to e.g.?
  https://github.com/cloudfoundry/java-buildpack-security-provider/blob/main/src/main/java/org/cloudfoundry/security/FileWatchingX509ExtendedKeyManager.java

[ZhenLian]

@ejona86 Thanks for the review so far! There are still two remaining comments and I will work on fixing them the week after next week(Just FYI, I am OOO next week).

[ZhenLian]

@cfredri4 Please see my comments below.

This should fix #5335?

Yeah, strictly speaking, this is a utility class, so it doesn't "fix" anything, but it did make reloading certificates easier in Java.

I would like support for PKCS1 keys (generated by e.g. Istio), would you accept a small merge to CertificateUtils?

Sure! I think in general, contributions to support other key formats are welcome. Probably @ejona86 can help review it, or I can also review once I am back(I am OOO next week).

Can OpenSslCachingX509KeyManagerFactory cause issues by caching the key material? (have not looked into it deeper)

I am not entirely sure what kind of issues are in your mind. If using the classes here to build your credentials in Netty, we will use SimpleKeyManagerFactory as the key manager factory. This is a simple implementation without any caching mechanism, so I don't see any correlations with OpenSslCachingX509KeyManagerFactory, unless I miss something.

Any reason for using a scheduled executor and not a file watcher, similar to e.g.?

Sorry, I didn't get a chance to look at the full details of the code you pointed me, but it seems the file watcher you pointed me is also using a executor(https://github.com/cloudfoundry/java-buildpack-security-provider/blob/a70aa0c6e87e311fcb645ebd747864e33426d101/src/main/java/org/cloudfoundry/security/FileWatcher.java#L43). So I guess inherently both ways are similar?

[cfredri4]

@ZhenLian ⬇️

I would like support for PKCS1 keys (generated by e.g. Istio), would you accept a small merge to CertificateUtils?

Sure! I think in general, contributions to support other key formats are welcome. Probably @ejona86 can help review it, or I can also review once I am back(I am OOO next week).

Given the small change required, I added a gist here: https://gist.github.com/cfredri4/5655c1e54dc94f236054ec97b43e0a08
Please look over feel free to change as required.

Can OpenSslCachingX509KeyManagerFactory cause issues by caching the key material? (have not looked into it deeper)

I am not entirely sure what kind of issues are in your mind. If using the classes here to build your credentials in Netty, we will use SimpleKeyManagerFactory as the key manager factory. This is a simple implementation without any caching mechanism, so I don't see any correlations with OpenSslCachingX509KeyManagerFactory, unless I miss something.

I had some recollection of seeing that OpenSslCachingX509KeyManagerFactory will wrap the given KeyManagerFactory and do caching, but after checking further it seems that this is only done when KeyManagerFactory has just been constructed from key material (not when given an existing KeyManagerFactory).

Any reason for using a scheduled executor and not a file watcher, similar to e.g.?

Sorry, I didn't get a chance to look at the full details of the code you pointed me, but it seems the file watcher you pointed me is also using a executor(https://github.com/cloudfoundry/java-buildpack-security-provider/blob/a70aa0c6e87e311fcb645ebd747864e33426d101/src/main/java/org/cloudfoundry/security/FileWatcher.java#L43). So I guess inherently both ways are similar?

The difference is that with a file watcher, the thread is sleeping until the file is changed and then woken up, i.e. there is no polling being done and changes are immedately notified.

[ZhenLian]

@cfredri4 Sorry for the delay. I just got some time to work on this.

Given the small change required, I added a gist here

The changes look good to me overall. Feel free to open a PR for that, and we can iterate on that. We will also need some tests.

The difference is that with a file watcher, the thread is sleeping until the file is changed and then woken up, i.e. there is no polling being done and changes are immedately notified.

OK I see. We've considered both approaches(polling model or the notification model), but finally decided to go for the polling model, for the following reasons:

1. The Java notification API we considered at that moment was WatchService, which seems to have some limitations, e.g. "If a watched file is not located on a local storage device then it is implementation specific if changes to the file can be detected"
2. We want this behavior to be consistent across multiple gRPC languages, but it seems at least for some languages, the actual notification behavior highly depends on the system it uses, which is hard to unify
3. If we adopt the notification model, if the credentials are updated and we are notified, but for some reasons our credential update failed(it could happen when the update is not done atomically), we won't get notified again with this newer version of credentials. But if we choose the polling model, hopefully the newer version can be picked up by the next poll

[ZhenLian]

@ejona86 Hi Eric, I think I've fixed all the comments and the build failures. Can you please take another look again, please? Thank you so much!

[ejona86]

Discussed outside of github. The KeyManager was looking better before the swap to KeyStore, although a KeyStore would be fine if we used KeyStore.Entry/PrivateKeyEntry; as-is though the KeyStore is pretty annoying and doesn't add much value. The TrustManager would be very well suited to mostly just save the delegate TrustManager as a field and delegate to it simply, instead of storing the KeyStore and the like.

We also discussed the fact that the KeyManager is racy, because the API itself is racy when an alias mutates. There's an option to mostly avoid the race by changing the alias name each time we do an update, and keeping a previous update. But that might not be essential.

[ZhenLian]

@ejona86 Hi Eric, I've updated the code based on the discussion we had last Friday. Would you mind taking a look again please? Thank you so much for the review so far!

[ZhenLian]

Thanks again for the review! Merging now...