Refactor: remove spring security jwt and use nimbus jose #2624

strehle · 2023-11-28T17:29:00Z

Intro:
UAA uses https://github.com/spring-attic/spring-security-oauth
See #2560 , we need to replace it

Replacement with library com.nimbusds:nimbus-jose-jwt see https://github.com/cloudfoundry/uaa/blob/develop/dependencies.gradle#L125
Nimbus in use already for some fixes in past...
Details :
https://connect2id.com/products/nimbus-jose-jwt

There are some incompatibilities with this replacement

HMAC signature
- key type was in UAA always MAC, but standard needs oct. Added workaround in JWK creation
- key length . The key size must fit to algorithm, e.g. HS256 needs 32 bit key length. Created UaaMacSigner fork to allow old keys
- JWS header parser. x5c and jwk header had wrong type, use now generic object, because we dont need the keys but we need to parse the header
RSA signature
- for small keys (<2KB) there is a deprecated allow option. Added this to have currently support for old keys. Need to have a solution in general.
JWS header processing

JWT header was not processed as expected, means a JW header with alg HS256 was verified internally with a RSA key. So signature validation was done on internal configured key and the header from JWT was not really checked. If this happed in past then this is a severe security issue which cannot happen with nimbus. Mentioned it only for completeness

Good news.
UAA allows now ESxxx signature family. The spring library had only support for validation of ES, now we can sign and verify all specified JWS signing algorithms!!!

nimbus already in use

cf-gitbot · 2023-11-28T17:29:06Z

We have created an issue in Pivotal Tracker to manage this:

https://www.pivotaltracker.com/story/show/186569273

The labels on this github issue will be updated when the story is started.

main issue. audience is empty in same cases tests correct where HS256 can be used to create RS256 signature HMAC key length corrected

# Conflicts: # uaa/src/test/java/org/cloudfoundry/identity/uaa/mock/token/TokenMvcMockTests.java

Add a legacy signature check for HMAC

…eSpringJwt

Tallicia · 2023-12-15T22:13:56Z

Will this be updated soon to address the merge conflicts?

strehle · 2023-12-15T23:07:19Z

Will this be updated soon to address the merge conflicts?

Do you see a merge conflict in this PR?

server/src/main/java/org/cloudfoundry/identity/uaa/oauth/InvalidSignatureException.java

…eSpringJwt

...ava/org/cloudfoundry/identity/uaa/provider/oauth/ExternalOAuthAuthenticationManagerTest.java

Tallicia

This is quite the PR, In the future if possible to reduce this to smaller segments, that would be fantastic. Though I spent a considerable amount of time, I could have easily spent three times as much to deeply scrutinize, I left comments, and are more for refactoring options following this PR than required as part of this PR.

Overall this is approved, It was good a large majority was duplicated updates and changes and many removals, though the additions require significant deep dives, which I do not feel I completely dove all the way through, but trust based on what I saw and the tests, it was meeting desired objectives.

Tallicia · 2024-01-09T19:20:56Z

model/src/main/java/org/cloudfoundry/identity/uaa/util/UaaStringUtils.java

