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

fix issue-2530 encrypted private keys is not working issue #2532

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix issue-2530 encrypted private keys is not working issue #2532

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
19 changes: 17 additions & 2 deletions src/main/java/com/microsoft/sqlserver/jdbc/SQLServerCertificateUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,11 @@
import org.bouncycastle.openssl.PEMParser;
import org.bouncycastle.openssl.jcajce.JcaPEMKeyConverter;
import org.bouncycastle.openssl.jcajce.JcePEMDecryptorProviderBuilder;
import org.bouncycastle.openssl.jcajce.JceOpenSSLPKCS8DecryptorProviderBuilder;
import org.bouncycastle.operator.InputDecryptorProvider;
import org.bouncycastle.operator.OperatorCreationException;
import org.bouncycastle.pkcs.PKCS8EncryptedPrivateKeyInfo;
import org.bouncycastle.pkcs.PKCSException;
Comment on lines +60 to +64
Copy link
Collaborator

Choose a reason for hiding this comment

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

You'll need to fix the issue without using Bouncy Castle. Try to use java.security instead. We want to limit Bouncy Castle use to Java 8 and drop it when Java 8 support ends.

Copy link
Author

Choose a reason for hiding this comment

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

@David-Engel Thanks..The existing code is already using bouncy castle for private keys, i've just added one more if condition for encrypted keys.

Copy link
Collaborator

@David-Engel David-Engel Nov 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I understand that. But the existing code is only used within the context of the Always Encrypted with secure enclaves feature with Java 8 only, (https://github.com/microsoft/mssql-jdbc/blob/main/README.md#dependencies)

We can't extend the use of Bouncy Castle for other contexts/features without approval from our security team, as they didn't like the use of an external encryption library at all. We only got approval because it would be dropped when support for Java 8 ends.

If you have to limit the fix to Java 11+ only, I'm fine with that. We just need to update the docs appropriately.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks @David-Engel I tried to use java security but looks with java 11 we do not have security providers to decrypt the keys and has to fall back on bouncy castle only...Yes, i want to limit this fix to java 11 only. please do lemme know if anything else needs to be done be from my end.

Copy link
Collaborator

@David-Engel David-Engel Nov 26, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

To clarify, I meant: If you have to limit this fix to Java 11+ only without using Bouncy Castle, I'm fine with that. I was assuming Bouncy Castle was only required for Java 8.

We can't extend our usage of Bouncy Castle to Java 11.

Can you fix it for Java 17+ without Bouncy Castle?

Copy link
Contributor

Choose a reason for hiding this comment

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

Bouncy Castle has security issues it is not recommended to be used and is not approved by Microsoft. We are looking into a solution that does not require Bouncy Castle.

Copy link

Choose a reason for hiding this comment

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

@lilgreenbird , thanks for the reply.

Can you pls help me more details around the security issues being present in the bouncy castle library? That will help me to see if I also need to have BC in my app or not. The more details around it would be appreciated.

Copy link

Choose a reason for hiding this comment

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

Any response here @lilgreenbird or @David-Engel ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not aware of any security issues in BC. For us in Microsoft, it's just not an approved encryption library (barring limited exceptions) for use in MS products.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks @David-Engel If i Understood correctly, then you are saying to fix this issue for java 17+ and the issue will still exist for java 8 and java 11?? We are using Java 11 in our applications and require a fix that works specifically for this version. I attempted to address the issue using the Java Security API, but encountered errors related to the provider. Upon investigation, it appears these errors stem from a known bug in JDK 11 (JDK-8231581) that remains unresolved. we need a solution for Java 11. Most suggestions I found online recommend using Bouncy Castle. If you have any references or approaches to resolve this for Java 11, I would be glad to explore them. Thank you for your help!

@muskaan62 I'm just saying we can't expand the scenarios under which we use BC. If this means the fix is limited, so be it.



final class SQLServerCertificateUtils {
Expand Down Expand Up @@ -318,6 +323,8 @@ private static void logSuccessMessage(String nameInCert, String hostName) {
private static final String CLIENT_KEY = "client-key";
// PKCS#1 format
private static final String PEM_RSA_PRIVATE_START = "-----BEGIN RSA PRIVATE KEY-----";
private static final String PEM_RSA_ENCRYPTED_PRIVATE_START =
"-----BEGIN ENCRYPTED PRIVATE KEY-----";
// PVK format
private static final long PVK_MAGIC = 0xB0B5F11EL;
private static final byte[] RSA2_MAGIC = {82, 83, 65, 50};
Expand Down Expand Up @@ -384,13 +391,19 @@ private static PrivateKey loadPrivateKeyFromPKCS1(String key, String keyPass) th
Object object = pemParser.readObject();
JcaPEMKeyConverter converter = new JcaPEMKeyConverter().setProvider("BC");
KeyPair kp;
if (object instanceof PEMEncryptedKeyPair && keyPass != null) {
if (object instanceof PKCS8EncryptedPrivateKeyInfo && keyPass != null) {
JceOpenSSLPKCS8DecryptorProviderBuilder builder = new JceOpenSSLPKCS8DecryptorProviderBuilder();
InputDecryptorProvider decryptorProvider = builder.build(keyPass.toCharArray());
return converter.getPrivateKey(((PKCS8EncryptedPrivateKeyInfo)object).decryptPrivateKeyInfo(decryptorProvider));
} else if (object instanceof PEMEncryptedKeyPair && keyPass != null) {
PEMDecryptorProvider decProv = new JcePEMDecryptorProviderBuilder().build(keyPass.toCharArray());
kp = converter.getKeyPair(((PEMEncryptedKeyPair) object).decryptKeyPair(decProv));
} else {
kp = converter.getKeyPair((PEMKeyPair) object);
}
return kp.getPrivate();
} catch (PKCSException | OperatorCreationException e) {
throw new RuntimeException(e);
}
}

Expand All @@ -400,6 +413,7 @@ private static PrivateKey loadPrivateKeyFromPVK(String keyPath,
ByteBuffer buffer = ByteBuffer.allocate((int) f.length());

try (FileInputStream in = new FileInputStream(f); FileChannel channel = in.getChannel()) {
channel.read(buffer);
((Buffer) buffer.order(ByteOrder.LITTLE_ENDIAN)).rewind();

long magic = buffer.getInt() & 0xFFFFFFFFL;
Expand Down Expand Up @@ -465,7 +479,8 @@ static PrivateKey loadPrivateKey(String privateKeyPemPath,

if (privateKeyPem.contains(PEM_PRIVATE_START)) { // PKCS#8 format
return loadPrivateKeyFromPKCS8(privateKeyPem);
} else if (privateKeyPem.contains(PEM_RSA_PRIVATE_START)) { // PKCS#1 format
} else if (privateKeyPem.contains(PEM_RSA_PRIVATE_START) ||
privateKeyPem.contains(PEM_RSA_ENCRYPTED_PRIVATE_START)) { // PKCS#1 format
return loadPrivateKeyFromPKCS1(privateKeyPem, privateKeyPassword);
} else {
return loadPrivateKeyFromPVK(privateKeyPemPath, privateKeyPassword);
Expand Down