CredentialProviderToken in accumulo-client.properties stores the serialized token which contains the password, exposing the password out of the JCEKS file

[gwynlionhart]

Describe the bug
Upgrading from 1.10.2 to 2.1.2. The accumulo-client.properties requires the auth.token to be a base64-encoded serialization of the Token class. I created a token for the CredentialProviderToken class using the create-token (which doesn't support CredentialProviderToken correctly in 2.1 btw, I'll elaborate on that later).

If someone has read access to that properties file, they can get the token and paste it into an online base64 decoder (that supports the ISO-8859-1 character set) in order to get the password. For CredentialProviderToken that defeats the purpose because it's supposed to be keeping the password "safe" (yes I know, not ideally safe) by keeping it inside the JCEKS. Right now I believe it only works because it's deserializing the token, kind of bypassing the point of the CredentialProviderToken.

In fact, I just tested by commenting out the "general.security.credential.provider.paths" and it initialized just fine by deserializing the CredentialProviderToken, so I it doesn't look like it's treating CredentialProviderToken any differently than a Password token? I think the fix would be that when auth.type=CredentialProviderToken then the auth.token value should be the JCEKS alias name and then the token is initialized by calling the public CredentialProviderToken(String name, String credentialProviders)

Or if it must be a serialized instance of the token, then CredentialProviderToken should override the serialization method so that it doesn't return the password but rather the alias name ("name" field of the class).

Ideally I'd be able to continue using this authentication method, especially because it is still supported on the server side. It was actually kind of hard to find documentation about CredentialProviderToken in the context of accumulo-client.properties. Is it intended to still be supported? Or should I move away from accumulo-client.properties and just use the newClient fluent builder and regular java properties?

As for the create-token shell command: In this section of the source code for CreateToken.java, you can see that it attempts to run the token.init() within the loop on TokenProperties rather than outside the loop. CredentialProviderToken requires two properties, so this ends up throwing an exception because it was init too soon.

   for (TokenProperty tp : token.getProperties()) {
      String input;
      if (pass != null && tp.getKey().equals("password")) {
        input = pass;
      } else {
        if (tp.getMask()) {
          input = getConsoleReader().readLine(tp.getDescription() + ": ", '*');
        } else {
          input = getConsoleReader().readLine(tp.getDescription() + ": ");
        }
      }
      props.put(tp.getKey(), input);
      token.init(props); //This is inside the for loop, so only one of the two properties of CredentialProviderToken has been looped on
    }
//token.init(props) should go here

Versions (OS, Maven, Java, and others, as appropriate):

• Affected version(s) of this project: 2.1.2
• OS: Testing the create-token on Ubuntu, running Accumulo on centos
• Others:

To Reproduce
Steps to reproduce the behavior (or a link to an example repository that reproduces the problem):

1. Try to run accumulo create-token -tc org.apache.accumulo.core.client.security.tokens.CredentialProviderToken -u user
2. Prompted for the alias inside the JCEKS. Enter anything (or whatever alias you have in your example JCEKS
3. Press enter and see error

Alias to extract from CredentialProvider: instance.password
Thread 'create-token' died.
java.lang.IllegalArgumentException: Expected name and credentialProviders properties.
        at org.apache.accumulo.core.client.security.tokens.CredentialProviderToken.init(CredentialProviderToken.java:99)
        at org.apache.accumulo.core.util.CreateToken.execute(CreateToken.java:110)
        at org.apache.accumulo.start.Main.lambda$execKeyword$0(Main.java:122)
        at java.base/java.lang.Thread.run(Thread.java:829)
2024-05-17T19:15:18,991 [start.Main] ERROR: Thread 'create-token' died.
java.lang.IllegalArgumentException: Expected name and credentialProviders properties.
        at org.apache.accumulo.core.client.security.tokens.CredentialProviderToken.init(CredentialProviderToken.java:99) ~[accumulo-core-2.1.2.jar:2.1.2]
        at org.apache.accumulo.core.util.CreateToken.execute(CreateToken.java:110) ~[accumulo-core-2.1.2.jar:2.1.2]
        at org.apache.accumulo.start.Main.lambda$execKeyword$0(Main.java:122) ~[accumulo-start-2.1.2.jar:2.1.2]
        at java.lang.Thread.run(Thread.java:829) [?:?]

Expected behavior
a base64-encoded CredentialProvider token using the alias name and JCEKS file path, then the token retrieves the password out of the JCEKS. Thus keeping the password itself out of the property file

Additional context
Add any other context about the problem here.

[ctubbsii]

It looks like this might have been broken by #508 in an attempt to remove some special case handling of the CredentialProviderToken with dedicated client properties, and replace it with the generic handling with AuthenticationToken serialization. The problem with that is that the CredentialProviderToken extends PasswordToken, and was not intended to be serialized, but instead was taking advantage of the PasswordToken's methods for retrieving the secret after retrieval of the secret from the provider during the CredentialProviderToken construction. With serialization, it does store the password, because that's how PasswordToken serialization works.

In the spirit of preserving the intent of #508, to remove the special properties that handle CredentialProviderToken serialization in a special way, I think the best path forward is to fix the CredentialProviderToken to make it safe for serialization.

The other issue, about the init method being called inside the loop looks like an easy fix.

[ctubbsii]

I created a PR to attempt to fix these issues in #4580 . Please take a look and see if it fixes the issues you're seeing. Note: the new serialization is incompatible with any earlier serialization for CredentialProviderToken (that's kind of the point, of course, since the problem is the current serialization, but still, I'm pointing it out to make it clear).

[gwynlionhart]

Checked out the PR and it looks good!

[dlmarion]

@gwynlionhart - just curious, did you happen to test the code in the PR, or just review it?

[ctubbsii]

@gwynlionhart - just curious, did you happen to test the code in the PR, or just review it?

@dlmarion They actually provided test code to verify it works as expected. I adapted that code into a unit test which I've now added to my PR.