Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make PrivateKeyUtils#load method file extension agnostic #267

Merged
merged 1 commit into from
Dec 8, 2024

Conversation

AlexTMjugador
Copy link
Contributor

@AlexTMjugador AlexTMjugador commented Dec 7, 2024

As outlined in #264, this is achieved by trying to parse the key file according to one of the supported formats in sequence until one works. Given that there are only two supported formats at the moment, and that PEM files are attempted first and more common in the wild than PVK, this approach should have good enough performance. Because a Java exception can only have a single cause, I've attached the underlying parse exceptions to the higher-level KeyException as supressed exceptions. These get displayed to consumers on e.g. stacktraces, allowing them to know what exactly went wrong when parsing either format.

While at it, I've added a test to ensure that this extension-agnostic behavior is maintained over time. Also, the PrivateKeyUtils.java file had inconsistent line endings with respect to the rest of the repository, so I fixed that, causing Git to report a big diff there.

Resolves #264.

Private key load errors display preview

Note how the underlying exceptions are marked as "suppressed", but still retain their message, cause and other diagnostic data.

$ java -jar jsign/target/jsign-7.0-SNAPSHOT.jar sign \
  --certfile jsign-core/src/test/resources/keystores/jsign-test-certificate.pem \
  --keyfile jsign-core/src/test/resources/keystores/privatekey.asc \
  jsign-core/src/test/resources/wineyes.exe
jsign: Failed to load the keystore 
java.security.KeyStoreException: Failed to load the private key from jsign-core/src/test/resources/keystores/privatekey.asc
        at net.jsign.KeyStoreType$1.getKeystore(KeyStoreType.java:95)
        at net.jsign.KeyStoreBuilder.build(KeyStoreBuilder.java:285)
        at net.jsign.SignerHelper.build(SignerHelper.java:327)
        at net.jsign.SignerHelper.sign(SignerHelper.java:450)
        at net.jsign.SignerHelper.execute(SignerHelper.java:305)
        at net.jsign.JsignCLI.execute(JsignCLI.java:213)
        at net.jsign.JsignCLI.main(JsignCLI.java:57)
Caused by: java.security.KeyException: Failed to load the private key from jsign-core/src/test/resources/keystores/privatekey.asc (valid PEM or PVK file expected)
        at net.jsign.PrivateKeyUtils.load(PrivateKeyUtils.java:80)
        at net.jsign.KeyStoreType$1.getKeystore(KeyStoreType.java:93)
        ... 6 more
        Suppressed: net.jsign.bouncycastle.util.encoders.DecoderException: unable to decode base64 string: invalid characters encountered in base64 data
                at net.jsign.bouncycastle.util.encoders.Base64.decode(Unknown Source)
                at net.jsign.bouncycastle.util.io.pem.PemReader.loadObject(Unknown Source)
                at net.jsign.bouncycastle.util.io.pem.PemReader.readPemObject(Unknown Source)
                at net.jsign.bouncycastle.openssl.PEMParser.readObject(Unknown Source)
                at net.jsign.PrivateKeyUtils.readPrivateKeyPEM(PrivateKeyUtils.java:119)
                at net.jsign.PrivateKeyUtils.load(PrivateKeyUtils.java:68)
                ... 7 more
        Caused by: java.io.IOException: invalid characters encountered in base64 data
                at net.jsign.bouncycastle.util.encoders.Base64Encoder.decode(Unknown Source)
                ... 13 more
        Suppressed: java.lang.IllegalArgumentException: PVK header signature not found
                at net.jsign.PVK.parse(PVK.java:71)
                at net.jsign.PVK.parse(PVK.java:61)
                at net.jsign.PrivateKeyUtils.load(PrivateKeyUtils.java:75)
                ... 7 more
Try `java -jar jsign.jar --help' for more information.

@ebourg
Copy link
Owner

ebourg commented Dec 7, 2024

It looks good, thanks a lot. Using suppressed exceptions is as good idea.

I know the line endings are inconsistent, but I'd prefer to keep them as is to keep the diff readable by default. I'll probably normalize the whole project at some point in the future anyway.

*/
public static PrivateKey load(File file, String password) throws KeyException {
try {
if (file.getName().endsWith(".pvk")) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about keeping the extension based detection and then falling back to the other method?

Copy link
Contributor Author

@AlexTMjugador AlexTMjugador Dec 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I considered that approach for a while, and realized it wouldn't handle cases where key files have misleading extensions (e.g., a PVK key with a .pem extension, or vice versa). Ultimately, I believe that allowing the key to be parsed regardless of the file extension, even if it's misleading, is more user-friendly. That said, I don’t feel strongly about this and am open to changing it if requested 😄

Edit: nevermind the reason above, it's kinda late over here and didn't really consider how a misleading extension could be satisfactorily handled anyway by falling back to the other key type... I think that could work well, but I'm not sure if it's worth the added complexity; PVK and PEM files have unambiguous headers with respect to the other.

@ebourg
Copy link
Owner

ebourg commented Dec 8, 2024

That's fine, let's keep it this way. Could you just squash your commits into one before I merge the PR please?

@AlexTMjugador AlexTMjugador force-pushed the feat/ext-agnostic-pkey-load branch from ca7e356 to 941c937 Compare December 8, 2024 13:48
@AlexTMjugador
Copy link
Contributor Author

Okay! I've just squashed the commits into one 👍

.gitignore Outdated Show resolved Hide resolved
As outlined in #264, this is achieved by trying to parse the key file
according to one of the supported formats in sequence until one works.
Given that there are only two supported formats at the moment, and that
PEM files are attempted first and more common in the wild than PVK, this
approach should have good enough performance. Because a Java exception
can only have a single cause, I've attached the underlying parse
exceptions to the higher-level `KeyException` as supressed exceptions.
These get displayed to consumers on e.g. stacktraces, allowing them to
know what exactly went wrong when parsing either format.

While at it, I've added a test to ensure that this extension-agnostic
behavior is maintained over time.

Resolves #264.
@AlexTMjugador AlexTMjugador force-pushed the feat/ext-agnostic-pkey-load branch from 941c937 to 18e4228 Compare December 8, 2024 17:45
@ebourg
Copy link
Owner

ebourg commented Dec 8, 2024

Perfect, thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consider implementing magic-based private key file type detection, or a CLI parameter to specify the type
2 participants