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

React Native Support #6

Closed
omerlh opened this issue Jan 19, 2017 · 24 comments
Closed

React Native Support #6

omerlh opened this issue Jan 19, 2017 · 24 comments

Comments

@omerlh
Copy link
Contributor

omerlh commented Jan 19, 2017

Hey,
We have an android app with React Native code. React Native allow to override the default ok HTTP client for ALL network request from React Native with the following code:

        OkHttpClient client =
                    new OkHttpClient().newBuilder()
                            .sslSocketFactory(trustKit.getSSLSocketFactory("www.mycompanydomain.com"),
                                    trustKit.getTrustManager("www.mycompanydomain.com"))
                            .connectTimeout(0, TimeUnit.MILLISECONDS)
                            .readTimeout(0, TimeUnit.MILLISECONDS)
                            .writeTimeout(0, TimeUnit.MILLISECONDS)
                            .cookieJar(new ReactCookieJarContainer())
                            .build();
            OkHttpClientProvider.replaceOkHttpClient(client);

This work perfectly well, excpet for the fact that it will block all the request to API that are not belonging to my company - for example, firebase - because it will try to pin them with our domain configuration.
To solve this, we can use an approach similar to what CWAC did:

  • Add a method to PinningTrustManager that allow changing the domain configuration
  • Use the same code above to add TrustKit PinningTrustManager to React Native client
  • Add an interceptor to the client used by React Native, and change the host configuration before each request

Or we can try another approach, I am pretty open to ideas - for example, try to implement pinning in network interceptor - I will try to see if that possible.
Do you encounter similar problems? What will you recommend?
Thanks!

@nabla-c0d3
Copy link
Member

Yes this is a limitation we are aware of - we wanted the simplest possible implementation for v1.0.0. I agree that we should address this in the next release.

The Interceptor approach seems like the right way to do this in OkHttp. Some comments tho:

  • Ideally, I don't want to be able to change the domain configuration for a TrustManager (with something like (setHost()) because if it is used by multiple connections simultaneously, there is a race condition where the two connections/threads may call setHost() at the same time (effectively swapping the configuration and making the validation fail). At least that's my understanding of it - @commonsguy can you confirm (sorry for the random spam :p ) ?
  • Hence, I don't know if that's do-able but it seems like the best approach would be to create the TrustManager with the right domain configuration right before the validation actually happens. However, the Interceptor API may not allow this ie. changing the TrustManager right before the connection happens ? Did you find a way?

Thanks!

@commonsguy
Copy link

Your point regarding race conditions seems valid, and it's something that I'll need to work on. Thanks!

In terms of how exactly to address it... I'm not sure that there is a clean solution. Ideally, we would have a TrustManager per request, as a thin delegating wrapper around the real TrustManager that has the actual implementation. The delegating wrapper would know about the host and could feed that into the extended TrustManager methods. Since it would be per-request, we would not need to worry about the race condition as that host changes (e.g., redirects). Unfortunately, the TrustManager in OkHttp 3 seems to purely be per-OkHttpClient.

@omerlh
Copy link
Contributor Author

omerlh commented Jan 22, 2017

I totaly agree. This is why I suggested one of the workearround - either doing what CWAC did or manual validation in interceptor. Do you think of those options is better?
Thanks,
Omer

@omerlh
Copy link
Contributor Author

omerlh commented Jan 24, 2017

So after discussing this with OkHTTP guys, look like using interceptor is a valid approach. This is the pseudo code:

new Interceptor() {
                    @Override
                    public Response intercept(Chain chain) throws IOException {
                        Request request = chain.request();
                        SSLSocket socket = (SSLSocket)chain.connection().socket();
                        trustKit.getInstance().getTrustManager(request.url().host()).checkServerTrusted(socket.getSession().getPeerCertificateChain(), "");
                        return chain.proceed(request);
                    }
                }

But I need help with the call to the trust manager - what paramters should I pass? Am I calling the correct method?
Thanks!

@commonsguy
Copy link

So after discussing this with OkHTTP guys

Was that discussion recorded somewhere public? I'd love to look at it, if it was.

Your proposed solution is interesting. I'll need to poke around with it some more with respect to my library. Going this route would have you not register the TrustManager with OkHttp directly on older devices, as otherwise OkHttp would call the TrustManager as well as your interceptor calling it, and OkHttp's direct invocation would fail for the reasons it fails now.

