Skip to content
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 49 additions & 1 deletion src/Mono.Android/Xamarin.Android.Net/AndroidClientHandler.cs
Original file line number Diff line number Diff line change
Expand Up @@ -219,6 +219,20 @@ string EncodeUrl (Uri url)
return bldr.ToString ();
}

/// <summary>
/// Returns a custom host name verifier for a HTTPS connection. By default it returns <c>null</c> and
/// thus the connection uses whatever host name verification mechanism the operating system defaults to.
/// Override in your class to define custom host name verification behavior. The overriding class should
/// not set the <see cref="m:HttpsURLConnection.HostnameVerifier"/> property directly on the passed
/// <paramref name="connection"/>
/// </summary>
/// <returns>Instance of IHostnameVerifier to be used for this HTTPS connection</returns>
/// <param name="connection">HTTPS connection object.</param>
protected virtual IHostnameVerifier GetSSLHostnameVerifier (HttpsURLConnection connection)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Naming conventions: Should this really be SSL or should it be Ssl?

I'm also not sure if it should be Hostname or HostName, though given that the (pre-existing?) type is IHostnameVerifier, Hostname should probably be preferred.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hostname is taken from the interface name (and I don't like it, but - consistency). And I don't like camel-casing TLAs...

{
return null;
}

/// <summary>
/// Creates, configures and processes an asynchronous request to the indicated resource.
/// </summary>
Expand All @@ -241,7 +255,19 @@ string EncodeUrl (Uri url)
};
while (true) {
URL java_url = new URL (EncodeUrl (redirectState.NewUrl));
URLConnection java_connection = java_url.OpenConnection ();
URLConnection java_connection;
if (UseProxy)
java_connection = java_url.OpenConnection ();
else
java_connection = java_url.OpenConnection (Java.Net.Proxy.NoProxy);

var httpsConnection = java_connection as HttpsURLConnection;
if (httpsConnection != null) {
IHostnameVerifier hnv = GetSSLHostnameVerifier (httpsConnection);
if (hnv != null)
httpsConnection.HostnameVerifier = hnv;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we really need to make property assignment conditional? I'd think that this should be fine:

httpsConnection.HostnameVerifier = GetSSLHostnameVerifier (httpsConnection);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't want to change the default property value if the method returns null

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, there's a default property value that isn't null...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

More that it's a property with unknown default value and we don't want to disturb the default state unless we're 100% sure what the effect would be

}

if (ConnectTimeout != TimeSpan.Zero)
java_connection.ConnectTimeout = checked ((int)ConnectTimeout.TotalMilliseconds);

Expand Down Expand Up @@ -729,11 +755,33 @@ void AppendEncoding (string encoding, ref List <string> list)
return httpConnection;
}

/// <summary>
/// Configure and return a custom <see cref="t:SSLSocketFactory"/> for the passed HTTPS <paramref
/// name="connection"/>. If the class overriding the method returns anything but the default
/// <c>null</c>, the SSL setup code will not call the <see cref="ConfigureKeyManagerFactory"/> nor the
/// <see cref="ConfigureTrustManagerFactory"/> methods used to configure a custom trust manager which is
/// then used to create a default socket factory.
/// Deriving class must perform all the key manager and trust manager configuration to ensure proper
/// operation of the returned socket factory.
/// </summary>
/// <returns>Instance of SSLSocketFactory ready to use with the HTTPS connection.</returns>
/// <param name="connection">HTTPS connection to return socket factory for</param>
protected virtual SSLSocketFactory ConfigureCustomSSLSocketFactory (HttpsURLConnection connection)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this instead be called CreateCustomSslSocketFactory()? Configure, to me, implies that it's configuring an existing instance/state, not creating and returning new state. (Based on return type, I'm assuming it can't be configuring existing state.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's more than just creation though, see the code below in SetupSSL - I debated calling it Create* but thought that it doesn't reflect the full spectrum

{
return null;
}

void SetupSSL (HttpsURLConnection httpsConnection)
{
if (httpsConnection == null)
return;

SSLSocketFactory socketFactory = ConfigureCustomSSLSocketFactory (httpsConnection);
if (socketFactory != null) {
httpsConnection.SSLSocketFactory = socketFactory;
return;
}

KeyStore keyStore = KeyStore.GetInstance (KeyStore.DefaultType);
keyStore.Load (null, null);
bool gotCerts = TrustedCerts?.Count > 0;
Expand Down