Add PasswordUtil for encrypting passwords client side

[sehrope]

Overall idea is the same as #3067 but couple changes to implementation and presesntation:

• PasswordUtil no longer concerns itself with connections. It just hashes passwords and encodes them for using in the DB.
• Adds some overloads for more specific options (e.g. customizing the SCRAM iterations and salt size). Defaults for the no-overload version match up with before which IIRC is the server defaults.
• Adds alterUserPassword(...) to PGConnnection. That's our "public API" for everything PG-specific so it seemed like a better home than having PasswordUtil deal with java.sql.*.

Splitting out the encoding allows the same functions to be used for CREATE USER ... (again without passing the credentials in plaintext). The updated test:

• Creates the user using the encoded password
• Verifies that the pg_shadow entry matches
• Tests that it can connect as the user using the valid password
• Tests that it cannot connect as the user using a bad password (in case we mess up the HBA and blindly succeed)
• Alters the user's password to a new one using the new PGConnection method
• Tests the credentaials again (both success and failure)

Tests are run against the servers's default password encryption method, the driver's default password encryption method, md5, and scram-sha-256 (for v11+).

If a new encryption method gets added to the server and is marked as the default then it should break our CI (which is good).

As part of this one more helper was added to TestUtil for executing SQL with a string arg. And an internal bytes-to-hex method in MD5Digest was marked public but it's not part of the "public API" package so I think that's fine.

@davecramer Besides the structure and hashing, take a peek at the comments too as I tried to explain what this is really doing despite the poor verbiage on the server (it's not "encryption", it's hashing...), whilst sticking to the server's language as much as possible.

===

All Submissions:

• Have you followed the guidelines in our Contributing document?
• Have you checked to ensure there aren't other open Pull Requests for the same update/change?

New Feature Submissions:

1. Does your submission pass tests?
2. Does ./gradlew styleCheck pass ?
3. Have you added your new test classes to an existing test suite in alphabetical order?

[vlsi]

Looks good overall. +1 for adding a method to PGConnection. I left comments inline.

Below are some some more ideas, hovewer, I am not sure about them.
I wonder if it makes sense to use builders to configure parameters instead of string/string overloads.

[vlsi]

Coulo you add the meaning of null to the docs?

[vlsi]

try-with-resources?

[sehrope]

Yes that's copy-pasta from it's sibling method without the arg.

[davecramer]

Overall idea is the same as #3067 but couple changes to implementation and presesntation:
presentation ?

• PasswordUtil no longer concerns itself with connections. It just hashes passwords and encodes them for using in the DB.
  "for use in the DB" ?

• Adds some overloads for more specific options (e.g. customizing the SCRAM iterations and salt size). Defaults for the no-overload version match up with before which IIRC is the server defaults.
  Probably should spell out what was there before as future readers won't remember that.

• Adds alterUserPassword(...) to PGConnnection. That's our "public API" for everything PG-specific so it seemed like a better home than having PasswordUtil deal with java.sql.*.

Splitting out the encoding allows the same functions to be used for CREATE USER ... (again without passing the credentials in plaintext). The updated test:

• Creates the user using the encoded password
• Verifies that the pg_shadow entry matches
• Tests that it can connect as the user using the valid password
• Tests that it cannot connect as the user using a bad password (in case we mess up the HBA and blindly succeed)
• Alters the user's password to a new one using the new PGConnection method
• Tests the credentaials again (both success and failure)
  credentials ?

Tests are run against the servers's default password encryption method, the driver's default password encryption method, md5, and scram-sha-256 (for v11+).

[davecramer]

So the reason the tests are failing is because you assume scram for anything above 10, however we have noscram tests for versions above 10.

[sehrope]

Looks good overall. +1 for adding a method to PGConnection. I left comments inline.

Good feedback. A lot of it aligns with things I was thinking about while working on this.

I wonder if it makes sense to use builders to configure parameters instead of string/string overloads.

I considered that as well but scrapped it because the options would be different for md5 vs scram-sha-256. The former needs a username (to use as the md5 salt) and the latter has random salt + iteration count. Seemed like too much complexity as the original goal was a simple function for end users to invoke.

If we go the options route, we'd need either a marker interface PGPasswordEncryptionOptions with specific implementations for each algo or some class hierarchy to the same effect.

