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 suppression for all removal warnings #1828

Merged
merged 2 commits into from
May 23, 2022

Conversation

peternied
Copy link
Member

Description

Use of the SecurityManager and AccessController have been deprecated and
will be removed in java versions after 17. While this is an issue its
also one that will take a concerted effort to resolve. These warning
messages making discovering build errors and other warnings more
difficult; hence adding this supression logic.

For tracking the effort to replace these components look into opensearch-project/OpenSearch#1687

Issues Resolved

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.

Use of the SecurityManager and AccessController have been deprecated and
will be removed in java versions after 17.  While this is an issue its
also one that will take a concerted effort to resolve.  These warning
messages making discovering build errors and other warnings more
difficult; hence adding this supression logic.

For tracking the effort to replace these components look into opensearch-project/OpenSearch#1687

Signed-off-by: Peter Nied <[email protected]>
@peternied peternied requested a review from a team May 6, 2022 16:08
@peternied peternied self-assigned this May 6, 2022
@codecov-commenter
Copy link

codecov-commenter commented May 6, 2022

Codecov Report

Merging #1828 (05321e8) into main (e189b7a) will decrease coverage by 0.00%.
The diff coverage is n/a.

@@             Coverage Diff              @@
##               main    #1828      +/-   ##
============================================
- Coverage     60.81%   60.80%   -0.01%     
+ Complexity     3187     3185       -2     
============================================
  Files           253      253              
  Lines         17931    17943      +12     
  Branches       3204     3205       +1     
============================================
+ Hits          10904    10910       +6     
- Misses         5450     5454       +4     
- Partials       1577     1579       +2     
Impacted Files Coverage Δ
...ic/auth/http/jwt/AbstractHTTPJwtAuthenticator.java 55.81% <ø> (ø)
...mazon/dlic/auth/http/jwt/HTTPJwtAuthenticator.java 84.90% <ø> (ø)
...ic/auth/http/kerberos/HTTPSpnegoAuthenticator.java 0.00% <ø> (ø)
...dlic/auth/http/saml/AuthTokenProcessorHandler.java 47.28% <ø> (ø)
...zon/dlic/auth/http/saml/HTTPSamlAuthenticator.java 69.72% <ø> (ø)
...zon/dlic/auth/http/saml/Saml2SettingsProvider.java 61.53% <ø> (ø)
...auth/http/saml/SamlFilesystemMetadataResolver.java 0.00% <ø> (ø)
.../dlic/auth/http/saml/SamlHTTPMetadataResolver.java 62.96% <ø> (ø)
...ic/auth/ldap/backend/LDAPAuthorizationBackend.java 57.57% <ø> (ø)
...ava/com/amazon/dlic/auth/ldap/util/LdapHelper.java 61.53% <ø> (ø)
... and 25 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e189b7a...05321e8. Read the comment docs.

DarshitChanpura
DarshitChanpura previously approved these changes May 9, 2022
Copy link
Member

@cliu123 cliu123 left a comment

Choose a reason for hiding this comment

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

A couple of questions here:

  1. Why removing warning message instead of disabling Java Security Manager?
  2. Quoted from the issue:

We could keep it as long as we can, but once removed from the JDK, it will be a problem.

If disabling JSM, what will be the problem?

@peternied
Copy link
Member Author

Thanks for the question @cliu123

Why removing warning message instead of disabling Java Security Manager?

We cannot disable the java security manager without exposing a risk that our or other plugins will do something they are not privileged to do. For example, another plugin could reflect on static configuration settings and then change the admin cert to whatever they would like (yikes!) by using the security manager there is a production build into running plugins within our JVM instance.

These messages are not useful making it hard to troubleshoot build errors, as these errors are embedded in the design of OpenSearch. Deprecation messages are useful if we were adding a component that we did not realize was deprecated and wanted to gate that dependency. You raise a good point, I'll push a change in the setting of our build to treat warnings as errors.

@cliu123
Copy link
Member

cliu123 commented May 17, 2022

Deprecation messages are useful if we were adding a component that we did not realize was deprecated and wanted to gate that dependency.

Exactly, JSM being deprecated should be warned in this case, shouldn't it? OpenSearch core doesn't suppress these warns either.

@peternied
Copy link
Member Author

@cliu123 This pull request doesn't eliminate deprecation messages, it suppresses them on components that we have no plans to fix. It is confusing for contributors or even maintainers such as myself to see 'warnings' in our build output that we should actively ignore, which is why I am suppressing warnings on these components.

If a new warning was to be added in a pull request we would have no mechanism to detect it. By suppressing these messages and failing builds on any new messages this puts the codebase in a better posture to prevent new dependencies on deprecated/removed features.

Is there a reason to keep the existing warnings as they are today?

@cliu123 cliu123 self-requested a review May 19, 2022 23:03
@cliu123
Copy link
Member

cliu123 commented May 19, 2022

Thanks for the clarification!

This pull request doesn't eliminate deprecation messages, it suppresses them on components that we have no plans to fix.

I have a couple of questions here:

  1. Will the deprecation messages still show? Could you elaborate more in the PR description:
    What exactly deprecatio messages are these changes suppressing? Is it the following?
WARNING: System::setSecurityManager will be removed in a future release
  1. Given that JSM is deprecated and will be removed, why shouldn't security plugin warn users this?
    @reta @nknize @dblock might have more context on this. How are these deprecation warnings handled in OpenSearch core where JSM is also being used?

@reta
Copy link
Collaborator

reta commented May 20, 2022

@cliu123 a few clarifications:

  1. Will the deprecation messages still show?

Yes, for JDK-17 and above you will get these warnings at runtime :( (even trickier for JDK-18+)

  1. Given that JSM is deprecated and will be removed, why shouldn't security plugin warn users this?

We have this large story [1] to clarify the path forward. OpenSearch heavily relies on SecurityManager at the moment and the fact that there is just no replacement makes things difficult (surely, dropping the SecurityManager sounds like the easiest option but not the best one).

[1] opensearch-project/OpenSearch#1687

@cliu123
Copy link
Member

cliu123 commented May 23, 2022

I see. Thanks for the clarification! @peternied @reta

@cliu123 cliu123 merged commit 1d89d1c into opensearch-project:main May 23, 2022
@peternied peternied deleted the suppress-removal branch May 25, 2022 17:31
stephen-crawford pushed a commit to stephen-crawford/security that referenced this pull request Nov 10, 2022
wuychn pushed a commit to ochprince/security that referenced this pull request Mar 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants