Extensions config for JWT signing/encryption key

[MaciejMierzwa]

Description

This PR adds a section for extension configuration with signing/encryption keys for JWT generation.

Issues Resolved

Create dynamic configuration section in config.yml for extensions

Testing

Manual tests were performed by checking if configurations were deserialized into POJO correctly. Existing tests in: ConfigTests check if the config file is deserialized correctly

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]

Hi @MaciejMierzwa , could you please sign the commits with git commit --signoff? You can see more information about DCO here: https://github.com/opensearch-project/.github/blob/main/CONTRIBUTING.md#developer-certificate-of-origin

[cwperks]

Thank you for opening a PR @MaciejMierzwa!

Can we also add 2 helper methods on DynamicConfigFactory to get these values from the dynamic configuration?

In this abstract class there can be 2 new abstract methods:

public abstract String getExtensionsSigningKey();
public abstract String getExtensionsEncryptionKey();

There are 2 subclasses of this class: DynamicConfigModelV6 and DynamicConfigModelV7.

For V6 you can return these values as null since these are not pertinent for V6, for V7 they can be obtained by config.dynamic.extensions.signing_key and config.dynamic.extensions.encryption_key.

[cwperks]

Can you update this toString and also add toString() to the Extensions class similar to how Kibana is done?

[MaciejMierzwa]

Is there a use case for allowing this part of the configuration to be printed? Since it stores encryption/signing keys I think it would be a good idea to hide it as much as possible.

[cwperks]

That's a good point, I'm checking to see if and where it is called.

