okhttp: Add restricted AppEngine SSL setup #2795
Conversation
|
If I do some hacky stuff to make IS_RESTRICTED_APPENGINE=true for a unit test, it still seems the JDK used for unit tests doesn't support the SHA1PRNG. This code path gets exercised in an AppEngine end to end test. |
|
@carl-mastrangelo If anything, it adds a false sense of security. I wouldn't feel comfortable making any changes to this logic without testing it in AppEngine. This code path gets exercised as part of an AppEngine end to end test, and I think that is the better method of testing. |
| try { | ||
| if (sslSocketFactory == null) { | ||
| SSLContext sslContext = SSLContext.getInstance("Default", Platform.get().getProvider()); | ||
| SSLContext sslContext = null; |
There was a problem hiding this comment.
The assignment of null is not necessary.
| SSLContext sslContext = SSLContext.getInstance("Default", Platform.get().getProvider()); | ||
| SSLContext sslContext = null; | ||
| if (GrpcUtil.IS_RESTRICTED_APPENGINE) { | ||
| // The following auth code circumvents the following AccessControlException: |
There was a problem hiding this comment.
This comment is good, but it doesn't really explain why or how. Where is that exception thrown from? Why does this block of code change that?
There was a problem hiding this comment.
I've added some details to the comment, but the root of the stack trace looks like this:
Caused by: java.security.AccessControlException: access denied ("java.util.PropertyPermission" "javax.net.ssl.keyStore" "read")
at java.security.AccessControlContext.checkPermission(AccessControlContext.java:484)
at java.security.AccessController.checkPermission(AccessController.java:698)
at java.lang.SecurityManager.checkPermission(SecurityManager.java:549)
at java.lang.SecurityManager.checkPropertyAccess(SecurityManager.java:1298)
at java.lang.System.getProperty(System.java:708)
at org.conscrypt.DefaultSSLContextImpl.getKeyManagers(DefaultSSLContextImpl.java:66)
at org.conscrypt.OpenSSLContextImpl.<init>(OpenSSLContextImpl.java:81)
at org.conscrypt.DefaultSSLContextImpl.<init>(DefaultSSLContextImpl.java:57)
at sun.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method)
at sun.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:57)
at sun.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45)
at java.lang.reflect.Constructor.newInstance(Constructor.java:526)
at java.security.Provider$Service.newInstance(Provider.java:1253)
... 56 more
| // access denied ("java.util.PropertyPermission" "javax.net.ssl.keyStore" "read") | ||
| sslContext = SSLContext.getInstance("TLS", Platform.get().getProvider()); | ||
| KeyStore keyStore = KeyStore.getInstance(KeyStore.getDefaultType()); | ||
| keyStore.load(null, null); |
There was a problem hiding this comment.
As it turns out, this KeyStore logic was not needed.
| keyStore.load(null, null); | ||
| KeyManagerFactory keyManagerFactory = | ||
| KeyManagerFactory.getInstance(KeyManagerFactory.getDefaultAlgorithm()); | ||
| keyManagerFactory.init(keyStore, null); |
There was a problem hiding this comment.
here too, what does null mean?
There was a problem hiding this comment.
This logic was extraneous and removed.
| keyManagerFactory.init(keyStore, null); | ||
| TrustManagerFactory trustManagerFactory = | ||
| TrustManagerFactory.getInstance(TrustManagerFactory.getDefaultAlgorithm()); | ||
| trustManagerFactory.init((KeyStore) null); |
There was a problem hiding this comment.
Why aren't you using the keyStore you made above?
There was a problem hiding this comment.
The key store was not being used.
| sslContext.init( | ||
| null, | ||
| trustManagerFactory.getTrustManagers(), | ||
| SecureRandom.getInstance("SHA1PRNG", Platform.get().getProvider())); |
There was a problem hiding this comment.
What is special about SHA1PRNG?
| return sslSocketFactory; | ||
| } catch (GeneralSecurityException gse) { | ||
| throw new RuntimeException("TLS Provider failure", gse); | ||
| } catch (IOException e) { |
There was a problem hiding this comment.
What throws this exception?
There was a problem hiding this comment.
The key store init was throwing it, but it has now been removed.
|
@kpayson64 We can't really accept untested code. You may not feel comfortable making changes to this code, but you may not be the person making changes, or know the context. Adding unit tests proves that code is not accidentally broken, but also documents how it should work. Look at it from gRPC Java team's point of view. This code has no tests so we don't know if changes we make to it will break. It isn't clear why it is doing things differently. It reduces our test coverage. It will have a really long time between breakages and finding it, since the commits will take 1 -2 weeks to be brought in. Lastly, we have no way to actually run these. It is unreasonable to expect a custom test environment to be brought up to try these out. Not all contributors may know how to do it, or even be able. This code is not maintainable. |
|
It seems sufficient to test that this code does not interfere with non-Appengine code paths. It's the responsibility of AppEngine folks to make sure that this works in their environment. The same would be true for other special environment detection that some other PaaS might want to do. The alternative is to find some way to inject behavior, possibly by AppEngine doing their own channel provider but that has proven painful in the past with other platforms |
This reverts commit b753231.
This reverts commit b753231.
3 participants
Per request of the VTK team, we want to bake in the needed SSL workaround for AppEngine standard.