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

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

Conversation

muskaan62
Copy link

client certificate without password (the test you provide for this scenario) [working]
client certificate with password [working]
client certificate with private key [working]
client certificate with private key and key password (i.e means encrypted key) [bug]
The above 3 ways are working fine but fourth is not ..
The logic for decrypt the private key is restricted to pvk though providing pvk key also its not working types and RSA private keys when decrypting also failing so added a fix for this. related to https://github.com/microsoft/mssql-jdbc/issues/2530

@muskaan62
Copy link
Author

@lilgreenbird FYI

@lilgreenbird
Copy link
Contributor

thanks the team will take a look

@muskaan62
Copy link
Author

muskaan62 commented Nov 8, 2024

Thanks @lilgreenbird @Jeffery-Wasty can we expect this fix in 12.8.1 version or it will be available with new version of driver?

@lilgreenbird
Copy link
Contributor

@muskaan62 please see above you will need to agree to CLA before the PR can be considered and tests run on it

Comment on lines +60 to +64
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;
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

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

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?

@muskaan62
Copy link
Author

@microsoft-github-policy-service agree company="Gaian Solutions"

@muskaan62
Copy link
Author

muskaan62 commented Nov 25, 2024

@muskaan62 please see above you will need to agree to CLA before the PR can be considered and tests run on it

@lilgreenbird Done, lemme know anything else to be done from my end.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In progress
4 participants