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

Add new settings for SAML and OIDC that allow for cookie splitting #3807

Merged
merged 6 commits into from
Apr 24, 2023

Conversation

cwillum
Copy link
Contributor

@cwillum cwillum commented Apr 18, 2023

Description

Two new settings are added to opensearch_dashboards.yml that allow for splitting session payloads into multiple cookies to help prevent from hitting cookie limits.

Issues Resolved

Added documentation for these settings in SAML and OIDC backend configuration.

Fixes #3691

Checklist

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

@cwillum cwillum added 2 - In progress Issue/PR: The issue or PR is in progress. backport 2.0 PR: Backport label for v2.0.x security backport 2.1 PR: Backport label for 2.1 backport 2.2 PR: Backport label for 2.2 backport 2.3 PR: Backport label for 2.3 backport 2.4 PR: Backport label for 2.4 v2.7.0 backport 2.5 PR: Backport label for 2.5 backport 2.6 PR: Backport label for 2.6 labels Apr 18, 2023
@cwillum cwillum self-assigned this Apr 18, 2023
@cwillum cwillum added 3 - Tech review PR: Tech review in progress and removed 2 - In progress Issue/PR: The issue or PR is in progress. labels Apr 19, 2023
@cwillum
Copy link
Contributor Author

cwillum commented Apr 19, 2023

