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

Client credentials authentication - token cache returns unintended auth token #2334

Closed
malkaviancz opened this issue Feb 21, 2024 · 6 comments · Fixed by #2341
Closed

Client credentials authentication - token cache returns unintended auth token #2334

malkaviancz opened this issue Feb 21, 2024 · 6 comments · Fixed by #2341
Projects

Comments

@malkaviancz
Copy link

malkaviancz commented Feb 21, 2024

Driver version

> 12.2.x

SQL Server version

Microsoft Azure SQL Data Warehouse (RTM) - 12.0.2000.8

Client Operating System

macOS Sonoma 14.3.1

JAVA/JVM version

jdk 17

Problem description

Using client credentials to authenticate, create 2 connections with a valid and an invalid client secret (in that order)

Expected behavior

The second connection attempt should throw SQLException stating invalid client secret provided

Actual behavior

The second connection succeeds, no exception was thrown

Any other details that can be helpful

With client credentials, after any successful authentication attempt, the auth token will be saved in the token cache, and subsequent connection with only client secret changed incorrectly returns the previous token
Tested and reproduceable on versions > 12.2.x
Previous versions doesn't use the token cache and behaved as expected

Repro code

public class Application {

    public static void main(String[] args) throws SQLException {
        String url = "jdbc:sqlserver://**************************-**************************.datawarehouse.fabric.microsoft.com;authentication=ActiveDirectoryServicePrincipal";
        String clientId = "clientId";
        String clientSecret = "clientSecret";
        String invalidClientSecret = "invalidClientSecret";

        try (Connection connection = DriverManager.getConnection(url, clientId, clientSecret);
             Connection invalidConnection = DriverManager.getConnection(url, clientId, invalidClientSecret)) {
        }
    }
}
@tkyc
Copy link
Member

tkyc commented Feb 21, 2024

I reached out to the MSAL4J project to get their insight on the topic.

AzureAD/microsoft-authentication-library-for-java#787

@tkyc
Copy link
Member

tkyc commented Feb 22, 2024

This is expected behaviour from MSAL. You can refer to the MSAL thread above for the detailed explanation. It's still up to debate whether we want to fail subsequent connections ourselves within the driver (since MSAL doesn't). I'm of the opinion that it's fine as right now an invalid secret will fail next validity check once the cached token expires.

@lilgreenbird lilgreenbird added this to Under Investigation in MSSQL JDBC via automation Feb 22, 2024
@lilgreenbird lilgreenbird moved this from Under Investigation to Waiting for Customer in MSSQL JDBC Feb 22, 2024
@malkaviancz
Copy link
Author

This still feels wrong to me, so if my app has 2 users trying to connect to the same database using client credentials, there could be cases where the second user gets access without entering the correct client secret?
Is there a way to disable this caching behavior?

@tkyc
Copy link
Member

tkyc commented Feb 26, 2024

It doesn't look like it's possible to disable the cache. I'll discuss this more with the team to see the direction that we'd want to take.

@David-Engel
Copy link
Contributor

FWIW, in the SqlClient library, we something similar to what the MSAL team commented. We cache a hash of passwords for AAD Password auth. If a new password is used, we don't try to get a token silently. We should do the same in the JDBC driver for AAD Password and AAD SP Secret. MSAL's behavior is appropriate for single-user apps, but not for us, since we might be in a multi-user/tenant environment.

@tkyc
Copy link
Member

tkyc commented Feb 26, 2024

Hm, I was too quick to judge since the behaviour was from MSAL. I'll make the necessary changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
MSSQL JDBC
  
Closed Issues
3 participants