Adding CredentialsProvider, deprecating PasswordProvider

[jihoonson]

Motivation

We are using PasswordProvider to secure user passwords in various places. However, it doesn't secure the user name (or account whatever).

Proposed changes

This proposal is to add a new class CredentialsProvider. Similar to PasswordProvider, the CredentialsProvider provides various ways to get the user credential, but both user name and password.

public interface CredentialsProvider
{
  Map<String, String> getCredentials();
}

Rationale

Since the PasswordProvider has one method of getPassword(), we are already using two different PasswordProviders to secure both user name and password in some places. AWSCredentialsConfig is an example:

public class AWSCredentialsConfig
{
  @JsonProperty
  private PasswordProvider accessKey = new DefaultPasswordProvider("");

  @JsonProperty
  private PasswordProvider secretKey = new DefaultPasswordProvider("");
...

This is not good for user experience because they generally prefer to secure their account and password in the same way.

Operational impact

The PasswordProvider will be deprecated in favor of the new CredentialsProvider. This would deprecate some configurations as well, but they will be still supported in a couple of next releases.

[maytasm]

• Do we need to deprecate PasswordProvider? Can we have both? The reason is that there might be no username and just a password or a single field that needs to be secured. i.e. things that are keys (access key, API keys, etc.)
• What if Druid user wants to secured username and password differently? Is this still possible? For example, a user might pass username as plaintext and password with environment variable

[jihoonson]

@maytasm3 thank you for taking a look.

• Do we need to deprecate PasswordProvider? Can we have both? The reason is that there might be no username and just a password or a single field that needs to be secured. i.e. things that are keys (access key, API keys, etc.)

I think the use cases of PasswordProvider and CredentialProvider are pretty overlapped. Would you elaborate more on the example applications that require a password but not an account we should consider?

• What if Druid user wants to secured username and password differently? Is this still possible? For example, a user might pass username as plaintext and password with environment variable

I think this would be pretty rare, but it's still possible with a custom credential provider.

[a2l007]

@jihoonson Just to clarify, would this be impacting existing usages of getPassword() in any way? I would expect the only change would be PasswordProvider.getPassword() usages changing to CredentialProvider.getPassword()

[jihoonson]

Hi @a2l007, I think the internal usage would be pretty much same. But, the user-facing configurations would change which I expect to achieve in this proposal. For example, let say we changed AWSCredentialsConfig to use the CredentialsProvider as below.

public class AWSCredentialsConfig
{
  @JsonProperty
  @Deprecated
  private PasswordProvider accessKey = new DefaultPasswordProvider("");

  @JsonProperty
  @Deprecated
  private PasswordProvider secretKey = new DefaultPasswordProvider("");

  @JsonProperty
  private CredentialsProvider credentials = new DefaultCredentialsProvider();
...
}

Then, the s3 credentials configurations would be changed from druid.s3.accessKey and druid.s3.secretKey to druid.s3.credentials.accessKey and druid.s3.credentials.secretKey, respectively.

HttpInputSource is another example on the ingestion spec side.

  @JsonCreator
  public HttpInputSource(
      @JsonProperty("uris") List<URI> uris,
      @JsonProperty("httpAuthenticationUsername") @Deprecated @Nullable String httpAuthenticationUsername,
      @JsonProperty("httpAuthenticationPassword") @Deprecated @Nullable PasswordProvider httpAuthenticationPasswordProvider,
      @JsonProperty("httpAuthentication") @Nullable CredentialsProvider httpAuthentication
  )

The new httpAuthentication will be a JSON object that has two fields of userName and password. As a result, the ingestion spec would be changed from

      "inputSource": {
        "type": "http",
        "uris": ["http://example.com/uri1", "http://example2.com/uri2"],
        "httpAuthenticationUsername": "username",
        "httpAuthenticationPassword": {
          "type": "environment",
          "variable": "MY_PASSWORD_VAR"
        }
      }

to