public String encodePassword(char[] password, PGPasswordEncryptionOptions options) {
  if (options instanceof MD5PasswordEncryptionOptions) {
     // ...
  } else if (options instanceof ScramSha256PasswordEncryptionOptions) {
     // ...
  } else {
    throw new IllegalArgumentException("Unhandled encryption option...");
}

The driver defaults could then just be some static value (i.e. scram with 4K iterations). And the server default could would be responsible for instantiating some variant of that.

Thoughts? Any other ways to handle the divergent args?

[sehrope]

So the reason the tests are failing is because you assume scram for anything above 10, however we have noscram tests for versions above 10.

That's weird. One of the failing tests in the matrix is for running SCRAM on 8.4 but that shouldn't have executed. Maybe the skip annotations I added aren't being used correctly.

Why wouldn't SCRAM work for >=10 (v.s. >10)? It added in that version.

[davecramer]

So the reason the tests are failing is because you assume scram for anything above 10, however we have noscram tests for versions above 10.

That's weird. One of the failing tests in the matrix is for running SCRAM on 8.4 but that shouldn't have executed. Maybe the skip annotations I added aren't being used correctly.

Why wouldn't SCRAM work for >=10 (v.s. >10)? It added in that version.

If you look at the docker scripts you can see that if scram is not specified then it creates a user using MD5. This is why in my tests I read pg_shadow for the user to determine the encryption to use.

[sehrope]

Why would that matter for any new user created in CI though? I thought the server's default is just the default if it interprets your password as plain text and you're asking it to handle the hashing. If it has the magic md5... prefix or SCRAM:... prefix it's supposed to use the encoded value. Even if the default is md5 for the server, you can still use SCRAM if you create the user or alter the password to a SCRAM encoded value.

[davecramer]

Before you create the user you need to set the encryption method on the connection

[sehrope]

Not according to the docs: https://www.postgresql.org/docs/current/sql-createrole.html

The password is always stored encrypted in the system catalogs. The ENCRYPTED keyword has no effect, but is accepted for backwards compatibility. The method of encryption is determined by the configuration parameter password_encryption. If the presented password string is already in MD5-encrypted or SCRAM-encrypted format, then it is stored as-is regardless of password_encryption (since the system cannot decrypt the specified encrypted password string, to encrypt it in a different format). This allows reloading of encrypted passwords during dump/restore.

[davecramer]

Not according to the docs: https://www.postgresql.org/docs/current/sql-createrole.html

The password is always stored encrypted in the system catalogs. The ENCRYPTED keyword has no effect, but is accepted for backwards compatibility. The method of encryption is determined by the configuration parameter password_encryption. If the presented password string is already in MD5-encrypted or SCRAM-encrypted format, then it is stored as-is regardless of password_encryption (since the system cannot decrypt the specified encrypted password string, to encrypt it in a different format). This allows reloading of encrypted passwords during dump/restore.

What I meant is you need to say set password_encryption to md5|scram-256 before creating the user.

[sehrope]

You only need to do that if the password string is not already encoded. The server checks if it has the magic prefix and then uses it as is:

postgres=# SHOW password_encryption;
 password_encryption 
---------------------
 md5
(1 row)

postgres=# CREATE USER tp_001 WITH PASSWORD 'abcd';
CREATE ROLE
postgres=# CREATE USER tp_002 WITH PASSWORD 'SCRAM-SHA-256$4096:3FZ7MGI94esnPG7F1Yv1yA==$Ni2QInjygTvB6uJJv3aqRuqn05yCMFT7DUfYCLz1K2Y=:H3uYxCIwRsfXOnhEUn4tz/lwQotUkHD4xg2V5KTLUq0=';
CREATE ROLE
postgres=# SET password_encryption = 'scram-sha-256';
SET
postgres=# CREATE USER tp_003 WITH PASSWORD 'abcd';
CREATE ROLE
postgres=# CREATE USER tp_004 WITH PASSWORD 'md525dd342d721d2cf330f0f979e35e3e37';
CREATE ROLE
postgres=# SELECT usename, substring(passwd, 1, 20) FROM pg_shadow WHERE usename LIKE 'tp_%';
 usename |      substring       
---------+----------------------
 tp_001  | md5f1eedcaa29247ac83
 tp_002  | SCRAM-SHA-256$4096:3
 tp_003  | SCRAM-SHA-256$4096:U
 tp_004  | md525dd342d721d2cf33
(4 rows)

Note how tp_002 has a SCRAM password and tp_004 has an md5 password despite the password_encryption being something else.

[davecramer]

hmmm... well that was the only way I could get the tests to pass.

[davecramer]

ah, I think the problem is that in the docker script we specify md5 or scram in pg_hba.conf. If we specified password instead that would work.

[sehrope]

Pushed a series of commits that address the feedback. If this looks good I'll rebase it to clean up the commits. Figure it's easier to review with the individual commits as each one addresses a specific piece of feedback.

I didn't add the try-with-resources to the new queryForString(...) helper. There's a ton of siblings in that file that need to be corrected so we'll do that in a separate PR. Everything else from above should be addressed though.

@davecramer I had forgotten to include the @Rule stuff so the "ignore if my PG version is less than X" annotation wasn't doing anything! Besides the per-class or per-method annotation, the test class needs define a ServerVersionRule so JUnit applies it.

Also, trying to fix the test for 8.4 lead me to realize that the genAlterUserPasswordSql(...) function was ignoring the encryptionType arg (fixed now). That's why that the "use server default" test was failing on 8.4:

diff --git a/pgjdbc/src/main/java/org/postgresql/util/PasswordUtil.java b/pgjdbc/src/main/java/org/postgresql/util/PasswordUtil.java
index 98699853..21b35840 100644
--- a/pgjdbc/src/main/java/org/postgresql/util/PasswordUtil.java
+++ b/pgjdbc/src/main/java/org/postgresql/util/PasswordUtil.java
@@ -180,7 +180,7 @@ public class PasswordUtil {
     // The choice of true / false for standard conforming strings does not matter
     // here as the value being escaped is generated by us and known to be hex
     // characters for all of the implemented password encryption methods.
-    Utils.escapeLiteral(sb, encodePassword(user, password), true);
+    Utils.escapeLiteral(sb, encodePassword(user, password, encryptionType), true);
     sb.append("'");
     return sb.toString();
   }

I think this is good to go. Let me know if any other thoughts and I'll rebase, run through CI, and merge.

[sehrope]

Actually hang on ... need to add one more to change the password arg to be a char[].

[sehrope]

Pushed another commit that changes the API to use a char[] for the password fields. Internally it converts them into a String as rest of the code that is being called is all String based. That could be improved down to directly read from the char[] arrays but it's out of scope for now (e.g. we'd need to expand the SCRAM library).

