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

8284694: Avoid evaluating SSLAlgorithmConstraints twice #8199

Closed
wants to merge 9 commits into from

Conversation

djelinski
Copy link
Member

@djelinski djelinski commented Apr 12, 2022

During TLS handshake, hundreds of constraints are evaluated to determine which cipher suites are usable. Most of the evaluations are performed using HandshakeContext#algorithmConstraints object. By default that object contains a SSLAlgorithmConstraints instance wrapping another SSLAlgorithmConstraints instance. As a result the constraints defined in SSLAlgorithmConstraints are evaluated twice.

This PR improves the default case; if the user-specified constraints are left at defaults, we use a single SSLAlgorithmConstraints instance, and avoid duplicate checks.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8284694: Avoid evaluating SSLAlgorithmConstraints twice

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/8199/head:pull/8199
$ git checkout pull/8199

Update a local copy of the PR:
$ git checkout pull/8199
$ git pull https://git.openjdk.java.net/jdk pull/8199/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 8199

View PR using the GUI difftool:
$ git pr show -t 8199

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/8199.diff

@bridgekeeper
Copy link

bridgekeeper bot commented Apr 12, 2022

👋 Welcome back djelinski! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk
Copy link

openjdk bot commented Apr 12, 2022

@djelinski The following label will be automatically applied to this pull request:

  • security

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.

@openjdk openjdk bot added the security label Apr 12, 2022
@djelinski
Copy link
Member Author

djelinski commented Apr 12, 2022

Results of the attached benchmark:
Before:

Benchmark                 (resume)  (tlsVersion)   Mode  Cnt     Score     Error  Units
SSLHandshake.doHandshake      true       TLSv1.2  thrpt    5  1407.320 ± 302.562  ops/s
SSLHandshake.doHandshake      true           TLS  thrpt    5   391.037 ±  13.014  ops/s
SSLHandshake.doHandshake     false       TLSv1.2  thrpt    5   280.003 ±  69.273  ops/s
SSLHandshake.doHandshake     false           TLS  thrpt    5   233.401 ±   9.371  ops/s

After:

Benchmark                 (resume)  (tlsVersion)   Mode  Cnt     Score     Error  Units
SSLHandshake.doHandshake      true       TLSv1.2  thrpt    5  2267.325 ± 119.800  ops/s
SSLHandshake.doHandshake      true           TLS  thrpt    5   490.465 ±  24.698  ops/s
SSLHandshake.doHandshake     false       TLSv1.2  thrpt    5   340.275 ±  72.833  ops/s
SSLHandshake.doHandshake     false           TLS  thrpt    5   271.656 ±   5.444  ops/s

The results show a nice double-digit improvement across the board.

@djelinski djelinski marked this pull request as ready for review Apr 12, 2022
@openjdk openjdk bot added the rfr label Apr 12, 2022
@mlbridge
Copy link

mlbridge bot commented Apr 12, 2022

Webrevs

cl4es
cl4es approved these changes Apr 12, 2022
@openjdk
Copy link

openjdk bot commented Apr 12, 2022

@djelinski 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:

8284694: Avoid evaluating SSLAlgorithmConstraints twice

Reviewed-by: redestad, xuelei, coffeys

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 111 new commits pushed to the master branch:

  • cb16e41: 8283541: Add Statical counters and some comments in PhaseStringOpts
  • 15ce8c6: 8284553: Deprecate the DEFAULT static field of OAEPParameterSpec
  • e8016f7: 8281006: Module::getResourceAsStream should check if the resource is open unconditionally when caller is null
  • 018017a: 8266247: Swing test bug7154030.java sometimes fails on macOS 11 ARM
  • e6c5f28: 8280594: Refactor annotation invocation handler handling to use Objects.toIdentityString
  • b4a85cd: 8284742: x86: Handle integral division overflow during parsing
  • 5291ec8: 8277969: HttpClient SelectorManager shuts down when custom Executor rejects a task
  • 6c6d522: 8284758: [linux] improve print_container_info
  • 46b2e54: 8075816: Deprecate AliasLevel flag since it is broken
  • 1b71621: 8042381: Test javax/swing/JRootPane/4670486/bug4670486.java fails with Action has not been received
  • ... and 101 more: https://git.openjdk.java.net/jdk/compare/4cd0921cf6075cedd710e96d939df3df6c007d74...master

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 master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready label Apr 12, 2022
@bradfordwetmore
Copy link
Contributor

bradfordwetmore commented Apr 12, 2022

Please make sure @XueleiFan has a chance to look at this before integrating.

Copy link
Member

@XueleiFan XueleiFan left a comment

Nice catch. Thank you!

Copy link
Contributor

