Skip to content

feat(scs): also set the Spring Cloud Config 5.1 OAuth2 properties - #503

Open
kvmw wants to merge 1 commit into
mainfrom
topic/kvmw/main/config-client-oauth2-properties
Open

kvmw wants to merge 1 commit into
mainfrom
topic/kvmw/main/config-client-oauth2-properties

Conversation

@kvmw

@kvmw kvmw commented Nov 9, 2026

Copy link
Copy Markdown
Collaborator

tl;dr: CfConfigClientProcessor also sets the properties Spring Cloud Config 5.1 reads for OAuth2, when that support is on the classpath. The old properties stay.

Motivation

Config 5.1 reads OAuth2 settings from spring.cloud.config.oauth2.* and a Spring Security client registration. CfConfigClientProcessor only sets spring.cloud.config.client.oauth2.*, so an application that uses Config 5.1 or code that wants one set of names must map the binding itself.

What Changed

  • When org.springframework.cloud.config.client.oauth2.ConfigClientOAuth2Properties is present, also set from the config-server binding:
    • spring.cloud.config.oauth2.enabled=true
    • spring.cloud.config.oauth2.client-registration-id=config-server
    • spring.security.oauth2.client.registration.config-server.{client-id,client-secret,authorization-grant-type,provider} (client_credentials)
    • spring.security.oauth2.client.provider.config-server.token-uri
  • The old spring.cloud.config.client.oauth2.* properties and propertyPrefixes are unchanged.
  • New public constructor CfConfigClientProcessor(ClassLoader) for the class check (the no-arg constructor still uses the default class loader).
  • Tests for both cases. A stand-in ConfigClientOAuth2Properties class in test sources avoids a Spring Cloud Config dependency.

Risk

  • The processor runs after config data loading (order ConfigDataEnvironmentPostProcessor.ORDER + 1). These properties help beans created after startup, not the config import itself.
  • spring.security.oauth2.client.registration.* makes Spring Boot register a ClientRegistrationRepository and apply the OAuth2 client auto-configuration if Spring Security's OAuth2 client is on the classpath. That is why the properties are set only when Config's OAuth2 support is present. Worth a second look from maintainers.
  • propertyPrefixes is not extended, so an application's own spring.security.oauth2.client.* settings do not stop this processor.

Validation

  • ./gradlew :java-cfenv-boot-pivotal-scs:build: 6 tests pass, checkstyle clean.
  • ./gradlew build without :java-cfenv-tests-boot:test: passes. That test fails on main too on an arm Mac (embedded Redis binary, Bad CPU type in executable).

Spring Cloud Config 5.1 reads its OAuth2 settings from
spring.cloud.config.oauth2.* and a Spring Security client registration,
not from spring.cloud.config.client.oauth2.*. When its OAuth2 support is
on the classpath, set these as well from the config-server binding. Keep
the old properties for backward compatibility.

Signed-off-by: kvmw <mohammad.shamsi@broadcom.com>
@kvmw
kvmw marked this pull request as ready for review November 9, 2026 12:22

@pivotal-david-osullivan pivotal-david-osullivan left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Important

AI-assisted; checked by me.

Strengths

  • The property names match upstream: spring.cloud.config.oauth2.enabled / client-registration-id (ConfigClientOAuth2Properties.PREFIX).
  • It only adds properties. The existing spring.cloud.config.client.oauth2.* properties and propertyPrefixes are unchanged (CfConfigClientProcessor.java:68-72).
  • The no-arg constructor still works, which SpringFactoriesLoader needs.
  • The tests cover both the "present" and "absent" cases. Using FilteredClassLoader with a stand-in class avoids adding a Config dependency.
  • The PR description is honest about the ordering and Spring Security risks.

Issues

