Backport cookie splitter #1662

adrian-golawski-eliatra · 2023-11-21T11:06:04Z

Description

This Pull Request backports cookie-splitter functionality together with later regression fixes to 1.x branch of Security Plugin.

Why these changes are required?

It was a response to a user's demand

1567 - issue where the need of backport is discussed

What is the old behavior before changes and new behavior after changes?

If the user was granted a large number of roles (>100), their size of the cookie could pass the upper limit defined by the browsers (4093 bytes). This PR solves the issue by splitting them in multiple cookie values.

Described in details in the 1352 - The original cookie splitting PR

Issues Resolved

It's a backport.

Related Issues:

Related PRs:

Testing

Backported functionality comes with the tests

Check List

New functionality includes testing
New functionality has been documented
Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: Jochen Kressin <[email protected]>

…n set within the current request Signed-off-by: Jochen Kressin <[email protected]>

codecov · 2023-11-21T12:34:46Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (0319257) 72.48% compared to head (a870d54) 72.48%.

❗ Current head a870d54 differs from pull request most recent head 2022b66. Consider uploading reports for the commit 2022b66 to get more accurate results

Additional details and impacted files

@@           Coverage Diff           @@
##              1.x    #1662   +/-   ##
=======================================
  Coverage   72.48%   72.48%           
=======================================
  Files          88       88           
  Lines        1926     1926           
  Branches      251      246    -5     
=======================================
  Hits         1396     1396           
  Misses        474      474           
  Partials       56       56

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

leanneeliatra · 2023-12-04T15:33:16Z

The fix has been tested with OS version 1.3.12 and the cookie is successfully split into multiple cookies when the size of the cookie is too large.
In the photo attached you can see the SAML cookie is split into multiple cookies once the cookie got too large, for testing purposes I changed export const MAX_LENGTH_OF_COOKIE_BYTES = 4000; to export const MAX_LENGTH_OF_COOKIE_BYTES = 300;

See the successful cookie splitting in the image below:

The Jest integration test can be found here: 2b49375, it appears to have been moved but I would like to get feedback from the team at this point, As the issue is fixed and tests have been wrote, opinions on how to proceed would be favoured.

This PR is ready to be moved from Draft into Ready to get feedback on the changes.

DarshitChanpura

looks good to me.

DarshitChanpura · 2023-12-05T18:14:36Z

As the issue is fixed and tests have been wrote, opinions on how to proceed would be favoured.

@leanneeliatra is there anything in particular that you were looking for?

leanneeliatra · 2023-12-05T19:01:28Z

As the issue is fixed and tests have been wrote, opinions on how to proceed would be favoured.

@leanneeliatra is there anything in particular that you were looking for?

Thank you @DarshitChanpura, yes just guidance really. Can this issue be backported and merged now given I have confirmed it's working and the selenium tests were updated (as linked) or do we need to add more coverage? I'm not sure if what has been done is sufficient as the test was updated. Or is there a need to get all checks passing above for merging. Thanks.

cwperks · 2023-12-05T19:35:37Z

@leanneeliatra What's blocking this PR is passing integ tests. I believe that 1.x hasn't gotten the same fix that 1.3 has yet. This line may need to be added to 1.x.

Can you push a commit to add that line in this PR?

Signed-off-by: [email protected] <[email protected]>

leanneeliatra · 2023-12-06T17:32:13Z

@leanneeliatra What's blocking this PR is passing integ tests. I believe that 1.x hasn't gotten the same fix that 1.3 has yet. This line may need to be added to 1.x.

Can you push a commit to add that line in this PR?

Hi @cwperks thank you, that is done!

leanneeliatra · 2023-12-06T17:43:35Z

Hopefully this will now be ready to be merged & complete the backport process.

cwperks · 2023-12-06T18:44:20Z

@leanneeliatra Looks like the 1.x branch is out of sync and needs to be version bumped.

In the security repo, the 1.x branch is given version 1.4 even if it will never be released. Should this repo also point to 1.4.0.0?

cwperks · 2023-12-06T22:01:19Z

@leanneeliatra I opened up a PR to update the 1.x branch: #1688

leanneeliatra · 2023-12-07T10:51:54Z

Thanks a million Craig for creating the a PR to update the 1.x branch: #1688

Do you mean for this Should this repo also point to 1.4.0.0? to point the PR to merge back into 1.4.0 (rather than opensearch-project:1.x)

Or, do you mean I should ensure these changes run locally on 1.4.0.0? I tested this on 1.3.12
Thanks for all your help on this.

cwperks · 2023-12-07T14:18:48Z

@leanneeliatra According to the branching strategy of opensearch, the .x branches should contain the next unreleased minor version. Since the last minor version of 2.x released was 2.11, that means that 2.x contains 2.12 now.

1.x is a branch that's supposed to be ready for release at any moment for the 1.x line even if there are no planned releases. The last released 1.x minor version was 1.3 which means that 1.x should point to 1.4. The security plugin's build.gradle file in 1.x points to 1.4.

Its unrelated to the changes in this PR, but it may have been missed because the version increment automation did not exist at the time that 1.3 was released.

leanneeliatra · 2023-12-07T14:34:23Z

@leanneeliatra According to the branching strategy of opensearch, the .x branches should contain the next unreleased minor version. Since the last minor version of 2.x released was 2.11, that means that 2.x contains 2.12 now.

Okay thank you @cwperks thanks for the references. So from reading I think a solution here would be to rebase off 1.x? To ensure all changes from 1.4 are included? Does that sound correct?

DarshitChanpura · 2023-12-07T16:19:28Z

Okay thank you @cwperks thanks for the references. So from reading I think a solution here would be to rebase off 1.x? To ensure all changes from 1.4 are included? Does that sound correct?

A simple merge will suffice. And I have merged that now. CI should pass now that @cwperks's PR has been merged

cwperks · 2023-12-07T20:36:35Z

The integ tests are experiencing a new error.

simple-git has supported promises / async await since version 2.6.0.
 Importing from 'simple-git/promise' has been deprecated and will
 report this error until the next major release of version 4.
To upgrade, change all 'simple-git/promise' imports to just 'simple-git'

Edit: nvm, it is showing on the latest run of 1.3 too: https://github.com/opensearch-project/security-dashboards-plugin/actions/runs/7036213187/job/19148332875

leanneeliatra · 2023-12-08T15:38:12Z

From what I can see on that PR [1.3] Bump debug and browserify-sign dependencies #1674 the checks seem to be okay? So I think I should address the CI in my own PR and make the edit suggested To upgrade, change all 'simple-git/promise' imports to just 'simple-git what do you think @cwperks? Just to make sure I haven't misunderstood your last comment.

leanneeliatra · 2023-12-11T13:47:51Z

The integ tests are experiencing a new error.
simple-git has supported promises / async await since version 2.6.0.
 Importing from 'simple-git/promise' has been deprecated and will
 report this error until the next major release of version 4.
To upgrade, change all 'simple-git/promise' imports to just 'simple-git'
Edit: nvm, it is showing on the latest run of 1.3 too: https://github.com/opensearch-project/security-dashboards-plugin/actions/runs/7036213187/job/19148332875

Should I fix the error locally @cwperks? Thanks

derek-ho · 2023-12-11T17:56:40Z

@leanneeliatra I don't think that is the culprit I think it is more likely:

Node.js process-warning detected:

UnhandledPromiseRejectionWarning: TypeError: Cannot read property 'error' of undefined
    at emitWarning (internal/process/promises.js:99:15)
    at emitPromiseRejectionWarnings (internal/process/promises.js:143:7)
    at process._tickCallback (internal/process/next_tick.js:69:34)```
    
Do the integration tests work on your local?

cwperks · 2023-12-18T20:00:18Z

FYI The CI check failures are an issue that need to be resolved. There is a difference between 1.3 and 2.x/main that explains the CI failures. In 1.3, init() is called with the constructor of the authenticator and was refactored in #1110 (not backported to the 1.x line).

https://github.com/opensearch-project/security-dashboards-plugin/blob/1.3/server/auth/types/openid/openid_auth.ts#L84

The init call within the constructor is giving the UnhandledPromise error because that is an async function and the error is not being handled (hence UnhandledPromise). Parts of #1110 will need to be backported to this PR.

Edit: Opened a PR that incorporates the changes needed from the multiauth PR: #1702

CI checks failing

Signed-off-by: Jochen Kressin <[email protected]>

Signed-off-by: Craig Perkins <[email protected]>

jochen-kressin · 2023-12-27T12:29:24Z

Hi @cwperks,

I've been trying to wrap my head around the issue here, and while testing locally I stumbled on an alternative solution that does not require multiple changes related to the init method.

I realized that in the tests on the 2.x branch we don't really care about what happens in the init method.
So I tested with overriding the init method in the openid test: 85db227#diff-88fa528fff81cd4fcdef46499478b53f8e803ba16a76fcab1120ae4f015685aaR66

Then, just to add a little bit more context to the test, I run createExtraStorage (which would otherwise be run in the init method) "manually". Not really needed, but maybe it helps spot errors early in the future.
I also added some mocking to have that method run through. This way, the test does not fail when OpenId is trying to get the endpoints.

I figured this way, we don't have to refactor a bunch of code just to make the tests play nice.
But my apologies if I've forgotten - maybe there where other reasons for the changes to init() in #1702 ?
Otherwise, what do you think about this approach?

The tests for OpenId and SAML now run through locally, but I think there were some updates from your PR that may still need to be merged here, e.g. "Stabilize SAML integ test on 1.3". So any advice on that would be appreciated.
I thought it best to clarify the overall approach here with you first.

Signed-off-by: Jochen Kressin <[email protected]>

jochen-kressin · 2023-12-27T12:47:06Z

The integration tests ran through before the last linting commit. I think the one integration test that failed on this run is the one that you've stabilized in your PR. Can I just merge that branch into this PR?

cwperks · 2023-12-27T15:00:18Z

Otherwise, what do you think about this approach?

Looks good to me and it minimizes the amount of changes on the 1.x line. The only argument I can think of backporting the portion of init changes from the multiauth PR is that it will make backporting from main -> 1.x/1.3 easier, but I agree that we should minimize the amount of changes needed for this backport.

Signed-off-by: Craig Perkins <[email protected]>

jochen-kressin · 2023-12-31T18:01:46Z

Thanks for the response @cwperks, and sorry for interrupting the holidays.
I went ahead and cherry picked the changes you did to stabilize the flakey SAML tests, and after a push the integration tests turned green again.

DarshitChanpura · 2024-01-03T16:21:24Z

.github/actions/install-dashboards/action.yml

@@ -48,7 +48,7 @@ runs:
    - id: branch-switch-if-possible
      continue-on-error: true # Defaults onto main if the branch switch doesn't work
      if: ${{ steps.osd-version.outputs.osd-version }}
-      run: git checkout ${{ steps.osd-version.outputs.osd-version }} || git checkout ${{ steps.osd-version.outputs.osd-x-version }}x
+      run: git checkout 1.3.14 || git checkout ${{ steps.osd-version.outputs.osd-version }} || git checkout ${{ steps.osd-version.outputs.osd-x-version }}x


why hardcode: git checkout 1.3.14?

I'm not sure, and maybe that one should go. I cherry picked that commit from Craig's PR, but maybe that was more of a PoC? Without it, the tests would not run, although I can't remember what the error was.
@cwperks Do you know if this was a temporary fix, and what the alternative should be?

If I remember correctly, this was added temporarily because the 1.x branch was stale and #1688 was not merged yet. Since the branch was stale, it was trying to check out an older patch release of OSD core so I added this line in.

Hi @DarshitChanpura is the comment you were awaiting addressed above please, just checking? Thank you.

This line should be removed then I think since the branch is not longer stale.

waiting for a comment to be addressed.

DarshitChanpura · 2024-01-18T15:47:06Z

@jochen-kressin @leanneeliatra Would you need any help from the maintainers to get this through?

leanneeliatra · 2024-01-19T08:24:35Z

@jochen-kressin @leanneeliatra Would you need any help from the maintainers to get this through?

I can make that change @DarshitChanpura, hopefully then we are good to go. cc fyi @jochen-kressin

Thank you for the prod.

…on callout. Signed-off-by: [email protected] <[email protected]>

leanneeliatra · 2024-01-23T10:46:24Z

@DarshitChanpura that change is now complete.

DarshitChanpura · 2024-01-23T15:25:45Z

CI will be unblocked post merge of #1698

DarshitChanpura · 2024-01-31T18:54:53Z

@leanneeliatra @jochen-kressin The CI for 1.x is blocked currently pending 1.4.0 artifact builds to run. Can we rebase this to 1.3 and get it merged there directly? and then forward-port it to 1.x if needed?

cc: @cwperks

pc-jedi · 2024-03-20T07:15:41Z

Backport to 1.3.x was done in #1831

DarshitChanpura · 2024-03-20T18:27:53Z

Closing this PR as it will be covered via: #1752

jochen-kressin added 2 commits November 14, 2023 13:54

Backport cookie compression

01931ad

Signed-off-by: Jochen Kressin <[email protected]>

The cookie splitter should be able to use cookie values that have bee…

21e7a42

…n set within the current request Signed-off-by: Jochen Kressin <[email protected]>

leanneeliatra mentioned this pull request Nov 28, 2023

[BUG] opensearch login not working with more than 15 roles assign to user #1567

Closed

adrian-golawski-eliatra marked this pull request as ready for review December 5, 2023 09:13

DarshitChanpura previously approved these changes Dec 5, 2023

View reviewed changes

cwperks added the backport 1.3 label Dec 5, 2023

cwperks previously approved these changes Dec 5, 2023

View reviewed changes

adding config as per 1x branch and craig p instructions

f6f7a89

Signed-off-by: [email protected] <[email protected]>

Merge branch '1.x' into backport-cookie-splitter

9c3a8f6

cwperks mentioned this pull request Dec 18, 2023

[Backport 1.x] Split up a value into multiple cookie payloads #1702

Closed

3 tasks

Mocking for the tests

85db227

Signed-off-by: Jochen Kressin <[email protected]>

Checkout 1.3.14 tag

9330781

Signed-off-by: Craig Perkins <[email protected]>

Lint fixes

38bb292

Signed-off-by: Jochen Kressin <[email protected]>

Stabilize SAML integ test on 1.3

a870d54

Signed-off-by: Craig Perkins <[email protected]>

DarshitChanpura reviewed Jan 3, 2024

View reviewed changes

removing unneeded version from action.yml. Reverting to dynamic versi…

2022b66

…on callout. Signed-off-by: [email protected] <[email protected]>

cwperks mentioned this pull request Mar 18, 2024

[FEATURE] Downport 'Handle too large cookies gracefully' fix to OS 1.3.x opensearch-project/security#4114

Closed

DarshitChanpura closed this Mar 20, 2024

Backport cookie splitter #1662

Backport cookie splitter #1662

Conversation

adrian-golawski-eliatra commented Nov 21, 2023

Description

Category

Why these changes are required?

What is the old behavior before changes and new behavior after changes?

Issues Resolved

Testing

Check List

codecov bot commented Nov 21, 2023 • edited Loading

Codecov Report

leanneeliatra commented Dec 4, 2023 • edited Loading

DarshitChanpura left a comment

Choose a reason for hiding this comment

DarshitChanpura commented Dec 5, 2023

leanneeliatra commented Dec 5, 2023

cwperks commented Dec 5, 2023

leanneeliatra commented Dec 6, 2023 • edited Loading

leanneeliatra commented Dec 6, 2023

cwperks commented Dec 6, 2023

cwperks commented Dec 6, 2023

leanneeliatra commented Dec 7, 2023

cwperks commented Dec 7, 2023

leanneeliatra commented Dec 7, 2023 • edited Loading

DarshitChanpura commented Dec 7, 2023

cwperks commented Dec 7, 2023 • edited Loading

leanneeliatra commented Dec 8, 2023 • edited Loading

leanneeliatra commented Dec 11, 2023

derek-ho commented Dec 11, 2023

cwperks commented Dec 18, 2023 • edited Loading

jochen-kressin commented Dec 27, 2023

jochen-kressin commented Dec 27, 2023

cwperks commented Dec 27, 2023

jochen-kressin commented Dec 31, 2023

DarshitChanpura Jan 3, 2024

Choose a reason for hiding this comment

jochen-kressin Jan 9, 2024

Choose a reason for hiding this comment

cwperks Jan 9, 2024

Choose a reason for hiding this comment

leanneeliatra Jan 11, 2024

Choose a reason for hiding this comment

DarshitChanpura Jan 13, 2024

Choose a reason for hiding this comment

DarshitChanpura commented Jan 18, 2024

leanneeliatra commented Jan 19, 2024 • edited Loading

leanneeliatra commented Jan 23, 2024

DarshitChanpura commented Jan 23, 2024 • edited Loading

DarshitChanpura commented Jan 31, 2024 • edited Loading

pc-jedi commented Mar 20, 2024

DarshitChanpura commented Mar 20, 2024

codecov bot commented Nov 21, 2023 •

edited

Loading

leanneeliatra commented Dec 4, 2023 •

edited

Loading

leanneeliatra commented Dec 6, 2023 •

edited

Loading

leanneeliatra commented Dec 7, 2023 •

edited

Loading

cwperks commented Dec 7, 2023 •

edited

Loading

leanneeliatra commented Dec 8, 2023 •

edited

Loading

cwperks commented Dec 18, 2023 •

edited

Loading

leanneeliatra commented Jan 19, 2024 •

edited

Loading

DarshitChanpura commented Jan 23, 2024 •

edited

Loading

DarshitChanpura commented Jan 31, 2024 •

edited

Loading