[vlsi]

WDYT of Arrays.fill(password, ' ') as soon as the password was consumed? (e.g. after conversion no string)

A slightly better approach would probably be CharBuffer.wrap(password), then encode it to byte array with Charset.encode, then zero out both arrays.

[sehrope]

I don't think it's the job of the encoder to wipe out the caller's data structures. The caller can choose to do that if necessary.

That would make more sense if the function signature was something like:

public static String encodeMd5(String user, Supplier<char[]> passwordSource) { 
  char[] password = passwordSource.get();
  // do stuff
  Arrays.fill(password, ' '); // clear password array
}

In that situation we "own" the array and it'd be our responsibility for clearing it.

I'm of two minds here ... partly think we should go that route as it's the most flexible. And partly think nobody is going to care about this and we should just make String encode(String user, String password) overloads as that's what 99% of people are going to use.

[vlsi]

There are two points of using char[] for passwords:

1. The API that uses the password can erase the password shortly after use so it minimizes the time window when the password can be grabbed from the memory dump
2. There are much fewer chances of logging the passwords as char[].toString() would not dump contents.

In this case, as soon as you compute MD5, you can erase the char[] array so the password is not accidentally leaked.

[sehrope]

I know why it's being done. I'm the one that suggested it in Dave's original PR!

I'm saying the reality of how people program in Java lends itself to this being a moot point because the caller is going to have the password in a String and simply convert it to a char[] at the call site.