      "inputSource": {
        "type": "http",
        "uris": ["http://example.com/uri1", "http://example2.com/uri2"],
        "httpAuthentication": {
          "type": "environment",
          "userNameVariable": "MY_USER_NAME_VAR",
          "passwordVariable": "MY_PASSWORD_VAR"
        }
      }

[a2l007]

@jihoonson Thanks for the clarification. SGTM

[jihoonson]

@a2l007 thank you for taking a look!

[himanshug]

coming back to peek into Druid PRs after a while and noticed this. Just wanted to let you know of #6666 which proposes something related for other reasons and it would be good to have that in mind as well.

Also, wouldn't

public interface CredentialsProvider
{
  String getPassword(String  key);
}

be more generic then which can handle more than two logically related secrets.

Would you elaborate more on the example applications that require a password but not an account we should consider?

For DB , In all the places I used druid, username was never a secret . Also I see some passwords in https://druid.apache.org/docs/latest/development/extensions-core/druid-basic-security.html which don't necessarily need an "account". I am not saying that we need to keep PasswordProvider but just noting down the different use cases.

[jihoonson]

@himanshug thank you for bring up #6666. That's a really good point.

Also, wouldn't

public interface CredentialsProvider
{
  String getPassword(String  key);
}

be more generic then which can handle more than two logically related secrets.

Hmm, should one CredentialsProvider be able to handle multiple secrets? Would you tell me some examples?

For DB , In all the places I used druid, username was never a secret . Also I see some passwords in https://druid.apache.org/docs/latest/development/extensions-core/druid-basic-security.html which don't necessarily need an "account". I am not saying that we need to keep PasswordProvider but just noting down the different use cases.

Good point. I think these are pretty rare cases and still can be handled by the CredentialProvider. For example, we can add a PlainUsernameAndSecuredPasswordProvider for people who really don't want to secure the username. Similarly, we can add PasswordOnlyCredentialsProvider for the use case where the username is not required.

[himanshug]

Hmm, should one CredentialsProvider be able to handle multiple secrets? Would you tell me some examples?

For example, https://github.com/apache/druid/blob/master/extensions-core/simple-client-sslcontext/src/main/java/org/apache/druid/https/SSLClientConfig.java has three secrets which could be logically related
Also, now that I remember, even the interface String getPassword(String key); would have problems, only way to prevent the problem mentioned in #7400 would be to have an interface like below ( as suggested in #6666 (comment) )

interface CredentialsProvider {
  Map<String, String> getCredentials();
}

as you wanna get a snapshot of secrets in single call , if the code needs to make two/multiple calls to CredentialsProvider then you would continue to have the race mentioned in #7400

hopefully it made sense or I need to be more descriptive :)

[jihoonson]

@himanshug makes sense. I updated the proposal. Thanks for comments!

[bananaaggle]

@jihoonson Hi! I want to know is there any plan to deprecate PasswordProvider. I see it marked as deprecate in code, but only few classes use DynamicConfigProvider which claimed to replace PasswordProvider. I think DynamicConfigProvider not very convenience in some places. For example, when user use config with single String like druid.metadata.storage.connector.user=druid, PasswordProvider is more proper than a map. So can we reserve both of them? And I try to create a EnvironmentVariableDynamicConfigProvider which works as EnvironmentVariablePasswordProvider in #11377 , can you help me review it?

[jihoonson]

Hi @bananaaggle, I just left some comments on the PR.

@jihoonson Hi! I want to know is there any plan to deprecate PasswordProvider. I see it marked as deprecate in code, but only few classes use DynamicConfigProvider which claimed to replace PasswordProvider. I think DynamicConfigProvider not very convenience in some places. For example, when user use config with single String like druid.metadata.storage.connector.user=druid, PasswordProvider is more proper than a map. So can we reserve both of them? And I try to create a EnvironmentVariableDynamicConfigProvider which works as EnvironmentVariablePasswordProvider in #11377 , can you help me review it?

I still think we should deprecate PasswordProvider. If your concern is that users now have to set a map instead of a simple string in their config, I think it's more about how we construct DynamicConfigProvider from the user configuration rather than using DynamicConfigProvider. For example, we can provide user configurations like below

• druid.metadata.storage.connector.credentials.type = mapString
• druid.metadata.storage.connector.credentials.user = druid
• druid.metadata.storage.connector.credentials.password = mypassword

, but construct a MapStringDynamicConfigProvider from these configs that has a map of { "user": "druid", "password": "mypassword" }.

[bananaaggle]

I still think we should deprecate PasswordProvider. If your concern is that users now have to set a map instead of a simple string in their config, I think it's more about how we construct DynamicConfigProvider from the user configuration rather than using DynamicConfigProvider. For example, we can provide user configurations like below

• druid.metadata.storage.connector.credentials.type = mapString
• druid.metadata.storage.connector.credentials.user = druid
• druid.metadata.storage.connector.credentials.password = mypassword

, but construct a MapStringDynamicConfigProvider from these configs that has a map of { "user": "druid", "password": "mypassword" }.

Hi @jihoonson! I make some attempts to adapt some classes to DynamicConfigProvider in #11389. I think we can provide DynamicConfigProvider for users in config, and users should know which key needed in a map. For example, if users want to add user by dynamicConfigProvider, they should add config like this:

druid.metadata.storage.connector.dynamicConfigProvider = 
{
  "type": "environment",
  "variables":
         {
            "user": "MY_USER_NAME_VAR"
         }
}

When we deprecate PasswordProvider, we should change field which is in type of PasswordProvider to String, then users can continue using plaintext in their config. Do you think it's a proper design? I'll add unit test and document for this PR when we finishing planning.