Skip to content
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.

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

Change the application access authentication flow #17592

Merged
merged 8 commits into from
Jan 12, 2023

Conversation

ryanclark
Copy link
Contributor

@ryanclark ryanclark commented Oct 19, 2022

Previously, we'd pass the value of the cookie that needed to be set to /x-teleport-auth via the POST body. This opened it up to CSRF attacks, which was mediated by implementing a ?state= query param and a cookie.

This added quite a few different redirects, which can be simplified by allowing cross origin requests from the proxy address to the application's /x-teleport-auth endpoint, and sending the cookie value via a custom X-Cookie-Value header instead.

Custom headers cannot be added to <form> elements, nor included in a no-cors fetch() request. This means a user cannot be tricked into launching an application by a third party malicious actor.

This would fix #15935, as we're no longer relying on the cookie that gets unset by the x-teleport-auth dance.

Copy link

@ikkisoft ikkisoft left a comment

Choose a reason for hiding this comment

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

In term of security and protection against Login CSRF, the current implementation is comparable to the previous one. I did leave one comment, since I believe this mechanism will break for teleport installations that are configured with multiple public IPs/Domain names.

However, it's important to note that this security mechanism fully relies on a properly configured CORS policy. Given that the Access-Control-Allow-Origin uses the teleport's configuration (public_addr field), it is particularly important to ensure that all hostnames listed in this setting are trusted. Arbitrary HTML/Javascript hosted under such domains will have full access to cross-origin requests hence bypassing this protection.

lib/web/app/auth.go Outdated Show resolved Hide resolved
lib/web/apiserver.go Outdated Show resolved Hide resolved
@ryanclark
Copy link
Contributor Author

In term of security and protection against Login CSRF, the current implementation is comparable to the previous one. I did leave one comment, since I believe this mechanism will break for teleport installations that are configured with multiple public IPs/Domain names.

However, it's important to note that this security mechanism fully relies on a properly configured CORS policy. Given that the Access-Control-Allow-Origin uses the teleport's configuration (public_addr field), it is particularly important to ensure that all hostnames listed in this setting are trusted. Arbitrary HTML/Javascript hosted under such domains will have full access to cross-origin requests hence bypassing this protection.

Arbitrary HTML/JS on any proxy address is an issue in general, as it'll give access to the proxy API without CORS being needed.

I don't see this being an issue, there would be more attack vectors if there was arbitrary HTML/JS on a proxy endpoint than a POST /x-teleport-auth setting a cookie value (that doesn't guarantee app access, as the cookie value needs to be known).

That being said, I wouldn't be against a HMAC signature for the cookie that's also passed along, so brute force attempts to guess a cookie value couldn't be achieved.

Thanks for taking the time to review this.

@ikkisoft
Copy link

Agree. I was mostly concerned about a user's misconfiguration in which the CORS Access-Control-Allow-Origin returns an host that is not owned by the admin. E.g. dangling DNS records

