Integrates OpenSAML 4.3 with SAML authenticator by willyborankin · Pull Request #3651 · opensearch-project/security

willyborankin · 2023-11-04T14:16:12Z

Description

In order to exclude permission org.opensearch.secure_sm.ThreadPermission "modifyArbitraryThread"; we must use the dedicated cleaners thread factory in OpenSearch. Since OpenSAML packages are sealed it it impossible to replace CleanerSupport class with our own solution. Instead we have to change configuration of how such objects should be parsed. There are only 2 classes in the library which use CleanerSupport:

X509CertificateImpl
X509CRLImpl

This fix uses the same solution as in OpenSAML and replaces CleanerSupport with our custom implementation CleanerFactory.

Issues Resolved

#2989

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.

src/main/java/com/amazon/dlic/auth/http/saml/HTTPSamlAuthenticator.java

src/main/java/org/opensearch/security/opensaml/integration/SecurityX509CRLImpl.java

src/main/java/org/opensearch/security/opensaml/integration/CleanerCreator.java

DarshitChanpura

Thank you for opening this PR @willyborankin. A general request: could you please add description to all new classes to help understand the role of each class?

Also, would you mind adding some tests for this?

src/main/java/com/amazon/dlic/auth/http/saml/HTTPSamlAuthenticator.java

src/main/java/org/opensearch/security/opensaml/integration/CleanerCreator.java

.../java/org/opensearch/security/opensaml/integration/SecurityXMLObjectProviderInitializer.java

Signed-off-by: Andrey Pleskach <ples@aiven.io>

willyborankin · 2023-11-06T20:15:48Z

@reta and @DarshitChanpura comments addressed

Signed-off-by: Andrey Pleskach <ples@aiven.io>

stephen-crawford

Since this is a refactor of SAML, I think the existing tests should be sufficient. This looks good to me.

DarshitChanpura · 2023-11-07T15:14:22Z

@reta I've resolved the conversations since you've given your approval.

DarshitChanpura

Thank you @willyborankin !

DarshitChanpura · 2023-11-07T15:17:32Z

@willyborankin Is this ready to be backported to 2.x?

willyborankin · 2023-11-07T17:17:06Z

@willyborankin Is this ready to be backported to 2.x?

@DarshitChanpura lets see how it will behave with other tests. If it is ok lets add the label.

### Description We have added Spotless formatting rules already, this pull request adds import ordering to Spotless formatting rules ### Issues Resolved Follow up on #3651 ### Testing N/A ### Check List - [ ] New functionality includes testing - [ ] New functionality has been documented - [X] 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](https://github.com/opensearch-project/OpenSearch/blob/main/CONTRIBUTING.md#developer-certificate-of-origin). Signed-off-by: Andriy Redko <andriy.redko@aiven.io>

…3666) We have added Spotless formatting rules already, this pull request adds import ordering to Spotless formatting rules Follow up on opensearch-project#3651 N/A - [ ] New functionality includes testing - [ ] New functionality has been documented - [X] 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](https://github.com/opensearch-project/OpenSearch/blob/main/CONTRIBUTING.md#developer-certificate-of-origin). Signed-off-by: Andriy Redko <andriy.redko@aiven.io> (cherry picked from commit 905c97d)

opensearch-trigger-bot · 2023-11-08T20:12:57Z

The backport to 2.11 failed:

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

To backport manually, run these commands in your terminal:

# Navigate to the root of your repository
cd $(git rev-parse --show-toplevel)
# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/security/backport-2.11 2.11
# Navigate to the new working tree
pushd ../.worktrees/security/backport-2.11
# Create a new branch
git switch --create backport/backport-3651-to-2.11
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 44a03c5a5929357a2e936b45dd754085fd282f0f
# Push it to GitHub
git push --set-upstream origin backport/backport-3651-to-2.11
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/security/backport-2.11

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

opensearch-trigger-bot · 2023-11-08T20:15:24Z

The backport to 2.x failed:

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

To backport manually, run these commands in your terminal:

# Navigate to the root of your repository
cd $(git rev-parse --show-toplevel)
# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/security/backport-2.x 2.x
# Navigate to the new working tree
pushd ../.worktrees/security/backport-2.x
# Create a new branch
git switch --create backport/backport-3651-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 44a03c5a5929357a2e936b45dd754085fd282f0f
# Push it to GitHub
git push --set-upstream origin backport/backport-3651-to-2.x
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/security/backport-2.x

Then, create a pull request where the base branch is 2.x and the compare/head branch is backport/backport-3651-to-2.x.

) In order to exclude `permission org.opensearch.secure_sm.ThreadPermission "modifyArbitraryThread";` we must use the dedicated `cleaners` thread factory in `OpenSearch`. Since `OpenSAML` packages are sealed it it impossible to replace `CleanerSupport` class with our own solution. Instead we have to change configuration of how such objects should be parsed. There are only 2 classes in the library which use `CleanerSupport`: - `X509CertificateImpl` - `X509CRLImpl` This fix uses the same solution as in `OpenSAML` and replaces `CleanerSupport` with our custom implementation `CleanerFactory`. Signed-off-by: Andrey Pleskach <ples@aiven.io> (cherry picked from commit 44a03c5) Signed-off-by: Andrey Pleskach <ples@aiven.io>