The HttpAuthenticator does print the config (See https://github.com/opensearch-project/security/blob/main/src/main/java/org/opensearch/security/securityconf/impl/v7/ConfigV7.java#L324-L331) which in the case of a JWT backend does include the signing key or other JWK settings if using a different type of encryption than HS512.

[MaciejMierzwa]

So it's ok to include this secret strings in toString() methods. Pushed my changes

[MaciejMierzwa]

Regarding comment about adding abstract methods. What do you think about just adding concrete methods into DynamicConfigModelV7 (without adding those to the abstract class)? I could also separate DynamicConfigModel abstract class into 2 interfaces, or maybe add changes to V6 class so config would be also available there.
Returning null looks like a programming smell that will bite somebody sometime in the future.

[cwperks]

@MaciejMierzwa Any V6 classes you see are for the old security configuration that supported ES6 and before. From ES7 and all versions of OpenSearch, V7 is the configuration that is utilized. For extensions related work, it will only be supported in OpenSearch so its only applicable to V7. If you added the same Extension section to ConfigV6 and referenced those in the method implementations in DynamicConfigModelV6 than that would also be ok, but it would never be used.

There are some methods that may return null if no value has been set in the config. See Kibana.opendistro_role which is set to null by default:

• https://github.com/opensearch-project/security/blob/main/src/main/java/org/opensearch/security/securityconf/impl/v6/ConfigV6.java#L91
• https://github.com/opensearch-project/security/blob/main/src/main/java/org/opensearch/security/securityconf/DynamicConfigModelV6.java#L131-L134

[cwperks]

This PR connects with a Draft PR that @RyanL1997 opened yesterday which consumes these settings in an Authenticator for the tokens that will be issued for extensions.

See https://github.com/opensearch-project/security/pull/2672/files#diff-efd217975f23af4bb84808e2771bfc85211f12b59bb619da430a3363902207ceR264-R272 for one of the locations these settings will be consumed. The other location will they will be consumed is the JWTVendor where they are used to sign the token that is issued and encrypt sensitive claims like the roles and the backend roles.

[cwperks]

@MaciejMierzwa On second thought, I wonder if it would be possible to have a single Abstract method. Something like:

public abstract Settings getDynamicExtensionsSettings();

The signing_key is used for the default symmetric HS512 signing for the JWT issued for extensions, but there is an open issue for JWK configurability: #2680

The http_authenticator config is returned as a Settings object for an http_authenticator. See below for an example JWT backend config.

jwt_auth_domain:
  enabled: true
  order: 0
  http_authenticator:
    type: jwt
    challenge: false
    config:
      signing_key: "base64 encoded key"
      jwt_header: "Authorization"
      jwt_url_parameter: null
      subject_key: null
      roles_key: null
  authentication_backend:
     type: noop

Everything in the config: area of this backend would be returned as a Settings object.

See relevant areas of the code:

1. https://github.com/opensearch-project/security/blob/main/src/main/java/org/opensearch/security/securityconf/DynamicConfigModelV7.java#L279

Settings.builder().loadFromSource(ad.getValue().http_authenticator.configAsJson(), XContentType.JSON).build()

2. https://github.com/opensearch-project/security/blob/main/src/main/java/org/opensearch/security/securityconf/impl/v7/ConfigV7.java#L324-L331

Would it be possible to make a single abstract method to return all dynamic settings for extensions?

[stephen-crawford]

Hi Maciej, thank you for this. Overall your changes look great. If you could add a couple tests to verify the expected behavior of your changes, I think that would be all that needs to be done for this to be merged. For instance, you could confirm that the config changes do in fact take place in a couple short tests.

[MaciejMierzwa]

Hi, @cwperks let me know if the changes that I made in the latest commit are more or less what you meant. I replaced method returning Settings object instead of two following methods in DynamicConfigModel:

1. public abstract String getExtensionsSigningKey();
2. public abstract String getExtensionsEncryptionKey();

[cwperks]

Hi @MaciejMierzwa, the new changes look great! Just a couple of things remaining and then this looks good to go:

1. Can you merge the latest changes from main into this branch? There was a build fixing PR that renewed some expired certs used in tests
2. As @scrawfor99 mentioned, we should add a test for this change and verify the dynamic nature of the configuration. It would also be useful to test any validation logic if the settings need to be validated. There's an example of security configuration integ tests that @lukasz-soszynski-eliatra has contributed to this repo. See https://github.com/opensearch-project/security/blob/main/src/integrationTest/java/org/opensearch/security/SecurityConfigurationTests.java#L201-L214 for an example test that updates configuration dynamically. To run integ tests locally you can use ./gradlew integrationTest --tests SecurityConfigurationTests.shouldUseSecurityAdminTool -i

[MaciejMierzwa]

Hi @cwperks, @scrawfor99, pushed in integration test that realoads cluster extensions configuration. After recent merge from main branch random commits are not recognized as signed-off. I'll probably have to nuke this PR and rebase everything.

[stephen-crawford]

Hi @MaciejMierzwa, thank you for adding tests. You should be able to rebase following the DCO output steps. This will let you overwrite the DCO issue. I went ahead and manually set this version as signed for you as well.

[RyanL1997]

Hi @MaciejMierzwa, thank you for putting these together and adding the test! In generally this looks good to me. I just initiated the rerun of the rebase. In addition, as @cwperks mentioned above, my work of authentication backend will take in these configurations.

[codecov-commenter]

Codecov Report

Merging #2671 (0de11fa) into feature/extensions (73ab1fc) will increase coverage by 0.17%.
The diff coverage is 74.05%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@                   Coverage Diff                    @@
##             feature/extensions    #2671      +/-   ##
========================================================
+ Coverage                 61.24%   61.42%   +0.17%     
- Complexity                 3337     3393      +56     
========================================================
  Files                       262      272      +10     
  Lines                     18605    18815     +210     
  Branches                   3278     3288      +10     
========================================================
+ Hits                      11395    11557     +162     
- Misses                     5607     5655      +48     
  Partials                   1603     1603              

Impacted Files	Coverage Δ
...earch/security/dlic/rest/api/MigrateApiAction.java	4.30% <0.00%> (ø)
...security/dlic/rest/api/SecuritySSLCertsAction.java	71.13% <ø> (-0.59%)	⬇️
...arch/security/securityconf/DynamicConfigModel.java	100.00% <ø> (ø)
...ch/security/securityconf/DynamicConfigModelV6.java	0.00% <0.00%> (ø)
...g/opensearch/security/support/ConfigConstants.java	94.44% <ø> (ø)
...ch/security/securityconf/DynamicConfigModelV7.java	62.26% <40.00%> (-0.73%)	⬇️
...dlic/rest/api/RestApiAdminPrivilegesEvaluator.java	75.75% <41.66%> (-2.58%)	⬇️
.../action/tenancy/TenancyConfigRetrieveResponse.java	54.16% <54.16%> (ø)
...search/security/securityconf/impl/v6/ConfigV6.java	62.37% <54.54%> (-0.96%)	⬇️
...ensearch/security/action/tenancy/EmptyRequest.java	60.00% <60.00%> (ø)
... and 19 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[stephen-crawford]

@MaciejMierzwa The spotless issue is a result of using the default encoding in the JWTVendor. It can be fixed by swapping to a non-default encoding--specifying UTF-8 etc.

[MaciejMierzwa]

Yea, I think this part of the code got to my branch during a merge. Added charset UTF-8
EDIT:
Hi @scrawfor99, @cwperks it look like breaking changes got merged to main opensearch repo with opensearch-project/OpenSearch#7165. How do we go about fixing that?

[cwperks]

@MaciejMierzwa @scrawfor99 Has a PR open in this repo to fix the issue #2715. We need to wait for the PR to be merged and then you can merge the latest changes from main into this branch to re-run the CI or one of the maintainers can also synchronize the branches assuming there are no merge conflicts.

[cwperks]

Nice! This is great that you were able to show the dynamic config settings being updated using the integration test framework. 🚀

[cwperks]

Merging this into the feature branch for support for extension REST handlers.