@ryanclark ryanclark force-pushed the ryan/change-app-access-login-flow branch from f283234 to 3676f04 Compare November 30, 2022 20:30
@ryanclark ryanclark marked this pull request as ready for review November 30, 2022 20:31
@ryanclark ryanclark force-pushed the ryan/change-app-access-login-flow branch from 3676f04 to 1927781 Compare November 30, 2022 20:31
@ryanclark ryanclark force-pushed the ryan/change-app-access-login-flow branch from 1927781 to fcaa36c Compare November 30, 2022 20:38
lib/web/app/auth.go Outdated Show resolved Hide resolved
lib/web/app/auth.go Outdated Show resolved Hide resolved
lib/web/app/handler.go Outdated Show resolved Hide resolved
@r0mant r0mant requested a review from mdwn December 1, 2022 02:34
@ryanclark ryanclark force-pushed the ryan/change-app-access-login-flow branch from d5fa235 to e5ff95d Compare December 1, 2022 10:18
lib/web/app/handler.go Outdated Show resolved Hide resolved
lib/web/app/auth.go Show resolved Hide resolved
lib/web/app/handler_test.go Show resolved Hide resolved
lib/web/app/auth.go Outdated Show resolved Hide resolved
@ryanclark ryanclark force-pushed the ryan/change-app-access-login-flow branch 3 times, most recently from 428323b to 3fbbe08 Compare December 7, 2022 16:03
lib/web/app/auth.go Outdated Show resolved Hide resolved
lib/web/apiserver.go Show resolved Hide resolved
lib/web/app/handler.go Show resolved Hide resolved
lib/web/app/handler.go Show resolved Hide resolved
@ryanclark ryanclark force-pushed the ryan/change-app-access-login-flow branch 2 times, most recently from e9e4394 to 6a0c4d6 Compare December 16, 2022 18:54
lib/web/app/handler_test.go Outdated Show resolved Hide resolved
lib/web/app/handler_test.go Outdated Show resolved Hide resolved
lib/web/app/handler_test.go Outdated Show resolved Hide resolved
lib/web/app/handler_test.go Outdated Show resolved Hide resolved
lib/web/app/handler_test.go Outdated Show resolved Hide resolved
lib/web/app/handler_test.go Outdated Show resolved Hide resolved
lib/web/app/handler_test.go Outdated Show resolved Hide resolved
lib/web/app/handler_test.go Outdated Show resolved Hide resolved
lib/web/app/handler_test.go Outdated Show resolved Hide resolved
@ryanclark ryanclark force-pushed the ryan/change-app-access-login-flow branch from 6a0c4d6 to fc69f29 Compare December 19, 2022 15:30
r0mant
r0mant previously requested changes Dec 22, 2022
Copy link
Collaborator

@r0mant r0mant left a comment

Choose a reason for hiding this comment

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

@ryanclark I tried this branch in my local cluster and I can't connect to any applications. I get "Access to XXX was denied" from the browser.
Screenshot 2022-12-21 at 7 35 00 PM

@ryanclark ryanclark force-pushed the ryan/change-app-access-login-flow branch from dcd9fb4 to 639c9e5 Compare January 12, 2023 21:44
@ryanclark ryanclark dismissed r0mant’s stale review January 12, 2023 21:57

Changes addressed

@ryanclark ryanclark merged commit f2fbe92 into master Jan 12, 2023
@ryanclark ryanclark deleted the ryan/change-app-access-login-flow branch January 12, 2023 21:57
kimlisa added a commit that referenced this pull request Aug 14, 2023
Reverted parts of:
#17592

Kept json field renames
kimlisa added a commit that referenced this pull request Jan 6, 2024
Reverted parts of:
#17592

Kept json field renames
github-merge-queue bot pushed a commit that referenced this pull request Jan 12, 2024
…okie block (#30321)

* Copy pasta revert back to previous app redirection logic

Reverted parts of:
#17592

Kept json field renames

* Remove unused fields (were renamed while back)

* Refactor previous implementation

- Create unique cookie for each state token created
- Preserve path and queries

* Split handlers

* Fix tests and lint

* Address CR

* Add backwards compatability

* Emit errs with invalid vals and attempt to delete session

* Update test

* Address CRs

* Fix lint
kimlisa added a commit that referenced this pull request Feb 1, 2024
…okie block (#30321)

* Copy pasta revert back to previous app redirection logic

Reverted parts of:
#17592

Kept json field renames

* Remove unused fields (were renamed while back)

* Refactor previous implementation

- Create unique cookie for each state token created
- Preserve path and queries

* Split handlers

* Fix tests and lint

* Address CR

* Add backwards compatability

* Emit errs with invalid vals and attempt to delete session

* Update test

* Address CRs

* Fix lint
github-merge-queue bot pushed a commit that referenced this pull request Feb 12, 2024
…rty cookie block (#37692)

* Fix app redirection loop on browser's incognito mode and 3rd party cookie block (#30321)

* Copy pasta revert back to previous app redirection logic

Reverted parts of:
#17592

Kept json field renames

* Remove unused fields (were renamed while back)

* Refactor previous implementation

- Create unique cookie for each state token created
- Preserve path and queries

* Split handlers

* Fix tests and lint

* Address CR

* Add backwards compatability

* Emit errs with invalid vals and attempt to delete session

* Update test

* Address CRs

* Fix lint

* Fix import
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.

State cookie race condition when opening multiple app sessions simultaneously
7 participants