alexrainman / ModernHttpClient

ModernHttpClient
MIT License
126 stars 28 forks source link

Pin wildcard hostnames #34

Closed xabre closed 5 years ago

xabre commented 5 years ago

First of all thank you for breathing life back into the ModernHttpClient. I have a question regarding defining hostnames for pins.

The current implementation seems to only accept complete hostnames like: test-example.com

We have several subdomains/enviornments for our app and I would like to pin them all using a single Pin entry. For example: dev.test-example.com, prod1.test-example.com, prod2.test-example.com.

The app should be able to deal with new sub-domains without us having to add a new certificate pin and rebuild. It would be totally cool to be able to pin something like *.test-example.com.

Is this possible with your current implementation or is it planned as a future enhancement?

alexrainman commented 5 years ago

Is this the same as #33? That I will do but, your sub-domains need to be included in the certificate alt names extension.

What is failing here is not the pins themselves but the hostname validation.

xabre commented 5 years ago

I wouldn't dismiss it that easy. The underlying OkHttp implementation for Android already supports this: https://square.github.io/okhttp/3.x/okhttp/okhttp3/CertificatePinner.html -> see wildcard pinning section. If this is still on the table I am willing to help with a PR.

alexrainman commented 5 years ago

Will definitely take a look.

alexrainman commented 5 years ago

HA, I can easily do it. Expect a new version later today.

alexrainman commented 5 years ago

It will be out today.

xabre commented 5 years ago

Thanks for the quick implementation. I have tested this out today and it works as expected on Android but there is an issue on UWP.

I have narrowed it down to the following section of ModernHttpClient/ModernHttpClient/Utility.cs

#if __PORTABLE__ || WINDOWS_UWP
            if (string.Compare(hostname, end) != 0) {
                return false;
            }
#else
            if (string.Compare(hostname, length, end, 0, end.Length, true, CultureInfo.InvariantCulture) != 0)
            {
                return false;
            }
#endif

specifically the #if __PORTABLE__ || WINDOWS_UWP does not account for the sub-string of hostname and thus always compares the full hostname with the sub-pattern. The fix would be to compute the substring of the hostaname starting from the first '.' and then compare.

Example: hostname = test.example.com pattern = *.example.com

for UWP will compare test.example.com with .example.com and will always fail :(`

If possbile I would suggest for a .netstandard implementation to avoid the #compiler preprocessor directives.

alexrainman commented 5 years ago

Ok will fix that later today.