@@ -49,6 +49,8 @@ public final class UaaStringUtils {

    public static final String EMPTY_STRING = "";

+    public static final String DEFAULT_UAA_URL = "http://localhost:8080/uaa";


Nice, that is a step in the right direction. Following this PR, the port should be defaulted to 8080 with the capability to override.

Tallicia

Looks good

Tallicia · 2024-01-09T19:24:30Z

@strehle The PR is indicating "This branch cannot be rebased due to conflicts" Are you planning to rebase to address whatever conflicts github is referring to?

strehle · 2024-01-10T14:59:01Z

@strehle The PR is indicating "This branch cannot be rebased due to conflicts" Are you planning to rebase to address whatever conflicts github is referring to?

yes, I would like to release soon (after our meeting) whithout this PR and then I merge and work on further refactorings . .... because of our pipeline hickups I waited here

Refactor: remove spring security jwt and use nimbus jose

9eb9f61

nimbus already in use

cf-gitbot added the unscheduled label Nov 28, 2023

strehle added DO NOT MERGE Internal Test or WIP, please DO NOT MERGE and removed unscheduled labels Nov 28, 2023

strehle linked an issue Nov 28, 2023 that may be closed by this pull request

Spring Security Jwt End of Life #2229

Closed

strehle added this to the EOL_Removal milestone Nov 28, 2023

strehle added 10 commits November 28, 2023 21:21

fixes from non OIDC standard tests

f76d255

main issue. audience is empty in same cases tests correct where HS256 can be used to create RS256 signature HMAC key length corrected

Merge branch 'fix/tokenNonStandard' into eol/replaceSpringJwt

20d6831

# Conflicts: # uaa/src/test/java/org/cloudfoundry/identity/uaa/mock/token/TokenMvcMockTests.java

refactoring token to claim parser

a105e63

Add a legacy signature check for HMAC

Merge remote-tracking branch 'origin/develop' into eol/replaceSpringJwt

affc0cc

test fix

5d7c941

test fixes

96b1cc1

sonar fixes

4897c98

Merge branch 'develop' of github.com:cloudfoundry/uaa into eol/replac…

6cdf61f

…eSpringJwt

sonar refactorings

f6bed5b

Merge branch 'develop' of github.com:cloudfoundry/uaa into eol/replac…

8685f62

…eSpringJwt

strehle mentioned this pull request Dec 1, 2023

fixes from non OIDC standard tests #2625

Closed

renaming to have less differences in library replacement

ebd2433

strehle added in_review The PR is currently in review and removed DO NOT MERGE Internal Test or WIP, please DO NOT MERGE labels Dec 1, 2023

strehle requested a review from a team December 1, 2023 09:01

tack-sap approved these changes Dec 11, 2023

View reviewed changes

strehle assigned Tallicia Dec 12, 2023

strehle added 4 commits December 16, 2023 13:04

Merge remote-tracking branch 'origin/develop' into eol/replaceSpringJwt

a6a79f6

fixed sonar smells

d66603b

Merge remote-tracking branch 'origin/develop' into eol/replaceSpringJwt

56e1bc7

Merge remote-tracking branch 'origin/develop' into eol/replaceSpringJwt

b7e4c01

strehle added this to the EOL_Removal milestone Dec 21, 2023

strehle added 4 commits December 21, 2023 17:00

Merge remote-tracking branch 'origin/develop' into eol/replaceSpringJwt

5b45227

review

aadb0d8

use interface Verifier

39b2d85

review

e22e9c3

hsinn0 reviewed Dec 21, 2023

View reviewed changes

server/src/main/java/org/cloudfoundry/identity/uaa/oauth/InvalidSignatureException.java Outdated Show resolved Hide resolved

generated value

1ed82f8

strehle requested a review from hsinn0 December 21, 2023 20:15

hsinn0 approved these changes Dec 21, 2023

View reviewed changes

strehle removed the in_review The PR is currently in review label Dec 22, 2023

strehle assigned strehle and unassigned Tallicia Dec 22, 2023

strehle added 2 commits January 7, 2024 09:44

Merge branch 'develop' of github.com:cloudfoundry/uaa into eol/replac…

662bfed

…eSpringJwt

fix rebase from develop

a50fc60

strehle mentioned this pull request Jan 8, 2024

Support FIPS compliant BC #2230

Closed

Tallicia reviewed Jan 8, 2024

View reviewed changes

...ava/org/cloudfoundry/identity/uaa/provider/oauth/ExternalOAuthAuthenticationManagerTest.java Outdated Show resolved Hide resolved

Tallicia approved these changes Jan 8, 2024

View reviewed changes

Tallicia added in progress accepted Accepted the issue labels Jan 8, 2024

strehle added 2 commits January 9, 2024 09:18

Merge remote-tracking branch 'origin/develop' into eol/replaceSpringJwt

2b664fa

introduce constant DEFAULT_UAA_URL

6bc3228

Tallicia reviewed Jan 9, 2024

View reviewed changes

Tallicia approved these changes Jan 9, 2024

View reviewed changes

strehle merged commit f5c3890 into develop Jan 11, 2024
20 checks passed

strehle deleted the eol/replaceSpringJwt branch January 11, 2024 22:04

cf-gitbot removed in progress accepted Accepted the issue labels Jan 11, 2024

strehle removed a link to an issue Jan 12, 2024

Spring Security Jwt End of Life #2229

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor: remove spring security jwt and use nimbus jose #2624

Refactor: remove spring security jwt and use nimbus jose #2624

strehle commented Nov 28, 2023 •

edited

Loading

cf-gitbot commented Nov 28, 2023

Tallicia commented Dec 15, 2023

strehle commented Dec 15, 2023

Tallicia left a comment

Tallicia Jan 9, 2024

Tallicia left a comment

Tallicia commented Jan 9, 2024

strehle commented Jan 10, 2024

		@@ -49,6 +49,8 @@ public final class UaaStringUtils {

		public static final String EMPTY_STRING = "";

		public static final String DEFAULT_UAA_URL = "http://localhost:8080/uaa";

Refactor: remove spring security jwt and use nimbus jose #2624

Refactor: remove spring security jwt and use nimbus jose #2624

Conversation

strehle commented Nov 28, 2023 • edited Loading

cf-gitbot commented Nov 28, 2023

Tallicia commented Dec 15, 2023

strehle commented Dec 15, 2023

Tallicia left a comment

Choose a reason for hiding this comment

Tallicia Jan 9, 2024

Choose a reason for hiding this comment

Tallicia left a comment

Choose a reason for hiding this comment

Tallicia commented Jan 9, 2024

strehle commented Jan 10, 2024

strehle commented Nov 28, 2023 •

edited

Loading