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

Remove deprecated route and endpoint #5066

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

willyborankin
Copy link
Collaborator

@willyborankin willyborankin commented Jan 29, 2025

Description

Removed:

  • /_opendistro route
  • /_opendistro/kibanainfo endpoint

Testing

[Please provide details of testing done: unit testing, integration testing and manual testing]

Check List

  • New functionality includes testing
  • New functionality has been documented
  • New Roles/Permissions have a corresponding security dashboards plugin PR
  • API changes companion pull request created
  • 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.

@willyborankin willyborankin force-pushed the remove-opendistro-path branch from 9ee8334 to 6d4cfb1 Compare January 29, 2025 16:57
@cwperks
Copy link
Member

cwperks commented Jan 29, 2025

@willyborankin To fully complete removing _opendistro from the SAML route would also need a chance to security-dashboards-plugin.

Something similar to opensearch-project/security-dashboards-plugin#895 which was reverted due to not having a backend PR merged in simultaneously.

There were 2 backend PRs after the break:

@shikharj05
Copy link
Contributor

Kindly check - #5031 (comment)

@willyborankin willyborankin force-pushed the remove-opendistro-path branch 4 times, most recently from 724cf6b to 650f7d4 Compare January 29, 2025 18:52
@willyborankin willyborankin changed the title Clean up for 3.x Remove deprecated paths Jan 29, 2025
@willyborankin willyborankin force-pushed the remove-opendistro-path branch from 650f7d4 to b403d43 Compare January 29, 2025 20:18
@willyborankin willyborankin changed the title Remove deprecated paths Remove deprecated route and endpoint Jan 29, 2025
@willyborankin willyborankin force-pushed the remove-opendistro-path branch 2 times, most recently from 02ad520 to 6025216 Compare January 29, 2025 20:55
Copy link

codecov bot commented Jan 29, 2025

Codecov Report

Attention: Patch coverage is 83.33333% with 1 line in your changes missing coverage. Please review.

Project coverage is 71.45%. Comparing base (cac7743) to head (7f85017).

Files with missing lines Patch % Lines
...zon/dlic/auth/http/saml/Saml2SettingsProvider.java 50.00% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #5066      +/-   ##
==========================================
- Coverage   71.47%   71.45%   -0.03%     
==========================================
  Files         336      336              
  Lines       22625    22623       -2     
  Branches     3601     3601              
==========================================
- Hits        16171    16165       -6     
- Misses       4657     4660       +3     
- Partials     1797     1798       +1     
Files with missing lines Coverage Δ
...dlic/auth/http/saml/AuthTokenProcessorHandler.java 50.28% <ø> (ø)
...zon/dlic/auth/http/saml/HTTPSamlAuthenticator.java 67.51% <ø> (ø)
.../opensearch/security/OpenSearchSecurityPlugin.java 83.79% <ø> (ø)
...pensearch/security/auditlog/impl/AuditMessage.java 78.70% <100.00%> (ø)
...earch/security/dlic/rest/api/AccountApiAction.java 98.59% <ø> (ø)
...nsearch/security/dlic/rest/api/AuditApiAction.java 90.76% <ø> (-3.08%) ⬇️
...rch/security/dlic/rest/api/WhitelistApiAction.java 100.00% <ø> (ø)
...g/opensearch/security/dlic/rest/support/Utils.java 59.45% <100.00%> (ø)
...opensearch/security/filter/SecurityRestFilter.java 84.32% <ø> (ø)
...nsearch/security/http/OnBehalfOfAuthenticator.java 89.38% <ø> (ø)
... and 8 more

... and 9 files with indirect coverage changes

@@ -221,9 +221,9 @@ private SingleLogoutService findSingleLogoutService(IDPSSODescriptor idpSsoDescr
private String buildAssertionConsumerEndpoint(String dashboardsRoot) {

if (dashboardsRoot.endsWith("/")) {
return dashboardsRoot + "_opendistro/_security/saml/acs";
return dashboardsRoot + "_security/saml/acs";
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
return dashboardsRoot + "_security/saml/acs";
return dashboardsRoot + "_plugins/_security/saml/acs";

This instance and the one below both need to start with _plugins/. I think I saw another instance above as well. Can you make sure the prefix still exists, but _opendistro is replaced with _plugins?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed

@willyborankin willyborankin force-pushed the remove-opendistro-path branch from 6025216 to 7f85017 Compare January 31, 2025 16:01
 Removed:
 - `/_opendistro` route
 - `/_opendistro/kibanainfo` endpoint

Signed-off-by: Andrey Pleskach <[email protected]>
@@ -36,23 +36,23 @@
* SuperAdmin certificate for the default superuser is stored as a kirk.pem file in config folder of OpenSearch
* <p>
* Example calling the PUT API as SuperAdmin using curl (if http basic auth is on):
* curl -v --cacert path_to_config/root-ca.pem --cert path_to_config/kirk.pem --key path_to_config/kirk-key.pem -XPUT https://localhost:9200/_opendistro/_security/api/whitelist -H "Content-Type: application/json" -d’
* curl -v --cacert path_to_config/root-ca.pem --cert path_to_config/kirk.pem --key path_to_config/kirk-key.pem -XPUT https://localhost:9200/_security/api/whitelist -H "Content-Type: application/json" -d’
Copy link
Member

@cwperks cwperks Jan 31, 2025

Choose a reason for hiding this comment

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

There's other instances in this file too. I see a few others across the files covered in this PR that need to be updated.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I plan to remove the White List API in a follow-up PR, as the Allow List now fully replaces it.

Copy link
Member

Choose a reason for hiding this comment

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

Got it. Do we need to introduce another API like the migrate API to handle cases where a cluster may still be referencing whitelist.yml?

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.

3 participants