alexrainman / ModernHttpClient

ModernHttpClient
MIT License
126 stars 28 forks source link

Certificate Pinning with multiple pins in same domain and Skip pinning in iOS for specific domain #38

Closed cybvitthal closed 5 years ago

cybvitthal commented 5 years ago

Hi, I recently updated ModernHttpClient 2.4.2 with this library due to an error like: Latest build tools and SDK causes Didn't find class on path: DexPathList

Previously we were doing certificate pinning using ServicePointManager callback and validating multiple local certificates(current and future certificate).

This library has inbuilt functionality for pinning. We need to supply PublicKeys and Hostname in Pins. How can we add multiple public keys for same domain/host using this approach? Meaning, I added 2 Pins with same Hostname, first public key is invalid and 2nd is valid. In this case, pinning should work using 2nd public key.

Also, pinning works in sha256 format public key generated using domain name and OkHttp3's CertificatePinner class. If I have test.cer file, using "openssl x509 -inform PEM -in test.cer -pubkey -noout", I can get public key in base64 format. That does not work in this library.

alexrainman commented 5 years ago

If you read the documentation, you will find out how to add multiple public keys for the same domain. You will also find out that I am using sha256 and I recommend using OkHttp client to get them.

cybvitthal commented 5 years ago

Thanks for your immediate reply! Yes, I already read the documentation carefully. Using OkHttp3 I am able to retrieve the public keys in sha256 format using the domain name.

Although, I was not able to retrieve public key in required format using offline certificate and openssl.

Anyway, by multiple public keys(current and future certificate's key), using single pin, for same domain(here, domain.com), you mean like this?

NativeMessageHandler handler = new NativeMessageHandler (false, new TLSConfig () {
        Pins = new List<Pin> ()
        {
                new Pin()
                {
                    Hostname = "*." + "domain.com",
                    PublicKeys = new []
                    {
                        //current public key
                        "sha256/xyz=", // I will use only this, to have exact root cert pinning
                        "sha256/abc=",
                        "sha256/r/lmv=",

                        //future public key
                        "sha256/xyz1=", // I will use only this, to have exact root cert pinning
                        "sha256/abc1=",
                        "sha256/r/lmv1="
                    }
                },
                new Pin()
                {
                    Hostname = "*." + "skippinning.com",
                    PublicKeys = new string[]{ }
                },
        },
            DangerousAcceptAnyServerCertificateValidator = false,
            DangerousAllowInsecureHTTPLoads = false
}) {
        DisableCaching = true,
        Timeout = new TimeSpan (0, 0, 30)
    };

Here, I need to skip pinning for "skippinning.com" domain, is that a correct way? Its working fine in Android, as I am able to skip pinning using empty PublicKeys array. Getting "Certificate pinning failure: pins mismatch.", on iOS. What would be the reason?

Last one, If possible, can you let me know, best approach to handle pin mismatch in PCL, to show specific error to user. Also, If possible, way to handle invalid base64 error, if any.

cybvitthal commented 5 years ago

Hey @alexrainman, If possible, let me know, workaround to skip pinning in iOS. Above scenario with empty PublicKeys array working fine for Android only. There seems to something different in iOS. Awaiting your reply, thanks.

alexrainman commented 5 years ago

I will have to ship an update for that.

cybvitthal commented 5 years ago

I will be very glad, if you will release a new version with the additions. Otherwise, we can not use this library anymore.

Let me know, if you plan to release and update, how long it will take. Thanks a lot again for the support.

cybvitthal commented 5 years ago

If possible, Let me know your release plan. Thanks.

alexrainman commented 5 years ago

Early this week.

alexrainman commented 5 years ago

What do you mean by "best approach to handle pin mismatch in PCL"? What do you mean by "way to handle invalid base64 error"?

cybvitthal commented 5 years ago

Ok, thanks again for the reply and your planned release for this issue.

  1. For pin mismatch, If I have a REST service call with HttpClient in PCL, I can handle the exception "System.OperationCanceledException: Certificate pinning failure: pins mismatch.". To exactly catch it and display an error to user, we need to identify error by any means like error code, or a flag which says pinning is failed. Otherwise, I need to identify this exception using error message or something like that, or platform specific exception identification and handling in PCL, which does seems to be correct approach.

2.In Android, This case is not realistic, but, If accidentally, I entered wrong public key (Add % character and make it invalid base64 key), here library throws "Java.Lang.IllegalArgumentException: pins must be base64" exception. In iOS, I received above "pin mismatch" exception. If possible, we can handle this case as well.

alexrainman commented 5 years ago
  1. From PCL or platform specific projects, look into the InnerException inside HttpRequestException. What we have in the library right now is the best you can get.
  2. This is something i tried but couldn't make it happen. Will give it another try.
cybvitthal commented 5 years ago
  1. Yes, InnerException contains the SAME error message and Platform specific exception objects, Eg. Android - Javax.Net.Ssl.SSLPeerUnverifiedException and iOS - Foundation.NSErrorException. To handle them in PCL seems to be unrealistic due to different error types. I handled them using error message in PCL. But, is that a correct way, as error message can be changed in future.
  2. Ok, This is not urgent though. Can be an improvement in future releases.

Currently, for me, Skipping iOS certificate pinning using empty public key array, is on higher priority. If possible, please check for the release of this specific issue. Thanks again.

alexrainman commented 5 years ago

Nothing i can do about the error messages, other than what i already did.

Yes, both skipping with empty/null pins and verifying the keys before adding them to the pinner will be out in the next release.

cybvitthal commented 5 years ago

No problem for the error message for moment as long as it does not changes. Ok great!, If possible, can you let me know the release date for 2nd point(skip pinning). Please try asap, Thanks.

cybvitthal commented 5 years ago

@alexrainman ,Let me know, if its possible for you to release an update with iOS skip pinning, in 1 or 2 days?, Thanks.

alexrainman commented 5 years ago

Today.

cybvitthal commented 5 years ago

Cool, Thanks!

cybvitthal commented 5 years ago

Let me know, if the update will be released today, thanks.

cybvitthal commented 5 years ago

Please let me know, if its possible for you to release today, else I will need to check for the custom handler implementations, thanks!

cybvitthal commented 5 years ago

Thanks for releasing new version with the fix. I will check and let you know, tomorrow.

cybvitthal commented 5 years ago

Skip pinning is working fine. FYI, I received Internal Server Error/500, if we hit same REST request one after other. I need to clear cookies before creating handler. Thanks again for your actions!

rafcabezas commented 4 years ago

If you read the documentation, you will find out how to add multiple public keys for the same domain. You will also find out that I am using sha256 and I recommend using OkHttp client to get them.

It's also not clrear to me how to handle multiple keys for the same domain. The documentation only includes the keys for the certificate chain for a domain. So it's a single set of keys. Am I missing something?