Important (Should Fix)

  1. Pulling in Config 5.1 can change an app's default web security

    • File: CfConfigClientProcessor.java:77-81
    • Setting any spring.security.oauth2.client.registration.* makes Boot 4.1 create a ClientRegistrationRepository and an OAuth2AuthorizedClientService.
    • In a servlet app that has spring-security-config and oauth2-client but no SecurityFilterChain of its own, OAuth2ClientWebSecurityAutoConfiguration.OAuth2SecurityFilterChainConfiguration then replaces the default chain with oauth2Login(). The only registration is client_credentials, so nobody can log in interactively.
    • The generated default user probably also goes away, because Boot's user-details auto-config steps aside when a ClientRegistrationRepository exists.
    • The app doesn't have to opt in for this to happen. It only needs to be bound to a config-server and have Config 5.1 on the classpath.
    • Why it matters: just upgrading Spring Cloud could break login.
    • Fix options:
      • Make it opt-in, e.g. cfenv.config-client.oauth2.enabled.
      • Or don't publish the spring.security.* registration at all. Leave Config to build its own registration repository in the bootstrap context.
  2. For spring.config.import=configserver: apps, Config never sees the new properties when fetching config

    • Upstream, ConfigClientOAuth2Support.registerInterceptor runs from ConfigServerConfigDataLocationResolver, inside ConfigDataEnvironmentPostProcessor. This processor runs at ORDER + 1, after that point.
    • So the main thing spring.cloud.config.oauth2.enabled=true suggests (Config fetches an OAuth2 token to call the config server) only works in the legacy bootstrap flow.
    • Why it matters: users will think cfenv sets up config-server auth when, on the import path, it doesn't. The Motivation and Risk sections of the PR also contradict each other on this.
    • Fix: name a concrete consumer that reads these properties after startup. If there isn't one, cut the scope back to the bootstrap case and document that limit in the class Javadoc.
  3. It targets an unreleased upstream API

    • ConfigClientOAuth2Properties was added in spring-cloud/spring-cloud-config#3129 (2026-10-01). The latest tag is v5.1.0-RC2; there's no GA release yet.
    • The class name or package could still change before GA.
    • Fix: wait for 5.1.0 GA, or confirm with the Config team that the API is final.

Minor (Nice to Have)

  1. The check doesn't look for the OAuth2 client library

    • File: CfConfigClientProcessor.java:74
    • ConfigClientOAuth2Properties ships in the main spring-cloud-config-client jar. So the check really means "Config ≥ 5.1", not "Config's OAuth2 support is in use", which is what the PR text says.
    • Upstream checks for OAuth2ClientHttpRequestInterceptor (ConfigClientOAuth2Support.OAUTH2_CLIENT_IS_PRESENT). Checking for that class too would match upstream and make the condition do what the description says.
  2. Missing assertion for .provider

    • File: CfConfigClientProcessorTest.java (shouldSetTheConfigOAuth2PropertiesWhenSupported)
    • registration.config-server.provider is set at :80 but no test checks it.
  3. A public constructor added only for tests

    • File: CfConfigClientProcessor.java:57
    • CfConfigClientProcessor(ClassLoader) becomes public API only so the tests can pass a class loader. The CONFIG_OAUTH2_CLASS and REGISTRATION_ID constants are package-private, but the test is in ...scs.serviceregistry, so it can't use them.
    • Moving the test into io.pivotal.cfenv.boot.scs would let both be package-private.
  4. Users can only opt out of spring.cloud.config.oauth2.enabled=true from the command line

    • This property source is added before application.yml, so it overrides the app's own setting. To opt out, users need a command-line argument or cfenv's service-disable switch.
    • This works the same way as the existing properties, but it's worth a sentence in the Javadoc.

Recommendations

  • Decide on the target flow first: bootstrap vs config-data import. Then check it end to end with a sample app on Config 5.1 GA bound to an SCS config-server.
  • Add a test, or at least a manual check, for a servlet app with spring-boot-starter-security and oauth2-client and no custom filter chain. That covers the security issue above.

Assessment

Ready to merge? No

Reasoning: The code is clean and correct as far as it goes. But as written, it can change security behaviour in apps that never asked for it, it doesn't help apps that use configserver: import, and it depends on an RC-only upstream API.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants