feat: Change AuthenticationPlugin interface to use char[] rather than String

[sehrope]

Updates the recently introduced, but not yet released, AuthenticationPlugin interface to use a char[] array rather than a String.

This was simpler than I thought for the majority of the call sites as both PLAIN and MD5 used an byte[] encoded password and GSS already used a char[] array. All that remained was SCRAM which still used String for all it's internals so I made the ConnectionFactoryImpl create one at the only call site that interacts with the password. A follow up PR could clean up the internals of the SASL library to use char[] rather than String.

Two specific points to review:

1. Do the @Nullable make sense? It's weird having it in between the type and array declaration but from the rest of the codebase I think that's how it's supposed to work.
2. Do we need to make a copy of the char[] password array in the GSS handler? I don't think so as any usage of it should complete before MakeGSS.authenticate(...) returns. We could push the password retrieval even deeper into that function so it happens at the last moment possible, I didn't go down that road as I'm trying to minimize the number of things we're actually changing.

Thoughts? @vlsi @davecramer

[vlsi]

We should probably consider changing ScramAuthenticator to char[] as well

[sehrope]

Yes I mention that in the PR. It's self-contained so somebody can follow up with that separately.

[davecramer]

seems we should also refactor it to move it out of ...jre7.. as well
Is there any reason not to change it to char[] now ?

[vlsi]

I would agree with @sehrope , the key point for now is to get the API right == move to char[] in the interface.
The implementation can be adjusted later.

[davecramer]

So if you have no other issues should we merge this then ?

[vlsi]

Ultimately we need to call ongres/scram, and they don't seem to support char[] yet: https://gitlab.com/ongresinc/scram/-/issues/14

[davecramer]

@jorsol can you provide some insight/suppport here ?

[vlsi]

What do you think of adding the following to the javadoc?

The callers should erase the contents of `char[] password` with 0 chars like in
{@code Arrays.fill(password, (char) 0)} after the password is used for security reasons.

[sehrope]

Yes that's a good idea. I'm going to add that to the javadoc for the interface as that's the only part that a user would interact with. And it will be the user's responsibility to clear the array as they have ownership of it.

[bokken]

Which “user” are you talking about? The caller of this api (eg. the driver itself as it needs password to connect) or the user of the driver who is implementing class to provides password dynamically?

It seems to me we should define the api to always have to return a new ‘char[]’ and make the driver responsible for clearing once value no longer needed.

[sehrope]

@vlsi @bokken Yes the more I'm thinking about this, the hairier it'd get to have the caller clear the char[] array as it's provided to us in the plugin interface, but the end user cannot clear it until after the connection is established which might be handled somewhere else entirely. I'm going to change the code to wipe it out after use and make a note in the Javadoc for the interface that any value provided will be destroyed after use. So the caller will be responsible for providing a new char[] array each time the same method is called.

[vlsi]

Which “user” are you talking about?

Anyone who calls char @Nullable [] getPassword( method.
Mainly I expect that it is the driver who will call the method.

However, the plugin implementation should understand how the value would be used, so it still makes sense to clarify the intention:

1. The value could be erased after it is returned in getPassword. For instance, if the plugin requests the password from some sort of token server, the plugin should erase all the other copies of the password (e.g. clear buffers, etc, etc)
2. The caller of getPassword (==driver in 99% of the cases) would erase char[] after use

That ensures no passwords keep floating around after use.

[sehrope]

I pushed an update to this PR that has the driver clear the user provided char[] array after use.

Rather than have the internal code of the driver provide password as a return value, I made AuthenticationManger.withPassword(...) take a callback so that the code for clearing the char[] array cannot be accidentally missed. The char[] password and encoded byte[] are always cleared in a finally block.

The one spot where the callback code is a bit odd is for the creation of the SASL authenticator as the compiler doesn't let you initialize it in an anonymous block as the variable reference isn't final. I handled that by wrapping the variable declaration in an single entry array:

https://github.com/pgjdbc/pgjdbc/pull/2420/files#diff-dba283e2ce6b2f6a8a889e9dac54cce198602b32dea901c429eb845e02b2d7b0L626-R628

I think that's still better than having to sprinkle clean up code all over the place. The other option was having withPassword(...) allow returning a generic arg so that the SASL piece could return the initialized object. That seemed like it'd be too easy to abuse for returning the password itself (which wouldn't work as it'd be cleared) so I specifically avoided that.

This also updates the comments in the AuthenticationPlugin interface to make clear that every invocation of the char[] getPassword() method must return a new char[] as the previous one will be wiped by the driver.

[sehrope]

One more push to make the null checker happy and I think we're be good.

[vlsi]

Should withEncodedPassword be confined to MD5Digest.encode only?

[sehrope]

The cleartext code uses it as well.

[vlsi]

Can you please clarify?
You do not need encodedPassword past MD5Digest.encode, so why make a bigger withEncodedPassword block?

[sehrope]

Oh I thought you were asking why that method exists and meant that it's also used when sending the plaintext password.

In all those cases I wrapped the entire sub-block that is using the password or encoded password to keep the changeset simpler.

[vlsi]

The intention should be to minimize the use of the password, and erase it as soon as it is no longer needed.

In this case, making the block smaller yields easier to follow changeset.

However, I guess we need to erase md5-encoded block as well, as it basically allows relay attack.

[sehrope]

FYI the AppVeyor failure is the usual query timeout race condition. The other AppVeyor build ran fine.

[sehrope]

Pushed another that moves as much as possible out of the withXYZ(...) blocks.

Also fills zeroes into the md5 digest buffer after it's done being used.

Here's the diff relative to the previous push as I forced atop it:

diff --git a/pgjdbc/src/main/java/org/postgresql/core/v3/ConnectionFactoryImpl.java b/pgjdbc/src/main/java/org/postgresql/core/v3/ConnectionFactoryImpl.java
index d42c1535..69efc796 100644
--- a/pgjdbc/src/main/java/org/postgresql/core/v3/ConnectionFactoryImpl.java
+++ b/pgjdbc/src/main/java/org/postgresql/core/v3/ConnectionFactoryImpl.java
@@ -668,12 +668,16 @@ public class ConnectionFactoryImpl extends ConnectionFactory {
                     LOGGER.log(Level.FINEST, " FE=> Password(md5digest={0})", new String(digest, StandardCharsets.US_ASCII));
                   }
 
-                  pgStream.sendChar('p');
-                  pgStream.sendInteger4(4 + digest.length + 1);
-                  pgStream.send(digest);
-                  pgStream.sendChar(0);
-                  pgStream.flush();
+                  try {
+                    pgStream.sendChar('p');
+                    pgStream.sendInteger4(4 + digest.length + 1);
+                    pgStream.send(digest);
+                  } finally {
+                    java.util.Arrays.fill(digest, (byte) 0);
+                  }
                 });
+                pgStream.sendChar(0);
+                pgStream.flush();
 
                 break;
               }
@@ -686,9 +690,9 @@ public class ConnectionFactoryImpl extends ConnectionFactory {
                   pgStream.sendChar('p');
                   pgStream.sendInteger4(4 + encodedPassword.length + 1);
                   pgStream.send(encodedPassword);
-                  pgStream.sendChar(0);
-                  pgStream.flush();
                 });
+                pgStream.sendChar(0);
+                pgStream.flush();
 
                 break;
               }
@@ -791,9 +795,9 @@ public class ConnectionFactoryImpl extends ConnectionFactory {
                         PSQLState.CONNECTION_REJECTED);
                   }
                   scramAuthenticator[0] = new org.postgresql.jre7.sasl.ScramAuthenticator(user, castNonNull(String.valueOf(password)), pgStream);
-                  scramAuthenticator[0].processServerMechanismsAndInit();
-                  scramAuthenticator[0].sendScramClientFirstMessage();
                 });
+                castNonNull(scramAuthenticator[0]).processServerMechanismsAndInit();
+                castNonNull(scramAuthenticator[0]).sendScramClientFirstMessage();
                 // This works as follows:
                 // 1. When tests is run from IDE, it is assumed SCRAM library is on the classpath
                 // 2. In regular build for Java < 8 this `if` is deactivated and the code always throws

[davecramer]

Are we waiting for anything else here ?

[sehrope]

I don't think so. The only related item would be clearing the internal stream buffers but I don't want to risk messing with that here.

@vlsi Any other comments? Otherwise I'm going to merge this in a bit.

[sehrope]

@vlsi I didn't add a generic returning method for the callbacks so that the caller does not return back a value from the callback handler that potentially contains the password or a derivative:

I think that's still better than having to sprinkle clean up code all over the place. The other option was having withPassword(...) allow returning a generic arg so that the SASL piece could return the initialized object. That seemed like it'd be too easy to abuse for returning the password itself (which wouldn't work as it'd be cleared) so I specifically avoided that.

[sehrope]

Why the false &&?

[vlsi]

I can probably keep it as is, however, it just bothers me we log credentials in plain text

[sehrope]

It's likely there to allow debugging the protocol so replacing it with false && wouldn't make sense. Either we'd remove it entirely or put low level security related logging behind some other feature flag.

Either way it does not belong in this PR and should be done separately.

[vlsi]

I agree it does not belong to the PR, however, the PR does touch the related lines, so I think it would be fine to disable the log for now, and later we can do something with it.

[vlsi]

I didn't add a generic returning method for the callbacks so that the caller does not return back a value from the callback handler that potentially contains the password or a derivative

withPassword are internal to driver methods, so we can see and inspect all the usages.
withEncodedPassword is one of the cases, where the return value has a purpose for being a derivative.

[sehrope]

My comments regarding avoid returning the value or its derivatives were for future developers of the driver. Not just us.

I appreciate the feedback and review, but you shouldn't push changes atop a PR that go directly against something that was explained without at least addressing the topic. It wasn't an arbitrary decision and I outlined why it was implemented like that.

Anyway, I'm not particularly wed to not allowing the return values. I'm going to back out the logging changes (that false && thing) and keep the rest as it is now. After that, once it cycles through CI I'll rebase and merge.

[vlsi]

My comments regarding avoid returning the value or its derivatives were for future developers of the driver. Not just us

Unfortunately, there's no way "missing return value" ensures the future developers would be safe.
The only way to verify password leak is to use some SAST tool with taint analysis and code review.

For instance, the change when you converted scramAuthenticator to an array was exactly "leaking password derivative", and you had to add an array to make the leak happen.

That is why I think returning values is just fine, and enforcing void return type does not really help anything.

I agree the whole thing sounds like it touches more code than it was intended in the first place as in "only change the return type in the AuthenticationPlugin API".

However, I believe, it is important to ensure the returned value from the authentication plugin is usable, so making sure the core works with char[] would be nice, and it does not touch that much.

---

If we go back to the signature:

char @Nullable [] getPassword(AuthenticationRequestType type) throws PSQLException;

then I would say it is a bit strange that we pass half of the parameters via Properties, another half via getPassword argument, and there are arguments we even fail to pass: target host/port for the connection.

The thing is we might have multiple hosts in the connection url, and we might be in failover mode. Then, the auth plugin might be interested to know where we are connecting as different servers (e.g. primary and the replica) might have different credentials.

[sehrope]

then I would say it is a bit strange that we pass half of the parameters via Properties, another half via getPassword argument, and there are arguments we even fail to pass: target host/port for the connection.

The Properties is passed to the constructor and it matches how all the other plugin classes are instantiated. That way it leverages the existing ObjectFactory.

Those other args might be helpful but if we can add it to the end user interface with a default implementation that points to the single arg version. Let's have at least one person come back with a specific use case first in case there is something else we'd need to add as well.

I pushed an update and added you as a co-author. It has the last round of your changes minus the false && logging stuff. That doesn't belong in here and needs to be its own PR, either removing it entirely or putting it behind a "log-everything-including-secrets" feature flag. And it's already disabled unless the user explicitly opts into FINEST log level.

I think this good to merge.

[vlsi]

Let's have at least one person come back with a specific use case first in case there is something else we'd need to add as well.

Having 0 users is a good sign that the feature should not be merged.

Just a reminder, log4shell vulnerability appeared in log4j 2.x sources "just in case", "somebody might need variable replacement and jndi resolution". The sad thing is that nobody really asked for that replacement feature.

[davecramer]

This is a bit of a catch-22 how would we have users without merging the feature in ?

[sehrope]

By "no users" I was referring to no users asking for additional arguments be included in the password callback to justify expanding it further. Outside of host / port, the TLS parameters immediately come to mind as potentially useful. Rather than guess though, since nobody is explicitly asking for any of that as of yet, I'm leaving it out.

In contrast, we have multiple users asking for the ability to specify a dynamic password at connection time, such in this original issue going back to last March: #2102