AzureAD / microsoft-authentication-library-for-java

Microsoft Authentication Library (MSAL) for Java http://aka.ms/aadv2
MIT License
280 stars 137 forks source link

[Bug] DefaultHttpClient ignores supplied SSLSocketFactory #820

Closed stefanlourens closed 4 weeks ago

stefanlourens commented 1 month ago

Library version used

1.15.0

Java version

21.0.2

Scenario

ConfidentialClient - web site (AcquireTokenByAuthCode)

Is this a new or an existing app?

This is a new app or experiment

Issue description and reproduction steps

Supplying a SSLSocketFactory to the ConfidentialClientApplication builder has no effect.

I traced this down to the DefaultHttpClient's handling of SSL connections:

It currently checks if the connection is an instance of HttpURLConnection, but since HttpsURLConnection extends HttpURLConnection it's always true and the else is never executed.

if (connection instanceof HttpURLConnection) {
    return (HttpURLConnection) connection;
} else {
    HttpsURLConnection httpsConnection = (HttpsURLConnection) connection;

    if (sslSocketFactory != null) {
        httpsConnection.setSSLSocketFactory(sslSocketFactory);
    }

    return httpsConnection;
}

https://github.com/AzureAD/microsoft-authentication-library-for-java/blob/dev/msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/DefaultHttpClient.java#L93-L103

I suggest changing this to:

if (connection instanceof HttpsURLConnection) {
    HttpsURLConnection httpsConnection = (HttpsURLConnection) connection;

    if (sslSocketFactory != null) {
        httpsConnection.setSSLSocketFactory(sslSocketFactory);
    }

    return httpsConnection;
} else {
    return (HttpURLConnection) connection;
}

Relevant code snippets

No response

Expected behavior

The supplied SSLSocketFactory should be set on the HttpsUrlConnection httpsConnection.setSSLSocketFactory(sslSocketFactory); or setting the default ssl handing externally.

Identity provider

Microsoft Entra ID (Work and School accounts and Personal Microsoft accounts)

Regression

No response

Solution and workarounds

Currently the only workaround is to supply your own http client, with the logic fixed.

Avery-Dunn commented 1 month ago

Hello @stefanlourens : Thanks for bringing this to our attention! I was able to reproduce the issue, and this is definitely unintended behavior: if the URL is 'https' then the instance of Http... and instance of Https... checks are both true and the else block will never be reached.

I don't have an ETA but the fix will be in either our next release or the one after, and I'll update this thread once it's done.

bgavrilMS commented 1 month ago

@stefanlourens - please contribute your code fix by opening a PR if you need this to be resolved faster.

stefanlourens commented 1 month ago

@bgavrilMS Was originally hoping to add a test as well, but ran into some lombok compatibility issues, ended up just doing the change as suggested: https://github.com/AzureAD/microsoft-authentication-library-for-java/pull/821

Avery-Dunn commented 4 weeks ago

Done as part of https://github.com/AzureAD/microsoft-authentication-library-for-java/pull/821 and release in 1.15.1

Thanks again for the fix!