Re: RFR: 8212136: Remove BaseSSLSocketImpl finalizer method [v2]
On Thu, 7 Apr 2022 20:17:28 GMT, Xue-Lei Andrew Fan wrote:
>> Please review the update to remove finalizer method in the SunJSSE provider
>> implementation. It is one of the efforts to clean up the use of finalizer
>> method in JDK.
>
> Xue-Lei Andrew Fan has updated the pull request incrementally with two
> additional commits since the last revision:
>
> - typo blank linke correction
> - revise the update
I parsed the finalize() code one more time. The sending close_notify may be
not an expected part of the finalize() implementation.
The finalize() calls the close() method. If the socket is layered over a
preexisting socket, the preexisting socket is closed by calling it close()
method:
`self.close();
`
Otherwise, the SSLSocket.close() method will be called:
`super.close();
`
See the BaseSSLSocketImpl.close() method:
@Override
public void close() throws IOException {
if (self == this) {
super.close();
} else {
self.close();
}
}
For layered over socket case, if the preexisting socket is not an SSLSocket
object(which is the common case), no close_notify will be sent off course. If
the preexisting socket is an SSLSocket object (which may be not common), the
SSLSocketImpl.close() will be called and the close_notify could be sent.
For non-layered over sockets, as super.close() is called, there is no
close_notify delivered during the finalizing.
Based on the cases above, the close_notify delivery may be not an expected
behavior during finalization in practice. I would like to remove the
finalize() method without adding a cleaner, as shown in the current PR. But
if you read the code and behavior differently , it's acceptable to me to clean
up the preexisting socket, by adding a cleaner like:
BaseSSLSocketImpl(Socket socket) {
super();
this.self = socket;
this.consumedInput = null;
+ CleanerFactory.cleaner().register(this, self::close);
}
Please let me know your preference.
-
PR: https://git.openjdk.java.net/jdk/pull/8065
Re: RFR: 8284694: Avoid evaluating SSLAlgorithmConstraints twice [v2]
On Wed, 13 Apr 2022 20:32:02 GMT, Daniel Jeliński wrote:
>> 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.
>
> Daniel Jeliński has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Avoid nesting SSLAlgorithmConstraints
src/java.base/share/classes/sun/security/ssl/SSLAlgorithmConstraints.java line
73:
> 71:
> 72: static AlgorithmConstraints wrap(AlgorithmConstraints
> userSpecifiedConstraints) {
> 73: if (userSpecifiedConstraints == DEFAULT) {
Maybe, DEFAULT could be returned for null userSpecifiedConstraints.
-if (userSpecifiedConstraints == DEFAULT) {
+if (userSpecifiedConstraints == null &&
+ userSpecifiedConstraints== DEFAULT) {
-
PR: https://git.openjdk.java.net/jdk/pull/8199
Re: RFR: 8186958: Need method to create pre-sized HashMap [v18]
On Thu, 14 Apr 2022 01:13:18 GMT, XenoAmess wrote:
>> src/java.xml/share/classes/com/sun/org/apache/xerces/internal/impl/xs/traversers/XSAttributeChecker.java
>> line 1819:
>>
>>> 1817: Map items;
>>> 1818: LargeContainer(int size) {
>>> 1819: items = HashMap.newHashMap(size*2+1);
>>
>> I'm wondering if we should change this to just `newHashMap(size)` since it
>> looks like this container is intended to hold up to `size` items.
>> @JoeWang-Java ? I suspect the `size*2+1` was a failed attempt at allocating
>> a HashMap of the correct capacity for `size` mappings.
>
>> I suspect the `size*2+1` was a failed attempt at allocating a HashMap of the
>> correct capacity for `size` mappings.
>
> I looked the codes and don't think so..
> If I'm wrong, I'm glad to fix.
Stuart's right, I looked at the code, it's as you said a failed attempt, "size"
would be good. So HashMap.newHashMap(size) would actually be a small
improvement.
It's an interesting impl the way it used HashMap, but it's 20 year code.
-
PR: https://git.openjdk.java.net/jdk/pull/7928
Re: RFR: 8209038: Clarify the javadoc of Cipher.getParameters() [v2]
On Wed, 13 Apr 2022 22:04:03 GMT, Valerie Peng wrote:
>> Anyone can help review this javadoc update? The main change is the wording
>> for the method javadoc of
>> Cipher.getParameters()/CipherSpi.engineGetParameters(). The original wording
>> is somewhat restrictive and request is to broaden this to accommodate more
>> scenarios such as when null can be returned.
>> The rest are minor things like add {@code } to class name and null, and
>> remove redundant ".".
>>
>> Will file CSR after the review is close to being wrapped up.
>> Thanks~
>
> Valerie Peng has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Update w/ review feedbacks
Marked as reviewed by xuelei (Reviewer).
-
PR: https://git.openjdk.java.net/jdk/pull/8117
Re: RFR: 8209038: Clarify the javadoc of Cipher.getParameters() [v2]
On Wed, 13 Apr 2022 21:53:33 GMT, Valerie Peng wrote: >> I like the revised spec more. >> >> BTW, for "... contain additional default and random parameter values ...", >> is "or" more common than "and" in English? > > Hmm, the original javadoc has "and", so I just picked it up. Perhaps I should > change it to "or" then. Thanks. Thank you, I have no further comments. - PR: https://git.openjdk.java.net/jdk/pull/8117
Re: RFR: 8186958: Need method to create pre-sized HashMap [v18]
On Thu, 14 Apr 2022 01:15:05 GMT, XenoAmess wrote: >> src/java.xml/share/classes/com/sun/org/apache/xalan/internal/xsltc/dom/DocumentCache.java >> line 171: >> >>> 169: _current = 0; >>> 170: _size = size; >>> 171: _references = HashMap.newHashMap(_size); >> >> Not `_size+2` ? > >> Not `_size+2` ? > > I don't have a idea here why he original use the + 2. > Is there any guy more familiar with this code tell me why? > Thanks! size, not size + 2, same situation as size*2+1 below. - PR: https://git.openjdk.java.net/jdk/pull/7928
Re: RFR: 8186958: Need method to create pre-sized HashMap [v18]
On Wed, 13 Apr 2022 22:57:33 GMT, Stuart Marks wrote: > Not `_size+2` ? I don't have a idea here why he original use the + 2. Is there any guy more familiar with this code tell me why? Thanks! > I suspect the `size*2+1` was a failed attempt at allocating a HashMap of the > correct capacity for `size` mappings. I looked the codes and don't think so.. If I'm wrong, I'm glad to fix. - PR: https://git.openjdk.java.net/jdk/pull/7928
Re: RFR: JDK-8284112 Minor cleanup could be done in javax.crypto [v2]
> JDK-8284112 Minor cleanup could be done in javax.crypto Mark Powers has updated the pull request incrementally with two additional commits since the last revision: - merge stuff - review comments from Brad Wetmore - Changes: - all: https://git.openjdk.java.net/jdk/pull/8214/files - new: https://git.openjdk.java.net/jdk/pull/8214/files/a6754642..86a546bc Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=8214&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=8214&range=00-01 Stats: 24 lines in 10 files changed: 4 ins; 0 del; 20 mod Patch: https://git.openjdk.java.net/jdk/pull/8214.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8214/head:pull/8214 PR: https://git.openjdk.java.net/jdk/pull/8214
Re: RFR: 8186958: Need method to create pre-sized HashMap [v16]
On Wed, 13 Apr 2022 23:48:06 GMT, Stuart Marks wrote: > but I suspect the cleanup may simply be removing them entirely. +1 for removing it. - PR: https://git.openjdk.java.net/jdk/pull/7928
Re: RFR: 8186958: Need method to create pre-sized HashMap [v16]
On Wed, 13 Apr 2022 20:06:34 GMT, Naoto Sato wrote: >> XenoAmess has updated the pull request incrementally with one additional >> commit since the last revision: >> >> revert changes in: >> src/java.desktop >> src/java.management >> src/jdk.internal.vm.ci >> src/jdk.jfr >> src/jdk.management.jfr >> src/jdk.management >> src/utils/IdealGraphVisualizer > > Looks good for changes in i18n related call sites, assuming that the > copyright years will be updated. > > Should we need some additions/modifications to the hashmap optimal capacity > utility `test/lib/jdk/test/lib/util/OptimalCapacity.java`? @naotoj wrote: > Should we need some additions/modifications to the hashmap optimal capacity > utility `test/lib/jdk/test/lib/util/OptimalCapacity.java`? The only thing that uses this utility now is `test/jdk/java/lang/Enum/ConstantDirectoryOptimalCapacity.java`, which is on the problem list. The cleanup is covered by [JDK-8282120](https://bugs.openjdk.java.net/browse/JDK-8282120). After this PR gets integrated, the Enum ConstantDirectory will be initialized with `HashMap.newHashMap(universe.length)` so the OptimalCapacity test won't really be testing anything. I'll take another look at it and the library utility, but I suspect the cleanup may simply be removing them entirely. - PR: https://git.openjdk.java.net/jdk/pull/7928
Re: RFR: 8186958: Need method to create pre-sized HashMap [v18]
On Wed, 13 Apr 2022 22:20:14 GMT, XenoAmess wrote:
>> 8186958: Need method to create pre-sized HashMap
>
> XenoAmess has updated the pull request incrementally with one additional
> commit since the last revision:
>
> update LastModified
src/java.base/unix/classes/java/lang/ProcessEnvironment.java line 102:
> 100: /* Only for use by Runtime.exec(...String[]envp...) */
> 101: static Map emptyEnvironment(int capacity) {
> 102: return new StringEnvironment(HashMap.newHashMap(capacity));
This change is correct, since this is called with the length of an array that's
used to populate the environment. It really should be named `size` instead of
`capacity`. However the windows version of this code simply calls
`super(capacity)` as it's a subclass of `HashMap`, which is wrong. Well,
probably not worth changing this now. We may need to revisit this later to do
some cleaning up. (And also the strange computation in the static initializer
at line 71.)
-
PR: https://git.openjdk.java.net/jdk/pull/7928
Re: RFR: 8186958: Need method to create pre-sized HashMap [v18]
On Wed, 13 Apr 2022 22:20:14 GMT, XenoAmess wrote:
>> 8186958: Need method to create pre-sized HashMap
>
> XenoAmess has updated the pull request incrementally with one additional
> commit since the last revision:
>
> update LastModified
src/java.xml/share/classes/com/sun/org/apache/xerces/internal/impl/xs/traversers/XSAttributeChecker.java
line 1819:
> 1817: Map items;
> 1818: LargeContainer(int size) {
> 1819: items = HashMap.newHashMap(size*2+1);
I'm wondering if we should change this to just `newHashMap(size)` since it
looks like this container is intended to hold up to `size` items. @JoeWang-Java
? I suspect the `size*2+1` was a failed attempt at allocating a HashMap of the
correct capacity for `size` mappings.
-
PR: https://git.openjdk.java.net/jdk/pull/7928
Re: RFR: 8186958: Need method to create pre-sized HashMap [v18]
On Wed, 13 Apr 2022 22:20:14 GMT, XenoAmess wrote: >> 8186958: Need method to create pre-sized HashMap > > XenoAmess has updated the pull request incrementally with one additional > commit since the last revision: > > update LastModified src/java.xml/share/classes/com/sun/org/apache/xalan/internal/xsltc/dom/DocumentCache.java line 171: > 169: _current = 0; > 170: _size = size; > 171: _references = HashMap.newHashMap(_size); Not `_size+2` ? - PR: https://git.openjdk.java.net/jdk/pull/7928
Re: RFR: 8186958: Need method to create pre-sized HashMap [v18]
On Wed, 13 Apr 2022 22:40:38 GMT, Stuart Marks wrote:
>> XenoAmess has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> update LastModified
>
> src/java.base/share/classes/java/lang/Character.java line 8574:
>
>> 8572: private static final HashMap
>> aliases;
>> 8573: static {
>> 8574: aliases = HashMap.newHashMap(162);
>
> @naotoj Seems like this magic number is likely to go out of date. Should
> there be a test for it like the one you updated for NUM_ENTITIES?
> [JDK-8283465](https://bugs.openjdk.java.net/browse/JDK-8283465).
Good point! Filed an issue: https://bugs.openjdk.java.net/browse/JDK-8284856
-
PR: https://git.openjdk.java.net/jdk/pull/7928
Re: RFR: 8284853: Fix varios 'expected' typo
On Wed, 13 Apr 2022 20:36:48 GMT, Andrey Turbanov wrote:
> Found various typos of expected: `exepected`, `exept`, `epectedly`,
> `expeced`, `Unexpeted`, etc.
test/jdk/java/lang/StackWalker/StackStreamTest.java line 218:
> 216: private static void equalsOrThrow(String label, List list,
> List expected) {
> 217: System.out.println("List: " + list);
> 218: System.out.println("Expected: " + list);
I think there is a per-existing bug here. Shouldn't the second println print
`expected` instead of `list`?
-
PR: https://git.openjdk.java.net/jdk/pull/8231
Re: RFR: 8186958: Need method to create pre-sized HashMap [v18]
On Wed, 13 Apr 2022 22:20:14 GMT, XenoAmess wrote:
>> 8186958: Need method to create pre-sized HashMap
>
> XenoAmess has updated the pull request incrementally with one additional
> commit since the last revision:
>
> update LastModified
src/java.base/share/classes/java/lang/Character.java line 8574:
> 8572: private static final HashMap
> aliases;
> 8573: static {
> 8574: aliases = HashMap.newHashMap(162);
@naotoj Seems like this magic number is likely to go out of date. Should there
be a test for it like the one you updated for NUM_ENTITIES?
[JDK-8283465](https://bugs.openjdk.java.net/browse/JDK-8283465).
-
PR: https://git.openjdk.java.net/jdk/pull/7928
Re: RFR: 8186958: Need method to create pre-sized HashMap [v18]
> 8186958: Need method to create pre-sized HashMap XenoAmess has updated the pull request incrementally with one additional commit since the last revision: update LastModified - Changes: - all: https://git.openjdk.java.net/jdk/pull/7928/files - new: https://git.openjdk.java.net/jdk/pull/7928/files/4476c761..d110ecfd Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=7928&range=17 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=7928&range=16-17 Stats: 2 lines in 2 files changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/7928.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7928/head:pull/7928 PR: https://git.openjdk.java.net/jdk/pull/7928
Re: RFR: 8186958: Need method to create pre-sized HashMap [v16]
On Wed, 13 Apr 2022 20:06:34 GMT, Naoto Sato wrote: >> XenoAmess has updated the pull request incrementally with one additional >> commit since the last revision: >> >> revert changes in: >> src/java.desktop >> src/java.management >> src/jdk.internal.vm.ci >> src/jdk.jfr >> src/jdk.management.jfr >> src/jdk.management >> src/utils/IdealGraphVisualizer > > Looks good for changes in i18n related call sites, assuming that the > copyright years will be updated. > > Should we need some additions/modifications to the hashmap optimal capacity > utility `test/lib/jdk/test/lib/util/OptimalCapacity.java`? @naotoj @JoeWang-Java copyright updated to 2022. LastModified updated to 2022. - PR: https://git.openjdk.java.net/jdk/pull/7928
Re: RFR: 8284853: Fix varios 'expected' typo
On Wed, 13 Apr 2022 20:36:48 GMT, Andrey Turbanov wrote: > Found various typos of expected: `exepected`, `exept`, `epectedly`, > `expeced`, `Unexpeted`, etc. Expect the Unexpeted. - Marked as reviewed by bpb (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8231
Re: RFR: 8186958: Need method to create pre-sized HashMap [v17]
> 8186958: Need method to create pre-sized HashMap XenoAmess has updated the pull request incrementally with one additional commit since the last revision: Copyright latest year to 2022 - Changes: - all: https://git.openjdk.java.net/jdk/pull/7928/files - new: https://git.openjdk.java.net/jdk/pull/7928/files/2f5617bb..4476c761 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=7928&range=16 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=7928&range=15-16 Stats: 24 lines in 24 files changed: 0 ins; 0 del; 24 mod Patch: https://git.openjdk.java.net/jdk/pull/7928.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7928/head:pull/7928 PR: https://git.openjdk.java.net/jdk/pull/7928
Re: RFR: 8209038: Clarify the javadoc of Cipher.getParameters() [v2]
> Anyone can help review this javadoc update? The main change is the wording
> for the method javadoc of
> Cipher.getParameters()/CipherSpi.engineGetParameters(). The original wording
> is somewhat restrictive and request is to broaden this to accommodate more
> scenarios such as when null can be returned.
> The rest are minor things like add {@code } to class name and null, and
> remove redundant ".".
>
> Will file CSR after the review is close to being wrapped up.
> Thanks~
Valerie Peng has updated the pull request incrementally with one additional
commit since the last revision:
Update w/ review feedbacks
-
Changes:
- all: https://git.openjdk.java.net/jdk/pull/8117/files
- new: https://git.openjdk.java.net/jdk/pull/8117/files/b0463fba..9b8b71a6
Webrevs:
- full: https://webrevs.openjdk.java.net/?repo=jdk&pr=8117&range=01
- incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=8117&range=00-01
Stats: 19 lines in 2 files changed: 0 ins; 0 del; 19 mod
Patch: https://git.openjdk.java.net/jdk/pull/8117.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/8117/head:pull/8117
PR: https://git.openjdk.java.net/jdk/pull/8117
Re: RFR: 8209038: Clarify the javadoc of Cipher.getParameters()
On Wed, 13 Apr 2022 07:01:19 GMT, Xue-Lei Andrew Fan wrote:
>> How about this then?
>>
>> * The returned parameters may be the same that were used to
>> initialize
>> * this cipher, or may contain additional default and random parameter
>> * values used by the underlying cipher implementation if this
>> * cipher can successfully generate the required parameter values when
>> * not supplied. Otherwise, {@code null} is returned.
>
> I like the revised spec more.
>
> BTW, for "... contain additional default and random parameter values ...", is
> "or" more common than "and" in English?
Hmm, the original javadoc has "and", so I just picked it up. Perhaps I should
change it to "or" then. Thanks.
-
PR: https://git.openjdk.java.net/jdk/pull/8117
Re: [Internet]Re: JEP Review Request: TLS Certificate Compression
On Apr 13, 2022, at 2:07 PM, Bernd Eckenfels mailto:e...@zusammenkunft.net>> wrote: Hello, For multiple connections session- or ticket reuse would be much more efficient. In fact I think cert compression looks like the wrong solution. Having a immutable certificate download Chain would be a cool alternative solution - especially with future large postquantumcrypto certificates. That’s also easy to cache. I agreed, it would be cool as well if the certificate chain could be cached in the DNS record. Thanks, Xuelei (But I recon that’s not for this list to discuss, it’s just an argument against implementing a draft standard) Gruss Bernd -- http://bernd.eckenfels.net Von: security-dev mailto:security-dev-r...@openjdk.java.net>> im Auftrag von Daniel Jeliński mailto:djelins...@gmail.com>> Gesendet: Wednesday, April 13, 2022 10:01:29 PM An: xueleifan(XueleiFan) mailto:xuelei...@tencent.com>> Cc: OpenJDK Dev list mailto:security-dev@openjdk.java.net>> Betreff: Re: JEP Review Request: TLS Certificate Compression I like the idea of implementing certificate compression. Only one concern: TLS handshakes are generally a CPU-intensive operation, and certificate compression / decompression will only make it worse. Will it be possible to compress a certificate once and use it across multiple handshakes? Decompression has to be performed every time, obviously. Regards, Daniel pon., 21 mar 2022 o 16:49 xueleifan(XueleiFan) mailto:xuelei...@tencent.com>> napisał(a): > > Hi, > > > The JDK Enhancement Proposal, TLS Certificate Compression, has been opened > for community review. Detailed, please refer to the draft: > > https://bugs.openjdk.java.net/browse/JDK-8281710 > > and the discussion of this potential feature at security-dev: > > > https://mail.openjdk.java.net/pipermail/security-dev/2022-March/029242.html > > > Please feel free to make comments and review the JEP. > > Thanks, > Xuelei
Re: [Internet]Re: JEP Review Request: TLS Certificate Compression
Hi Daniel, Actually, I’m considering the improvement, by using cached compressed certificates, for the implementation. The solution is not straightforward yet to me. But it is a direction I will consider seriously. Thanks, Xuelei > On Apr 13, 2022, at 1:01 PM, Daniel Jeliński wrote: > > I like the idea of implementing certificate compression. Only one > concern: TLS handshakes are generally a CPU-intensive operation, and > certificate compression / decompression will only make it worse. Will > it be possible to compress a certificate once and use it across > multiple handshakes? Decompression has to be performed every time, > obviously. > > Regards, > Daniel > > pon., 21 mar 2022 o 16:49 xueleifan(XueleiFan) > napisał(a): >> >> Hi, >> >> >> The JDK Enhancement Proposal, TLS Certificate Compression, has been opened >> for community review. Detailed, please refer to the draft: >> >>https://bugs.openjdk.java.net/browse/JDK-8281710 >> >> and the discussion of this potential feature at security-dev: >> >> >> https://mail.openjdk.java.net/pipermail/security-dev/2022-March/029242.html >> >> >> Please feel free to make comments and review the JEP. >> >> Thanks, >> Xuelei >
Re: RFR: 8284694: Avoid evaluating SSLAlgorithmConstraints twice [v2]
On Wed, 13 Apr 2022 16:02:50 GMT, Xue-Lei Andrew Fan wrote: >> Thanks @XueleiFan for the review! >> If we do that, this will result in a behavior change for cases where >> `enabledX509DisabledAlgConstraints` = false; is that okay? Or should we set >> `enabledX509DisabledAlgConstraints` = true if `userSpecifiedConstraints == >> DEFAULT`? > > I think it is OK. The enabledX509DisabledAlgConstraints should be specified > with the withDefaultCertPathConstraints parameterm, and should not be > overrode by the userSpecifiedConstraints. I think it is a behavior that we'd > like to correct. updated the patch. Let me know if that's what you had in mind. - PR: https://git.openjdk.java.net/jdk/pull/8199
Re: RFR: 8186958: Need method to create pre-sized HashMap [v16]
On Wed, 13 Apr 2022 16:29:11 GMT, XenoAmess wrote: >> 8186958: Need method to create pre-sized HashMap > > XenoAmess has updated the pull request incrementally with one additional > commit since the last revision: > > revert changes in: > src/java.desktop > src/java.management > src/jdk.internal.vm.ci > src/jdk.jfr > src/jdk.management.jfr > src/jdk.management > src/utils/IdealGraphVisualizer The ZipFS and Jar changes seem OK - Marked as reviewed by lancea (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/7928
Re: JEP Review Request: TLS Certificate Compression
Hello, For multiple connections session- or ticket reuse would be much more efficient. In fact I think cert compression looks like the wrong solution. Having a immutable certificate download Chain would be a cool alternative solution - especially with future large postquantumcrypto certificates. That’s also easy to cache. (But I recon that’s not for this list to discuss, it’s just an argument against implementing a draft standard) Gruss Bernd -- http://bernd.eckenfels.net Von: security-dev im Auftrag von Daniel Jeliński Gesendet: Wednesday, April 13, 2022 10:01:29 PM An: xueleifan(XueleiFan) Cc: OpenJDK Dev list Betreff: Re: JEP Review Request: TLS Certificate Compression I like the idea of implementing certificate compression. Only one concern: TLS handshakes are generally a CPU-intensive operation, and certificate compression / decompression will only make it worse. Will it be possible to compress a certificate once and use it across multiple handshakes? Decompression has to be performed every time, obviously. Regards, Daniel pon., 21 mar 2022 o 16:49 xueleifan(XueleiFan) napisał(a): > > Hi, > > > The JDK Enhancement Proposal, TLS Certificate Compression, has been opened > for community review. Detailed, please refer to the draft: > > https://bugs.openjdk.java.net/browse/JDK-8281710 > > and the discussion of this potential feature at security-dev: > > > https://mail.openjdk.java.net/pipermail/security-dev/2022-March/029242.html > > > Please feel free to make comments and review the JEP. > > Thanks, > Xuelei
Re: RFR: 8186958: Need method to create pre-sized HashMap [v16]
On Wed, 13 Apr 2022 16:29:11 GMT, XenoAmess wrote: >> 8186958: Need method to create pre-sized HashMap > > XenoAmess has updated the pull request incrementally with one additional > commit since the last revision: > > revert changes in: > src/java.desktop > src/java.management > src/jdk.internal.vm.ci > src/jdk.jfr > src/jdk.management.jfr > src/jdk.management > src/utils/IdealGraphVisualizer Looks good for the changes in java.xml, like Naoto, assuming copyright years and the LastModified tag are updated. I generally prefer for the source level to stay at 8 as the upstream source is maintained at an older JDK level, but these changes seem to be small enough to tolerate. - Marked as reviewed by joehw (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/7928
Re: RFR: JDK-8284112 Minor cleanup could be done in javax.crypto
On Wed, 13 Apr 2022 06:31:51 GMT, Bradford Wetmore wrote:
>> JDK-8284112 Minor cleanup could be done in javax.crypto
>
> src/java.base/share/classes/javax/crypto/CipherInputStream.java line 159:
>
>> 157: try {
>> 158: ofinish = cipher.update(ibuffer, 0, readin, obuffer,
>> ostart);
>> 159: } catch (IllegalStateException e) {
>
> This is another one of those I would probably leave alone, just so it's clear
> what should be done.
Keeping the original.
> src/java.base/share/classes/javax/crypto/CipherSpi.java line 29:
>
>> 27:
>> 28: import java.nio.ByteBuffer;
>> 29: import java.security.*;
>
> This is another one that some people will complain about. I personally
> prefer this style, others prefer everything written out as long as it's not
> "too many."
Keeping the change for now.
> src/java.base/share/classes/javax/crypto/CryptoPermission.java line 437:
>
>> 435: // may be the best try.
>> 436: return this.algParamSpec.equals(algParamSpec);
>> 437: } else return !this.checkParam;
>
> Please use the standard coding format.
>
> } else {
> return !this.checkParam;
> }
>
> or
>
> }
>
> return !this.checkParam;
Using the first format.
> src/java.base/share/classes/javax/crypto/CryptoPermissions.java line 158:
>
>> 156:
>> 157: /**
>> 158: * Checks if this object's PermissionCollection for permissions
>
> Just FYI and not for this review, but this class should really be updated to
> use proper javadoc comment style, which is to tag all objects with < code
> >PermissionCollection< /code >
No change required.
> src/java.base/share/classes/javax/crypto/ExemptionMechanism.java line 142:
>
>> 140: * @see java.security.Provider
>> 141: */
>> 142: public static ExemptionMechanism getInstance(String algorithm)
>
> Others might disagree with this comment, but I would encourage you not to
> make these changes throughout your PR. (@jddarcy ?)
>
> Even though a static method is implicitly final and IJ is correct, the final
> keyword does prevent subclasses from inadvertently using the same method
> signature.
>
> Once this is resolved, I can approve.
Keeping the original here and elsewhere.
> src/java.base/share/classes/javax/crypto/SealedObject.java line 449:
>
>> 447: final class extObjectInputStream extends ObjectInputStream {
>> 448: extObjectInputStream(InputStream in)
>> 449: throws IOException {
>
> This is another "I probably wouldn't do that..."
>
> It's nice to know what kind of IOExceptions you might get, so leaving
> StreamCorruptedException is ok.
Keeping the original. Still, why would IJ suggest removing this unless it had
already figured out the exception could never happen?
> src/java.base/share/classes/javax/crypto/spec/DESKeySpec.java line 49:
>
>> 47: * Weak/semi-weak keys copied from FIPS 74.
>> 48: *
>> 49: * "...The first 6 keys have duals different from themselves, hence
>
> Suggest you leave this alone as this is a direct quote from FIPS 74 (page 10).
Keeping the original.
-
PR: https://git.openjdk.java.net/jdk/pull/8214
RFR: 8284853: Fix varios 'expected' typo
Found various typos of expected: `exepected`, `exept`, `epectedly`, `expeced`, `Unexpeted`, etc. - Commit messages: - [PATCH] Fix 'expected' typo - [PATCH] Fix 'expected' typo - [PATCH] Fix 'expected' typo Changes: https://git.openjdk.java.net/jdk/pull/8231/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=8231&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8284853 Stats: 65 lines in 28 files changed: 0 ins; 0 del; 65 mod Patch: https://git.openjdk.java.net/jdk/pull/8231.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8231/head:pull/8231 PR: https://git.openjdk.java.net/jdk/pull/8231
Re: RFR: 8284694: Avoid evaluating SSLAlgorithmConstraints twice [v2]
> 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. Daniel Jeliński has updated the pull request incrementally with one additional commit since the last revision: Avoid nesting SSLAlgorithmConstraints - Changes: - all: https://git.openjdk.java.net/jdk/pull/8199/files - new: https://git.openjdk.java.net/jdk/pull/8199/files/9bcd0b66..b6e46ae9 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=8199&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=8199&range=00-01 Stats: 14 lines in 1 file changed: 9 ins; 0 del; 5 mod Patch: https://git.openjdk.java.net/jdk/pull/8199.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8199/head:pull/8199 PR: https://git.openjdk.java.net/jdk/pull/8199
Re: RFR: 8186958: Need method to create pre-sized HashMap [v16]
On Wed, 13 Apr 2022 16:29:11 GMT, XenoAmess wrote: >> 8186958: Need method to create pre-sized HashMap > > XenoAmess has updated the pull request incrementally with one additional > commit since the last revision: > > revert changes in: > src/java.desktop > src/java.management > src/jdk.internal.vm.ci > src/jdk.jfr > src/jdk.management.jfr > src/jdk.management > src/utils/IdealGraphVisualizer Looks good for changes in i18n related call sites, assuming that the copyright years will be updated. Should we need some additions/modifications to the hashmap optimal capacity utility `test/lib/jdk/test/lib/util/OptimalCapacity.java`? - Marked as reviewed by naoto (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/7928
Re: JEP Review Request: TLS Certificate Compression
I like the idea of implementing certificate compression. Only one concern: TLS handshakes are generally a CPU-intensive operation, and certificate compression / decompression will only make it worse. Will it be possible to compress a certificate once and use it across multiple handshakes? Decompression has to be performed every time, obviously. Regards, Daniel pon., 21 mar 2022 o 16:49 xueleifan(XueleiFan) napisał(a): > > Hi, > > > The JDK Enhancement Proposal, TLS Certificate Compression, has been opened > for community review. Detailed, please refer to the draft: > > https://bugs.openjdk.java.net/browse/JDK-8281710 > > and the discussion of this potential feature at security-dev: > > > https://mail.openjdk.java.net/pipermail/security-dev/2022-March/029242.html > > > Please feel free to make comments and review the JEP. > > Thanks, > Xuelei
Re: RFR: 8186958: Need method to create pre-sized HashMap [v16]
On Wed, 13 Apr 2022 16:29:11 GMT, XenoAmess wrote: >> 8186958: Need method to create pre-sized HashMap > > XenoAmess has updated the pull request incrementally with one additional > commit since the last revision: > > revert changes in: > src/java.desktop > src/java.management > src/jdk.internal.vm.ci > src/jdk.jfr > src/jdk.management.jfr > src/jdk.management > src/utils/IdealGraphVisualizer Reviewers for i18n, net, nio, and security, please review call site changes in your areas. Thanks. - PR: https://git.openjdk.java.net/jdk/pull/7928
Re: RFR: 8284553: Deprecate the DEFAULT static field of OAEPParameterSpec [v2]
> This trivial change is to deprecate the DEFAULT static field of > OAEPParameterSpec class. Wordings are mostly the same as the previous > PSSParameterSpec deprecation change. Rest are just minor code re-factoring. > > The CSR will be filed once review is somewhat finished. > > Thanks, > Valerie Valerie Peng has updated the pull request incrementally with one additional commit since the last revision: Update w/ review feedback. - Changes: - all: https://git.openjdk.java.net/jdk/pull/8191/files - new: https://git.openjdk.java.net/jdk/pull/8191/files/371afccb..ba572eed Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=8191&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=8191&range=00-01 Stats: 8 lines in 1 file changed: 5 ins; 0 del; 3 mod Patch: https://git.openjdk.java.net/jdk/pull/8191.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8191/head:pull/8191 PR: https://git.openjdk.java.net/jdk/pull/8191
Re: RFR: 8284553: Deprecate the DEFAULT static field of OAEPParameterSpec
On Wed, 13 Apr 2022 18:28:33 GMT, Valerie Peng wrote:
>> src/java.base/share/classes/javax/crypto/spec/OAEPParameterSpec.java line 81:
>>
>>> 79: * parameters for mgf -- MGF1ParameterSpec.SHA1
>>> 80: * source of encoding input -- PSource.PSpecified.DEFAULT
>>> 81: *
>>
>> I think you should leave these lines in, so users can still see what the
>> default parameters are. Nit, change:
>> "the OAEPParameterSpec.DEFAULT uses the following"
>> to:
>> "{@code OAEPParameterSpec.DEFAULT} uses the following default values:"
>
> Leaving this in meaning we may have to duplicate the new wordings for the
> field here. The default values are covered in the paragraph above where the
> ASN.1 structure for RSAES-OAEP-params is defined. I can move this down to the
> javadoc field description for the DEFAULT field.
Ok.
-
PR: https://git.openjdk.java.net/jdk/pull/8191
Re: RFR: 8284553: Deprecate the DEFAULT static field of OAEPParameterSpec
On Wed, 13 Apr 2022 15:35:38 GMT, Sean Mullan wrote:
>> This trivial change is to deprecate the DEFAULT static field of
>> OAEPParameterSpec class. Wordings are mostly the same as the previous
>> PSSParameterSpec deprecation change. Rest are just minor code re-factoring.
>>
>> The CSR will be filed once review is somewhat finished.
>>
>> Thanks,
>> Valerie
>
> src/java.base/share/classes/javax/crypto/spec/OAEPParameterSpec.java line 81:
>
>> 79: * parameters for mgf -- MGF1ParameterSpec.SHA1
>> 80: * source of encoding input -- PSource.PSpecified.DEFAULT
>> 81: *
>
> I think you should leave these lines in, so users can still see what the
> default parameters are. Nit, change:
> "the OAEPParameterSpec.DEFAULT uses the following"
> to:
> "{@code OAEPParameterSpec.DEFAULT} uses the following default values:"
Leaving this in meaning we may have to duplicate the new wordings for the field
here. The default values are covered in the paragraph above where the ASN.1
structure for RSAES-OAEP-params is defined. I can move this down to the javadoc
field description for the DEFAULT field.
-
PR: https://git.openjdk.java.net/jdk/pull/8191
Re: RFR: 8284553: Deprecate the DEFAULT static field of OAEPParameterSpec
On Tue, 12 Apr 2022 01:27:35 GMT, Valerie Peng wrote:
> This trivial change is to deprecate the DEFAULT static field of
> OAEPParameterSpec class. Wordings are mostly the same as the previous
> PSSParameterSpec deprecation change. Rest are just minor code re-factoring.
>
> The CSR will be filed once review is somewhat finished.
>
> Thanks,
> Valerie
> _Mailing list message from [Michael StJohns](mailto:mstjo...@comcast.net) on
> [security-dev](mailto:security-...@mail.openjdk.java.net):_
>
> On 4/11/2022 9:34 PM, Valerie Peng wrote:
>
> Hi Valerie -
>
> I think deprecating DEFAULT? is wrong.? RFC8017 still has this definition:
>
> > RSAES-OAEP-params ::= SEQUENCE {
> > hashAlgorithm [0] HashAlgorithm DEFAULT sha1,
> > maskGenAlgorithm [1] MaskGenAlgorithm DEFAULT mgf1SHA1,
> > pSourceAlgorithm [2] PSourceAlgorithm DEFAULT pSpecifiedEmpty
> > }
>
> and DEFAULT is what you should be getting if you see an empty sequence in the
> param field.? It's DEFAULT in ASN1 terms, not DEFAULT in terms of what you
> should use going forward? to create signatures, and the ASN1 DEFAULT won't
> change.
>
> In any event, I can't find where RFC8017 says anything about deprecating the
> defaults.? AFAICT, although there's general guidance to go away from SHA1,?
> the math suggests that SHA1 is still sufficient for OAEP,? and there's no
> guidance specific to OAEP's use of SHA1 that I can find other than the
> requirement in NIST SP800-56B rev 2 to use "Approved Hash Functions" for
> OAEP. If there's a section in 8017 that you're looking at for this guidance
> that I missed, you may want to update your comment to point to it.
>
> Take care - Mike
>
> -- next part -- An HTML attachment was scrubbed...
> URL:
>
Hi Mike,
Thanks for the comment/feedback. Deprecating the static field in javadoc does
not mean we are changing its parameters with something other than those in RFC
8017. Providers can and should still follow the default ASN.1 values when the
DER encoding omit the particular field(s). Besides what Sean mentioned already,
this deprecation is an effort to moving toward the "explicitly specify what you
want to use" direction. Hope this clarifies thing up?
I like the suggestion for using a more specific link and will change it to
point to the particular section.
Regards,
Valerie
-
PR: https://git.openjdk.java.net/jdk/pull/8191
AlgorithmConstraints caching [ was Re: RFR: 8284694: Avoid evaluating SSLAlgorithmConstraints twice]
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: On Tue, 12 Apr 2022 11:28:12 GMT, Daniel Jeliński wrote: 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. 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) 1637100 % boolean java.lang.String.regionMatches(boolean, int, String, int, int) 1637 100 % boolean java.lang.String.equalsIgnoreCase(String) 1637100 % boolean sun.security.util.AbstractAlgorithmConstraints.checkAlgorithm(List, String, AlgorithmDecomposer)163199.6 % boolean sun.security.util.DisabledAlgorithmConstraints.permits(Set, String, AlgorithmParameters)163199.6 % boolean sun.security.ssl.SSLAlgorithmConstraints.permits(Set, String, AlgorithmParameters) 163199.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 % - Marked as reviewed by coffeys (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8199
Re: [Internet]Re: JEP Review Request: TLS Certificate Compression
Ping … Xuelei > On Mar 24, 2022, at 1:05 PM, Sean Mullan wrote: > > > > On 3/21/22 11:49 AM, xueleifan(XueleiFan) wrote: >> Hi, >> >> >> The JDK Enhancement Proposal, TLS Certificate Compression, has been opened >> for community review. Detailed, please refer to the draft: >> >> https://bugs.openjdk.java.net/browse/JDK-8281710 > > Or a more readable version at https://openjdk.java.net/jeps/8281710 > > --Sean > >> >> and the discussion of this potential feature at security-dev: >> >> https://mail.openjdk.java.net/pipermail/security-dev/2022-March/029242.html >> >> >> Please feel free to make comments and review the JEP. >> >> Thanks, >> Xuelei > >
Re: RFR: 8284694: Avoid evaluating SSLAlgorithmConstraints twice
On Wed, 13 Apr 2022 07:50:55 GMT, Daniel Jeliński wrote:
>> src/java.base/share/classes/sun/security/ssl/SSLAlgorithmConstraints.java
>> line 72:
>>
>>> 70: }
>>> 71:
>>> 72: static AlgorithmConstraints wrap(AlgorithmConstraints
>>> userSpecifiedConstraints) {
>>
>> I may update all of the constructors so that the accumulation of the
>> reference of userSpecifiedConstraints could be avoid further.
>>
>>
>> - this.userSpecifiedConstraints = userSpecifiedConstraints;
>> + this.userSpecifiedConstraints = userSpecifiedConstraints == DEFAULT ?
>> + null : userSpecifiedConstraints;
>>
>>
>>
>> Similar update could be placed in the getUserSpecifiedConstraints()
>> implementation.
>
> Thanks @XueleiFan for the review!
> If we do that, this will result in a behavior change for cases where
> `enabledX509DisabledAlgConstraints` = false; is that okay? Or should we set
> `enabledX509DisabledAlgConstraints` = true if `userSpecifiedConstraints ==
> DEFAULT`?
I think it is OK. The enabledX509DisabledAlgConstraints should be specified
with the withDefaultCertPathConstraints parameterm, and should not be overrode
by the userSpecifiedConstraints. I think it is a behavior that we'd like to
correct.
-
PR: https://git.openjdk.java.net/jdk/pull/8199
Re: RFR: 8284553: Deprecate the DEFAULT static field of OAEPParameterSpec
On Tue, 12 Apr 2022 01:27:35 GMT, Valerie Peng wrote:
> This trivial change is to deprecate the DEFAULT static field of
> OAEPParameterSpec class. Wordings are mostly the same as the previous
> PSSParameterSpec deprecation change. Rest are just minor code re-factoring.
>
> The CSR will be filed once review is somewhat finished.
>
> Thanks,
> Valerie
src/java.base/share/classes/javax/crypto/spec/OAEPParameterSpec.java line 81:
> 79: * parameters for mgf -- MGF1ParameterSpec.SHA1
> 80: * source of encoding input -- PSource.PSpecified.DEFAULT
> 81: *
I think you should leave these lines in, so users can still see what the
default parameters are. Nit, change:
"the OAEPParameterSpec.DEFAULT uses the following"
to:
"{@code OAEPParameterSpec.DEFAULT} uses the following default values:"
-
PR: https://git.openjdk.java.net/jdk/pull/8191
Re: RFR: 8284694: Avoid evaluating SSLAlgorithmConstraints twice
On Tue, 12 Apr 2022 11:28:12 GMT, Daniel Jeliński wrote: > 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. 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) 1637100 % boolean java.lang.String.regionMatches(boolean, int, String, int, int) 1637 100 % boolean java.lang.String.equalsIgnoreCase(String) 1637100 % boolean sun.security.util.AbstractAlgorithmConstraints.checkAlgorithm(List, String, AlgorithmDecomposer)163199.6 % boolean sun.security.util.DisabledAlgorithmConstraints.permits(Set, String, AlgorithmParameters)163199.6 % boolean sun.security.ssl.SSLAlgorithmConstraints.permits(Set, String, AlgorithmParameters) 163199.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 % - Marked as reviewed by coffeys (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8199
Re: RFR: 8284694: Avoid evaluating SSLAlgorithmConstraints twice
On Tue, 12 Apr 2022 11:28:12 GMT, Daniel Jeliński wrote:
> 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.
test/micro/org/openjdk/bench/java/security/SSLHandshake.java line 54:
> 52: @OutputTimeUnit(TimeUnit.SECONDS)
> 53: @State(Scope.Benchmark)
> 54: public class SSLHandshake {
Default number of iterations and forks is usually excessive, I suggest some
tuning runs to minimize benchmark execution time, e.g., something like:
@Warmup(iterations = 10, time = 500, timeUnit = TimeUnit.MILLISECONDS)
@Measurement(iterations = 5, time = 1000, timeUnit = TimeUnit.MILLISECONDS)
@Fork(3)
-
PR: https://git.openjdk.java.net/jdk/pull/8199
Re: RFR: 8284694: Avoid evaluating SSLAlgorithmConstraints twice
On Wed, 13 Apr 2022 06:45:20 GMT, Xue-Lei Andrew Fan wrote:
>> 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.
>
> src/java.base/share/classes/sun/security/ssl/SSLAlgorithmConstraints.java
> line 72:
>
>> 70: }
>> 71:
>> 72: static AlgorithmConstraints wrap(AlgorithmConstraints
>> userSpecifiedConstraints) {
>
> I may update all of the constructors so that the accumulation of the
> reference of userSpecifiedConstraints could be avoid further.
>
>
> - this.userSpecifiedConstraints = userSpecifiedConstraints;
> + this.userSpecifiedConstraints = userSpecifiedConstraints == DEFAULT ?
> + null : userSpecifiedConstraints;
>
>
>
> Similar update could be placed in the getUserSpecifiedConstraints()
> implementation.
Thanks @XueleiFan for the review!
If we do that, this will result in a behavior change for cases where
`enabledX509DisabledAlgConstraints` = false; is that okay? Or should we set
`enabledX509DisabledAlgConstraints` = true if `userSpecifiedConstraints ==
DEFAULT`?
-
PR: https://git.openjdk.java.net/jdk/pull/8199
Re: RFR: 8284796: sun.security.ssl.Finished::toString misses a line feed in the message format pattern
On Wed, 13 Apr 2022 06:07:43 GMT, Xue-Lei Andrew Fan wrote:
>> The log for Finished message looks like the below,
>>
>> "Finished": {
>> "verify data": {
>> : ... ...
>> }'} // looks weird
>>
>> because a line feed is missing in the format pattern.
>>
>> ""Finished": '{'\n" +
>> " "verify data": '{'\n" +
>> "{0}\n" +
>> " '}'" + // a line feed is needed
>> "'}'",
>
> src/java.base/share/classes/sun/security/ssl/Finished.java line 150:
>
>> 148: "{0}\n" +
>> 149: " '}'\n" +
>> 150: "'}'",
>
> As you are already here to touch the code, would you like to use text block
> instead?
Could I not change this style?
Maybe another PR should be filed for applying text block feature in
sun.security.ssl package.
-
PR: https://git.openjdk.java.net/jdk/pull/8215
Re: RFR: 8209038: Clarify the javadoc of Cipher.getParameters()
On Tue, 12 Apr 2022 03:27:59 GMT, Valerie Peng wrote:
>> I read the following methods in com.sun.crypto.provider.CipherCore:
>>
>> void init(int opmode, Key key, AlgorithmParameterSpec params,
>> SecureRandom random)
>>
>> where the 'params' are converted to IV byte array for further processing.
>>
>>
>> void init(int opmode, Key key, AlgorithmParameters params,
>> SecureRandom random)
>>
>> where the spec is retrieved from the 'params', and then pass the call to the
>> init() method above.
>>
>>
>> AlgorithmParameters getParameters(String algName) {
>>
>> where the returned parameters are re-constructed.
>>
>> It may be fine to use a strict spec, but there is a chance to have
>> compatibility impact unless we check the implementation carefully. It may
>> not worthy the risks as a behavioral update may be not necessary for
>> developers.
>
> How about this then?
>
> * The returned parameters may be the same that were used to initialize
> * this cipher, or may contain additional default and random parameter
> * values used by the underlying cipher implementation if this
> * cipher can successfully generate the required parameter values when
> * not supplied. Otherwise, {@code null} is returned.
I like the revised spec more.
BTW, for "... contain additional default and random parameter values ...", is
"or" more common than "and" in English?
-
PR: https://git.openjdk.java.net/jdk/pull/8117
