SOLR-15857: Add AWS Secret Manager support for ZK ACL credentials #1994

laminelam · 2023-10-10T01:01:07Z

https://issues.apache.org/jira/browse/SOLR-17020

Description

Solr currently connects to Zookeeper and manages ACLs using a set of credentials (when using Digest scheme). These credentials are either passed via the command line (system properties) or fetched from an unencrypted file on every host in the cluster. Clearly, this approach lacks sufficient security.

The proposed change of this contribution intends to integrate AWS Secret Manager for credential storage, offering two primary advantages:
• Improved security measures.
• Streamlined operations by centralizing credential storage, eliminating the need to update multiple hosts/files whenever passwords are changed.

Solution

This is a new module to support storing Zookeeper credentials in an AWS Secret Manager.

Added a new implementation of ZkCredentialsInjector that pulls the creds from AWS SM.

Ref guide in progress...

Tests

Added test cases.
Integration tests also made in AWS environment.

Checklist

Please review the following and check all that apply:

I have reviewed the guidelines for How to Contribute and my code conforms to the standards described there to the best of my ability.
I have created a Jira issue and added the issue ID to my pull request title.
I have given Solr maintainers access to contribute to my PR branch. (optional but recommended)
I have developed this patch against the main branch.
I have run ./gradlew check.
I have added tests for my changes.
I have added documentation for the Reference Guide

settings.gradle

...aws-secret-provider/src/test/org/apache/solr/secret/zk/AWSSecretCredentialsProviderTest.java

sort modules in alphabetical order

risdenk · 2023-10-11T15:39:31Z

...aws-secret-provider/src/test/org/apache/solr/secret/zk/AWSSecretCredentialsProviderTest.java

+        ZK_CREDENTIALS_INJECTOR_CLASS_NAME_VM_PARAM_NAME,
+        AllAndReadonlyCredentialZkCredentialsInjector.class.getName());
+
+    SolrZkClient zkClient =


Why so many ZK clients? Can't the one be used over and over for each create? At least most of the zk clients look to be the same. Also use try-with-resources to avoid the explicit need to close the zk clients

Hi @risdenk
This is an existing (copied) code. For some reason (race condition?) you need to close and recreate ZKClient to make the nodes creation successful. You don't want to reuse it in the last block because you want to connect to ZK with 'OPEN_ACL_UNSAFE' ACLs.

Refactored the existing code and added new tests cases to void code duplication. Created a new setUpZKNodes method.

I think AbstractDigestZkACLAndCredentialsProvidersTestBase (which BTW is not abstract) needs more refactoring. Didn't want to make this PR bigger than it already is, but we should consider favoring composition over inheritance here.

HoustonPutman

Overall I think there might be issues with the various cli tools, using the modules provided by SOLR_MODULES.

HoustonPutman · 2023-11-28T16:03:14Z

...-secret-provider/src/java/org/apache/solr/secret/zk/AWSSecretManagerCredentialsInjector.java

