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

Jump to bottom

Add support for password-based encryption scheme 2 params (PBES2) #13539

Merged
merged 4 commits into from
Aug 22, 2023
Merged

Add support for password-based encryption scheme 2 params (PBES2) #13539

merged 4 commits into from
Aug 22, 2023

Conversation

xiezhaokun
Copy link
Contributor

Motivation:

Add support for password-based encryption scheme 2 params (PBES2)

Modification:

Describe the modifications you've done.

Result:

Fixes #13536

If there is no issue then describe the changes introduced by this PR.

@hyperxpro
Copy link
Contributor

Please also add unit tests.

@normanmaurer
Copy link
Member

@xiezhaokun as @hyperxpro said... can you please add a unit test ?

normanmaurer
normanmaurer reviewed Aug 16, 2023
View reviewed changes
handler/src/main/java/io/netty/handler/ssl/SslContext.java
@@ -102,6 +103,8 @@ public abstract class SslContext {

private final boolean startTls;
private final AttributeMap attributes = new DefaultAttributeMap();
private static final String OID_PKCS5_PBES2 = "1.2.840.113549.1.5.13";
private static final String PBES2 = "PBES2";
Copy link
Member

Choose a reason for hiding this comment

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

can you add a comment to explain where these values come from ?

@chrisvest
Copy link
Contributor

Sounds like PKCS12 is a bit of a messy landscape but I'm told PBES2 is the "state of the art" there.

A good test for this would be to generate a .p12 file with openssl, that uses PBES2, and verify that the code can load it and use it. Also include the shell script that generated the file, so we don't loose that information.

@normanmaurer
Copy link
Member

@xiezhaokun @chrisvest I added a test-case

@normanmaurer normanmaurer added this to the 4.1.97.Final milestone Aug 21, 2023
@normanmaurer
Copy link
Member

/cc @hyperxpro

hyperxpro
hyperxpro reviewed Aug 21, 2023
View reviewed changes
handler/src/main/java/io/netty/handler/ssl/SslContext.java
private static String getPBEAlgorithm(EncryptedPrivateKeyInfo encryptedPrivateKeyInfo) {
AlgorithmParameters parameters = encryptedPrivateKeyInfo.getAlgParameters();
String algName = encryptedPrivateKeyInfo.getAlgName();
// Java 8 ~ 16 returns OID_PKCS5_PBES2
Copy link
Contributor

@hyperxpro hyperxpro Aug 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Does this mean Java < 8 are not supported?

Copy link
Member

Choose a reason for hiding this comment

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

@hyperxpro good point... maybe we should just add a version check as well. Let me do this

@hyperxpro
Copy link
Contributor

@chrisvest

Error:  Failures: 
Error:    SingleThreadEventLoopTest.scheduleLaggyTaskAtFixedRateB:236->testScheduleLaggyTaskAtFixedRate:276 
Expected: is a value less than or equal to <10000000L>
     but: <113421600L> was greater than <10000000L>

hyperxpro
hyperxpro approved these changes Aug 21, 2023
View reviewed changes
Copy link
Contributor

@hyperxpro hyperxpro left a comment

Choose a reason for hiding this comment

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

:)

@normanmaurer normanmaurer merged commit a026444 into netty:4.1 Aug 22, 2023
11 of 14 checks passed
normanmaurer added a commit that referenced this pull request Aug 22, 2023
@xiezhaokun @normanmaurer @chrisvest
Add support for password-based encryption scheme 2 params (PBES2) (#1…
0c7cf3f 
…3539)

Motivation:

Add support for password-based encryption scheme 2 params (PBES2)

Modification:

Describe the modifications you've done.

Result:

Fixes #13536

Co-authored-by: Norman Maurer <norman_maurer@apple.com>
Co-authored-by: Chris Vest <christianvest_hansen@apple.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@normanmaurer normanmaurer normanmaurer left review comments

@chrisvest chrisvest chrisvest approved these changes

@hyperxpro hyperxpro hyperxpro approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
4.1.97.Final
Development

Successfully merging this pull request may close these issues.

Add support for password-based encryption scheme 2 params (PBES2)
4 participants
@xiezhaokun @hyperxpro @normanmaurer @chrisvest