Regardless, destroying the contents of an argument (like we'd be doing here without changing the method signature) isn't the same as destroying the contents of the return value of a function (e.g. what AuthenticationPluginManager does).

In this specific case it'd be weird because it'd break the most common use case of this function:

char[] password = generateNewPassword();
conn.alterUserPassword(user, password);
saveNewPasswordSomewhere(password);

The user owns the char[] so the user is responsible for clearing it.

[vlsi]

If the user wants saving the password, they should clone the array

[sehrope]

Couple more to address @vlsi last feedback. And one to refactor the way the connection tests throw errors to get the details in the logs. It doesn't change any of the output of the tests themselves but helped debug when things weren't working because the SCRAM library mandates >=4096 iterations: ongres/scram#20 (comment)

[sehrope]

Added the "on"/"off" tests and a note to CHANGELOG referencing the PR (kind of funny how you have to create the PR then push that after you know the PR number...).

I'm happy with this and will rebase and merge this tomorrow.

[sehrope]

What do you think @vlsi ?

[vlsi]

What is the purpose of Supplier in the signature?
I think a regular char[] should be enough.

[sehrope]

The rationale is that by providing a callback rather than a value, it's explicit that the ownership of the char[] object lies with the method and not the caller. This also matches how we provide passwords in the auth callbacks.

[sehrope]

A simple use case would match the tests where a string is simply converted to a char[].

A more elaborate use case could have the callback itself read from a KMS. It's the ultimate in flexibility while ensuring the char[] never lingers.

[vlsi]

You don't need to have supplier for that. Just document that in the javadoc: password will be erased after use and that is it. Supplier makes it harder for the clients to pass the value.

[sehrope]

If the caller is supplying a char[] then the caller owns that object and the caller must wipe it.

If we want to wipe it within the method, we need to own the char[] and the way to signify that is to have it's creation happen via the supplier. Plus we get the flexibility for the more elaborate use cases.

Plus it's actually easier with the supplier style as the only usage within the driver of the password is wrapped around the supplier which ensures that the raw char[] value never lingers.

If the method signature has the char[] and overload A calls overload B (which is supposed to eventually wipe the password), how do you ensure the password was wiped and something else did not throw an exception before that step?

[vlsi]

No. We declare in javadoc who owns the object and that is it.

how do you ensure the password was wiped and something else did not throw an exception before that step?

Wipe the password in finally

[sehrope]

That's my point. You end up with the same code in every overload as otherwise you never know if a change to the overload impacts when exactly the password gets cleared.

Anyway, I'm not keen to change it. The callback method is the most versatile and covers all the more complex use cases I've described.

If we're deadlocked on this, then get someone else to agree to the change in the signature. Otherwise I'm done with this and plan to rebase and merge.

[vlsi]

You end up with the same code in every overload

There are only a few methods dealing with passwords, and, yes, security code is not the most easy to implement correctly.

Neither https://docs.oracle.com/javase%2Ftutorial%2Fuiswing%2F%2F/components/passwordfield.html nor https://docs.oracle.com/javase/8/docs/technotes/guides/security/crypto/CryptoSpec.html suggest using Supplier in the method parameters.

[vlsi]

Technically speaking, JUnit4 does not use test.. method naming. test... method prefix comes from JUnit3 when @Test annotation did not exist.

[sehrope]

I still like it though as it makes the method name an imperative mood. Without the prefix we get public void serverDefault() which sounds like it returns a value. Could be more clever and do something like "validateServerDefault" but meh...

[vlsi]

testServerDefault does not explain what the test verifies though

[davecramer]

I'm +1 to just using char[] in the signature and we zero it out. We are after all providing the user with a utility method to "take care of it for them"

[vlsi]

Objects.

Suggested change

	if (password == null) {
	throw new NullPointerException("password is null");
	}
	Objects.requireNonNull(password, "password");

[vlsi]

Suggested change

	String passwordText = String.valueOf(password);
	byte[] passwordBytes = passwordText.getBytes(StandardCharsets.UTF_8);
	final MessageDigest md = MessageDigest.getInstance("MD5");
	md.update(passwordBytes);
	byte[] passwordBytes = StandardCharsets.UTF_8.encode(CharBuffer.wrap(password)).array();
	try {
	MessageDigest md = MessageDigest.getInstance("MD5");
	md.update(passwordBytes);
	} finally {
	Arrays.fill(passwordBytes, (byte) 0);
	}

[sehrope]

Nope that doesn't work. Was going nuts trying to figure out why as it broke a bunch of the md5 tests. Turns out the .array() call returns the backing array of the ByteBuffer which is not necessarily the same size as the encoded password. In the tests it ends up being 38 bytes (extra zeros at the end) vs. 35 bytes and the extra zeros get included in the digest.

Going to try something similar to see if we can avoid the extra String in between. I think we can write the ByteBuffer directly to the MessageDigest then clear it.

[sehrope]

Yes the ByteBuffer worked when directly writing it to the MessageDigest. That way it's aware of the contents and not the entire backing array.

[sehrope]

Force pushed a commit that replaces the password callbacks with wiping the char[] in each method.

Also adds to the tests to check that the char[] arrays have bene wiped. And a new test for a "bad encryption type" to ensure that even in that situation the password is wiped.

[sehrope]

Rebased atop master, squashed down the intermediate commits, and updated the commit message to reflect the additional function in PGConnection. Patch itself is the same as before the squash.

Once this runs through CI I'll merge it in.

[sehrope]

No clue why one of the test matrix entries never started and the other has been hanging for 1h30m. Going to try manually bouncing them. Probably some transient GitHub Actions issue.

[sehrope]

The ARM matrix is taking forever and my suspicion is that SecureRandom is trying to use /dev/random and the ARM runner does not have enough entropy. On modern kernels it shouldn't hang after boot but I bet it's an older one that still treats them separately. The entropy pool must be drained and it's hanging waiting for random network activity to the host to give it enough jitter to supply more bits.

Looks like this is the first we're directly using that class in the project:

$ git ls-files | grep .java | xargs grep SecureRandom
pgjdbc/src/main/java/org/postgresql/util/PasswordUtil.java:import java.security.SecureRandom;
pgjdbc/src/main/java/org/postgresql/util/PasswordUtil.java:  private static class SecureRandomHolder {
pgjdbc/src/main/java/org/postgresql/util/PasswordUtil.java:    static final SecureRandom INSTANCE = new SecureRandom();
pgjdbc/src/main/java/org/postgresql/util/PasswordUtil.java:  private static SecureRandom getSecureRandom() {
pgjdbc/src/main/java/org/postgresql/util/PasswordUtil.java:    return SecureRandomHolder.INSTANCE;
pgjdbc/src/main/java/org/postgresql/util/PasswordUtil.java:      SecureRandom rng = getSecureRandom();
pgjdbc/src/test/java/org/postgresql/test/util/PasswordUtilTest.java:import java.security.SecureRandom;
pgjdbc/src/test/java/org/postgresql/test/util/PasswordUtilTest.java:  private static final SecureRandom rng = new SecureRandom();

Should be able to force it to use /dev/urandom via: -Djava.security.egd=file:/dev/./urandom

Going to see if I can figure out how to inject it just for that one build.

I wonder if this caused less (but similar) slow down in other tests simply from the TLS stack pulling from the same source.

[sehrope]

I pushed a fix for the RNG thing that applies the JVM flag to all environments generated by the matrix. I couldn't figure an easy way of only applying it to the ARM builds and it should be harmless elsewhere.

With the fix it passed through CI and did execute on an ARM combo. Unfortunately due to the matrix randomization it's not exactly the same combination as before so I don't know 100% if the fix did anything (JDK 22 failed but JDK 8 succeeded).

I'm going to merge this and we'll see if anything else comes up later.

[vlsi]

Why adding empty comments?

[sehrope]

Why show up with feedback on lines that have not changed since the PR was first opened after the PR is merged?

Are you just looking for something to nitpick?

[vlsi]

You did not fix javadocs (see #3082 (comment)), and I thought it would be easier and faster to just wait for you to merge the PR and then fix the style issues

[vlsi]

It looks like the method is not reliable from the users' perspective.
What does driver's default mean?
What if somebody uses the method, and then they upgrade the driver some time later. Is the driver allowed to change the default encoding method?

Apparently, for backward compatibility, we can't change the method. In that regard, encodePassword duplicates encodeScramSha256.

[sehrope]

It looks like the method is not reliable from the users' perspective. What does driver's default mean?

It means the driver encodes the password with whatever the latest version of password encoding the driver wants, using the defaults built into the driver.

What if somebody uses the method, and then they upgrade the driver some time later. Is the driver allowed to change the default encoding method?

Yes. That's exactly the point. So that code targeting that method uses the latest, most recommended method of encoding passwords without being connected to a specific server.

Apparently, for backward compatibility, we can't change the method. In that regard, encodePassword duplicates encodeScramSha256.

Compatibility with what? We haven't haven't released anything yet. Are you suggesting changing the signature?

It doesn't duplicate the SCRAM-SHA-256 function, it it delegates to it because that's the current driver default.

If in the future if the SCRAM-SHA-256 default is replaced with SCRAM-SHA-512 or something else entirely, we'd change that delegation.

[vlsi]

If in the future if the SCRAM-SHA-256 default is replaced with SCRAM-SHA-512 or something else entirely, we'd change that delegation

If we ever make such a change, then we effectively break backward compatibility. That means we can't easily make such a change.

So, please suggest what is the use case for having "driver's default" encodePassword method. Why add the method assuming there's not a single use case for it?

[sehrope]

If we ever make such a change, then we effectively break backward compatibility. That means we can't easily make such a change.

We don't break anything because the definition of that method is encoding the password with whatever the driver considers to be the most secure and recommended approach. The user is delegating to this driver, as the de facto Java driver for PosgreSQL, to make a determination of how the user should be encoding passwords.

If a user wants to use a specific algo or parameters then there's other overloads to use instead.

So, please suggest what is the use case for having "driver's default" encodePassword method. Why add the method assuming there's not a single use case for it?

It's in the original PR description:

Splitting out the encoding allows the same functions to be used for CREATE USER ... (again without passing the credentials in plaintext).

A user can leverage that to generate their own SQL that involves encoding passwords.

[davecramer]

If we ever make such a change, then we effectively break backward compatibility. That means we can't easily make such a change.

We don't break anything because the definition of that method is encoding the password with whatever the driver considers to be the most secure and recommended approach. The user is delegating to this driver, as the de facto Java driver for PosgreSQL, to make a determination of how the user should be encoding passwords.

I think the issue is that if the server uses SCRAM-512 for the latest version and previous versions use SCRAM-256 using the latest driver would fail on older versions of the server.

[sehrope]

I think you mean if the driver is bumped to SCRAM-512 and the server does not yet support it, then it would fail.

Yes, that's expected because the output of that method is not for a particular server. It's for generating literals for the encoded password using the latest recommended method per the driver. I'd see it being used by something that is generating it's own SQL, potentially for future execution out of band. The tie in to the driver is that the driver, as the de factor Java driver for PostgreSQL, is aware of the recommended password encoding.

[davecramer]

I think you mean if the driver is bumped to SCRAM-512 and the server does not yet support it, then it would fail.

Yes, that's expected because the output of that method is not for a particular server. It's for generating literals for the encoded password using the latest recommended method per the driver. I'd see it being used by something that is generating it's own SQL, potentially for future execution out of band. The tie in to the driver is that the driver, as the de factor Java driver for PostgreSQL, is aware of the recommended password encoding.

No, what I mean is server version 17 comes out with SCRAM-512. The driver uses SCRAM-512 as the default and now the default only works for server version 17.

[sehrope]

I don't follow ... isn't that the same situation I described in my previous comment?

[davecramer]

I suppose it is, but we can't break backward compatibility. If a user upgrades the driver their code should continue to work.
Generally if we do a major version upgrade we would mention that we have breaking changes but a breaking change to a default seems wrong somehow

[sehrope]

I guess I don't see it as a break as the functional meaning of the method ("encode my password using the recommended...) has not changed. I see it like the following changing over server versions:

CREATE USER foo WITH PASSWORD 'abcd1234';
SELECT passwd FROM pg_shadow WHERE usename = 'foo'

It'd be md5 encoded in <=10 and some variant SCRAM after that. Though that's not as user facing so maybe not the best example.

The original goal was to have a method that users could rely on when making things like SQL script generating tooling that they know will always be the latest recommendation. That way when SCRAM-SHA-256 is replaced with SCRAM-SHA-512 or something else entirely, a user that bumps their driver to the latest pgjdbc would automatically get the newer recommendation.

If that doesn't make sense as valid use case or if you foresee misuse of it causing complication, then let's remove it.