[v2.12.0] Ensure CI/documentation reflect changes to default admin credentials

[derek-ho]

Background

Previously, when installing the security plugin demo configuration, the cluster was spun up with the default admin credentials, admin:admin. A change was made in main and backported to 2.x for the 2.12.0 release, which now requires an initial admin password to be passed in via the environment variable OPENSEARCH_INITIAL_ADMIN_PASSWORD. This will break some CI/testing that relies on OpenSearch to come up without setting this environment variable. This tracking issue is to ensure compliance with the new changes.

Coming from: opensearch-project/security#3624

Acceptance Criteria

• All documentation references to the old default credentials admin:admin are removed
• Ensure that CI/testing is working with main and 2.x branches

[dbwiddis]

I just searched the code and we do not rely on admin credentials. The only equivalent we have is in our IT's where we set a (non-working) default here:

flow-framework/src/test/java/org/opensearch/flowframework/FlowFrameworkRestTestCase.java

Lines 153 to 164 in 8fa9b3f

	protected Settings restAdminSettings() {
	return Settings.builder()
	// disable the warning exception for admin client since it's only used for cleanup.
	.put("strictDeprecationMode", false)
	.put("http.port", 9200)
	.put(OPENSEARCH_SECURITY_SSL_HTTP_ENABLED, isHttps())
	.put(OPENSEARCH_SECURITY_SSL_HTTP_PEMCERT_FILEPATH, "sample.pem")
	.put(OPENSEARCH_SECURITY_SSL_HTTP_KEYSTORE_FILEPATH, "test-kirk.jks")
	.put(OPENSEARCH_SECURITY_SSL_HTTP_KEYSTORE_PASSWORD, "changeit")
	.put(OPENSEARCH_SECURITY_SSL_HTTP_KEYSTORE_KEYPASSWORD, "changeit")
	.build();
	}

And then require it to be provided by the build script:

flow-framework/src/test/java/org/opensearch/flowframework/FlowFrameworkRestTestCase.java

Lines 249 to 253 in 8fa9b3f

	builder.setHttpClientConfigCallback(httpClientBuilder -> {
	String userName = Optional.ofNullable(System.getProperty("user"))
	.orElseThrow(() -> new RuntimeException("user name is missing"));
	String password = Optional.ofNullable(System.getProperty("password"))
	.orElseThrow(() -> new RuntimeException("password is missing"));

And our tests do pass.

Leaving this open for a second opinion...

[DarshitChanpura]

Seems like the admin credentials were added here: https://github.com/opensearch-project/flow-framework/pull/403/files#diff-49a96e7eea8a94af862798a45174e6ac43eb4f8b4bd40759b5da63ba31ec3ef7R205

@owaiskazi19 @dbwiddis this should be re-opened and addressed. Please let @derek-ho and I know if you need any support.

[dbwiddis]

@joshpalis please address

[joshpalis]

Coming from #415 (comment), this modification is not necessary for our plugin since we do not directly use the security demo configuration script when running integration tests. Instead we pull the necessary configuration files from the security plugin directly and manually set the cluster settings during security enabled integration tests. Due to this, closing this issue out

[dbwiddis]

@joshpalis I'm confused. What is the purpose of the "admin" password in build.gradle if it is not used? If it is used, why is it not broken?

Can we put any other placeholder in there and it will work? Or is it a backup if the config files aren't accessible (which should probably be a failure)?

Can we at least add comments in the build.gradle explaining why the "admin" pw is OK?

[DarshitChanpura]

+1 to @dbwiddis's suggestion about adding comments

[DarshitChanpura]

@joshpalis @owaiskazi19 Has this been addressed via: #424?

[owaiskazi19]

@joshpalis @owaiskazi19 Has this been addressed via: #424?

#424 addresses #410. I will let @joshpalis confirm about this issue.

[dbwiddis]

The added lines are fallbacks to a node.getCredentials() call:

flow-framework/build.gradle

Lines 205 to 210 in f039d7f

	var creds = node.getCredentials()
	if (creds.isEmpty()) {
	creds.add(Map.of('username', 'admin', 'password', 'admin'))
	} else {
	creds.get(0).putAll(Map.of('username', 'admin', 'password', 'admin'))
	}

Those fetch user/password from tests.opensearch.username/tests.opensearch.password:

https://github.com/opensearch-project/OpenSearch/blob/bea196f4af5a717a91385eab6db482611024910d/buildSrc/src/main/java/org/opensearch/gradle/testclusters/OpenSearchNode.java#L490-L497

Neither of these appears to look at OPENSEARCH_INITIAL_ADMIN_PASSWORD environment variable per the issue.

Looking across the org I don't see very many examples elsewhere, but there is one such example here:
https://github.com/opensearch-project/cross-cluster-replication/blob/3ea239b5b308b8c053511fb2da79ac37f12b2340/build.gradle#L426-L439

[dbwiddis]

@joshpalis can we resolve the confusion on this issue today? From what I understand:

• we don't use the admin credentials here because we import them from security plugin
• based on this comment we don't need the credentials:

But in case if security plugin's demo configuration is not being used at all, this logic is not needed at all.

So I think we are in compliance with this issue request but it is still confusing and the best solution is to add comments where the admin credentials are (un)used explaining why they are there. Or better yet, replace the "admin" word with self-documenting text like "notused" or an empty string, etc.

[joshpalis]

@dbwiddis From what I understand, plugins that use the docker image to run security enabled integration tests (such as AD) will need to make modifications to their workflow to set the OPENSEARCH_INITIAL_ADMIN_PASSWORD when running the image (reference comment).

In our case, we do not rely on the docker image and instead manually pull all the necessary configuration files from the security plugin itself and install and configure the security plugin within our own build.gradle.

So I think we are in compliance with this issue request but it is still confusing and the best solution is to add comments where the admin credentials are (un)used explaining why they are there. Or better yet, replace the "admin" word with self-documenting text like "notused" or an empty string, etc.

I think we're in compliance as well, I can change the admin password to some other default value

[DarshitChanpura]

+1 to @dbwiddis's comment. If at any point during the flow, install_demo_configuration.sh is used to set-up the fresh cluster, only then OPENSEARCH_INITIAL_ADMIN_PASSWORD is required to be set, and will be picked up for versions >=2.12. If the demo config script is not used at all, this issue can be marked as compliant and closed.

[DarshitChanpura]

I think I understand why the admin:admin credentials work here. Let me try to explain and see if it makes sense.

Flow-framework doesn't use install_demo_configuration.sh to setup security for cluster. However, it does download the security plugin and installs it. Once install is complete, it then configures the opensearch.yml by adding the settings "manually" to the file. It also copies over necessary certificates from security repo.

Since the demo install script modifies the static config internal_users.yml file with the hash of new password, that hash is never updated in this case. And so when the cluster is spun up, security index is loaded with the hash value of admin password (since the internal_users.yml was never updated). Thus admin:admin credential works because of this custom setup.

[joshpalis]

@DarshitChanpura Thanks for the insight, I do see that the internal_users.yml has this default admin configuration when looking through the integ test cluster configurations. Perhaps we can support using non-admin credentials by emulating the modifications the install_demo_configuration makes to the internal_users.yml, then we can remove any references to admin:admin credentials here. Can you point me to the related sections of the script?

admin:
  hash: "$2a$12$VcCDgh2NDk07JGN0rjGbM.Ad41qVR/YFJcgHp0UGns5JDymv..TOG"
  reserved: true
  backend_roles:
  - "admin"
  description: "Demo admin user"