Add Windows and MacOS native certificate support #3124
Conversation
Add three new SSLSocketFactory implementations to support native keystores on Windows and Mac. org.postgresql.ssl.MSCAPILocalMachineSSLFactory org.postgresql.ssl.MSCAPISSLFactory org.postgresql.ssl.KeychainSSLFactory Add the sslsubject parameter to limit the chosen certificate where more than one certificate might match for a given connection.
|
Would it be possible to approve the workflows outstanding on this PR? |
Yes, sorry. I should have realized they weren't running |
|
|
||
| try { | ||
|
|
||
| keyManagerFactory.init(keyStore, keyPassphrase); |
There was a problem hiding this comment.
I'm getting a test failure, more specifically a compilation failure that I don't understand.
[Task :postgresql:compileJava] [argument] incompatible argument for parameter arg1 of KeyManagerFactory.init.
keyManagerFactory.init(keyStore, keyPassphrase);
^
found : @initialized @nonnull char @FBCBottom @nullable []
keyPassphrase is a char[], the extra annotations seem sane.
Can you confirm for me if possible what specifically is wrong with this line so I can fix it?
There was a problem hiding this comment.
These warnings might be relevant:
> Task :postgresql:compileJava
warning: /home/runner/work/pgjdbc/pgjdbc/config/checkerframework/Assert.astub:(line 1,col 1): Package not found: org.junit
warning: /home/runner/work/pgjdbc/pgjdbc/config/checkerframework/Assert.astub:(line 6,col 1): Type not found: org.junit.Assert
There was a problem hiding this comment.
Quick ping on this comment - I don't understand how it compiles everywhere else but not in this specific case. Is someone familiar with CheckerFramework able to confirm?
https://github.com/pgjdbc/pgjdbc/actions/runs/8323877965/job/23147420376?pr=3124
| keyManagerFactory.init(keyStore, keyPassphrase); | ||
|
|
||
| } catch (NoSuchAlgorithmException ex) { | ||
| throw new PSQLException(GT.tr("Could not find a java cryptographic algorithm: {0}.", |
|
|
||
| } catch (NoSuchAlgorithmException ex) { | ||
| throw new PSQLException(GT.tr("Could not find a java cryptographic algorithm: {0}.", | ||
| ex.getMessage()), PSQLState.CONNECTION_FAILURE, ex); |
There was a problem hiding this comment.
Isn't ex.getMessage() redudant since the target message will be recursively built?
There was a problem hiding this comment.
Java is fixed.
The ex.getMessage() is there on purpose so that the whole error message is on one single line.
The target audience for this patch are data scientists who aren't Java developers, nor are they experts in SSL. They see the message "Could not find a java cryptographic algorithm", they don't understand it (fair: "which crypto algorithm?"), and I have to pick it all apart for them, first helping them give me the whole exception (I'll get a screenshot of the top few lines, then I'll explain how to cut and paste), then finding the needle in the haystack of "caused by", then googling for them.
With the whole message on one line, they have one line to google themselves, on the top line, and lots and lots of make work is avoided.
| trustStoreType), PSQLState.CONNECTION_FAILURE, ex); | ||
| } catch (NoSuchAlgorithmException ex) { | ||
| throw new PSQLException(GT.tr("Could not find a java cryptographic algorithm: {0}.", | ||
| ex.getMessage()), PSQLState.CONNECTION_FAILURE, ex); |
| throw new PSQLException(GT.tr("SSL truststore {0} could not be loaded.", | ||
| trustStoreType), PSQLState.CONNECTION_FAILURE, ex); | ||
| } catch (NoSuchAlgorithmException ex) { | ||
| throw new PSQLException(GT.tr("Could not find a java cryptographic algorithm: {0}.", |
|
|
||
| } catch (NoSuchAlgorithmException ex) { | ||
| throw new PSQLException(GT.tr("Could not find a java cryptographic algorithm: {0}.", | ||
| ex.getMessage()), PSQLState.CONNECTION_FAILURE, ex); |
|
|
||
| } catch (NoSuchAlgorithmException ex) { | ||
| throw new PSQLException(GT.tr("Could not find a java cryptographic algorithm: {0}.", | ||
| ex.getMessage()), PSQLState.CONNECTION_FAILURE, ex); |
|
|
||
| } catch (IllegalArgumentException ex) { | ||
| throw new PSQLException(GT.tr("Could not parse sslsubject {0}: {1}.", | ||
| sslsubject, ex.getMessage()), PSQLState.CONNECTION_FAILURE, ex); |
|
Another gentle bump - is it possible to trigger workflows? |
|
sorry about that |
|
@minfrin You can enable actions to run on your pgjdbc fork as well. This is particularly useful if you want to try running something in CI without opening a PR in this repo. The GitHub Actions should work with no issues on your fork. The Windows / AppVeyor stuff is a bit finicky but in theory should work if you set up an account there as well. (Note that I'm not suggesting opening this PR and running the CI on pgjdbc/pgjdbc was a bad idea ... I'm just suggesting an alternative for the future if you're trying out something else) |
|
@minfrin I took the liberty of fixing the checker errors |
Much appreciated, thank you. |
Add three new SSLSocketFactory implementations to support native keystores on Windows and Mac.
org.postgresql.ssl.MSCAPILocalMachineSSLFactory
org.postgresql.ssl.MSCAPISSLFactory
org.postgresql.ssl.KeychainSSLFactory
Add the sslsubject parameter to limit the chosen certificate where more than one certificate might match for a given connection.
All Submissions:
New Feature Submissions:
./gradlew styleCheckpass ?Changes to Existing Features: