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

Preserve URL Hash for SAML based login #1039

Merged
merged 5 commits into from
Aug 5, 2022
Merged

Preserve URL Hash for SAML based login #1039

merged 5 commits into from
Aug 5, 2022

Conversation

devardee
Copy link
Contributor

@devardee devardee commented Jul 20, 2022

Description

In Opensearch Dashboards URL Hash encodes the state of a visualization (state could be the filters to apply, the time range etc). Currently for SAML based authentication when User logins-for-the-first-time/re-authenticates the url hash is being dropped because of redirecting to the IDP. This PR ensures that URL Hash is preserved after authenticating with the IDP.

Old Closed PR for reference : #1001.

Category

Enhancement

Why these changes are required?

This PR ensures that state of visualization remains consistent after authenticating via IDP.

What is the old behavior before changes and new behavior after changes?

In the old behavior when the dashboard plugin redirects browser to IDP the hash is lost and User looses their unsaved work. In the new behavior, before redirecting to IDP the dashboard plugin will store the hash in browser's local storage, later after the User is authenticated the hash is restored from the browser's local storage.

Issues Resolved

#543 and #831

Testing

Intergation Testing:

Manual Testing:

To Test the flow manually we need 3 servers.

  1. Opensearch Server is running with the following settings in the yml file.
plugins.security.ssl.transport.pemkey_filepath: esnode-key.pem
plugins.security.ssl.transport.pemtrustedcas_filepath: root-ca.pem
plugins.security.ssl.transport.enforce_hostname_verification: false
plugins.security.ssl.http.enabled: true
plugins.security.ssl.http.pemcert_filepath: esnode.pem
plugins.security.ssl.http.pemkey_filepath: esnode-key.pem
plugins.security.ssl.http.pemtrustedcas_filepath: root-ca.pem
plugins.security.allow_unsafe_democertificates: true
plugins.security.allow_default_init_securityindex: true
plugins.security.authcz.admin_dn:
  - CN=kirk,OU=client,O=client,L=test, C=de

plugins.security.unsupported.restapi.allow_securityconfig_modification: true
plugins.security.audit.type: internal_opensearch
plugins.security.enable_snapshot_restore_privilege: true
plugins.security.check_snapshot_restore_write_privileges: true
plugins.security.restapi.roles_enabled: ["all_access", "security_rest_api_access"]
plugins.security.system_indices.enabled: true
plugins.security.system_indices.indices: [".plugins-ml-model", ".plugins-ml-task", ".opendistro-alerting-config", ".opendistro-alerting-alert*", ".opendistro-anomaly-results*", ".opendistro-anomaly-detector*", ".opendistro-anomaly-checkpoints", ".opendistro-anomaly-detection-state", ".opendistro-reports-*", ".opensearch-notifications-*", ".opensearch-notebooks", ".opensearch-observability", ".opendistro-asynchronous-search-response*", ".replication-metadata-store"]
  1. For SAML IDP, Clone this repo and run the following commands in a separate terminal
openssl req -x509 -new -newkey rsa:2048 -nodes \
>     -subj '/C=US/ST=California/L=San Francisco/O=JankyCo/CN=Test Identity Provider' \
>     -keyout idp-private-key.pem \
>     -out idp-public-cert.pem -days 7300

node app.js --acsUrl http://localhost:5601/woq --audience https://localhost:9200
  1. Now start the opensearch dashboard server with the following settings in the dashboards.yml file:
server.host: "localhost"
server.basePath: "/woq"
server.xsrf.whitelist: ["/_opendistro/_security/saml/acs/idpinitiated", "/_opendistro/_security/saml/acs",
                        "/_opendistro/_security/saml/logout"]
opensearch.hosts: ["https://localhost:9200"]
opensearch.ssl.verificationMode: none
opensearch.username: "kibanaserver"
opensearch.password: "kibanaserver"
opensearch.requestHeadersWhitelist: [ authorization,securitytenant ]
opensearch_security.multitenancy.enabled: true
opensearch_security.multitenancy.tenants.preferred: ["Private", "Global"]
opensearch_security.readonly_mode.roles: ["kibana_read_only"]
opensearch_security.auth.type: "saml"
opensearch_security.auth.anonymous_auth_enabled: true

# Use this setting if you are running opensearch-dashboards without https
opensearch_security.cookie.secure: false
  1. Perform a PATCH request to Openserach Server to enable SAML
curl -X PATCH https://localhost:9200/_plugins/_security/api/securityconfig -H 'Content-Type: application/json' -H 'Accept: application/json' -d '[{"op": "add","path": "/config/dynamic/authc/saml_auth_domain","value": {"http_enabled": "true","transport_enabled": "false","order": 5,"http_authenticator": {"challenge": true,"type": "saml","config": {"idp": {"metadata_url": "http://localhost:7000/metadata","entity_id": "urn:example:idp"},"sp": {"entity_id": "https://localhost:9200"},"kibana_url": "http://localhost:5601/woq","exchange_key": "6aff3042-1327-4f3d-82f0-40a157ac4464"}}, "authentication_backend":{"type": "noop","config": {}}}}]' -k -u 'admin:admin'
  1. saml.jackson@example.com is the default username, that can be changed in the config of the IDP. PATCH Request to map all_access role to that user for testing purpose.
curl -X PATCH https://localhost:9200/_plugins/_security/api/rolesmapping/all_access -H 'Content-Type: application/json' -H 'Accept: application/json' -d '[{"op":"add", "path": "/users", "value": ["saml.jackson@example.com"]}]'
  1. Login as the user and download the sample data and verify if the hash is being preserved after re-logging/loginng-for-the-first-time.

HAR File:

SamlHashPreserve.har.zip

Video demonstrating the flow:

SamlHashPreserve

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.

