Fix app redirection loop on browser's incognito mode and 3rd party cookie block by kimlisa · Pull Request #30321 · gravitational/teleport

kimlisa · 2023-08-11T05:16:44Z

Updated RFD that explains the flow: #31103

The cookie failure issue, is not a problem if an app is accessed by the default teleport subdomain. It becomes a problem if user sets a public _addr for the app where the domain is not teleports.

Given SiteA (teleport) and SiteB (domain of app), the current implementation, responded with app cookies from SiteB, when the current site context was still on SiteA. To the browser this is considered 3rd party cookie and is blocked by default for the following major browsers (reproduced it too):

firefox: for both incognito and normal modes
safari: for both incognito and normal modes
brave
chrome: only incognito, however, google will be phasing out 3rd party cookie by mid next year (2024)

Our previous implementation that used the oauth state exchange flow was not affected by this problem because we set the app cookies while we are in the app domain.

This PR reverts some parts of the current implementation with the previous implementation with some small refactors, notably the state token race condition which was resolved by creating unique cookies for each state token created.

Tested

Tested the following by updating teleport versions (from my reproduction steps) that includes this PR's changes:

with updated cluster and app service still running previous version
github login

Local testing to test parameters are getting saved:

redirecting with github login and local login with this URL: https://grafana-test-127-0-0-1.nip.io:3080/alerting/list?search=state:inactive%20type:alerting%20health:nodata

With Browsers:

firefox both incognito and normal
as suggested by google, tested in chrome by running the following in CLI /Applications/Google\ Chrome.app/Contents/MacOS/Google\ Chrome --test-third-party-cookies-phaseout

Reproduction

Running version: Teleport Enterprise v15.0.0-dev git:pull-4093-g68d8647cd6 go1.21.3 (before this PR)

Create two ec2 instances
On the main cluster, install certbot, then create two certs (one for cluster and one for app) and include them in main teleport config under proxy_service.https_keypairs (note: need to add inbound rule for port 80)
start teleport with config
point custom domain (kimlisa.app) to this cluster's ip address (eg: 3.83.9.149):

ec2 instance 1 (main cluster) cfg:

version: v3
teleport:
  nodename: ip-172-31-93-149
  data_dir: /var/lib/teleport
  log:
    output: stderr
    severity: INFO
    format:
      output: text
  ca_pin: ""
  diag_addr: ""
auth_service:
  enabled: "yes"
  listen_addr: 0.0.0.0:3025
  cluster_name: root.3.83.9.149.nip.io
  license_file: /var/lib/teleport/license.pem
  proxy_listener_mode: multiplex
  authentication:
    type: local
    second_factor: off
ssh_service:
  enabled: "yes"
  commands:
  - name: hostname
    command: [hostname]
    period: 1m0s
proxy_service:
  enabled: "yes"
  web_listen_addr: 0.0.0.0:443
  public_addr: root.3.83.9.149.nip.io:443
  https_keypairs:
    - key_file: /etc/letsencrypt/live/app1.kimlisa.app/privkey.pem
      cert_file: /etc/letsencrypt/live/app1.kimlisa.app/fullchain.pem
    - key_file: /etc/letsencrypt/live/root.3.83.9.149.nip.io/privkey.pem
      cert_file: /etc/letsencrypt/live/root.3.83.9.149.nip.io/fullchain.pem
  https_keypairs_reload_interval: 0s
  acme: {}

ec2 instance 2 for app.

