Add support for TLS1.2 on pre-lollipop devices. #128

dj-mal · 2017-11-06T02:58:02Z

Add method enableTls12OnPreLollipop to UsersAPIClient and AuthenticationAPIClient, which would enable support for TLS 1.2 on Pre-lollipop devices when called.
Add OkHttpTls12Compat and Tls12SocketFactory to support TLS 1.2 by patching the socket on devices with API level 16-21.
References:
- https://developer.android.com/reference/javax/net/ssl/SSLSocket.html
- support enabling TLSv1.2 on Android 4.1-4.4. square/okhttp#2372

Fixes #126

lbalmaceda

Hey! thanks for the contribution. I've left you some comments. I'd prefer the new classes to be package private and I also don't like the use of PowerMock as well. Please, try to refactor the OkHttpTLS12Compat class and it's usage to achieve the same result without static methods. That would help you not requiring PowerMock at all. Cheers

lbalmaceda · 2017-11-08T20:47:13Z

auth0/src/main/java/com/auth0/android/management/UsersAPIClient.java

@@ -132,6 +133,12 @@ public void setUserAgent(String userAgent) {
        factory.setUserAgent(userAgent);
    }

+    @SuppressWarnings("unused")
+    public void enableTls12OnPreLollipop() {


Rename to enableTLS12OnPreLollipop, try to make it package private.

If this is made package private, the users have no means to enable TLS 1.2 on the client, since they need to call this method explicitly to enable support. Do you want to move this into the constructor?

lbalmaceda · 2017-11-08T20:50:32Z

auth0/src/main/java/com/auth0/android/util/Tls12SocketFactory.java

+ * @link https://developer.android.com/reference/javax/net/ssl/SSLSocket.html
+ * @see SSLSocketFactory
+ */
+public class Tls12SocketFactory extends SSLSocketFactory {


Rename to TLS12SocketFactory, try to make it package private

Renamed and made package private.

lbalmaceda · 2017-11-08T20:53:08Z

auth0/src/main/java/com/auth0/android/authentication/AuthenticationAPIClient.java

@@ -160,6 +161,11 @@ public void setUserAgent(String userAgent) {
        factory.setUserAgent(userAgent);
    }

+    @SuppressWarnings("unused")
+    public void enableTls12OnPreLollipop() {


Rename to enableTLS12OnPreLollipop, try to make it package private.

If this is made package private, the users have no means to enable TLS 1.2 on the client, since they need to call this method explicitly to enable support. Do you want to move this into the constructor?

lbalmaceda · 2017-11-08T20:53:49Z

auth0/src/main/java/com/auth0/android/util/OkHttpTls12Compat.java

+
+import javax.net.ssl.SSLContext;
+
+public final class OkHttpTls12Compat {


Rename to OkHttpTLS12Compat, try to make it package private.

We can't make this package private because both AutehnticationAPIClient and UsersAPIClient use it, and they're in different package.

lbalmaceda · 2017-11-08T20:55:26Z

auth0/src/main/java/com/auth0/android/util/OkHttpTls12Compat.java

+     * @see Tls12SocketFactory
+     * @param client OkHttpClient instance to be modified
+     */
+    public static void enableSupportOnPreLollipop(OkHttpClient client) {


Because this method is "transparent" and will do nothing on already valid API versions, let's rename it to enableForClient. Also keep in mind that static methods are difficult to test if you're just using Mockito/Robolectric.

Method renamed.

lbalmaceda · 2017-11-08T20:58:27Z

auth0/src/main/java/com/auth0/android/util/OkHttpTls12Compat.java

+            // Lollipop is included because some Samsung devices face the same problem on API 21.
+            return;
+        } else if (Build.VERSION.SDK_INT < Build.VERSION_CODES.JELLY_BEAN) {
+            Log.w(TAG, "TLS 1.2 not supported on API < 16");


The whole IF condition can be simplified to:

if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.JELLY_BEAN && Build.VERSION.SDK_INT < Build.VERSION_CODES.LOLLIPOP_MR1)

I don't find useful the log.w line.

Removed the log line and simplified the if condition.

lbalmaceda · 2017-11-08T21:01:45Z

auth0/src/test/java/com/auth0/android/util/OkHttpTls12CompatTest.java

+    }
+
+    @Test
+    public void shouldConfigTlsOnOrPreApi21() {


I know the class already has it, but add an explicit @Config(sdk=21) line here too.

Added config annotation.

lbalmaceda · 2017-11-08T21:02:18Z

auth0/src/test/java/com/auth0/android/util/OkHttpTls12CompatTest.java

+
+    @Test
+    @Config(sdk=22)
+    public void shouldNotConfigTlsPostApi21() {


Because the min API for this library is 15 and that's excluded from the TLS logic, please add a test for that one as well.

I tried but Robolectric doesn't support API level 15 so we can't just add that like other supported APIs.

dj-mal · 2017-11-09T04:17:17Z

Thanks for reviewing! I've made the changes accordingly. Most of the comments are reflected in the updated code, although some isn't implemented with reasons explained in individual comments.

I tried making OkHttpTLS12Compat non static, but the outcome may be a bit clumsy in my opinion. I've made a new branch with that, you can check the changes over there:
https://github.com/dj-mal/Auth0.Android/compare/prelollipop-tls...dj-mal:prelollipop-tls-non-static?expand=1

If you think that's OK I can merge the changes into this branch / PR.
Cheers.

lbalmaceda · 2017-11-10T18:22:33Z

I thought about this and actually, it makes sense to have the TLS flag inside the Auth0 class, like we do for the logging and the OIDC flag. Based on the non-static branch, these are the changes I'd make:

Rename OkHttpTLS12Compat to OkHttpClientFactory, which has a single method "createClient(boolean loggingEnabled, boolean tls12Enforced)". Both values are obtained from the Auth0 instance.
Change the non-public constructors of the API Clients that receive an OkHttpClient instance to just receive the factory instance. These constructors exist for test purposes only.
Factory won't be stored in any variable/field on the API Clients... Use it to create and assign the client on the constructor, but don't assign the factory anywhere (let gc do it's thing).
Helper methods inside the Factory will be private. (or package-private if you need to test them).
The class will be public and stored inside com.auth0.android/request/internal. Can't think of a better place now.. it must be public because both clients are in separate packages.. so keeping it inside the "internal" package will make people aware that we can break them at any time, and that they should not use it directly. That can be clarified in the class javadoc as well..
Remove flag setters/getters from the API Client. Move them to Auth0. Test them.

Please share your thoughts. I think it's cleaner this way.. Ideally we should hide any helper method/class from the users (or keep them in "internal") and try not to put external dependencies on the methods signatures.

* Create and modify OkHttpClient instance in the factory. * Get / set preference on enforcing TLS 1.2 in Auth0. * Remove compat logic in API clients. * Test TLS / logging in factory instead of API clients.

dj-mal · 2017-11-13T07:34:57Z

All are good suggestions and can reduce duplication across the API clients. They're implemented accordingly.

However, since OkHttpClientFactory.createClient uses only parameters from Auth0, maybe an Auth0 instance could be passed (i.e., createClient(auth0)) instead of separate booleans (i.e., createClient(loggingEnabled, tlsEnforced))?

lbalmaceda

Regarding your last comment, I prefer you don't so we avoid coupling it to another class. The class is already in "internal" so it won't matter a lot the amount of parameters we choose. 👍 I left you the final comments, looks much clearer now thanks 🙂 .

lbalmaceda · 2017-11-13T19:07:13Z

auth0/src/main/java/com/auth0/android/request/internal/OkHttpClientFactory.java

@@ -14,32 +17,49 @@

 import javax.net.ssl.SSLContext;

-public class OkHttpTLS12Compat {
+public class OkHttpClientFactory {


Please add an additional javadoc at the class level:

Factory class used to configure and obtain a new OkHttpClient instance. This class is meant for internal use only, breaking changes may appear at any time without backwards compatibility guarantee.

or something like that 🙂

lbalmaceda · 2017-11-13T19:07:19Z

auth0/src/main/java/com/auth0/android/request/internal/OkHttpClientFactory.java

    }

    /**
     * Enable TLS 1.2 on the OkHttpClient on API 16-21, which is supported but not enabled by default.
     * @link https://github.com/square/okhttp/issues/2372
     * @see TLS12SocketFactory
     */
-    public OkHttpTLS12Compat enableForClient() {
+    private void enforceTls12(OkHttpClient client) {
        // No need to modify client as TLS 1.2 is enabled by default on API21+
        // Lollipop is included because some Samsung devices face the same problem on API 21.
        if (client == null || Build.VERSION.SDK_INT < Build.VERSION_CODES.JELLY_BEAN


Remove client==null check as the client is always valid, since the only entry point is the createClient method. Add a @NonNull annotation to these methods parameters if you want instead, just to help devs as users won't see them.

lbalmaceda · 2017-11-13T19:13:03Z

auth0/src/test/java/com/auth0/android/request/internal/OkHttpClientFactoryTest.java

+        verifyTLS12NotEnforced(client);
+    }
+
+    private static List generateMockList(OkHttpClient client) {


rename generateInterceptorsMockList

dj-mal · 2017-11-14T03:02:20Z

Updated according to the comments. Cheers 😃

Add support for TLS1.2 on pre-lollipop devices.

232a2dd

dj-mal mentioned this pull request Nov 6, 2017

Support TLSv1.2 on older Android API #126

Closed

dj-mal force-pushed the prelollipop-tls branch 2 times, most recently from 09f4ffd to 6c0a739 Compare November 6, 2017 05:30

Add PowerMockIgnore conditions on API client tests.

557545a

dj-mal force-pushed the prelollipop-tls branch from 6c0a739 to 557545a Compare November 6, 2017 06:16

Add tests on TLS support utils.

99f2be0

dj-mal force-pushed the prelollipop-tls branch from 6a98a7c to 99f2be0 Compare November 7, 2017 01:25

lbalmaceda suggested changes Nov 8, 2017

View reviewed changes

dj-mal added 2 commits November 9, 2017 10:52

Rename TLS related methods; make TLS12SocketFactory package private.

f9eb054

Make OkHttpTLS12Compat non static.

f0d8ce0

dj-mal force-pushed the prelollipop-tls branch from d291615 to eccb072 Compare November 13, 2017 07:09

Refactor OkHttpTLS12Compat to OkHttpClientFactory.

c931684

* Create and modify OkHttpClient instance in the factory. * Get / set preference on enforcing TLS 1.2 in Auth0. * Remove compat logic in API clients. * Test TLS / logging in factory instead of API clients.

dj-mal force-pushed the prelollipop-tls branch from eccb072 to c931684 Compare November 13, 2017 07:32

lbalmaceda suggested changes Nov 13, 2017

View reviewed changes

Clean up OkHttpClientFactory.

318b95c

lbalmaceda approved these changes Nov 14, 2017

View reviewed changes

lbalmaceda added this to the v1-Next milestone Nov 14, 2017

lbalmaceda added the CH: Added label Nov 14, 2017

lbalmaceda merged commit ba14cc5 into auth0:master Nov 16, 2017

lbalmaceda modified the milestones: v1-Next, 1.12.0 Nov 16, 2017

dj-mal deleted the prelollipop-tls branch November 17, 2017 01:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for TLS1.2 on pre-lollipop devices. #128

Add support for TLS1.2 on pre-lollipop devices. #128

dj-mal commented Nov 6, 2017 •

edited by lbalmaceda

Loading

lbalmaceda left a comment

lbalmaceda Nov 8, 2017

dj-mal Nov 9, 2017

lbalmaceda Nov 8, 2017

dj-mal Nov 9, 2017

lbalmaceda Nov 8, 2017

dj-mal Nov 9, 2017

lbalmaceda Nov 8, 2017

dj-mal Nov 9, 2017

lbalmaceda Nov 8, 2017

dj-mal Nov 9, 2017

lbalmaceda Nov 8, 2017

dj-mal Nov 9, 2017

lbalmaceda Nov 8, 2017

dj-mal Nov 9, 2017

lbalmaceda Nov 8, 2017

dj-mal Nov 9, 2017

dj-mal commented Nov 9, 2017 •

edited

Loading

lbalmaceda commented Nov 10, 2017

dj-mal commented Nov 13, 2017

lbalmaceda left a comment

lbalmaceda Nov 13, 2017

lbalmaceda Nov 13, 2017

lbalmaceda Nov 13, 2017

dj-mal commented Nov 14, 2017


		import javax.net.ssl.SSLContext;

		public final class OkHttpTls12Compat {

Add support for TLS1.2 on pre-lollipop devices. #128

Add support for TLS1.2 on pre-lollipop devices. #128

Conversation

dj-mal commented Nov 6, 2017 • edited by lbalmaceda Loading

lbalmaceda left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dj-mal commented Nov 9, 2017 • edited Loading

lbalmaceda commented Nov 10, 2017

dj-mal commented Nov 13, 2017

lbalmaceda left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dj-mal commented Nov 14, 2017

dj-mal commented Nov 6, 2017 •

edited by lbalmaceda

Loading

dj-mal commented Nov 9, 2017 •

edited

Loading