what paramters should I pass?

I can't speak for this library. In my case, I would call the three-parameter checkServerTrusted() method on my CompositeTrustManager:

  public List<X509Certificate> checkServerTrusted(X509Certificate[] certs,
                                                  String authType,
                                                  String hostname)

Off the cuff, socket.getSession().getPeerCertificateChain() would be the value of the first parameter, and request.url().host() would be the value of the third parameter. I haven't ever looked at authType, so I would need to run some tests to see what values get used there.

Thanks for pointing this out!

@nabla-c0d3
Copy link
Member

nabla-c0d3 commented Jan 24, 2017

Hi @omerlh ,
Thanks for looking into this! Would also love to see the discussion if it's public (edit: nvm I saw the link above).

Your approach seems valid. The only downside I can see is that the certificate chain will be validated twice in a row:

  • First by OkHttp
  • Then by the Interceptor itself

This means that for invalid certificate chains, TrustKit will be unable to "see" them because the connection will have failed before even making it to the Interceptor, preventing TrustKit from sending a report about the SSL problem.
However, I can't think of a better way to do this and it might be a reasonable trade-off. Or is there a way to get the certificate chain also when the connection failed?

Thanks again!

@omerlh
Copy link
Contributor Author

omerlh commented Jan 25, 2017

@commonsguy see the link to the issue above in OkHTTP, this is the discussion I was talking about. Feel free to reopen it and respond there. The auth type, according to the documentation, is actually the key algorithm. So I guess I should be able to get it from the SSL session, but I could not find any property with that value. This is the main issue I have right now with implementing this solution. If you could help with that this will be great. Also, what do you think about opening an issue at your library and link to here? In that way, we can track this issue in all the relevant places.
@nabla-c0d3 (edit: I just saw the discussion at OkHttp, I am not sure, but I think my solution might be better, as it will give us reporting on Android 7) I just thought about a better solution: In your documentation for PinningTrustManager, you said that on Android N, the overload with the hostname gets called. So maybe we can do something like that:

  • Below Android N: Do not load TrustKit's TrustManger in the client. Use the interceptor for pinning. As we are below Android N, the handshake will fail only if the certificate is not trusted by the OS root CAs, and this is the only reports we will loose. Maybe later we can create ReportingTrustManager that send reports on such failures, but I think we can leave that for now.
  • Above Android N: Use TrustKit's TrustManger in the client. If we implement the relevant overload, we can use the host name to get the correct domain configuration and get full reporting.
    What do you think about that? I think this is our best shot.

@commonsguy
Copy link

@omerlh I tried this approach yesterday and got rid of it, as it simply was not working the way that I want. I will address the problem in my library by serializing multiple-host access, to eliminate the race condition.

@omerlh
Copy link
Contributor Author

omerlh commented Jan 25, 2017

I am not sure I am following, but I guess it will be clearer when you complete this.

@nabla-c0d3
Copy link
Member

nabla-c0d3 commented Jan 25, 2017

@commonsguy I think it might be tricky to serialize it as depending on network conditions, the first host that gets set may not be the first host that triggers a call to your checkServerTrusted() ?

@omerlh I need to think about this more, based on the OkHttp person's suggestion, but feel free to try the implementation you described and let me know what happens.

@commonsguy
Copy link

@nabla-c0d3 Actually, it dawned on me a few hours ago to use ThreadLocal to wrap the host, as one thread will not need more than one host at once, and this will allow each thread to work with its own host value.

@nabla-c0d3
Copy link
Member

Yeah that makes sense and should work. This really shouldn't be so painful =)

@omerlh
Copy link
Contributor Author

omerlh commented Jan 26, 2017

I think @commonsguy solution still has some issues, as the interceptor get called after the handshake - meaning, after the TrustManager get called. Unless I am missing something here...

@omerlh
Copy link
Contributor Author

omerlh commented Feb 2, 2017

Update: I want to solve it in the following way:

  • Extract the pinning validation logic to another class that can be called from interceptor
  • Create new PinningTrustManager for Android N and above which does not need the hostname

Thank I can do the following:

  • Create an interceptor that will use the pinning validation class (will only validate the certificate pin, not the hostname)
  • Create the client, and either pass it the Android N PinningTrustManager or the interceptor, depend on the OS version.

This looks to me like the best option so far, but I would like to hear your thought before going on and creating a PR.

@nabla-c0d3, what do you think?
Thanks

