Re: RFR: 8284926: Share the certificate NamedGroup in SignatureScheme::getSignerOfPreferableAlgorithm [v2]
On Mon, 18 Apr 2022 12:37:15 GMT, John Jiang wrote:
>> It would not to generate the certificate's ECParameterSpec and NamedGroup
>> multiple times in method `SignatureScheme::getSignerOfPreferableAlgorithm`.
>
> John Jiang has updated the pull request incrementally with one additional
> commit since the last revision:
>
> cache ParamSpec and NamedGroup in X509Possession
src/java.base/share/classes/sun/security/ssl/ECDHClientKeyExchange.java line
274:
> 272: // Iteratively determine the X509Possession type's
> ParameterSpec.
> 273: ECParameterSpec ecParams =
> x509Possession.getECParameterSpec();
> 274: NamedParameterSpec namedParams =
> x509Possession.getXECParameterSpec();
It may not necessary to define 'ecParams' and 'namedParams' any longer, which
was used to find out the named group. Now, the checking could be placed on the
"namedGroup" (if the named group is EC/CDH) around line 293.
src/java.base/share/classes/sun/security/ssl/SignatureScheme.java line 476:
> 474: PrivateKey signingKey = x509Possession.popPrivateKey;
> 475:
> 476: ECParameterSpec params = x509Possession.getECParameterSpec();
This 'params' variable is used for debug only. Maybe, it could be moved to the
debug log block.
src/java.base/share/classes/sun/security/ssl/X509Authentication.java line 157:
> 155: }
> 156:
> 157: private ECParameterSpec getECParams() {
'getECParamSpec' may be a better method name.
src/java.base/share/classes/sun/security/ssl/X509Authentication.java line 182:
> 180:
> 181: // Similar to above, but for XEC.
> 182: private NamedParameterSpec getXECParams() {
'getXECParamSpec' may be a better method name.
-
PR: https://git.openjdk.java.net/jdk/pull/8271
Integrated: JDK-6782021: It is not possible to read local computer certificates with the SunMSCAPI provider
On Tue, 12 Apr 2022 19:03:40 GMT, Mat Carter wrote: > On Windows you can now access the local machine keystores using the strings > "Windows-MY-LOCALMACHINE" and "Windows-ROOT-LOCALMACHINE"; note the > application requires admin privileges. > > "Windows-MY" and "Windows-ROOT" remain unchanged, however given these > original keystore strings mapped to the current user, I added > "Windows-MY-CURRENTUSER" and "Windows-ROOT-CURRENTUSER" so that a developer > can explicitly specify the current user location. These two new strings > simply map to the original two strings, i.e. no duplication of code paths etc > > No new tests added, keystore functionality and API remains unchanged, the > local machine keystore types would require the tests to run in admin mode > > Tested on windows, passes tier1 and tier2 tests This pull request has now been integrated. Changeset: 5e5500cb Author:Mat Carter <54955201+maca...@users.noreply.github.com> Committer: Weijun Wang URL: https://git.openjdk.java.net/jdk/commit/5e5500cbd79b40a32c20547ea0cdb81ef6904a3d Stats: 187 lines in 5 files changed: 155 ins; 1 del; 31 mod 6782021: It is not possible to read local computer certificates with the SunMSCAPI provider Reviewed-by: weijun - PR: https://git.openjdk.java.net/jdk/pull/8211
Re: RFR: JDK-6782021: It is not possible to read local computer certificates with the SunMSCAPI provider [v6]
On Wed, 11 May 2022 16:01:39 GMT, Mat Carter wrote: >> On Windows you can now access the local machine keystores using the strings >> "Windows-MY-LOCALMACHINE" and "Windows-ROOT-LOCALMACHINE"; note the >> application requires admin privileges. >> >> "Windows-MY" and "Windows-ROOT" remain unchanged, however given these >> original keystore strings mapped to the current user, I added >> "Windows-MY-CURRENTUSER" and "Windows-ROOT-CURRENTUSER" so that a developer >> can explicitly specify the current user location. These two new strings >> simply map to the original two strings, i.e. no duplication of code paths etc >> >> No new tests added, keystore functionality and API remains unchanged, the >> local machine keystore types would require the tests to run in admin mode >> >> Tested on windows, passes tier1 and tier2 tests > > Mat Carter has updated the pull request incrementally with one additional > commit since the last revision: > > Change admin privilege test to reliable method Marked as reviewed by weijun (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8211
Re: RFR: 8253176: Signature.getParameters should specify that it can throw UnsupportedOperationException [v7]
On Mon, 16 May 2022 13:28:13 GMT, Sean Mullan wrote:
>> With this modification of 2nd sentence. The whole paragraph becomes:
>>
>> * The returned parameters may be the same that were used to
>> initialize
>> * this signature, or may contain additional default or random parameter
>> * values used by the underlying signature scheme. If the required
>> * parameters were not supplied and can be generated by the signature,
>> * the generated parameters are returned; otherwise {@code null} is
>> * returned. However, if the signature scheme does not support returning
>> * the parameters as {@code AlgorithmParameters}, {@code null} is always
>> * returned.
>>
>> For the last sentence, would it be better to use "also" instead of "always"?
>
> I think "also" would not be quite right, because I would read that as there
> is something else that is also returned with `null`. I think you can remove
> the word "always" -- it isn't really necessary and that might address your
> concern.
Max suggested moving the last sentence to a new paragraph, so I did that and
leave the "always" in.
-
PR: https://git.openjdk.java.net/jdk/pull/8396
Re: RFR: 8253176: Signature.getParameters should specify that it can throw UnsupportedOperationException [v8]
> This is to update the method javadoc of > java.security.Signature.getParameters() with the missing `@throws > UnsupportedOperationException`. In addition, the wording on the returned > parameters are updated to match those in Cipher and CipherSpi classes. > > CSR will be filed later. > > Thanks, > Valerie Valerie Peng has updated the pull request incrementally with one additional commit since the last revision: review feedback - Changes: - all: https://git.openjdk.java.net/jdk/pull/8396/files - new: https://git.openjdk.java.net/jdk/pull/8396/files/68b36fc1..94a584bc Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=8396&range=07 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=8396&range=06-07 Stats: 12 lines in 2 files changed: 6 ins; 0 del; 6 mod Patch: https://git.openjdk.java.net/jdk/pull/8396.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8396/head:pull/8396 PR: https://git.openjdk.java.net/jdk/pull/8396
Re: RFR: 8283577: SSLEngine.unwrap on read-only input ByteBuffer [v2]
On Mon, 16 May 2022 21:08:48 GMT, Anthony Scarpino
wrote:
> There is too much grey area. It says the src buffer maybe modified, which one
> could interpret it cannot be a read-only, but that would still need
> clarification to explicitly say "no read only buffers". And other than these
> internal 'in-place' crypto reason, there is no API reason to not allow
> read-only buffers as input. I did think about closing the CSR as the text was
> already there about the src buffer, even thought it was using a different
> term. But I felt strongly enough that I wanted to prevent that confusion in
> the future.
I think it is good to make the clarification with a CSR.
As the spec says, "The inbound network buffer, {@code src}, may be modified",
applications cannot assume that the inbound network buffer cannot be modified
(read-only) any longer. It does not grant that an application will work with
read-only inbound buffers for another JSSE provider. As a result, an
application that want to support read-only buffers may also need to support
non-read-only buffers. As makes it more complicated in both application layers
and the JSSE implementation layer. It may be simple that applications and JSSE
implementation codes only need to handle non-read-only buffers.
Just my $0.02. I will let you and Brad for the final decision.
-
PR: https://git.openjdk.java.net/jdk/pull/8462
Re: RFR: 8283577: SSLEngine.unwrap on read-only input ByteBuffer [v2]
On Sat, 14 May 2022 03:29:14 GMT, Anthony Scarpino wrote: >> Hi, >> >> I need a review of this fix to allow a read-only 'src' buffer to be used >> with SSLEngine.unwrap(). A temporary read-write buffer is created in the >> SSLCipher operation when a read-only buffer is passed. If the 'src' is >> read-write, there is no effect on the current operation >> >> The PR also includes a CSR for an API implementation note to the >> SSLEngine.unwrap. The 'src' buffer may be modified during the decryption >> operation. 'unwrap()' has had this behavior forever, so there is no >> compatibility issue with this note. Using the 'src' buffer for in-place >> decryption was a performance decision. >> >> Tony > > Anthony Scarpino has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains four commits: > > - review update > - update some nits > - PR ready > - Initial There is too much grey area. It says the src buffer maybe modified, which one could interpret it cannot be a read-only, but that would still need clarification to explicitly say "no read only buffers". And other than these internal 'in-place' crypto reason, there is no API reason to not allow read-only buffers as input. I did think about closing the CSR as the text was already there about the src buffer, even thought it was using a different term. But I felt strongly enough that I wanted to prevent that confusion in the future. - PR: https://git.openjdk.java.net/jdk/pull/8462
Re: RFR: 8286715: Generalize MemorySegment::ofBuffer [v2]
> This patch makes MemorySegment::ofBuffer more general, by allowing clients to > pass *any* `Buffer` instance, not just `ByteBuffer`. > This allows us to match expressiveness of JNI API, where JNI clients can > obtain the address of any direct buffer instance, using the > `GetDirectBufferAddress` function. > > We thought about also providing a more general way to view a segment as a > buffer (e.g. asIntBuffer) but doing that doesn't seem worth it: direct > buffers can only created form `ByteBuffer`. > So, to create a direct `IntBuffer`, clients have to first create a direct > `ByteBuffer` then to view that buffer as an `IntBuffer`. > > In other words, `IntBuffer` and friends are not first-class citizens in the > `Buffer` API. As such it would not be possible to map many memory segments > into an `IntBuffer`; in fact, the only segment we could safely map into an > `IntBuffer` would be an _heap_ segment backed by an `int[]`. As such it > doesn't seem worth adding a lot of API surface (in terms of additional > overloads) for such a corner case. Maurizio Cimadamore has updated the pull request incrementally with one additional commit since the last revision: Update src/java.base/share/classes/jdk/internal/foreign/AbstractMemorySegmentImpl.java Co-authored-by: Jorn Vernee - Changes: - all: https://git.openjdk.java.net/jdk/pull/8701/files - new: https://git.openjdk.java.net/jdk/pull/8701/files/02494e2f..b6629787 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=8701&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=8701&range=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/8701.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8701/head:pull/8701 PR: https://git.openjdk.java.net/jdk/pull/8701
Re: RFR: JDK-6782021: It is not possible to read local computer certificates with the SunMSCAPI provider [v5]
On Mon, 16 May 2022 16:59:02 GMT, Weijun Wang wrote: >> @christophbrejla - my goal is to backport to latest (18 or 19), 17 and 11 > > @macarte I think Sean's comment on your CSR about the scope is correct. The > "algorithm" name should be at the JDK level so user knows what to write in > their app. Once you think there's no more to update, you can finalize the CSR. Appologies i misread the process - waiting for @wangweij to approve - PR: https://git.openjdk.java.net/jdk/pull/8211
Re: RFR: 8286090: Add RC2/RC4 to jdk.security.legacyAlgorithms
On Mon, 16 May 2022 12:59:09 GMT, Sean Mullan wrote:
>> Please review the small change to add RC2 and ARCFOUR to
>> jdk.security.legacyAlgorithms. So it enables keytool -genseckey, -list, and
>> -importkeystore commands to warn users when RC2 or ARCFOUR algorithm is used.
>
> test/jdk/sun/security/tools/keytool/WeakSecretKeyTest.java line 67:
>
>> 65:
>> 66: SecurityTools.keytool("-keystore ks.p12 -storepass changeit " +
>> 67: "-genseckey -keyalg RC4 -alias rc4key -keysize 1024")
>
> `-keysize` should be 128.
Oops, I stand corrected. 1024 is a valid RC4 key size.
-
PR: https://git.openjdk.java.net/jdk/pull/8712
Re: RFR: 8286090: Add RC2/RC4 to jdk.security.legacyAlgorithms
On Sat, 14 May 2022 01:51:34 GMT, Hai-May Chao wrote: > Please review the small change to add RC2 and ARCFOUR to > jdk.security.legacyAlgorithms. So it enables keytool -genseckey, -list, and > -importkeystore commands to warn users when RC2 or ARCFOUR algorithm is used. Marked as reviewed by mullan (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8712
Re: RFR: JDK-6782021: It is not possible to read local computer certificates with the SunMSCAPI provider [v5]
On Wed, 11 May 2022 15:55:40 GMT, Mat Carter wrote: >> Mat Carter has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Add test from wangweij > > @christophbrejla - my goal is to backport to latest (18 or 19), 17 and 11 @macarte I think Sean's comment on your CSR about the scope is correct. The "algorithm" name should be at the JDK level so user knows what to write in their app. Once you think there's no more to update, you can finalize the CSR. - PR: https://git.openjdk.java.net/jdk/pull/8211
Re: RFR: 8286715: Generalize MemorySegment::ofBuffer
On Fri, 13 May 2022 12:33:10 GMT, Maurizio Cimadamore
wrote:
> This patch makes MemorySegment::ofBuffer more general, by allowing clients to
> pass *any* `Buffer` instance, not just `ByteBuffer`.
> This allows us to match expressiveness of JNI API, where JNI clients can
> obtain the address of any direct buffer instance, using the
> `GetDirectBufferAddress` function.
>
> We thought about also providing a more general way to view a segment as a
> buffer (e.g. asIntBuffer) but doing that doesn't seem worth it: direct
> buffers can only created form `ByteBuffer`.
> So, to create a direct `IntBuffer`, clients have to first create a direct
> `ByteBuffer` then to view that buffer as an `IntBuffer`.
>
> In other words, `IntBuffer` and friends are not first-class citizens in the
> `Buffer` API. As such it would not be possible to map many memory segments
> into an `IntBuffer`; in fact, the only segment we could safely map into an
> `IntBuffer` would be an _heap_ segment backed by an `int[]`. As such it
> doesn't seem worth adding a lot of API surface (in terms of additional
> overloads) for such a corner case.
Marked as reviewed by jvernee (Reviewer).
src/java.base/share/classes/jdk/internal/foreign/AbstractMemorySegmentImpl.java
line 572:
> 570: return new NativeMemorySegmentImpl(bbAddress + (pos <<
> scaleFactor), size << scaleFactor, modes, bufferSession);
> 571: } else {
> 572: // we can ignore scale factor here, a mapped buffer is
> always a byte buffer, so scaleFactor == 1.
Suggestion:
// we can ignore scale factor here, a mapped buffer is always a
byte buffer, so scaleFactor == 0.
-
PR: https://git.openjdk.java.net/jdk/pull/8701
Re: RFR: 8282662: Use List.of() factory method to reduce memory consumption [v3]
On Fri, 13 May 2022 13:36:52 GMT, ExE Boss wrote: > So if anything, the List.of(…) call should be moved into the ProxyMethod > constructor. And maybe the call to Method.getExceptionTypes() should be > changed to Method.getSharedExceptionTypes() Makes sense. Do you want me to do this within this PR (this would be a deviation from ticket's target), or it's better to create another one? - PR: https://git.openjdk.java.net/jdk/pull/7729
Re: RFR: 8253176: Signature.getParameters should specify that it can throw UnsupportedOperationException [v7]
On Fri, 13 May 2022 20:58:16 GMT, Valerie Peng wrote:
>> src/java.base/share/classes/java/security/Signature.java line 1012:
>>
>>> 1010: * values used by the underlying signature scheme. If the required
>>> 1011: * parameters were not supplied and can be generated by the
>>> signature,
>>> 1012: * the generated parameters are returned. However, if the
>>> signature scheme
>>
>> I think one small addition to the 2nd sentence would help to cover the case
>> where null is returned if params were not set and the impl does not generate
>> params (regardless as to whether it supports returning them as
>> `AlgorithmParameters`): "If the required parameters were not supplied and
>> can be generated by the signature, the generated parameters are returned;
>> otherwise `null` is returned."
>
> With this modification of 2nd sentence. The whole paragraph becomes:
>
> * The returned parameters may be the same that were used to initialize
> * this signature, or may contain additional default or random parameter
> * values used by the underlying signature scheme. If the required
> * parameters were not supplied and can be generated by the signature,
> * the generated parameters are returned; otherwise {@code null} is
> * returned. However, if the signature scheme does not support returning
> * the parameters as {@code AlgorithmParameters}, {@code null} is always
> * returned.
>
> For the last sentence, would it be better to use "also" instead of "always"?
I think "also" would not be quite right, because I would read that as there is
something else that is also returned with `null`. I think you can remove the
word "always" -- it isn't really necessary and that might address your concern.
-
PR: https://git.openjdk.java.net/jdk/pull/8396
Re: RFR: 8286090: Add RC2/RC4 to jdk.security.legacyAlgorithms
On Sat, 14 May 2022 01:51:34 GMT, Hai-May Chao wrote:
> Please review the small change to add RC2 and ARCFOUR to
> jdk.security.legacyAlgorithms. So it enables keytool -genseckey, -list, and
> -importkeystore commands to warn users when RC2 or ARCFOUR algorithm is used.
test/jdk/sun/security/tools/keytool/WeakSecretKeyTest.java line 67:
> 65:
> 66: SecurityTools.keytool("-keystore ks.p12 -storepass changeit " +
> 67: "-genseckey -keyalg RC4 -alias rc4key -keysize 1024")
`-keysize` should be 128.
-
PR: https://git.openjdk.java.net/jdk/pull/8712
