8000 Identity Zone Selection via X-Zid Header by adrianhoelzl-sap · Pull Request #3427 · cloudfoundry/uaa · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Identity Zone Selection via X-Zid Header #3427

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 16 commits into
base: develop
Choose a base branch
from
Open

Identity Zone Selection via X-Zid Header #3427

wants to merge 16 commits into from

Conversation

adrianhoelzl-sap
Copy link
Contributor

No description provided.

@adrianhoelzl-sap adrianhoelzl-sap requested review from tack-sap and a team April 29, 2025 09:22
@fhanik
Copy link
Contributor
fhanik commented Apr 30, 2025

@adrianhoelzl-sap How does this impact cookies and security?

What If I do a request like this

GET /some/path
Host: some.host.com
Header: X-Zid=zone-id-for-someother.host.com

and this sets up my cookies (including authentication stored in the session)
and then I do this request (and continue using the same JSESSIONID since the host hasn't changed)

GET /some/path
Host: some.host.com

Do I just gain access across zones?

@adrianhoelzl-sap
Copy link
Contributor Author

@adrianhoelzl-sap How does this impact cookies and security?

What If I do a request like this

GET /some/path
Host: some.host.com
Header: X-Zid=zone-id-for-someother.host.com

and this sets up my cookies (including authentication stored in the session) and then I do this request (and continue using the same JSESSIONID since the host hasn't changed)

GET /some/path
Host: some.host.com

Do I just gain access across zones?

This scenario should be addressed by this check here:

uaa/server/src/main/java/org/cloudfoundry/identity/uaa/authentication/SessionResetFilter.java

Line 60 in df50723

if (!Objects.equals(IdentityZoneHolder.getCurrentZoneId(), authentication.getPrincipal().getZoneId())) {

The zone resolved through the subdomain or X-Zid header (IdentityZoneHolder.getCurrentIdentityZoneId()) is compared to the zone for which the session was set up (authentication.getPrincipal().getZoneId()). If they do not match, the session is invalidated.

@fhanik
Copy link
Contributor
fhanik commented May 8, 2025

The SessionResetFilter is added at the end of each filter chain. it is the last filter that is used before an MVC endpoint is invoked.

But all the filters before that, that may contain logic and send redirects prior to the completion of the filter chain are still vulnerable to this.

A lot of the SAML/OAuth/OIDC happens in just filters, and not in MVC endpoints, and those filters would be working under incorrect assumptions.

Given the risk, is this header really needed?

If the HTTP client can set a header, it can set the "Host" header, and if there is a browser involved, cookie handling will be accurate.

@adrianhoelzl-sap
Refactor IdentityZoneResolvingFilter.doFilterInternal
c9c01d0
@adrianhoelzl-sap
Add reading of 'X-Zid' header
2ede7f0
@adrianhoelzl-sap
Add distinction between lookup of zone by ID and by subdomain
cfc1a82
@adrianhoelzl-sap
Refactor initialization of DAO in IdentityZoneResolvingFilterTests
92d5641
@adrianhoelzl-sap
Add unit tests for X-Zid header
563d9c7
@adrianhoelzl-sap
Add description of X-Zid header to overview section of API documentation
1272857
@adrianhoelzl-sap
Add toggle for X-Zid header
cee6802
@adrianhoelzl-sap
Add zidHeaderEnabled field to IdentityZoneResolvingFilter
ef0d97e
@adrianhoelzl-sap
Add check for flag being enabled to filter
89bee31
@adrianhoelzl-sap
Add IdentityZoneMismatchCheckFilter
0f5a3e7
@adrianhoelzl-sap
Add IdentityZoneMismatchCheckFilter to filter chain
648ac9f
@adrianhoelzl-sap
Remove IdZ check from SessionResetFilter
d473b18
@adrianhoelzl-sap
Add unit test for IdentityZoneMismatchCheckFilter
24af2bf
@adrianhoelzl-sap
Adjust version and mention feature flag in documentation
477bbf3
@adrianhoelzl-sap
Add codec to RandomValueStringGenerator to generate letter-only strings
bcca88e
@adrianhoelzl-sap
Add MockMvc tests for X-Zid header behavior
b69180d
strehle
strehle reviewed May 27, 2025
View reviewed changes
Copy link
Member
@strehle strehle left a comment

Choose a reason for hiding this comment

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

My hope was that we can consolidate the amount of filter (per each request) and optimize but can we integration this logic into one of other IdentityZoneXYZ filters ? ... maybe rename one and do more ?

...rc/main/java/org/cloudfoundry/identity/uaa/oauth/common/util/RandomValueStringGenerator.java
@@ -15,6 +15,8 @@ public class RandomValueStringGenerator {

private static final char[] DEFAULT_CODEC = "1234567890ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz-_"
.toCharArray();
private static final char[] CODEC_LETTERS_ONLY = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz"
Copy link
Member

Choose a reason for hiding this comment

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

pls use

uaa/server/src/main/java/org/cloudfoundry/identity/uaa/util/AlphanumericRandomValueStringGenerator.java

Line 7 in b449b5b

"1234567890ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz"

...rc/main/java/org/cloudfoundry/identity/uaa/oauth/common/util/RandomValueStringGenerator.java
@@ -90,4 +92,7 @@ public void setLength(int length) {
this.length = length;
}

public static RandomValueStringGenerator lettersOnly(final int length) {
Copy link
Member
@strehle strehle May 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

see above

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

@strehle strehle strehle left review comments

@tack-sap tack-sap Awaiting requested review from tack-sap

Assignees
No one assigned
Labels
None yet
Projects
4ECC
Foundational Infrastructure Working Group
Status: Inbox
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@adrianhoelzl-sap @fhanik @strehle
0