-
Notifications
You must be signed in to change notification settings - Fork 6.2k
8346129: Simplify EdDSA & XDH curve name usage #23647
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
Conversation
# Conflicts: # src/java.base/share/classes/sun/security/util/AbstractAlgorithmConstraints.java # src/java.base/share/classes/sun/security/util/KeyUtil.java
|
👋 Welcome back ascarpino! A progress list of the required criteria for merging this PR into |
|
@ascarpino This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be: You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 774 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. ➡️ To integrate this PR with the above commit message to the |
|
|
|
@ascarpino The following label will be automatically applied to this pull request:
When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command. |
Webrevs
|
| private static List<String> aliasEd25519 = null; | ||
| private static List<String> aliasXDH = null; | ||
| private static List<String> aliasX25519 = null; | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't it be more concise to create a static algorithm-to-aliases map and then make getAliases() to do the map lookup?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's purely a memory allocation solution here. If I make a Map, I have to populate the entries at initialization. Right now XDH and EdDSA are very unlikely to be disabled as they are relatively new algorithms.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, I thought it was probably done to minimize memory usage. But all those string literals will be stored in a string pool at runtime and simply re-used when we allocate a list with them. So real memory saving should be very small comparing to a static map.
| // Check `algorithm` against disabled algorithms and their aliases | ||
| for (String a : algorithms) { | ||
| if (algorithm.equalsIgnoreCase(a) || | ||
| getAliases(a).contains(algorithm)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do case-insensitive match for the algorithm itself but then we do case-sensitive aliases lookup and case-sensitive match for the aliases contains call?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, thanks.
| /* | ||
| If the key is a sub-algorithm of a larger group of algorithms, this method | ||
| will return that sub-algorithm. For example, key.getAlgorithm() returns | ||
| "EdDSA", but the underlying key maybe "Ed448". For | ||
| DisabledAlgorithmConstraints (DAC), this distinction is important. | ||
| "EdDSA" means all curves for DAC, but when using it with | ||
| KeyPairGenerator, EdDSA means Ed25519. | ||
| */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use the common form for comments here, i.e.:
/**
* ...
*/
| return switch (key) { | ||
| case EdECKey ed -> ed.getParams().getName(); | ||
| case XECKey xe -> ((NamedParameterSpec) xe.getParams()).getName(); | ||
| default -> key.getAlgorithm(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you also want to add cases for ML-KEM and ML-DSA keys?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wangweij is planning on name usage for those. I'm focusing on these older curves.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They are already defined. I think you just want to add something like:
If (key.getAlgorithm().equals("ML-KEM") || key.getAlgorithm().equals("ML-DSA")) {
return ((NamedParameterSpec) key.getParams()).getName();
}
Not urgent, but useful if one of these algorithms were to weaken or be broken for some reason.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or what about this?
if (key instanceof AsymmetricKey ak) {
if (ak.getParams() instanceof NamedParameterSpec nps) {
return nps.getName();
}
}
return key.getAlgorithm();
AsymmetricKey was introduced to make our lives easier.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I stayed away from that because this is likely being backported
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, well our backporters are usually good about extracting only what is necessary, but if not, can you file another issue to add support for disabling PQC algorithms?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed my mind and will use the suggestion.
| /* | ||
| If the key is a sub-algorithm of a larger group of algorithms, this method | ||
| will return that sub-algorithm. For example, key.getAlgorithm() returns | ||
| "EdDSA", but the underlying key maybe "Ed448". For |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/maybe/may be/
| } | ||
| case "Ed25519" -> { | ||
| if (aliasEd25519 == null) { | ||
| aliasEd25519 = List.of("EdDSA", "Ed25519"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm. Should disabling Ed25519 also disable EdDSA? I can see the reverse, but isn't Ed25519 meant to be a specific curve for EdDSA?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is complicated by KeyPairGenerator.getInstance("EdDSA") returning an Ed25519 key
If someone were to check permits() with "EdDSA" the above code recognizes that "Ed25519" on the disabled algorithm list overlaps with "EdDSA". This is the first test in the test coded included in the PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we call permits before instantiating a KeyPairGenerator? What if people call kpg.initialize(NPS.Ed448) after the instantiation?
In reality, I think it depends on how many permits calls there are. Modern algorithms have the key same algorithm name and signature algorithm name. When a signature operation is carried out, do we check on both the signature algorithm and the key? It seems only checking on the key is enough. It's actually more precise, since you can get the exact parameter set name there. This is why I asked if the method is "never called on a family algorithm name". When checking a key, if we always call permits on the parameter set name, we get the precise result.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
permits() are used in situations for jdk[tls|certpath|jar].disabledAlgorithms, and the SSLAlgorithmConstraints. It's not called for APIs like KPG, Signature, etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's what I meant. Suppose in TLS when you verify a signature and you call permits on both the signature algorithm name and the key used to init the signature, it's OK if only one fails.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it should check both the signature algorithm and key algorithm in one permits call for signatures.
| private static List<String> aliasEdDSA = null; | ||
| private static List<String> aliasEd25519 = null; | ||
| private static List<String> aliasXDH = null; | ||
| private static List<String> aliasX25519 = null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am a little suspicious in this approach. At least this means for each "family" algorithm name like "EdDSA", we need to hardcode all its parameter set names here. Sounds not very sustainable.
An EdDSA key always has its getAlgorithm being "EdDSA" (at least inside SunEC) and its getParams() being the parameter set name. So it looks like it's enough if we do a name comparison on both.
Also, why no aliasEd448 and aliasX448 here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have to give more thought on checking the algorithm and the getParams() against the list. That may eliminate the need for the hardcoded list..
As to why 448 curves didn't need an alias, there is no other way to specify those curves other than their given name, like mentioned with the KPG/Ed25519 example in my comment to Sean
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So using the getAlgorithm() and getParams().getName() work because there is a Key, but not for non-key situation such as permits(Set.of(CryptoPrimitive.SIGNATURE), "EdDSA", null).
But this does bring up a point I had not considered. The permits() call does not specify any key details other than the family name. Perhaps it's ok for permits() to return a passing result with the information given. But for a permit() that had more detail, it could return a failing result. However, the KPG example does make me think that the consistency in the current PR is better.
| List<TestCase> expected = switch (algorithm) { | ||
| case "Ed25519" -> | ||
| Arrays.asList( | ||
| new TestCase("EdDSA", false), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As Sean mentioned in another comment, disabling "Ed25519" does not imply all EdDSA keys are not permitted. This means the result of permits(primitives, algorithmName, parameters) cannot be determined. That said, I noticed you've used KeyUtil::getAlgorithm in a lot of places. Can we guarantee that this permits method is never called on a family algorithm name? If so, we can get a definitive result.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe my comment to Sean answers this question, but I'm not sure I understand the last question in your comment. "never called on a family algorithm name". The change is to make sure these two families return the curve name and not the family name (EdDSA & XDH). But on the other side, someone using the family name of the disabled algorithm list would disable all curves.
The above test code is checking that this call permits(Set.of(CryptoPrimitive.SIGNATURE), "EdDSA", null) will fail for a Ed25519 key because of the precedent set by KPG.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are talking about the same in multiple comments now.
In this case, if both permits(SIGNATURE, "EdDSA", null) and permits(SIGNATURE, key) are called, it's safe to bypass the 1st check as long as the 2nd one blocks the key. So it's not necessary to cover "EdDSA" when only "Ed25519" is disabled.
| for (String a : algorithms) { | ||
| if (algorithm.equalsIgnoreCase(a)) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We already do case-insensitive match here because algorithms is a case-insensitive TreeMap constructed above in getAlgorithms(). Existing code is also faster because it's a tree lookup instead of a linear iteration. This whole file can stay unchanged if we removed aliases already.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm glad you noticed that. Thanks.
| algorithmConstraints.permits(algorithm, cp, checkKey); | ||
| } | ||
|
|
||
| private static List<String> getNamedCurveFromKey(Key key) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method should probably be renamed to getNamedParametersFromKey, and the comment in permits (line 255) should be changed to // Check if named parameters or curves in the key are disabled.
| } | ||
| case "Ed25519" -> { | ||
| if (aliasEd25519 == null) { | ||
| aliasEd25519 = List.of("EdDSA", "Ed25519"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it should check both the signature algorithm and key algorithm in one permits call for signatures.
| if (nc == null) { | ||
| yield List.of(); | ||
| } | ||
| yield List.of(nc.getNameAndAliases()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you want to add EC itself to the list? I am asking because for EdDSA you added both the algorithm name and the parameter set name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, "EC" isn't an alias for a particular curve.
| } | ||
| yield List.of(nc.getNameAndAliases()); | ||
| } | ||
| default -> List.of(key.getAlgorithm(), KeyUtil.getAlgorithm(key)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if these 2 are the same string?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you concerned about duplicate checks?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes.
| case "EdDSA" -> | ||
| Arrays.asList( | ||
| new TestCase("EdDSA", false), | ||
| new TestCase("Ed25519", true), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why should the above pass? If you disable EdDSA and you are still allowed Signature.getInstance("Ed25519")? If this is because it will reject whatever EdDSA key later? Why both check CryptoPrimitive.SIGNATURE at all?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm confused by this comment. With removing the hardcoded aliases in AbstractAlgorithmConstraints, which is what I thought you had suggested, EdDSA and Ed25519 are now separate as the check is effectively a string compare check against the disabledAlgorithm list
The second half of that case statement has a key that can check against both EdDSA and the NPS.
With respect to CryptoPrimitive.SIGNATURE, it just a value used in the test, it can't be null.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I remember that.
I understand there will be multiple checks in TLS and CertPath. Do we have existing tests on that level?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have other tests that check this.
| new CertPathConstraintsParameters(trustedPubKey, variant, | ||
| anchor, date); | ||
| dac.permits(trustedPubKey.getAlgorithm(), cp, true); | ||
| dac.permits(KeyUtil.getAlgorithm(trustedPubKey), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you plan to have a unit test for AlgorithmChecker changes? It looks like certificates using ED25519 algorithm didn't match that check before. It would be useful to have a test where we disable ED25519 in java.security and then try to use a certificate with ED25519 algorithm.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is checked by an existing test
test/jdk/sun/security/util/AlgorithmConstraints/DisabledAlgorithmPermits.java
Outdated
Show resolved
Hide resolved
test/jdk/sun/security/util/AlgorithmConstraints/DisabledAlgorithmPermits.java
Outdated
Show resolved
Hide resolved
| System.out.println("failed."); | ||
| throw new AssertionError("failed. Expected " + | ||
| tc.expected); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest replacing this check with assertEquals.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I did that, then I couldn't println to stdout. I'd rather keep it as is.
| System.out.println("failed."); | ||
| throw new AssertionError("failed. Expected " + | ||
| tc.expected); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above.
| * This code is free software; you can redistribute it and/or modify it | ||
| * under the terms of the GNU General Public License version 2 only, as | ||
| * published by the Free Software Foundation. Oracle designates this | ||
| * particular file as subject to the "Classpath" exception as provided |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The copyright notice for a test does not need the "Classpath" exception.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All these years I never noticed the test and src copyrights were different.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IANAL but the "Classpath" seems to allow you to include (or reference or link or whatever) this (or its compiled form or whatever) in your own non-GPL app. This is necessary for JDK since it is the runtime for an app. On the other hand, there is no need to make this relaxation for tests. Again, IANAL.
|
|
||
| record TestCase(int testType, String testAlgo, boolean expected) { | ||
| TestCase(String testAlgo, boolean expected) { | ||
| this( 0, testAlgo, expected); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove the space before '0'.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah.. intellij hid that from me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You let it show the arg name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
intellij put the variable name popup in the method line between the whitespace and the "0", so it wasn't obvious
| * returns "EdDSA", but the underlying key may be "Ed448". For | ||
| * DisabledAlgorithmConstraints (DAC), this distinction is important. | ||
| * "EdDSA" means all curves for DAC, but when using it with | ||
| * KeyPairGenerator, "EdDSA" means "Ed25519". |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe just say this allows more precise check for DAC. For KeyPairGenerator, "EdDSA" by default means "Ed25519", but you can always call init(NamedParameterSpec.ED448) to change it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see what your saying, but I was only explaining when EdDSA & Ed25519 can mean the same with KPG. As this is an internal method, I wasn't trying to explaining how to generate an Ed488 key.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK.
wangweij
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
artur-oracle
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
|
/integrate |
|
Going to push as commit e4e6278.
Your commit was automatically rebased without conflicts. |
|
@ascarpino Pushed as commit e4e6278. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
Hi,
I need a review for the following change. Naming conventions for EdDSA and XDH have inconsistencies between DisabledAlgorithms and KeyPairGenerator. These internal changes help make it more consistent when parsing the actual curve being used vs the broader algorithm name.
thanks
Tony
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/23647/head:pull/23647$ git checkout pull/23647Update a local copy of the PR:
$ git checkout pull/23647$ git pull https://git.openjdk.org/jdk.git pull/23647/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 23647View PR using the GUI difftool:
$ git pr show -t 23647Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/23647.diff
Using Webrev
Link to Webrev Comment