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.

Sign up for GitHub

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
Merged

Make PrivateKeyUtils#load method file extension agnostic #267

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 18 additions & 11 deletions jsign-crypto/src/main/java/net/jsign/PrivateKeyUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,26 +54,33 @@ private PrivateKeyUtils() {

/**
* Load the private key from the specified file. Supported formats are PVK and PEM,
* encrypted or not. The type of the file is inferred from its extension (<code>.pvk</code>
* for PVK files, <code>.pem</code> for PEM files).
*
* encrypted or not. The type of the file is inferred by trying the supported formats
* in sequence until one parses successfully.
*
* @param file the file to load the key from
* @param password the password protecting the key
* @return the private key loaded
* @throws KeyException if the key cannot be loaded
*/
public static PrivateKey load(File file, String password) throws KeyException {
Exception pemParseException;
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 •
edited
Loading

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.

return PVK.parse(file, password);
} else if (file.getName().endsWith(".pem")) {
return readPrivateKeyPEM(file, password != null ? password.toCharArray() : null);
}
return readPrivateKeyPEM(file, password != null ? password.toCharArray() : null);
} catch (Exception e) {
pemParseException = e;
}

Exception pvkParseException;
try {
return PVK.parse(file, password);
} catch (Exception e) {
throw new KeyException("Failed to load the private key from " + file, e);
pvkParseException = e;
}

throw new IllegalArgumentException("Unsupported private key format (PEM or PVK file expected");

KeyException keyException = new KeyException("Failed to load the private key from " + file + " (valid PEM or PVK file expected)");
keyException.addSuppressed(pemParseException);
keyException.addSuppressed(pvkParseException);
throw keyException;
}

/**
Expand Down
14 changes: 12 additions & 2 deletions jsign-crypto/src/test/java/net/jsign/PrivateKeyUtilsTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@
import java.io.File;
import java.io.FileWriter;
import java.math.BigInteger;
import java.nio.file.Files;
import java.nio.file.Path;
import java.security.KeyException;
import java.security.PrivateKey;
import java.security.interfaces.ECPrivateKey;
Expand Down Expand Up @@ -54,6 +56,14 @@ public void testLoadPKCS1PEM() throws Exception {
testLoadPEM(new File("target/test-classes/keystores/privatekey.pkcs1.pem"), null);
}

@Test
public void testLoadPKCS1PEMNonPEMExtension() throws Exception {
File targetFile = new File("target/test-classes/keystores/privatekey.pkcs1.pem.key");
Files.copy(new File("target/test-classes/keystores/privatekey.pkcs1.pem").toPath(), targetFile.toPath());

testLoadPEM(targetFile, null);
}

@Test
public void testLoadEncryptedPKCS1PEM() throws Exception {
testLoadPEM(new File("target/test-classes/keystores/privatekey-encrypted.pkcs1.pem"), "password");
Expand All @@ -71,7 +81,7 @@ private void testLoadPEM(File file, String password) throws Exception {
@Test
public void testLoadWrongPEMObject() {
Exception e = assertThrows(KeyException.class, () -> PrivateKeyUtils.load(new File("target/test-classes/keystores/jsign-test-certificate.pem"), null));
assertEquals("message", "Unsupported PEM object: X509CertificateHolder", e.getCause().getMessage());
assertEquals("message", "Unsupported PEM object: X509CertificateHolder", e.getSuppressed()[0].getMessage());
}

@Test
Expand All @@ -82,7 +92,7 @@ public void testLoadEmptyPEM() throws Exception {
writer.close();

Exception e = assertThrows(KeyException.class, () -> PrivateKeyUtils.load(file, null));
assertTrue(e.getCause().getMessage().startsWith("No key found in"));
assertTrue(e.getSuppressed()[0].getMessage().startsWith("No key found in"));
}

@Test
Expand Down
Loading