Backported PRs: - #2987 - #2927 - #3651 - #3690 into 2.x --------- Signed-off-by: Andrey Pleskach <ples@aiven.io>

willyborankin requested review from DarshitChanpura, RyanL1997, cliu123, cwperks, davidlago, peternied, reta and stephen-crawford as code owners November 4, 2023 14:16

willyborankin mentioned this pull request Nov 4, 2023

OpenSAML 4.3 integration #3644

Closed

3 tasks

willyborankin force-pushed the opensaml-fix-permission branch 6 times, most recently from e8d885f to 64f8bd7 Compare November 4, 2023 20:53

reta reviewed Nov 6, 2023

View reviewed changes

src/main/java/com/amazon/dlic/auth/http/saml/HTTPSamlAuthenticator.java Outdated Show resolved Hide resolved

reta reviewed Nov 6, 2023

View reviewed changes

src/main/java/org/opensearch/security/opensaml/integration/SecurityX509CRLImpl.java Show resolved Hide resolved

reta reviewed Nov 6, 2023

View reviewed changes

src/main/java/org/opensearch/security/opensaml/integration/CleanerCreator.java Outdated Show resolved Hide resolved

DarshitChanpura reviewed Nov 6, 2023

View reviewed changes

OpenSAML 4.3 integration

293d2cc

Signed-off-by: Andrey Pleskach <ples@aiven.io>

willyborankin force-pushed the opensaml-fix-permission branch from 64f8bd7 to 293d2cc Compare November 6, 2023 20:14

Fixed java docs

92cb857

Signed-off-by: Andrey Pleskach <ples@aiven.io>

stephen-crawford approved these changes Nov 6, 2023

View reviewed changes

reta approved these changes Nov 7, 2023

View reviewed changes

DarshitChanpura approved these changes Nov 7, 2023

View reviewed changes

DarshitChanpura changed the title ~~OpenSAML 4.3 integration~~ Integrates OpenSAML 4.3 with SAML authenticator Nov 7, 2023

DarshitChanpura merged commit 44a03c5 into opensearch-project:main Nov 7, 2023

reta mentioned this pull request Nov 7, 2023

Add import ordering to Spotless formatting rules #3666

Merged

3 tasks

willyborankin added the backport 2.11 Backport to 2.11 branch label Nov 8, 2023

opensearch-trigger-bot bot added the backport-failed label Nov 8, 2023

willyborankin added backport 2.x and removed backport 2.11 Backport to 2.11 branch backport-failed labels Nov 8, 2023

opensearch-trigger-bot bot added the backport-failed label Nov 8, 2023

willyborankin mentioned this pull request Nov 8, 2023

[Backport 2.x] Backport OpenSAML 4.3.0 to 2.x #3671

Merged

cwperks pushed a commit that referenced this pull request Dec 6, 2023

[Backport 2.x] Backport OpenSAML 4.3.0 to 2.x (#3671)

d7b10bb

Backported PRs: - #2987 - #2927 - #3651 - #3690 into 2.x --------- Signed-off-by: Andrey Pleskach <ples@aiven.io>

DarshitChanpura removed the backport-failed label Jul 30, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Integrates OpenSAML 4.3 with SAML authenticator#3651

Integrates OpenSAML 4.3 with SAML authenticator#3651
DarshitChanpura merged 2 commits intoopensearch-project:mainfrom
willyborankin:opensaml-fix-permission

willyborankin commented Nov 4, 2023 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

DarshitChanpura left a comment •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

willyborankin commented Nov 6, 2023

Uh oh!

stephen-crawford left a comment

Uh oh!

DarshitChanpura commented Nov 7, 2023 •

edited

Loading

Uh oh!

DarshitChanpura left a comment

Uh oh!

DarshitChanpura commented Nov 7, 2023

Uh oh!

willyborankin commented Nov 7, 2023

Uh oh!

opensearch-trigger-bot bot commented Nov 8, 2023

Uh oh!

opensearch-trigger-bot bot commented Nov 8, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

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

Uh oh!

Description

Issues Resolved

Check List

Uh oh!

Uh oh!

Uh oh!

Uh oh!

DarshitChanpura left a comment • 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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

willyborankin commented Nov 6, 2023

Uh oh!

stephen-crawford left a comment

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

DarshitChanpura left a comment

Choose a reason for hiding this comment

Uh oh!

DarshitChanpura commented Nov 7, 2023

Uh oh!

willyborankin commented Nov 7, 2023

Uh oh!

opensearch-trigger-bot bot commented Nov 8, 2023

Uh oh!

opensearch-trigger-bot bot commented Nov 8, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

willyborankin commented Nov 4, 2023 •

edited

Loading

DarshitChanpura left a comment •

edited

Loading

DarshitChanpura commented Nov 7, 2023 •

edited

Loading