Split access token into half and store to avoid "securecookie: the value is too long" error

[yubofredwang]

Tracking issue

#3750

Why are the changes needed?

Fix "securecookie: the value is too long" error

What changes were proposed in this pull request?

Split the access token in half and store each one in a different cookie.

How was this patch tested?

Used internally at Linkedin.

Setup process

Screenshots

Check all the applicable boxes

• I updated the documentation accordingly.
• All new and existing tests passed.
• All commits are signed-off.

Related PRs

Docs link

[codecov]

Codecov Report

Attention: Patch coverage is 65.78947% with 13 lines in your changes are missing coverage. Please review.

Project coverage is 59.10%. Comparing base (d68047a) to head (3d11035).
Report is 26 commits behind head on master.

Files	Patch %	Lines
flyteadmin/auth/cookie_manager.go	65.78%	9 Missing and 4 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4863      +/-   ##
==========================================
+ Coverage   58.56%   59.10%   +0.54%     
==========================================
  Files         494      645     +151     
  Lines       42987    55573   +12586     
==========================================
+ Hits        25176    32849    +7673     
- Misses      15816    20133    +4317     
- Partials     1995     2591     +596     

Flag	Coverage Δ
unittests	59.10% <65.78%> (+0.54%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[gdabisias]

@yubofredwang do you mind fixing the linting issue so that this passes all tests and we could merge it? (and sign off the commit?)

[davidmirror-ops]

Just FYI as I'm aware this PR is a WIP, a user is reporting the following error after using the solution proposed here:
{"json":{"src":"cookie.go:87","x-request-id":"7ad600c65f77a6ea6122172fa0ae240b"},"level":"error","msg":"Error reading existing secure cookie [flyte_idt]. Error: [SECURE_COOKIE_ERROR] Error reading secure cookie flyte_idt, caused by: securecookie: base64 decode failed - caused by: illegal base64 data at input byte 10","ts":"2024-02-20T13:17:18Z"}

[yubofredwang]

let me take a look another look at this issue.

[eapolinario]

Just FYI as I'm aware this PR is a WIP, a user is reporting the following error after using the solution proposed here: {"json":{"src":"cookie.go:87","x-request-id":"7ad600c65f77a6ea6122172fa0ae240b"},"level":"error","msg":"Error reading existing secure cookie [flyte_idt]. Error: [SECURE_COOKIE_ERROR] Error reading secure cookie flyte_idt, caused by: securecookie: base64 decode failed - caused by: illegal base64 data at input byte 10","ts":"2024-02-20T13:17:18Z"}

This PR doesn't touch that cookie, right, @davidmirror-ops ? Do you have more details about this error?

[davidmirror-ops]

@eapolinario

This PR doesn't touch that cookie, right,
that seems to be the case.

@DevendraJohari24 is this still an issue in your environment?

[davidmirror-ops]

I just had a call with Devendra and the error message illegal base64 data at input byte 10 was not caused by the work on this PR. It was solved by giving the Ingress cookie a different name to avoid collisions with the flyte_idt one

[eapolinario]

@yubofredwang , we did a round of testing using this in one of our Flyte clusters and it worked beautifully. Can we get this PR out of draft mode?

[wild-endeavor]

@yubofredwang actually, because of the potential impact of this, would you be willing to make this a backwards compatible change? It's okay to log people out... but the issue is that many people run multiple Admin pods (most people who aren't on single binary deployment do anyways). The concern is that during rollout, a newer version of admin with this change logs a user out, but then it redirects to an older admin (which hasn't been shut down yet). That older admin issues another cookie, and then if the user again hits the newer admin, they'll again be logged out. This is only a concern during the rollout process.

Can be mitigated by just renaming the new cookie, and checking for both the old cookie and the new pair of cookies. What do you think? If you're willing to make this change, we can run our internal test again.

Ty so much and sorry to be dragging this out.

[eapolinario]

Thank you!