run whatever app (mine is running on http://localhost:4000)
copy and paste the output from running ./tctl tokens add in ec2 instance 1
manually add a public-addr flag to custom domain

ubuntu@ip:~/teleport/e/build$ sudo ./tctl tokens add \
--type=app \
--app-name=test-app-1 \
--app-uri=http://localhost:4000
The invite token: 3c3...
This token will expire in 30 minutes.

Fill out and run this command on a node to make the application available:

> teleport app start \
   --token=3c3... \
   --ca-pin=sha256:80... \
   --auth-server=root.3.83.9.149.nip.io:443 \
   --name=test-app-1  \
   --uri=http://localhost:4000  \
   --public-addr=app1.kimlisa.app

Changelog: Fix app redirection loop on browser's incognito mode and 3rd party cookie block

gzdunek · 2023-08-17T16:18:02Z

Uh, this stuff is so complex. I looked through it and it looks fine, but I'm not very familiar that part, so I could have missed some things.

Also, do you think we need a security review here?

kimlisa · 2023-08-21T16:12:08Z

Also, do you think we need a security review here?

not likely, this PR is just a revert to the old implementation, the only thing that changed was that instead of overwriting our state token cookie with new requests, we create a unique state token cookie per request (short lived 1 minute)

kimlisa · 2023-08-21T16:12:18Z

friendly ping @ryanclark

kimlisa · 2023-08-21T16:20:43Z

Actually I will be updating the RFD to explain this flow to help make this whole flow more understandable

zmb3 · 2023-08-21T16:13:18Z

+
+	nonce, err := utils.CryptoRandomHex(auth.TokenLenBytes)
+	if err != nil {
+		h.log.WithError(err).Debugf("Failed to generate and encode random numbers.")


If I was running a Teleport cluster and I saw this in my logs I would have no idea what it means. Can we add a better message?

(Also, we use the exact same message above, so this adds addition burden for figuring out which line triggered the log message)

@kimlisa I guess you didn't fix all the messages. This one is still cryptic.

zmb3 · 2023-08-21T16:15:41Z

+	SetRedirectPageHeaders(w.Header(), nonce)
+
+	// Serving the HTML page.
+	fmt.Fprintf(w, appRedirectionJs, nonce)


nit: rename this constant

appRedirectPage or appRedirectHTML would work

zmb3 · 2023-08-21T16:19:13Z

+	SetRedirectPageHeaders(w.Header(), nonce)
+
+	// Serving the HTML page.
+	fmt.Fprintf(w, appRedirectionJs, nonce)


This is probably fine (we control the input), but you do have to be very careful using printf on HTML (it's prone to injection). Using html/template instead would be better, since it's HTML-aware and not a generic string replacement.

zmb3 · 2023-08-21T16:19:38Z

+
+// completeAppAuthExchange completes the auth exchange flow started by "startAppAuthExchange" handler
+// by validating the values passed in the request body, and upon success sets cookies required
+// for the current app session. User should be able to interact with the app now.


Suggested change

// for the current app session. User should be able to interact with the app now.

// for the current app session.

zmb3 · 2023-08-21T16:20:38Z

 	httplib.SetNoCacheHeaders(w.Header())
+	var req fragmentRequest
+	if err := json.NewDecoder(r.Body).Decode(&req); err != nil {
+		return trace.Wrap(err)


Since the primary reason this would fail is invalid JSON, what do you think about trace.BadParameter instead?

Or trace.AccessDenied, which is what you use below.

zmb3 · 2023-08-21T16:26:13Z

-	h.router.POST("/x-teleport-auth", makeRouterHandler(h.withCustomCORS(h.handleAuth)))
-	h.router.OPTIONS("/x-teleport-auth", makeRouterHandler(h.withCustomCORS(nil)))
+	h.router.GET("/x-teleport-auth", makeRouterHandler(h.startAppAuthExchange))
+	h.router.POST("/x-teleport-auth", makeRouterHandler(h.completeAppAuthExchange))


Do we need to keep the OPTIONS call available for a bit?

As written, app access may break during upgrades when not all proxies are on the same version.

how long will proxy versions differ though? (when ryan's changes went it, the app breakage wasn't taken into account)

if we were to handle preventing temporary breakage, I would need to make POST /x-teleport-auth differentiate between cors request with custom headers and non-cors request with state query param and auth cookie

i added backwards compat starting from this commit: cc691d0

tested with v15 build using new and legacy AppLauncher.tsx component with https://grafana-test-127-0-0-1.nip.io:3080/alerting/list?search=state:inactive%20type:alerting%20health:nodata

zmb3 · 2023-08-21T16:28:08Z

+	u := url.URL{
+		Scheme: "https",
+		Host:   proxyPublicAddr,
+		Path:   fmt.Sprintf("/web/launch/%s", hostname),


Use url.JoinPath instead of sprintf here?

i left it as is, b/c i don't think we need the extra functionality of url.JoinPath (cleaning paths). the url.JoinPath kinda complicates things by returning an error. if you feel strongly on it, i'll make it happen.

Just wanted to point out that url.JoinPath performs URL path encoding and path normalization (i.e. resolution, removal of redundant slashes) while Sprintf takes everything as-is.
On a different note, what happens if the path query parameter does not start with a /?

zmb3 · 2023-08-21T16:31:15Z

-		RawQuery: query,
+	// Presence of a stateToken means we are beginning an app auth exchange.
+	if req.stateToken != "" {
+		u.RawQuery = fmt.Sprintf("state=%s&path=%s", url.QueryEscape(req.stateToken), url.QueryEscape(req.path))


Similar comment - what you have is fine (you were careful to properly escape the values), but why not use url.Values to encode the string rather than sprintf?

var v url.Values v.Add("state", req.stateToken) v.Add("path", req.path) u.RawQuery = v.Encode()

github-actions · 2023-10-27T06:07:12Z

The PR changelog entry failed validation: Changelog entry not found in the PR body. Please add a "no-changelog" label to the PR, or changelog lines starting with changelog: followed by the changelog entries for the PR.

phosphore · 2023-11-24T15:51:28Z

@@ -210,31 +210,6 @@ func SetIndexContentSecurityPolicy(h http.Header, cfg proto.Features, urlPath st
 	h.Set("Content-Security-Policy", cspString)


Not be strictly related to this PR, but you should consider implementing a report-uri.

phosphore · 2023-11-24T15:54:32Z

+	}
+	if subtle.ConstantTimeCompare([]byte(secretToken), []byte(stateCookie.Value)) != 1 {
+		h.log.Warn("Request failed: state token does not match.")
+		return trace.AccessDenied("access denied")


See #31103 (comment), should this incident be audit logged?

emitted errors and attempt deleting app session starting with this commit: d24ddbb

Reverted parts of: #17592 Kept json field renames

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

ibeckermayer

I would need to spend a lot of time understanding context to give this a useful review. I'm happy to do that if necessary, but it seems better suited to somebody more well acquainted with app access and/or your #31103 RFD.

ibeckermayer · 2024-01-08T19:12:55Z

 	return trace.Wrap(metaRedirectTemplate.Execute(w, redirectURL))
 }
+
+const appRedirectHTML = `


Consider something similar to the line above

teleport/lib/web/app/redirect.go

Line 43 in de6bf60

var metaRedirectTemplate = template.Must(template.New("meta-redirect").Parse(metaRedirectHTML))

such that this template doesn't need to be re-parsed each time the respective endpoint is hit.

bl-nero · 2024-01-09T18:20:21Z

+
+	nonce, err := utils.CryptoRandomHex(auth.TokenLenBytes)
+	if err != nil {
+		h.log.WithError(err).Debugf("Failed to generate and encode random numbers.")


@kimlisa I guess you didn't fix all the messages. This one is still cryptic.

bl-nero · 2024-01-09T18:39:08Z

+		Path:     "/",
+		HttpOnly: true,
+		Secure:   true,
+		SameSite: http.SameSiteNoneMode,


Do we really want the "none" mode here, and not Strict?

i think it is required because of the same reason stated here: https://github.com/gravitational/teleport/pull/30321/files#diff-379535a3878805eca5813cd9aa07db7a254cb885df354203105c4984eabafd83R181

bl-nero · 2024-01-09T18:39:35Z

+		Path:     "/",
+		HttpOnly: true,
+		Secure:   true,
+		SameSite: http.SameSiteNoneMode,


bl-nero · 2024-01-09T18:41:18Z

+func (h *Handler) emitErrorEventAndDeleteAppSession(r *http.Request, f emitErrorEventFields) {
+	// Attempt to delete app session.
+	if f.sessionID != "" {
+		_ = h.c.AuthClient.DeleteAppSession(r.Context(), types.DeleteAppSessionRequest{


Perhaps we should log the error, even if we skip it?

bl-nero · 2024-01-09T18:59:11Z

 	// Cookies). Given this, we should only redirect to it when this format is
 	// already in use.
-	if !HasSession(r) {
+	if p.stateToken == "" && !HasSession(r) {


I don't quite get what this code path means. Could you double-check whether the error message below is still appropriate? What does "this format is already in use" mean in the comment above?

i think the error message, is missing a comma, does it make more sense if written like this:

"redirecting to launcher when using client certificate, is not valid"

you can access application through the CLI like this: https://goteleport.com/docs/application-access/cloud-apis/aws-console/#step-89-access-aws-cli

so i think from the CLI when you run into a forwaring error, it wouldn't make sense to redirect the user to the launcher from the cli, so we just return an error instead

but if the request came from the web, there'd be a cookie attached to it (HasSession), and if there is a cookie, then we can redirect

i'll add a clarifying comment

bl-nero · 2024-01-09T19:04:51Z

+      const url = getXTeleportAuthUrl({ fqdn, port });
+      url.searchParams.set('state', stateToken);
+      url.searchParams.set('subject', session.subjectCookieValue);
+      url.hash = `#value=${session.cookieValue}`;


Are we sure that whatever there is in the cookieValue variable doesn't need to be URL-escaped?

it should be safe since the cookieValue is the session id, which is a hex string

bl-nero · 2024-01-09T19:11:48Z

@kimlisa One general comment: I'm really impressed by this PR, it's really well-thought and VERY well commented. Though it's a very complex subject, the code wasn't at all difficult to understand. Just a handful of small issues, and you'll have my approval.

kimlisa · 2024-01-10T05:41:13Z

@kimlisa One general comment: I'm really impressed by this PR, it's really well-thought and VERY well commented. Though it's a very complex subject, the code wasn't at all difficult to understand. Just a handful of small issues, and you'll have my approval.

@bl-nero don't be impressed. this PR is mostly a revert of a previous implementation (i think it was charn and alexey who were the original creators). i just renamed/restructured some things and added bunch of comments because everyone was confused (especially me) 😅

r0mant

@kimlisa Let's get this in so we can cover this during v15 test plan.

…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

…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

kimlisa force-pushed the lisa/revert-and-fix-app-redirection branch from d1801e6 to 4de4101 Compare August 14, 2023 18:35

kimlisa marked this pull request as ready for review August 14, 2023 18:36

github-actions Bot requested review from gzdunek and ryanclark August 14, 2023 18:36

github-actions Bot added size/md ui labels Aug 14, 2023

zmb3 reviewed Aug 21, 2023

View reviewed changes

kimlisa mentioned this pull request Aug 28, 2023

Update app access RFD #31103

Merged

kimlisa requested a review from zmb3 August 28, 2023 21:00

zmb3 requested a review from jentfoo August 28, 2023 21:04

mdwn self-assigned this Aug 29, 2023

jakule self-requested a review August 29, 2023 18:19

kimlisa marked this pull request as draft October 27, 2023 18:01

phosphore reviewed Nov 24, 2023

View reviewed changes

kimlisa added 9 commits January 5, 2024 09:51

Copy pasta revert back to previous app redirection logic

598ffd4

Reverted parts of: #17592 Kept json field renames

Remove unused fields (were renamed while back)

93fefb9

Refactor previous implementation

4f73d52

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

Split handlers

129b7b4

Fix tests and lint

8dcbbf5

Address CR

92faced

Add backwards compatability

cc691d0

Emit errs with invalid vals and attempt to delete session

d24ddbb

Update test

de6bf60

kimlisa force-pushed the lisa/revert-and-fix-app-redirection branch from 89371e8 to de6bf60 Compare January 6, 2024 04:11

github-actions Bot added the size/lg label Jan 6, 2024

github-actions Bot requested review from avatus and ibeckermayer January 6, 2024 04:13

ibeckermayer reviewed Jan 8, 2024

View reviewed changes

bl-nero reviewed Jan 9, 2024

View reviewed changes

kimlisa requested a review from bl-nero January 10, 2024 05:41

kimlisa added 2 commits January 9, 2024 21:44

Address CRs

d2290be

Fix lint

6f11e0a

bl-nero approved these changes Jan 12, 2024

View reviewed changes

r0mant approved these changes Jan 12, 2024

View reviewed changes

public-teleport-github-review-bot Bot removed request for avatus, gzdunek, jakule, jentfoo and ryanclark January 12, 2024 18:49

Merge branch 'master' into lisa/revert-and-fix-app-redirection

49518f9

kimlisa enabled auto-merge January 12, 2024 22:20

kimlisa added this pull request to the merge queue Jan 12, 2024

Merged via the queue into master with commit 43eab8e Jan 12, 2024

kimlisa deleted the lisa/revert-and-fix-app-redirection branch January 12, 2024 23:07

This was referenced Feb 1, 2024

[v14] Fix app redirection loop on browser's incognito mode and 3rd party cookie block #37692

Merged

[v13] Fix app redirection loop on browser's incognito mode and 3rd party cookie block #37698

Merged

kimlisa mentioned this pull request May 6, 2024

Web: Remove legacy app access support #41243

Merged

kimlisa mentioned this pull request Jun 11, 2024

Applications with public_addr different than Teleport proxy based domain are not working #33138

Closed

	// for the current app session. User should be able to interact with the app now.
	// for the current app session.

		@@ -210,31 +210,6 @@ func SetIndexContentSecurityPolicy(h http.Header, cfg proto.Features, urlPath st
		h.Set("Content-Security-Policy", cspString)

Conversation

kimlisa commented Aug 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Tested

Reproduction

Uh oh!

gzdunek commented Aug 17, 2023

Uh oh!

kimlisa commented Aug 21, 2023

Uh oh!

kimlisa commented Aug 21, 2023

Uh oh!

kimlisa commented Aug 21, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

phosphore Nov 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

github-actions Bot commented Oct 27, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ibeckermayer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bl-nero commented Jan 9, 2024

Uh oh!

kimlisa commented Jan 10, 2024

kimlisa commented Aug 11, 2023 •

edited

Loading

phosphore Nov 24, 2023 •

edited

Loading