alternative fix: client secret re-encoding#3965
Conversation
gdgenchev
changed the title
cf-foundation-community-automation
Bot
moved this to Inbox
in Foundational Infrastructure Working Group
|
thanks, there is already another PR #3962 we will handle the fix for it with prio |
gdgenchev
changed the title
|
Confirmed: This fix resolves the issue. The bbr-run-drats jobs was green: |
github-project-automation
Bot
moved this from Inbox
to Pending Merge | Prioritized
in Foundational Infrastructure Working Group
|
Is there a regression test for this? For a client with empty secret, the test should verify that the secret hash doesn't change after a uaa restart. |
The test source set previously defined a class with the same fully-qualified name as the production PasswordEncoderConfig, shadowing it on the test classpath. Tests that instantiated `new PasswordEncoderConfig()` got the test version rather than the production one, hiding production-bean changes from test coverage. Renaming removes the shadow.
Pins matches("", encode("")) on the production nonCachingPasswordEncoder bean
and on the cachingPasswordEncoder bean wired in OauthEndpointBeanConfiguration.
Spring Security 7's BCryptPasswordEncoder rejects empty rawPassword, so both
beans must wrap a bcrypt encoder with empty-secret awareness; otherwise the cf
CLI empty-secret client breaks (DRATs v79.0.0 failure).
gdgenchev
force-pushed
the
fix-password-encoder
branch
from
c8e86b8 to
8844844
Compare
|
@gdgenchev Ok, thank you please fix the tests. we want your PR to get merged :-) |
Fix timing attack vulnerability in password encoders by replacing standard string comparison with MessageDigest.isEqual() for constant-time comparison.
Extract duplicated constantTimeEquals method from password encoders
|
@duanemay, sick! This was bugging me for hours. I was trying to fix it only test side, but the shadowing |
|
@gdgenchev after a review I can merge |
|
I am checking again this |
|
I tested and indeed before the rename of the shadowed test class |
Introduces a dedicated Gradle source set (prodWiringTest) that excludes test-scope class shadows, allowing wiring pin tests to instantiate the real PasswordEncoderConfig rather than the noop test double. Restores PasswordEncoderConfigTest and OauthEndpointBeanConfigurationTest in this source set, adds Javadoc to PasswordEncoderConfig explaining why EmptyAwareDelegatingPasswordEncoder must remain the outermost wrapper, and wires prodWiringTest into the test task so CI picks it up.
|
This is the best I could achieve with smallest change. Unit tests are back to 10 min. Other approaches caused a lot of issues - i guess it could be possible to somehow rename the shadowing test config class and adapt, but that led to a lot of failing tests. Thus, decided to keep the shadowing class and just focus on a way to achieve the new tests - and this was possible through gradle configuration and introducing new source set with prod classpath (so the shadowing class in tests does not overwrite the prod one). Seems a bit weird, but it is the simplest and best-working approach I found - now the new regression tests run as part of the integration tests - I have seen them red without the empty aware wrapper. |
…runs ClientAdminBootstrapProdEncoderTest runs in the prodWiringTest source set (real prod encoder, real DB, no test-scope shadows) and calls afterPropertiesSet() twice to simulate a UAA restart, asserting the stored hash is identical. This is the exact scenario that caused the v79.0.0 DRAT failure: without EmptyAwareDelegatingPasswordEncoder, Spring Security 7's BCryptPasswordEncoder rejects empty rawPassword in matches(), causing ClientAdminBootstrap to re-encode empty-secret clients on every startup and invalidating existing tokens.
…Gradle config Renames the source set and task from prodWiringTest to integrationTest, following the Gradle convention. Uses sourceSets["integrationTest"] directly instead of a local variable, and wires into check rather than test so that unit and integration tests remain separate tasks.
|
Hi @gdgenchev & @strehle the fix is reviewed and tested. Can you merge it and create a new UAA release as the issue is currently blocking the new CF release. |
The test did not measure timing; rename it to reflect that it verifies
empty raw passwords do not match a {noop} prefix with a non-empty value,
and drop the comments that admitted no timing was tested.
minor reviews
strehle
force-pushed
the
fix-password-encoder
branch
from
f702947 to
90ce08e
Compare
github-project-automation
Bot
moved this from Pending Merge | Prioritized
to Done
in Foundational Infrastructure Working Group
New UAA should appear in cf deployment next hours |
|
UAA release: https://bosh.io/d/github.com/cloudfoundry/uaa-release?v=79.3.0 already passed the update-release pipeline: https://concourse.wg-ard.ci.cloudfoundry.org/teams/main/pipelines/update-releases/jobs/update-uaa/builds/102 Starting at 9 AM today the release will run through the remaining validation pipelines: https://concourse.wg-ard.ci.cloudfoundry.org/teams/main/pipelines/cf-deployment |
This basically restores pre spring security 7 behaviour across all components that use a caching/non-caching password encoder bean. It reuses the empty aware delegating password encoder that was introduced in spring boot 4 migration, but it was used just in one place.
Alternative for #3962