@omerlh
Copy link
Contributor Author

omerlh commented Mar 20, 2017

@nabla-c0d3 I tried to implement this in PR, I think it should solved all the issues.

@nabla-c0d3
Copy link
Member

Hi @omerlh and sorry for the delay. We looked into both the HostnameVerifier and the NetworkInterceptor approaches.

None of them will work correctly tho, because on API level 24+, the system’s pinning validation (via the Network Security Configuration) is run before any custom HostnameVerifier or NetworkInterceptor. This is a problem because it means that TrustKit will not be able to send any pinning failure reports, since it will never “see” validation failures (as the system validation already blocked the connection). Overall it means that TrustKit on API 24+ provides no functionality/value over the Network Security Configuration, which is why these solutions do not seem very satisfying to me.

Hence, there does not seem to be any solution at the moment for the problem you are facing. An option would be to allow CertificatePinner to be overridden in OkHttp, but that is not possible right now.

Does that make sense and what do you think?
Thanks!

@omerlh
Copy link
Contributor Author

omerlh commented Mar 29, 2017

Hey @nabla-c0d3
I tried to solve that in the PR - by using custom TrustManager. The idea is for Api Level 24+ to use the TrustManager, and before that - to use interceptor. That way, on Api Level 24+ TrustKit function only for reporting, and under 24 it also performing the certificate pinning in the interceptor.
Does that make sense?

@nabla-c0d3
Copy link
Member

Hi @omerlh
Yes it does. It will create two very different code paths tho for different API levels, which will be difficult to maintain and keep bug-free. Also, it means that a lot of the SSL errors on API below 24 will not be seen by TrustKit (preventing reports from being sent).

I am now thinking about a slightly different implementation. We could expose two TrustManager:

  • HostSpecificTrustManager

    • Does pinning and SSL reporting for one domain (like the current implementation)
    • Works on all API levels
  • UniversalTrustManager

    • Does pinning and SSL reporting for all domains
    • Only works on API level 24+

It doesn't solve all the problems but it seems more manageable and easier to use as an App developer.
What do you think?
Thanks!

@omerlh
Copy link
Contributor Author

omerlh commented Apr 2, 2017

Hi @nabla-c0d3
This will not solve (unless I misunderstood you) my initial issue: Be able to use trust kit in React Native. Also:

  • Regarding maintainability: I tried to keep it easy to maintain - that's why TrustManager is using the new PinningValidator (I also tried to keep it similar to the iOS API).
  • It is true we will have better functionality for API 24+. But, I am fine with that - as we can expect that Android N will gain more popularity over time. Currently, Android M is the most popular and we can expect for it to change in the near future.
  • Another thing to keep mind is that OkHttp recommend to use a single instance of the client. This is why React Native has only single instance of the client - and that what create my initial issue.
  • Maybe it will be better to have 2 TrustManager? One, for API 24+ that does CertificatePinning and SSL Validation reporting, and one for API 23 and bellow - that only report on SSL validation errors? I can create a proposal that will not be hard to maintain, if you'd like.

What do you think?

@omerlh
Copy link
Contributor Author

omerlh commented Apr 30, 2017

Hey @nabla-c0d3
Could you please share if this is sound reasonable to you? This is starting to become urgent for us soon, as we are moving more code to React Native. I would like to know if this is something you could consider, or we should look for alternative options.
Thanks,
Omer

@nabla-c0d3
Copy link
Member

HI @omerlh , we will most likely go with the plan I described in my previous message (UniversalTrustManager and HostSpecificTrustManager) but I can't give you a timeline.

@omerlh
Copy link
Contributor Author

omerlh commented May 17, 2017

I see. As I explained before (unless I am missing something) - this will not solve the original issue for us, so it means we will have to look for an alternative solution.

@omerlh
Copy link
Contributor Author

omerlh commented Jun 26, 2017

Finally, I've used the regular OkHttp CertificatePinner. It is playing pretty well with React Native, and there is a good documentation on how to integrate it with ReactNative. I did that with a small change - I read the pinning certificates from TrustKit config, which has 2 advantages:

  • Android 7 compatibility.
  • Easy pinning for network requests from the native code.

I posted a sample code in a gist, it was pretty easy to set this up.

@omerlh omerlh closed this as completed Jun 26, 2017
@nabla-c0d3
Copy link
Member

Thanks for letting us know. I will check it out!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants