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

okhttp: support Conscrypt security provider #3971

Merged
merged 2 commits into from Jan 31, 2018

Conversation

@ericgribkoff
Copy link
Contributor

commented Jan 15, 2018

This addresses #3966 and incorporates the text changes to SECURITY.md from #3301. Tested with org.conscrypt:conscrypt-android:1.0.0.RC14, as well as Google Play's security provider.

This changes our detection of the Google Play security provider from using the class name to the provider name, GmsCore_OpenSSL. This would seem to be equally as stable, and recent versions of OkHttp do the check similarly, although I realized in testing this that OkHttp uses GMSCore_OpenSSL, which did not work in my tests (checking elsewhere, it should indeed be GmsCore_OpenSSL, and the lookup is case-sensitive).

assertEquals(TlsExtensionType.ALPN_AND_NPN, tlsExtensionType);

// Clean up
Security.removeProvider(fakeConscrypt.getName());

This comment has been minimized.

Copy link
@ejona86

ejona86 Jan 17, 2018

Member

try-finally or similar (could maybe remove it unconditionally in the @After).

SECURITY.md Outdated
import java.security.Security;
...
Security.addProvider(Conscrypt.newProvider());

This comment has been minimized.

Copy link
@ejona86

ejona86 Jan 17, 2018

Member

We should prefer insertProviderAt so that Conscrypt can be prioritized.

This comment has been minimized.

Copy link
@ericgribkoff

ericgribkoff Jan 19, 2018

Author Contributor

Done

*/
private static Provider getAndroidSecurityProvider() {
for (String providerClassName : ANDROID_SECURITY_PROVIDERS) {
for (String providerName : ANDROID_SECURITY_PROVIDERS) {

This comment has been minimized.

Copy link
@ejona86

ejona86 Jan 17, 2018

Member

We should loop through the providers in order and choose the first we can use. That way the user has some way of configuring our behavior, and our behavior will better match expectations.

This comment has been minimized.

Copy link
@ericgribkoff

ericgribkoff Jan 19, 2018

Author Contributor

Done

@ericgribkoff ericgribkoff force-pushed the ericgribkoff:conscrypt branch from c9808f6 to 8a71766 Jan 19, 2018

@ericgribkoff

This comment has been minimized.

Copy link
Contributor Author

commented Jan 19, 2018

Moved the logic to decide what TLS extensions are available into Platform. This seems like where it belongs, since the Platform already decides on the security provider using similar logic. This means that tests of this logic are gone from OkHttpProtocolNegotiatorTest (but this maybe isn't so much worse than the status quo since we never had tests for picking the actual provider anyways). I've manually tested the changes on emulators, but this almost certainly deserves some test coverage. I didn't add tests yet, as to-date we don't have any (?) tests of the copied OkHttp classes (and simulating all the different platforms/API levels looks a little tricky), but I can add tests if the approach here looks good.

Worth noting: recent Android builds have AndroidNSSP (Android Network Security Policy) as the #1 security provider. I haven't looked into it, but it might be worthwhile adding this to the list of recognized security providers later.

@ericgribkoff

This comment has been minimized.

Copy link
Contributor Author

commented Jan 29, 2018

@ejona86 FYI this was heavily refactored/redone since the earlier approval, so I'm holding off on merging until you have a chance to (re)review

@ericgribkoff ericgribkoff merged commit 722d6f0 into grpc:master Jan 31, 2018

8 checks passed

Bazel Kokoro build finished
Details
Cronet Kokoro build finished
Details
GAE Interop Kokoro build finished
Details
Macos Kokoro build finished
Details
Windows Kokoro build finished
Details
cla/linuxfoundation ericgribkoff authorized
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage decreased (-0.03%) to 88.352%
Details

@ericgribkoff ericgribkoff deleted the ericgribkoff:conscrypt branch Jan 31, 2018

@lock lock bot locked as resolved and limited conversation to collaborators Jan 19, 2019

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