Added client certificate options to support mutual TLS for OpenID endpoint

[Simple-Analysis]

Description

Added client certificate options to support mutual TLS for OpenID endpoint. Requests to an IdP's well-known configuration endpoint that require client certificate authentication can now succeed. The HTTPS agent will use client certificate options if configured.

Category

Enhancement, Bug fix

Why these changes are required?

The HTTPS agent is not currently configured to support mutual TLS.

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

Old behavior: Requests made to the OpenID endpoints fail with 401 errors when client certificate authentication is mandatory for the IdP endpoints.
New behavior: Users can configure options to enable mutual TLS.

Issues Resolved

#1647

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
• 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.

[cwperks]

Thank you for the PR @Simple-Analysis ! Have you been able to verify this change?

Would it be possible to validate this change with a functional test similar to tests present on this PR in the opensearch-dashboards-functional-test repo?

[cwperks]

There are 2 issues impacting the CI checks:

1. Babel imports need to be updated. There's an open PR to address it and this PR will need to be re-based after that's merged: Update babel imports #1652
2. Run yarn lint:es --fix to address the linting issues.

[DarshitChanpura]

Thank you for this contribution @Simple-Analysis ! The changes look good. Could you add some tests around the usage?

[Simple-Analysis]

Thank you for this contribution @Simple-Analysis ! The changes look good. Could you add some tests around the usage?

Sure! When I have a chance, I will write some tests for this. Can you point me in a good direction for similar oauth/oidc tests? I was most likely going to adapt the opensearch security plugin's tests (if they have them) because the same requests are made but with mutual tls already integrated. What are your thoughts?

I will also run some functional tests as @cwperks mentioned. I just had some time to start work on this, so hopefully I wasn't too premature with the PR?

[cwperks]

For this change, I would be happy with unit tests but please also confirm if this works for your setup as well.

One way to test this is change the visibility of the wreckClient here, but rename it to _wreckClient (a convention for private properties in JS).

In the test suite for openid_auth.test.ts you can add a test that verifies that the wreck client is properly assigned its configured values like this:

test('Make sure that wreckClient can be configured with mTLS', async () => {
    let customConfig = ({
      openid: {
        root_ca: 'test/certs/root-ca.pem',
        header: 'authorization',
        scope: [],
      },
    } as unknown) as SecurityPluginConfigType;

    const openIdAuthentication = new OpenIdAuthentication(
      customConfig,
      sessionStorageFactory,
      router,
      esClient,
      core,
      logger
    );

    console.log("wreckClient options: " + JSON.stringify(openIdAuthentication._wreckClient.agents.https.options));

    expect('ca' in openIdAuthentication._wreckClient.agents.https.options).toEqual(true);
  });

In this case, I had to add test files and created the folders test/certs as a place to put certificates for testing.

[Simple-Analysis]

I had some time to get the OpenSearch project's development environment running and verified that the change works. I tested against my keycloak instance as well as a web server that mirrors the mTLS enforcement of the gateway.

I will update the documentation as well to reflect this change.

[Simple-Analysis]

@cwperks I was thinking about this change and if it made sense to limit the ssl options to just the openid configuration. Granted, this solves my observed problem, but I was wondering if refactoring would be necessary in the future should these options need to be reused in a different operation and/or auth type.

That said, would you want to introduce a new schema object for ssl, similar to how the opensearch security plugin does? To ensure backwards compatibility, we could use a ternary for the openid.root_ca/ssl.root_ca and openid.verify_hostnames/ssl.verify_hostnames so that the options can be defined in both locations?

[cwperks]

@Simple-Analysis I think this may only be applicable to OpenID. In both SAML and LDAP, the frontend never makes any calls to the external Identity Provider only the backend (OpenSearch) would. In those cases, there are settings to configure SSL between OpenSearch and the external Identity Provider.

If I understand correctly, for OIDC the security-dashboards-plugin calls on the OpenID Connect URL and to setup a secure connection with the wreckClient in security-dashboards-plugin to the OpenID Connect URL, a user would need to set opensearch_security.openid.root_ca in opensearch_dashboards.yml. (Looks like this setting is undocumented: https://opensearch.org/docs/latest/security/authentication-backends/openid-connect/#certificate-validation)

FYI settings in opensearch_dashboards.yml would require a reboot of OpenSearch Dashboards if you need to change a setting.

[Simple-Analysis]

@Simple-Analysis I think this may only be applicable to OpenID. In both SAML and LDAP, the frontend never makes any calls to the external Identity Provider only the backend (OpenSearch) would. In those cases, there are settings to configure SSL between OpenSearch and the external Identity Provider.

If I understand correctly, for OIDC the security-dashboards-plugin calls on the OpenID Connect URL and to setup a secure connection with the wreckClient in security-dashboards-plugin to the OpenID Connect URL, a user would need to set opensearch_security.openid.root_ca in opensearch_dashboards.yml. (Looks like this setting is undocumented: https://opensearch.org/docs/latest/security/authentication-backends/openid-connect/#certificate-validation)

FYI settings in opensearch_dashboards.yml would require a reboot of OpenSearch Dashboards if you need to change a setting.

@cwperks Okay, perfect! I will hold off on that change then. Thanks for confirming.

I did notice that it was undocumented. I was hoping to create a documentation PR for all of these SSL options. I can add documentation for the existing options as well.

Is it standard procedure to wait until the change is merged to update the docs or can they happen in parallel?

[Simple-Analysis]

On another note, I ran the unit tests manually and they seemed solid but it seems like they don't run in these pipelines?

I saw the yarn test:jest_ui --coverage for the "Run unit tests"; however, I could not find yarn test:jest_server --coverage?

[cwperks]

Is it standard procedure to wait until the change is merged to update the docs or can they happen in parallel?

Yes, they can happen in parallel. If you open a PR on the documentation-website please also link back to this PR. The documentation PR won't merge until this PR is, but it would be helpful to go through that review process so that it is ready once this PR is ready to merge.

The tests in yarn test:jest_server --coverage are run as part of the Integration Test CI check: https://github.com/opensearch-project/security-dashboards-plugin/blob/main/.github/workflows/integration-test.yml#L93

[Simple-Analysis]

@cwperks Great! I will create that documentation PR.

We are just waiting on code review for this PR right?

[cwperks]

@peternied @DarshitChanpura @RyanL1997 @derek-ho Can this PR get another review?

[Simple-Analysis]

@cwperks Great! I will create that documentation PR.

Docs PR:
opensearch-project/documentation-website#5697

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (d863368) 67.06% compared to head (a7d9cb0) 67.06%.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1650   +/-   ##
=======================================
  Coverage   67.06%   67.06%           
=======================================
  Files          94       94           
  Lines        2402     2402           
  Branches      318      318           
=======================================
  Hits         1611     1611           
  Misses        713      713           
  Partials       78       78           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[cwperks]

Thank you for your contribution @Simple-Analysis!