@opensearch-project/security Hi team. Could you have a look at these brief additions to SAML and OIDC configuration sections? They include description of the new setting that allows you to split cookies for session management (merged in #1352.) Can you also confirm that the http.max_header_sizesetting is complete and accurate? I looked for it in opensearch.yml.example and couldn't find an example of it. Neither in documentation. Thanks.

Copy link
Member

@DarshitChanpura DarshitChanpura left a comment

Choose a reason for hiding this comment

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

@cwillum The changes look good to me!

@DarshitChanpura
Copy link
Member

@opensearch-project/security Hi team. Could you have a look at these brief additions to SAML and OIDC configuration sections? They include description of the new setting that allows you to split cookies for session management (merged in #1352.)

These changes look good to me. The description is accurate.

Can you also confirm that the http.max_header_sizesetting is complete and accurate? I looked for it in opensearch.yml.example and couldn't find an example of it. Neither in documentation. Thanks.

This description looks good to me. http.max_header_size setting applies to all components of the header and not just the token. And, as you correctly mentioned, if the ID token size overflows or cause the aggregate header-size to overflow then it might throw an exception.

@cwillum
Copy link
Contributor Author

cwillum commented Apr 21, 2023

@DarshitChanpura Big thanks for the review.

@cwillum cwillum added 4 - Doc review PR: Doc review in progress and removed 3 - Tech review PR: Tech review in progress labels Apr 21, 2023
Copy link
Collaborator

@kolchfa-aws kolchfa-aws left a comment

Choose a reason for hiding this comment

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

LGTM. Left some suggestions for clarification.

_security/authentication-backends/openid-connect.md Outdated Show resolved Hide resolved
_security/authentication-backends/openid-connect.md Outdated Show resolved Hide resolved
@cwillum cwillum added 5 - Editorial review PR: Editorial review in progress and removed 4 - Doc review PR: Doc review in progress labels Apr 24, 2023
opensearch_security.openid.extra_storage.additional_cookies: 3
```

Note that reducing the number of additional cookies can cause some of the cookies in use before the change to stop working. We recommend establishing a fixed number of additional cookies and not changing the configuration after that.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
Note that reducing the number of additional cookies can cause some of the cookies in use before the change to stop working. We recommend establishing a fixed number of additional cookies and not changing the configuration after that.
Note that reducing the number of additional cookies can cause some of the cookies that were in use before the change to stop working. We recommend establishing a fixed number of additional cookies and not changing the configuration after that.

cwillum pushed a commit that referenced this pull request Apr 27, 2023
…3807) (#3889)

* fix#3691 cookie spitting



* fix#3691 cookie spitting



* fix#3691 cookie spitting



* fix#3691 cookie spitting



* fix#3691 cookie splitting



* fix#3691 cookie spitting



---------


(cherry picked from commit 773559a)

Signed-off-by: cwillum <[email protected]>
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
@cwillum cwillum added backport 2.1 PR: Backport label for 2.1 and removed backport 2.1 PR: Backport label for 2.1 labels Apr 27, 2023
@opensearch-trigger-bot
Copy link

The backport to 2.1 failed:

The process '/usr/bin/git' failed with exit code 128

To backport manually, run these commands in your terminal:

# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/backport-2.1 2.1
# Navigate to the new working tree
pushd ../.worktrees/backport-2.1
# Create a new branch
git switch --create backport/backport-3807-to-2.1
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 773559ac755d6df14e0d49d93bceae71f644b84e
# Push it to GitHub
git push --set-upstream origin backport/backport-3807-to-2.1
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/backport-2.1

Then, create a pull request where the base branch is 2.1 and the compare/head branch is backport/backport-3807-to-2.1.

Naarcha-AWS pushed a commit that referenced this pull request Apr 27, 2023
…3807)

* fix#3691 cookie spitting

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

* fix#3691 cookie spitting

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

* fix#3691 cookie spitting

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

* fix#3691 cookie spitting

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

* fix#3691 cookie splitting

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

* fix#3691 cookie spitting

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

---------

Signed-off-by: cwillum <[email protected]>
cwillum added a commit that referenced this pull request Apr 27, 2023
cwillum added a commit that referenced this pull request Apr 27, 2023
cwillum added a commit that referenced this pull request Apr 27, 2023
cwillum added a commit that referenced this pull request Apr 27, 2023
cwillum added a commit that referenced this pull request Apr 27, 2023
cwillum added a commit that referenced this pull request Apr 27, 2023
cwillum added a commit that referenced this pull request Apr 27, 2023
cwillum added a commit that referenced this pull request Apr 27, 2023
cwillum added a commit that referenced this pull request Apr 27, 2023
cwillum added a commit that referenced this pull request Apr 27, 2023
cwillum added a commit that referenced this pull request Apr 27, 2023
cwillum added a commit that referenced this pull request Apr 27, 2023
@cwillum cwillum removed backport 2.0 PR: Backport label for v2.0.x backport 2.1 PR: Backport label for 2.1 backport 2.2 PR: Backport label for 2.2 backport 2.3 PR: Backport label for 2.3 backport 2.4 PR: Backport label for 2.4 backport 2.5 PR: Backport label for 2.5 backport 2.6 PR: Backport label for 2.6 labels Apr 27, 2023
@cwillum
Copy link
Contributor Author

cwillum commented Apr 27, 2023

  • I was mistaken about the plan to backport this feature. This feature will be released for 2.7. I've reverted all backports.

vagimeli pushed a commit that referenced this pull request May 4, 2023
…3807)

* fix#3691 cookie spitting

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

* fix#3691 cookie spitting

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

* fix#3691 cookie spitting

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

* fix#3691 cookie spitting

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

* fix#3691 cookie splitting

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

* fix#3691 cookie spitting

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

---------

Signed-off-by: cwillum <[email protected]>
vagimeli added a commit that referenced this pull request May 4, 2023
harshavamsi pushed a commit to harshavamsi/documentation-website that referenced this pull request Oct 31, 2023
…pensearch-project#3807)

* fix#3691 cookie spitting

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

* fix#3691 cookie spitting

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

* fix#3691 cookie spitting

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

* fix#3691 cookie spitting

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

* fix#3691 cookie splitting

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

* fix#3691 cookie spitting

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

---------

Signed-off-by: cwillum <[email protected]>
@hdhalter hdhalter deleted the fix#3691-split-cookie-settings branch March 28, 2024 21:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-notes PR: Include this PR in the automated release notes security v2.7.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[DOC] Document cookie split configuration parameter
6 participants