8268427: Improve AlgorithmConstraints:checkAlgorithm performance #4424
Conversation
|
👋 Welcome back dongbohe! A progress list of the required criteria for merging this PR into |
Webrevs
|
|
Mailing list message from Sean Mullan on security-dev: Nice, the benchmark results look impressive. I'll take a closer look at --Sean On 6/9/21 4:03 AM, Dongbo He wrote: |
1 similar comment
|
Mailing list message from Sean Mullan on security-dev: Nice, the benchmark results look impressive. I'll take a closer look at --Sean On 6/9/21 4:03 AM, Dongbo He wrote: |
| AlgorithmDecomposer decomposer) { | ||
| super(decomposer); | ||
| disabledAlgorithms = getAlgorithms(propertyName); | ||
| List<String> disabledAlgorithmsList = getAlgorithms(propertyName); |
There was a problem hiding this comment.
Is it doable to have the getAlgorithms() method return a Set?
There was a problem hiding this comment.
The collection required when new Constraints() should retain the default case of the elements, because some code will depend on this, for example, .
entry.startsWith("keySize").
But the set required by the permits should unify the case of the elements, because algorithm may be uppercase or lowercase, but the Set:contains() cannot handle this situation.
So we need to create a new Set that ignores the default case of elements.
There was a problem hiding this comment.
For the entry.startsWith("keySize") example, I don't think keySize is an algorithm that could be listed individually in the list. The "keySize" may be just a part one algorithm, for example "RSA keySize < 1024".
It's a good point about the lowercase and upper case. Did you check how constraints like the "keySize" are expressed in the list or set?
There was a problem hiding this comment.
Yes, you're right. The "keySize" is not an independent algorithm listed in list, it exists in a form like "EC keySize < 224". So when the elements in the collection returned by getAlgorithms are all uppercase or lowercase, an IllegalArgumentException will be thrown in throw new IllegalArgumentException.
In the case of "keySize", the object in the list stored in algorithmConstraints is KeySizeConstraint, then keysize will be checked in algorithmConstraints.permits(algorithm, parameters) by KeySizeConstraint:permits.
|
I think it would be worthwhile to see if we can take this a step further, and leverage the This way we could potentially eliminate the List/Set cache entirely and just use the HashMap to check if the algorithm is disabled. |
Thanks for the suggestion, |
| algorithmsInProperty = property.split(","); | ||
| for (int i = 0; i < algorithmsInProperty.length; i++) { | ||
| algorithmsInProperty[i] = algorithmsInProperty[i].trim(); | ||
| algorithmsInProperty[i] = algorithmsInProperty[i].trim().toLowerCase(Locale.ENGLISH); |
There was a problem hiding this comment.
Is it possible to keep the current behavior that the property could be case sensitive? It may be not desired to allow "keysize" for "keySize" spec in the property.
There was a problem hiding this comment.
If we keep property sensitive, we may need to use TreeSet. I have updated the PR with TreeSet. Fortunately, the performance hasn't changed much.
There was a problem hiding this comment.
I did not get the point to use TreeSet. Is it sufficient if the toLowerCase() is not added (and don't compare keywords like "keySize" by ignoring cases)?
- algorithmsInProperty[i] = algorithmsInProperty[i].trim().toLowerCase(...);
+ algorithmsInProperty[i] = algorithmsInProperty[i].trim();
There was a problem hiding this comment.
Sorry, I missed a "case" in the original comment (corrected). I meant to keep the property case sensitive in the hash set so that the keywords like "keySize" could be used correctly.
There was a problem hiding this comment.
checkAlgorithm check whether the item is in the collection by ignoring case. If the item in the HashSet is case-sensitive, the method will lose its original algorithmic logic, but will retain it by using a new TreeSet<>(String.CASE_INSENSITIVE_ORDER);
Can we use case sensitivity in checkAlgorithm to check an algorithm?
There was a problem hiding this comment.
The checkAlgorithm is using equalsIgnoreCase(), so it is safe for it. My concern is mainly about the keywords, like "keySize" used the property, not really the algorithm name. It is good to keep the current case sensitive checking behavior unchanged.
There was a problem hiding this comment.
Hi, Xuelei, thank you for your comments. I may not express it clearly, let me clarify. My concern is that if we use the HashSet:contains method to check whether an item is in the hash set, we cannot use equalsIgnoreCase(), so I used new TreeSet<>(String.CASE_INSENSITIVE_ORDER) that can support equalsIgnoreCase().
According to my understanding, the current checkAlgorithm is not case sensitive, because it ignores the case of the item being checked. Looking forward to your suggestions。
There was a problem hiding this comment.
I see your point now. Thank you for the clarification. I need more time to think about if there is an improvement. I will be back.
|
The following is the Benchmark results for the new commit: JMH: Benchmark (algorithm) Mode Cnt Score Error Units
AlgorithmConstraintsPermits.permits SSLv3 avgt 5 47.353 ? 3.193 ns/op
AlgorithmConstraintsPermits.permits DES avgt 5 60.307 ? 1.351 ns/op
AlgorithmConstraintsPermits.permits NULL avgt 5 59.065 ? 0.728 ns/op
AlgorithmConstraintsPermits.permits TLS1.3 avgt 5 428.311 ? 16.542 ns/opTomcat+Jmeter: summary + 60455 in 00:00:30 = 2016.4/s Avg: 198 Min: 164 Max: 256 Err: 0 (0.00%) Active: 400 Started: 400 Finished: 0
summary = 106982 in 00:00:55 = 1927.6/s Avg: 188 Min: 25 Max: 1194 Err: 0 (0.00%)
summary + 60430 in 00:00:30 = 2014.6/s Avg: 198 Min: 160 Max: 252 Err: 0 (0.00%) Active: 400 Started: 400 Finished: 0
summary = 167412 in 00:01:25 = 1958.1/s Avg: 191 Min: 25 Max: 1194 Err: 0 (0.00%)
summary + 60480 in 00:00:30 = 2014.6/s Avg: 198 Min: 161 Max: 245 Err: 0 (0.00%) Active: 400 Started: 400 Finished: 0
summary = 227892 in 00:01:56 = 1972.8/s Avg: 193 Min: 25 Max: 1194 Err: 0 (0.00%)
summary + 60506 in 00:00:30 = 2016.6/s Avg: 198 Min: 161 Max: 255 Err: 0 (0.00%) Active: 400 Started: 400 Finished: 0 |
|
I will let Xuelei finish his review. From what I see in the code, the Comparator is used when the algorithm is fully disabled. The use of keywords later on in the Constraints constructor uses the ConstraintsEntry which I believe is stored with the upper and lower case. I think the code should be ok. |
| throw new IllegalArgumentException("No algorithm name specified"); | ||
| } | ||
|
|
||
| Set<String> elements = null; |
There was a problem hiding this comment.
This line could be placed and merged with line 96-98.
Set<String> elements = decomposer.decompose(algorithm);
| } | ||
| // check the element of the elements | ||
| for (String element : elements) { | ||
| if (algorithms.contains(algorithm)) { |
There was a problem hiding this comment.
The check should be placed on the decomposed elements.
- if (algorithms.contains(algorithm))
+ if (algorithms.contains(element))
|
|
||
| // Check for alias | ||
| int ecindex = -1, i = 0; | ||
| Pattern INCLUDE_PATTERN = Pattern.compile("include " + PROPERTY_DISABLED_EC_CURVES, Pattern.CASE_INSENSITIVE); |
There was a problem hiding this comment.
Per the Java code conventions, the variable name could be "includePattern", rather than using capital all letters.
Please keep each line less than or equal to 80 bytes.
| // Check for alias | ||
| int ecindex = -1, i = 0; | ||
| Pattern INCLUDE_PATTERN = Pattern.compile("include " + PROPERTY_DISABLED_EC_CURVES, Pattern.CASE_INSENSITIVE); | ||
| Matcher matcher; |
There was a problem hiding this comment.
I think the performance impact should be trivial if moving the "matcher" declaration into the for-loop block.
| import java.security.AlgorithmParameters; | ||
| import java.security.CryptoPrimitive; | ||
| import java.security.Key; | ||
| import java.util.HashSet; |
There was a problem hiding this comment.
I guess this line could be removed.
| Matcher matcher; | ||
| if ((matcher = INCLUDE_PATTERN.matcher(s)).matches()) { | ||
| disabledAlgorithms.remove(matcher.group()); | ||
| disabledAlgorithms.addAll(getAlgorithms(PROPERTY_DISABLED_EC_CURVES)); |
There was a problem hiding this comment.
I guess this line exceed 80 characters? Please keep it in 80 characters for each line.
|
|
||
| // Check for alias | ||
| int ecindex = -1, i = 0; | ||
| Pattern INCLUDE_PATTERN = Pattern.compile( |
There was a problem hiding this comment.
You may miss my previous comment. This variable is not static, hence all capital letters naming does not apply here.
As you are already here, it may be nice if you'd like to have this variable as a static final class field. Then, the compile() will be called one time at most for this class. As a static class field, you could use the capitalized name.
There was a problem hiding this comment.
Sorry, I misunderstood what you meant before, I will fix it. Thank you!
| Matcher matcher; | ||
| if ((matcher = INCLUDE_PATTERN.matcher(s)).matches()) { | ||
| disabledAlgorithms.remove(matcher.group()); | ||
| disabledAlgorithms.addAll(getAlgorithms(PROPERTY_DISABLED_EC_CURVES)); |
There was a problem hiding this comment.
It may increase the readability a little bit if having the assignment in the declaration line:
- Matcher matcher;
- if ((matcher = INCLUDE_PATTERN.matcher(s)).matches()) {
+ Matcher matcher = INCLUDE_PATTERN.matcher(s);
+ if (matcher.matches()) {
XueleiFan
left a comment
XueleiFan
left a comment
There was a problem hiding this comment.
Looks good to me. Thank you!
|
@dongbohe 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 236 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. As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@XueleiFan, @ascarpino) but any other Committer may sponsor as well. ➡️ To flag this PR as ready for integration with the above commit message, type |
|
Thank you for your review, Xuelei. Hi, mullan, ascarpino. Can I get some comments from you? @seanjmullan @ascarpino |
| @@ -0,0 +1,66 @@ | |||
| /* | |||
| * Copyright (c) 2021, Huawei Technologies Co., Ltd. All rights reserved. | |||
There was a problem hiding this comment.
I'm verifying if this copyright is ok or if it needs to be changed. Please don't integrate until I hear back from others.
There was a problem hiding this comment.
This appears to be ok with the powers at be
|
/contributor add GaofengZhang zhanggaofeng9@huawei.com |
|
@dongbohe |
|
/integrate |
|
/sponsor |
|
Going to push as commit 3b83bc1.
Your commit was automatically rebased without conflicts. |
openjdk
bot
added
integrated
|
@Hamlin-Li @dongbohe Pushed as commit 3b83bc1. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
5 participants
Now AlgorithmConstraints:checkAlgorithm uses List to check if an algorithm has been disabled. It is less efficient when there are more disabled elements in the list, we can use Set instead of List to speed up the search.
Patch contains a benchmark that can be run with
make test TEST="micro:java.security.AlgorithmConstraintsPermits".Baseline results before patch:
Benchmark results after patch:
SSLv3, DES, NULL are the first, middle and last element in
jdk.tls.disabledAlgorithmsfromconf/security/java.security.Tomcat(maxKeepAliveRequests=1, which will disable HTTP/1.0 keep-alive)+Jmeter:
Before patch:
After patch:
Testing: tier1, tier2
Progress
Issue
Reviewers
Contributors
<zhanggaofeng9@huawei.com>Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/4424/head:pull/4424$ git checkout pull/4424Update a local copy of the PR:
$ git checkout pull/4424$ git pull https://git.openjdk.java.net/jdk pull/4424/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 4424View PR using the GUI difftool:
$ git pr show -t 4424Using diff file
Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/4424.diff