@coffeys coffeys left a comment

Nice work. I've been looking at this area myself in recent weeks also while debugging some JDK 11u performance issues. JFR shows this area as costly. Some other JDK fixes in this area have already improved performance. This will certainly help. Pasting a stacktrace[1] from an old Oracle JDK 11.0.12 report for history purposes.

I think there are other improvements that can be made in the TLS constraints code. Caching the last known values from a constraints permits test is something I've begun looking into.

[1]
Stack Trace Count Percentage
boolean java.lang.StringLatin1.regionMatchesCI(byte[], int, byte[], int, int) 1637 100 %
boolean java.lang.String.regionMatches(boolean, int, String, int, int) 1637 100 %
boolean java.lang.String.equalsIgnoreCase(String) 1637 100 %
boolean sun.security.util.AbstractAlgorithmConstraints.checkAlgorithm(List, String, AlgorithmDecomposer) 1631 99.6 %
boolean sun.security.util.DisabledAlgorithmConstraints.permits(Set, String, AlgorithmParameters) 1631 99.6 %
boolean sun.security.ssl.SSLAlgorithmConstraints.permits(Set, String, AlgorithmParameters) 1631 99.6 %
boolean sun.security.ssl.SSLAlgorithmConstraints.permits(Set, String, AlgorithmParameters) 836 51.1 %
boolean sun.security.ssl.HandshakeContext.isActivatable(CipherSuite, AlgorithmConstraints, Map) 428 26.1 %
List sun.security.ssl.HandshakeContext.getActiveCipherSuites(List, List, AlgorithmConstraints) 418 25.5 %
void sun.security.ssl.HandshakeContext.(SSLContextImpl, TransportContext) 418 25.5 %
void sun.security.ssl.ClientHandshakeContext.(SSLContextImpl, TransportContext) 418 25.5 %
void sun.security.ssl.TransportContext.kickstart() 418 25.5 %
void sun.security.ssl.SSLSocketImpl.startHandshake(boolean) 418 25.5 %
void sun.security.ssl.SSLSocketImpl.startHandshake() 418 25.5 %

@mlbridge
Copy link

mlbridge bot commented Apr 13, 2022

Mailing list message from Anthony Scarpino on security-dev:

Hi Sean,

Caching is an interesting idea. I've wondered for a while off and on
about how to speed it up, but hadn't come up with a solution I liked.
The complication with caching is while something like an algorithm name
only could be easy in a hashmap, it gets more complicated when you get
into key sizes. Such as, how to represent RSA 1k being disallowed and
but 2k allowed.. or certificate usage..

Tony

On 4/13/22 2:03 AM, Sean Coffey wrote:

@djelinski
Copy link
Member Author

djelinski commented Apr 14, 2022

I think I addressed all the concerns raised. Could you take another look?

@djelinski
Copy link
Member Author

djelinski commented Apr 19, 2022

hi @XueleiFan I'd appreciate your approval here. Thanks!

@XueleiFan
Copy link
Member

XueleiFan commented Apr 19, 2022

hi @XueleiFan I'd appreciate your approval here. Thanks!

Sorry, I missed the notification emails. I will have a look today.

Copy link
Member

@XueleiFan XueleiFan left a comment

Thanks for the update. It looks good to me, except comments for the coding style.