devardee and others added 2 commits July 21, 2022 04:02
Signed-off-by: Deepak Devarakonda <devardee@amazon.com>
Revert "Replace _opendistro route with _plugins (#895)" (#1035)

Signed-off-by: Deepak Devarakonda <devardee@amazon.com>
@devardee devardee requested a review from a team July 20, 2022 23:51
@devardee devardee changed the title Raise pr 1 Preserve URL Hash for SAML based login Jul 20, 2022
@devardee
Copy link
Contributor Author

cc: @varun-lodaya

@codecov-commenter
Copy link

codecov-commenter commented Jul 21, 2022

Codecov Report

Merging #1039 (983e6f5) into main (cc5b763) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #1039   +/-   ##
=======================================
  Coverage   72.27%   72.27%           
=======================================
  Files          87       87           
  Lines        1915     1915           
  Branches      244      244           
=======================================
  Hits         1384     1384           
  Misses        478      478           
  Partials       53       53           

Help us with your feedback. Take ten seconds to tell us how you rate us.

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.

Great Addition! One small request, can you add one-liner comments to explain the functionality you added in routes.ts

Signed-off-by: Deepak Devarakonda <devardee@amazon.com>
@devardee
Copy link
Contributor Author

@DarshitChanpura, I have added comments.

Copy link
Member

@peternied peternied left a comment

Choose a reason for hiding this comment

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

Thanks for this contribution - is there a reason you'd like to add the test cases in a separate PR? I'd like to merge this fix with its associated tests at once

@varun-lodaya
Copy link
Contributor

peternied@ the plugin does not have a SAML integration test framework currently. Adding a new framework to handle redirects is very involved. devardee@ has started some work but it's taking longer than expected and we don't want to block this while waiting on the framework.

Copy link
Member

@peternied peternied left a comment

Choose a reason for hiding this comment

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

I know the manual steps were called out in the PR description, but we don't have the capacity or process to execute manual test cases, this change needs to wait until there is a way to validate it works.

Without such validation the functionality could easily be broken and we would not know why. There was a mention of trouble getting the tests up and running, please open a draft PR if you'd like to brainstorm on how to make progress in this space before adding those changes to this PR.

@peternied
Copy link
Member

About the SAML testing, when we setup the cluster for integration testing a docker instance is used for OpenSearch. By expanding on the docker-compose example with a test SAML IdP this might be an easier to integrate into tests into the CI systems see link.

},
});
if (redirectHash) {
console.log('The server base path is : ' + this.coreSetup.http.basePath.serverBasePath);
Copy link
Member

Choose a reason for hiding this comment

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

Please remove this if not needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ack

devardee and others added 2 commits July 28, 2022 20:09
Signed-off-by: Deepak Devarakonda <devardee@amazon.com>
@devardee
Copy link
Contributor Author

devardee commented Aug 2, 2022

@peternied , @DarshitChanpura and @cliu123 I have addressed the comments, can you please take another look

@devardee devardee requested a review from peternied August 4, 2022 16:08
@peternied
Copy link
Member

#1044 is nearly there, lets get those updates and include them in this PR so we can merge everything at once.

Copy link
Member

@peternied peternied left a comment

Choose a reason for hiding this comment

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

Merging, seperately we will see about getting the tests integrated

@peternied peternied merged commit a9d10d8 into opensearch-project:main Aug 5, 2022
@peternied peternied added the backport 2.2 backport to 2.2 branch label Aug 5, 2022
opensearch-trigger-bot bot pushed a commit that referenced this pull request Aug 5, 2022
* Preserve URL HASH after user logs via SAML IDP

Co-authored-by: Darshit Chanpura <35282393+DarshitChanpura@users.noreply.github.com>
(cherry picked from commit a9d10d8)
peternied pushed a commit that referenced this pull request Aug 5, 2022
* Preserve URL HASH after user logs via SAML IDP

Co-authored-by: Darshit Chanpura <35282393+DarshitChanpura@users.noreply.github.com>
(cherry picked from commit a9d10d8)

Co-authored-by: Deepak Devarakonda <80896069+devardee@users.noreply.github.com>
This was referenced Aug 30, 2022
@cwperks cwperks added the backport 2.x backport to 2.x branch label Sep 19, 2022
opensearch-trigger-bot bot pushed a commit that referenced this pull request Sep 19, 2022
* Preserve URL HASH after user logs via SAML IDP

Co-authored-by: Darshit Chanpura <35282393+DarshitChanpura@users.noreply.github.com>
(cherry picked from commit a9d10d8)
peternied pushed a commit that referenced this pull request Sep 21, 2022
* Preserve URL HASH after user logs via SAML IDP

Co-authored-by: Darshit Chanpura <35282393+DarshitChanpura@users.noreply.github.com>
(cherry picked from commit a9d10d8)

Co-authored-by: Deepak Devarakonda <80896069+devardee@users.noreply.github.com>
expani pushed a commit to expani/security-dashboards-plugin that referenced this pull request Oct 6, 2022
* Preserve URL HASH after user logs via SAML IDP

Co-authored-by: Darshit Chanpura <35282393+DarshitChanpura@users.noreply.github.com>
(cherry picked from commit a9d10d8)
Signed-off-by: Aniketh Jain <anijainc@amazon.com>
opensearch-trigger-bot bot pushed a commit that referenced this pull request Nov 16, 2022
* Preserve URL HASH after user logs via SAML IDP

Co-authored-by: Darshit Chanpura <35282393+DarshitChanpura@users.noreply.github.com>
(cherry picked from commit a9d10d8)
cwperks added a commit that referenced this pull request Dec 1, 2022
* SAML Integration Tests (#1088)

* Preserve URL Hash for SAML based login (#1039)

* Preserve URL HASH after user logs via SAML IDP

Signed-off-by: Deepak Devarakonda <devardee@amazon.com>
Signed-off-by: Aniketh Jain <anijainc@amazon.com>
Signed-off-by: Craig Perkins <cwperx@amazon.com>
Co-authored-by: anijain-Amazon <110471048+anijain-Amazon@users.noreply.github.com>
Co-authored-by: Deepak Devarakonda <devardee@amazon.com>
Co-authored-by: Deepak Devarakonda <80896069+devardee@users.noreply.github.com>
Co-authored-by: Darshit Chanpura <35282393+DarshitChanpura@users.noreply.github.com>
cliu123 added a commit to cliu123/security-dashboards-plugin that referenced this pull request Jan 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 1.3 backport 2.x backport to 2.x branch backport 2.2 backport to 2.2 branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants