-
Notifications
You must be signed in to change notification settings - Fork 275
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
Integrates OpenSAML 4.3 with SAML authenticator #3651
Integrates OpenSAML 4.3 with SAML authenticator #3651
Conversation
e8d885f
to
64f8bd7
Compare
src/main/java/com/amazon/dlic/auth/http/saml/HTTPSamlAuthenticator.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/security/opensaml/integration/SecurityX509CRLImpl.java
Show resolved
Hide resolved
src/main/java/org/opensearch/security/opensaml/integration/CleanerCreator.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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/org/opensearch/security/opensaml/integration/CleanerCreator.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/security/opensaml/integration/CleanerCreator.java
Outdated
Show resolved
Hide resolved
.../java/org/opensearch/security/opensaml/integration/SecurityXMLObjectProviderInitializer.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Andrey Pleskach <[email protected]>
64f8bd7
to
293d2cc
Compare
@reta and @DarshitChanpura comments addressed |
Signed-off-by: Andrey Pleskach <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is a refactor of SAML, I think the existing tests should be sufficient. This looks good to me.
@reta I've resolved the conversations since you've given your approval. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @willyborankin !
@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 <[email protected]>
…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 <[email protected]> (cherry picked from commit 905c97d)
The backport to
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 |
The backport to
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 |
) 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 <[email protected]> (cherry picked from commit 44a03c5) Signed-off-by: Andrey Pleskach <[email protected]>
) 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 <[email protected]> (cherry picked from commit 44a03c5) Signed-off-by: Andrey Pleskach <[email protected]>
) 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 <[email protected]> (cherry picked from commit 44a03c5) Signed-off-by: Andrey Pleskach <[email protected]>
) 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 <[email protected]> (cherry picked from commit 44a03c5) Signed-off-by: Andrey Pleskach <[email protected]>
Backported PRs: - #2987 - #2927 - #3651 - #3690 into 2.x --------- Signed-off-by: Andrey Pleskach <[email protected]>
Description
In order to exclude
permission org.opensearch.secure_sm.ThreadPermission "modifyArbitraryThread";
we must use the dedicatedcleaners
thread factory inOpenSearch
. SinceOpenSAML
packages are sealed it it impossible to replaceCleanerSupport
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 useCleanerSupport
:X509CertificateImpl
X509CRLImpl
This fix uses the same solution as in
OpenSAML
and replacesCleanerSupport
with our custom implementationCleanerFactory
.Issues Resolved
#2989
Check List
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.