this.userSpecifiedConstraints = getUserSpecifiedConstraints(engine);
this.peerSpecifiedConstraints = null;
this.enabledX509DisabledAlgConstraints = withDefaultCertPathConstraints;
if (nullIfDefault(userSpecifiedConstraints) == null) {
Copy link
Member

@XueleiFan XueleiFan Apr 20, 2022

Choose a reason for hiding this comment

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

Do you wan to check DEFAULT_SSL_ONLY in the nullIfDefault() implementation? The logic of the block is a little bit hard to understand to me.

Copy link
Member Author

@djelinski djelinski Apr 20, 2022

Choose a reason for hiding this comment

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

No I don't; it's for the same reason why I'm using == and not equals: DEFAULT is the only SSLAlgorithmConstraints instance that is ever used as userSpecifiedConstraints here.

DEFAULT is used because SSLConfiguration sets userSpecifiedAlgorithmConstraints to SSLAlgorithmConstraints.DEFAULT. This feels wrong; the name suggests that the constraints should be specified by user, and should be null if the user doesn't touch them.
userSpecifiedAlgorithmConstraints are accessible by getSSLParameters().getAlgorithmConstraints() on SSLEngineImpl and SSLSocketImpl. Returning DEFAULT here also feels wrong; as a user I would be concerned that setting my own algorithm constraints would replace the default ones. It doesn't, but that is not immediately apparent.

We could initialize userSpecifiedAlgorithmConstraints to null, and back out all the other changes from this PR. The only reason why I didn't do that was because it would change the observable behavior (getSSLParameters().getAlgorithmConstraints() would return null). If you think we can live with that, I'll be happy to do that change.

Copy link
Member

@XueleiFan XueleiFan Apr 20, 2022

Choose a reason for hiding this comment

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

It is not interested to me to use 'null' constraints in ssl configure. I have no more comments. Thank you for the update!

if (nullIfDefault(userSpecifiedConstraints) == null) {
return withDefaultCertPathConstraints ? DEFAULT : DEFAULT_SSL_ONLY;
}
return new SSLAlgorithmConstraints(userSpecifiedConstraints, withDefaultCertPathConstraints);
Copy link
Member

@XueleiFan XueleiFan Apr 20, 2022

Choose a reason for hiding this comment

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

It would be nice to limit each line within 80 characters, which is useful for terminal users.

@djelinski
Copy link
Member Author

djelinski commented Apr 20, 2022

/integrate

@openjdk
Copy link

openjdk bot commented Apr 20, 2022

Going to push as commit d8446b4.
Since your change was applied there have been 111 commits pushed to the master branch:

  • cb16e41: 8283541: Add Statical counters and some comments in PhaseStringOpts
  • 15ce8c6: 8284553: Deprecate the DEFAULT static field of OAEPParameterSpec
  • e8016f7: 8281006: Module::getResourceAsStream should check if the resource is open unconditionally when caller is null
  • 018017a: 8266247: Swing test bug7154030.java sometimes fails on macOS 11 ARM
  • e6c5f28: 8280594: Refactor annotation invocation handler handling to use Objects.toIdentityString
  • b4a85cd: 8284742: x86: Handle integral division overflow during parsing
  • 5291ec8: 8277969: HttpClient SelectorManager shuts down when custom Executor rejects a task
  • 6c6d522: 8284758: [linux] improve print_container_info
  • 46b2e54: 8075816: Deprecate AliasLevel flag since it is broken
  • 1b71621: 8042381: Test javax/swing/JRootPane/4670486/bug4670486.java fails with Action has not been received
  • ... and 101 more: https://git.openjdk.java.net/jdk/compare/4cd0921cf6075cedd710e96d939df3df6c007d74...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated label Apr 20, 2022
@openjdk openjdk bot closed this Apr 20, 2022
@openjdk openjdk bot removed ready rfr labels Apr 20, 2022
@openjdk
Copy link

openjdk bot commented Apr 20, 2022

@djelinski Pushed as commit d8446b4.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

@mlbridge
Copy link

mlbridge bot commented Apr 20, 2022

Mailing list message from Se=c3=a1n Coffey on security-dev:

I think the work done with 8284694 will help alot in any case since I
suspect the same SSLAlgorithmConstraints Object will be shared much more
now (rather than spin off new copies)

Some recent JFRs I looked at show that alot of CPU cycles[1] get taken
in the HandshakeContext methods of :
sun.security.ssl.HandshakeContext#getActiveCipherSuites
sun.security.ssl.HandshakeContext#getActiveProtocols

Both methods get called per Handshakecontext construction and I think
each TLS handshake gets a new Handshakecontext.I was thinking that a
cache of the last known variables used to deduce the
getActiveCipherSuites and getActiveProtocols Lists could be created.
That might have the potential to avoid alot of needless CPU cycles in
this area since the parameters used to produce these Lists don't really
change that often. I'm still looking into this potential and hope to
share a patch shortly.

regards,
Sean.

[1]

Stack Trace??? Count??? Percentage
TreeMap$Entry java.util.TreeMap.getEntryUsingComparator(Object) 1035???
100?%
TreeMap$Entry java.util.TreeMap.getEntry(Object)??? 1035??? 100?%
boolean java.util.TreeMap.containsKey(Object)??? 1035??? 100?%
boolean java.util.TreeSet.contains(Object)??? 1035??? 100?%
boolean
sun.security.util.AbstractAlgorithmConstraints.checkAlgorithm(Set,
String, AlgorithmDecomposer)??? 1035??? 100?%
boolean sun.security.util.DisabledAlgorithmConstraints.permits(Set,
String, AlgorithmParameters)??? 1035??? 100?%
boolean sun.security.ssl.SSLAlgorithmConstraints.permits(Set, String,
AlgorithmParameters)??? 1035??? 100?%
boolean sun.security.ssl.SSLAlgorithmConstraints.permits(Set, String,
AlgorithmParameters)??? 553??? 53.4?%
boolean sun.security.ssl.HandshakeContext.isActivatable(CipherSuite,
AlgorithmConstraints, Map)??? 309??? 29.9?%
List sun.security.ssl.HandshakeContext.getActiveCipherSuites(List, List,
AlgorithmConstraints)??? 302??? 29.2?%
void sun.security.ssl.HandshakeContext.<init>(SSLContextImpl,
TransportContext)??? 302??? 29.2?%
void sun.security.ssl.ClientHandshakeContext.<init>(SSLContextImpl,
TransportContext)??? 302??? 29.2?%
void sun.security.ssl.TransportContext.kickstart()??? 302 29.2?%
void sun.security.ssl.SSLSocketImpl.startHandshake(boolean) 302??? 29.2?%

On 13/04/2022 18:05, Anthony Scarpino wrote:

Hi Sean,

Caching is an interesting idea.? I've wondered for a while off and on
about how to speed it up, but hadn't come up with a solution I liked.
The complication with caching is while something like an algorithm
name only could be easy in a hashmap, it gets more complicated when
you get into key sizes. Such as, how to represent RSA 1k being
disallowed and but 2k allowed.. or certificate usage..

Tony

On 4/13/22 2:03 AM, Sean Coffey wrote:

@mlbridge
Copy link

mlbridge bot commented Apr 22, 2022

Mailing list message from Daniel Jeliński on security-dev:

Hi Tony & Sean,
As it turns out, caching the availability of algorithms is sufficient
to get a massive speedup here. Check out the results on
https://github.com//pull/8349 and let me know what you
think.

Regards,
Daniel

?r., 20 kwi 2022 o 22:22 Se?n Coffey <sean.coffey at oracle.com> napisa?(a):

I think the work done with 8284694 will help alot in any case since I
suspect the same SSLAlgorithmConstraints Object will be shared much more
now (rather than spin off new copies)

Some recent JFRs I looked at show that alot of CPU cycles[1] get taken
in the HandshakeContext methods of :
sun.security.ssl.HandshakeContext#getActiveCipherSuites
sun.security.ssl.HandshakeContext#getActiveProtocols

Both methods get called per Handshakecontext construction and I think
each TLS handshake gets a new Handshakecontext.I was thinking that a
cache of the last known variables used to deduce the
getActiveCipherSuites and getActiveProtocols Lists could be created.
That might have the potential to avoid alot of needless CPU cycles in
this area since the parameters used to produce these Lists don't really
change that often. I'm still looking into this potential and hope to
share a patch shortly.

regards,
Sean.

[1]

Stack Trace Count Percentage
TreeMap$Entry java.util.TreeMap.getEntryUsingComparator(Object) 1035
100 %
TreeMap$Entry java.util.TreeMap.getEntry(Object) 1035 100 %
boolean java.util.TreeMap.containsKey(Object) 1035 100 %
boolean java.util.TreeSet.contains(Object) 1035 100 %
boolean
sun.security.util.AbstractAlgorithmConstraints.checkAlgorithm(Set,
String, AlgorithmDecomposer) 1035 100 %
boolean sun.security.util.DisabledAlgorithmConstraints.permits(Set,
String, AlgorithmParameters) 1035 100 %
boolean sun.security.ssl.SSLAlgorithmConstraints.permits(Set, String,
AlgorithmParameters) 1035 100 %
boolean sun.security.ssl.SSLAlgorithmConstraints.permits(Set, String,
AlgorithmParameters) 553 53.4 %
boolean sun.security.ssl.HandshakeContext.isActivatable(CipherSuite,
AlgorithmConstraints, Map) 309 29.9 %
List sun.security.ssl.HandshakeContext.getActiveCipherSuites(List, List,
AlgorithmConstraints) 302 29.2 %
void sun.security.ssl.HandshakeContext.<init>(SSLContextImpl,
TransportContext) 302 29.2 %
void sun.security.ssl.ClientHandshakeContext.<init>(SSLContextImpl,
TransportContext) 302 29.2 %
void sun.security.ssl.TransportContext.kickstart() 302 29.2 %
void sun.security.ssl.SSLSocketImpl.startHandshake(boolean) 302 29.2 %

On 13/04/2022 18:05, Anthony Scarpino wrote:

Hi Sean,

Caching is an interesting idea. I've wondered for a while off and on
about how to speed it up, but hadn't come up with a solution I liked.
The complication with caching is while something like an algorithm
name only could be easy in a hashmap, it gets more complicated when
you get into key sizes. Such as, how to represent RSA 1k being
disallowed and but 2k allowed.. or certificate usage..

Tony

On 4/13/22 2:03 AM, Sean Coffey wrote:

@djelinski djelinski deleted the handshake-bench branch May 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integrated security
6 participants