spring-projects / spring-authorization-server

Spring Authorization Server
https://spring.io/projects/spring-authorization-server
Apache License 2.0
4.78k stars 1.25k forks source link

X509 client certificate authentication enforces client_id without checking on client authentication method #1635

Closed martinvisser closed 3 weeks ago

martinvisser commented 1 month ago

We recently upgraded to Spring Boot 3.3 and Spring Authorization Server 1.3.0. We do an OAuth2 flow with the following configuration:

spring:
  security:
    oauth2:
      client:
        registration:
          local:
            authorization-grant-type: authorization_code
            client-id: client
            client-secret: secret
            redirect-uri: "{baseUrl}/{action}/oauth2/code/{registrationId}"
            scope: "openid,profile"
        provider:
          local:
            issuer-uri: http://localhost:8081
            user-name-attribute: sub

We have not set any client authentication method on this side, but the default is client_secret_basic, which we also use in our authorization server:

@Bean
fun registeredClientRepository(): RegisteredClientRepository {
    val registeredClient =
        RegisteredClient.withId(UUID.randomUUID().toString())
            .clientId("local")
            .clientSecret("{noop}secret")
            .clientAuthenticationMethod(CLIENT_SECRET_BASIC)
            .authorizationGrantType(AUTHORIZATION_CODE)
            .redirectUri("http://localhost:8080/login/oauth2/code/aad")
            .scope(OPENID)
            .scope(PROFILE)
            .clientSettings(ClientSettings.builder().requireAuthorizationConsent(false).build())
            .build()
    return InMemoryRegisteredClientRepository(registeredClient)
}

Locally this works all fine, because it runs on http. However, when we deploy our applications on Cloud Foundry, we connect via https. This then creates/adds a x509 certificate to the request coming in on the authorization server, which then triggers X509ClientCertificateAuthenticationConverter. This converter looks for the client_id, but that was never added by OAuth2AuthorizationCodeGrantRequestEntityConverter and thus throws an exception:

// client_id (REQUIRED)
String clientId = parameters.getFirst(OAuth2ParameterNames.CLIENT_ID);
if (!StringUtils.hasText(clientId) || parameters.get(OAuth2ParameterNames.CLIENT_ID).size() != 1) {
    throw new OAuth2AuthenticationException(OAuth2ErrorCodes.INVALID_REQUEST);
}
if (!ClientAuthenticationMethod.CLIENT_SECRET_BASIC
        .equals(clientRegistration.getClientAuthenticationMethod())) {
    parameters.add(OAuth2ParameterNames.CLIENT_ID, clientRegistration.getClientId());
}

Maybe our set-up is wrong and we should not use client_secret_basic. Hoever, we also tried using client_secret_post, which gave a different error message, because X509ClientCertificateAuthenticationConverter changes the authentication method to tls_client_auth:

ClientAuthenticationMethod clientAuthenticationMethod = clientCertificateChain.length == 1
        ? ClientAuthenticationMethod.SELF_SIGNED_TLS_CLIENT_AUTH : ClientAuthenticationMethod.TLS_CLIENT_AUTH;

It seems like you cannot use another authentication method once you have a certificate and X509ClientCertificateAuthenticationConverter triggers.

We also found a somewhat nasty workaround where we remove the request attribute jakarta.servlet.request.X509Certificate via a filter, this just blocks the inner workings of X509ClientCertificateAuthenticationConverter and won't change the client authentication method. But of course, we rather don't do that. I would expect that if I set a client authentication method this would be used, not overwritten by the converter just because there is a (unrelated) certificate passed along.

(Side note, I asked on StackOverflow too, but got no interaction going there.)

jgrandja commented 4 weeks ago

@martinvisser Thank you for the extra detail. I've confirmed this is a bug and I'll have a fix for you shortly.

jgrandja commented 3 weeks ago

@martinvisser I just pushed the fix.