+import software.amazon.awssdk.services.secretsmanager.model.SecretsManagerException;
+
+public class AWSSecretManagerCredentialsInjector implements ZkCredentialsInjector {
+  public static final String SECRET_CREDENTIAL_PROVIDER_SECRET_NAME_VM_PARAM =


I'm not sure that these names make sense to me. They are not consistent with each other.

solr.zk.credentials.secret

solrZkCredentials

solr.zk.credentials.region

I think these work better.

Actually, they are not consistent because they don't have the same functionality. Some are VM variable names (internal to Solr, so I followed the same naming convention of the existing creds/acl providers), one is an AWS variable value.
How about this new version?

public static final String AWS_SM_CREDENTIALS_SECRET_NAME_VM_PARAM = "zkAWSSecretCredentialsSecretName"; private static final String AWS_SM_CREDENTIALS_REGION_VM_PARAM = "zkAWSSecretCredentialsRegion"; private static final String AWS_SM_CREDENTIALS_SECRET_NAME_DEFAULT = "/solr/zk/credentials/dev/secret";

Followed AWS SM recommended naming convention.

Edit: slashes in variable names can cause problems. I am going with this syntax instead: solr.zk.credentials.dev.secret for secret name

HoustonPutman · 2023-11-28T16:05:34Z

solr/server/scripts/cloud-scripts/zkcli.sh

+
 PATH=$JAVA_HOME/bin:$PATH $JVM $SOLR_ZK_CREDS_AND_ACLS $ZKCLI_JVM_FLAGS -Dlog4j.configurationFile=$log4j_config -Dsolr.home=$solr_home \
-classpath "$sdir/../../solr-webapp/webapp/WEB-INF/lib/*:$sdir/../../lib/ext/*:$sdir/../../lib/*" org.apache.solr.cloud.ZkCLI ${1+"$@"}
+-Dsolr.install.dir=$SOLR_DIR  -Dsolr.modules=$SOLR_MODULES -classpath "$sdir/../../solr-webapp/webapp/WEB-INF/lib/*:$sdir/../../lib/ext/*:$sdir/../../lib/*" org.apache.solr.cloud.ZkCLI ${1+"$@"}


SOLR_MODULES might not be provided, so probably want to use "${SOLR_MODULES:-}"

Also $SOLR_DIR needs to be in quotes.

The operator has to set SOLR_MODULES in zkcli.*
In the same way we expect it in solr.in.* to enable different modules.
We'll add the missing quotes.

HoustonPutman · 2023-11-28T16:07:23Z

solr/server/scripts/cloud-scripts/zkcli.sh


 solr_home="$sdir/../../solr"

+# Get solr dir with fullpath


Are the Solr Modules auto-added with the bin/solr <tool> command?

Also are you sure that ZKCLI.java and SolrCLI.java use the solr.modules option? I thought that was enabled through the solr.xml, which those tools wouldn't know about.

I am curious to find out the answer!

Thank you @HoustonPutman for looking at this

Little background: We discussed this last year if you remember as part of a bigger PR.

The issue we faced at that time was that SolrJ doesn't "see" the libraries part of the other modules as Solr default class loader does't load classes from different modules. We decided at that time to split the contrib into small ones to make that possible.

One of those contribs (that answers your question) is this one. It enables passing a custom class loader to SolrZKClient.

To answer your question:

It's not possible to rely on solr.xml for the reason explained here.

ZKCli and SolrCli don't use solr.modules directly but via NodeConfig. NodeConfig reads solr.modules passed through SOLR_MODULES in zkcli.* or solr.in.*

Hi Houston,

I have added solr modules to solr and solr.cmd.

PS: There is another way to load the modules without touching solr and solr.cmd but this requires changing a lot of "tool" classes where SolrZkClient instantiation is duplicated (the code is duplicated 12 times in 8 diff classes). I think we should extract the code instantiating the SolrZkClient to a separate class as well as considering merging ZK related tool classes.
@HoustonPutman
@epugh

I would LOVE to see that refactoring on SolrZkClient.. I've been looking more at that code base, and I think there is a LOT of opportunity for clean up. We have a LOT of tickets that are around reducing what is in solr and solr.cmd in favour of Java code, so this is a bit of a step in the wrong direction!

Hi @epugh
I have a new PR where I separated code instantiating SolrZkClient and CloudSolrClient in ZK related SolrCli tool classes.
The PR adds also a SolrClassLoader to CloudHttp2SolrClient Builder which allows to load solr modules libs without relying on solr and solr.cmd scripts.
As soon as it's approved internally will push it upstream for your review.

This is exciting!

Add double quotes

HoustonPutman

ZkCli is getting replaced by SolrCLI, so SolrCLI needs support too

HoustonPutman · 2023-12-06T18:37:20Z

...-secret-provider/src/java/org/apache/solr/secret/zk/AWSSecretManagerCredentialsInjector.java

+  public static final String AWS_SM_CREDENTIALS_SECRET_NAME_VM_PARAM =
+      "zkAWSSecretCredentialsSecretName";
+  private static final String AWS_SM_CREDENTIALS_REGION_VM_PARAM = "zkAWSSecretCredentialsRegion";
+  private static final String AWS_SM_CREDENTIALS_SECRET_NAME_DEFAULT =
+      "solr.zk.credentials.dev.secret";


zkAWSSecretCredentialsSecretName -> zkCredentialsAWSSecretName

zkAWSSecretCredentialsRegion -> zkCredentialsAWSRegion

solr.zk.credentials.dev.secret -> solr.zk.credentials.secret

HoustonPutman · 2023-12-06T18:40:59Z

solr/server/scripts/cloud-scripts/zkcli.sh

+
 PATH=$JAVA_HOME/bin:$PATH $JVM $SOLR_ZK_CREDS_AND_ACLS $ZKCLI_JVM_FLAGS -Dlog4j.configurationFile=$log4j_config -Dsolr.home=$solr_home \
-classpath "$sdir/../../solr-webapp/webapp/WEB-INF/lib/*:$sdir/../../lib/ext/*:$sdir/../../lib/*" org.apache.solr.cloud.ZkCLI ${1+"$@"}
+-Dsolr.install.dir="$SOLR_DIR" -Dsolr.modules="$SOLR_MODULES" -classpath "$sdir/../../solr-webapp/webapp/WEB-INF/lib/*:$sdir/../../lib/ext/*:$sdir/../../lib/*" org.apache.solr.cloud.ZkCLI ${1+"$@"}


"$SOLR_MODULES" -> "${SOLR_MODULES:-}"

Hi @HoustonPutman ,
I have added the changes.
As soon as you're about to approve the PR will update the ref. guide.

github-actions · 2024-02-28T00:00:46Z

This PR had no visible activity in the past 60 days, labeling it as stale. Any new activity will remove the stale label. To attract more reviewers, please tag someone or notify the dev@solr.apache.org mailing list. Thank you for your contribution!

…pport_to_store_ZK_credentials

epugh

It looks like you've added the additional support to both the zkcli and the solr scripts, which is great. There is another PR, #2298 that I hope to merge in the next few days which establishes parity between solr zk subommands and what zkcli, and I hope to remove zkcli from the main branch. If that happened, would it make sense/help you to have this support be ONLY available for the solr zk commands?

epugh · 2024-03-22T12:37:31Z

solr/modules/aws-secret-provider/README.md

+```
+
+
+-  Pass the secret name and region trough `solr.in.sh:`


epugh · 2024-03-22T12:41:42Z

solr/server/scripts/cloud-scripts/zkcli.sh


 solr_home="$sdir/../../solr"

+# Get solr dir with fullpath


I would LOVE to see that refactoring on SolrZkClient.. I've been looking more at that code base, and I think there is a LOT of opportunity for clean up. We have a LOT of tickets that are around reducing what is in solr and solr.cmd in favour of Java code, so this is a bit of a step in the wrong direction!

janhoy · 2024-04-03T20:02:05Z

Are we sure this fits as a Solr module? Since this is client-side solrj code, it could be in e.g. solrj-aws-secret instead?

laminelam · 2024-04-04T01:58:37Z

Are we sure this fits as a Solr module? Since this is client-side solrj code, it could be in e.g. solrj-aws-secret instead?

Actually, this is a server side code.
When Solr starts, it reads ZK creds from a local (clear) text file and uses them to connect to ZK.
With this feature, Solr would get the ZK creds from an AWS Secret Manager, and then proceed to connect to ZK.

Now, from SolrJ side we have 3 options:

Use the existing mechanism. Get the ZK creds and set them in System Props using the standard way.
The client can connect to AWS SM to get the creds before passing them to System Props.
This module can be used to connect directly to AWS SM and inject the creds into SolrJ. Though, the libs have to
be added to class path.

    System.setProperty("zkACLProvider", "org.apache.solr.common.cloud.DigestZkACLProvider");
    System.setProperty("zkCredentialsProvider", "org.apache.solr.common.cloud.DigestZkCredentialsProvider");
    System.setProperty("zkCredentialsInjector", "org.apache.solr.secret.zk.AWSSecretManagerCredentialsInjector");
    System.setProperty("zkCredentialsAWSSecretName", "myZkSecret");
    System.setProperty("zkCredentialsAWSRegion", "us-east-1");

    CloudSolrClient client = new CloudHttp2SolrClient.Builder(zkHosts)...

Somewhere down the line SolrZkClient will instantiate an AWSSecretManagerCredentialsInjector.

github-actions · 2024-06-06T00:00:34Z

This PR had no visible activity in the past 60 days, labeling it as stale. Any new activity will remove the stale label. To attract more reviewers, please tag someone or notify the dev@solr.apache.org mailing list. Thank you for your contribution!

epugh · 2024-09-04T15:38:49Z

I don't know that it changes anything, but at this point the zkcli.bat and zkcli.sh are no longer part of solr! Does that help at all? Also, it sounds like we had a refactoring that you were hoping to push up, did that ever make it fruition?

github-actions · 2024-11-06T00:00:31Z

This PR has had no activity for 60 days and is now labeled as stale. Any new activity will remove the stale label. To attract more reviewers, please tag people who might be familiar with the code area and/or notify the dev@solr.apache.org mailing list. To exempt this PR from being marked as stale, make it a draft PR or add the label "exempt-stale". If left unattended, this PR will be closed after another 60 days of inactivity. Thank you for your contribution!

github-actions · 2025-01-05T00:00:39Z

This PR is now closed due to 60 days of inactivity after being marked as stale. Re-opening this PR is still possible, in which case it will be marked as active again.

Add AWS Secret Manager support to store ZK credentials

95e51ce

laminelam changed the title ~~SOLR-15857: Add Secret Manager support for ZK ACL credentials~~ SOLR-15857: Add AWS Secret Manager support for ZK ACL credentials Oct 10, 2023

risdenk reviewed Oct 10, 2023

View reviewed changes

settings.gradle Outdated Show resolved Hide resolved

...aws-secret-provider/src/test/org/apache/solr/secret/zk/AWSSecretCredentialsProviderTest.java Outdated Show resolved Hide resolved

remove extra 'createTempDir'

714892d

sort modules in alphabetical order

risdenk reviewed Oct 11, 2023

View reviewed changes

Lamine Idjeraoui added 3 commits October 11, 2023 14:46

Make solrHome is mandatory for 'cmd' option

87c2a2d

Make solrhome mandatory for 'cmd' option

d21a462

Refactor test classes

a8592c1

laminelam requested a review from risdenk October 19, 2023 19:29

HoustonPutman reviewed Nov 28, 2023

View reviewed changes

Change variables names

5690ce9

Add double quotes

laminelam requested a review from HoustonPutman December 1, 2023 20:34

HoustonPutman requested changes Dec 6, 2023

View reviewed changes

add modules libs to Solr CLI

9dd9533

laminelam requested a review from HoustonPutman December 8, 2023 21:49

github-actions bot added the stale PR not updated in 60 days label Feb 28, 2024

Lamine Idjeraoui added 2 commits March 19, 2024 14:32

Merge branch 'main' into feature/SOLR-17020-Add_AWS_Secret_Manager_su…

6a3dea2

…pport_to_store_ZK_credentials

add modules libs to solr.cmd

840327c

github-actions bot added dependencies Dependency upgrades test-framework tool:build start-scripts tests cat:cloud scripts labels Mar 21, 2024

epugh reviewed Mar 22, 2024

View reviewed changes

github-actions bot removed the stale PR not updated in 60 days label Mar 26, 2024

HoustonPutman mentioned this pull request Apr 3, 2024

SOLR-17205 De-couple SolrJ Java version from server Java version #2360

Merged

This was referenced Apr 18, 2024

SOLR-17194: CloudHttp2SolrClient doesn't properly use httpClient Builder on certain paths #2378

Closed

SOLR-17248: Refactor ZK related SolrCli tools to separate SolrZkClient and CloudSolrClient instantiation/usage #2417

Merged

github-actions bot added the stale PR not updated in 60 days label Jun 6, 2024

github-actions bot removed the stale PR not updated in 60 days label Sep 6, 2024

github-actions bot added the stale PR not updated in 60 days label Nov 6, 2024

github-actions bot added the closed-stale Closed after being stale for 60 days label Jan 5, 2025

github-actions bot closed this Jan 5, 2025

SOLR-15857: Add AWS Secret Manager support for ZK ACL credentials #1994

SOLR-15857: Add AWS Secret Manager support for ZK ACL credentials #1994

Uh oh!

Conversation

laminelam commented Oct 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Solution

Tests

Checklist

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laminelam Oct 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HoustonPutman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laminelam Nov 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laminelam Nov 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laminelam Mar 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HoustonPutman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Feb 28, 2024

Uh oh!

epugh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

janhoy commented Apr 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laminelam commented Apr 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Jun 6, 2024

Uh oh!

epugh commented Sep 4, 2024

Uh oh!

github-actions bot commented Nov 6, 2024

Uh oh!

laminelam commented Oct 10, 2023 •

edited

Loading

laminelam Oct 19, 2023 •

edited

Loading

laminelam Nov 30, 2023 •

edited

Loading

laminelam Nov 28, 2023 •

edited

Loading

laminelam Mar 25, 2024 •

edited

Loading

janhoy commented Apr 3, 2024 •

edited

Loading

laminelam commented Apr 4, 2024 •

edited

Loading