Integrated: 8264681: Use the blessed modifier order in java.security
On Sat, 3 Apr 2021 22:09:55 GMT, Alex Blewitt wrote: > 8264681: Use the blessed modifier order in java.security This pull request has now been integrated. Changeset: ebbce91e Author:Alex Blewitt Committer: Aleksey Shipilev URL: https://git.openjdk.java.net/jdk/commit/ebbce91e Stats: 160 lines in 53 files changed: 0 ins; 0 del; 160 mod 8264681: Use the blessed modifier order in java.security Reviewed-by: mullan, shade - PR: https://git.openjdk.java.net/jdk/pull/3338
Re: RFR: 8255410: Add ChaCha20 and Poly1305 support to SunPKCS11 provider [v3]
> Could someone (perhaps Jamil?) please help review this change? This enhances > SunPKCS11 provider with ChaCha20-Poly1305 cipher and ChaCha20 key generation > support. Majority of the regression tests are adapted from the existing ones > for SunJCE provider's ChaCha20-Poly1305 cipher impl. When testing against NSS > v3.57, it does not have support for ChaCha20 cipher, thus I did not add > support for ChaCha20 cipher and the corresponding parameter. > > Thanks! > Valerie Valerie Peng has updated the pull request incrementally with one additional commit since the last revision: Fixed an tagLen issue, no key+iv reuse check for decryption, and add regression test for ChaCha20 SKF. - Changes: - all: https://git.openjdk.java.net/jdk/pull/3420/files - new: https://git.openjdk.java.net/jdk/pull/3420/files/d0f16238..2e3b7968 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=3420&range=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=3420&range=01-02 Stats: 290 lines in 3 files changed: 122 ins; 165 del; 3 mod Patch: https://git.openjdk.java.net/jdk/pull/3420.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3420/head:pull/3420 PR: https://git.openjdk.java.net/jdk/pull/3420
Re: RFR: 8264208: Console charset API [v6]
On Tue, 13 Apr 2021 19:59:30 GMT, Naoto Sato wrote: >> Please review the changes for the subject issue. This has been suggested in >> a recent discussion thread for the JEP 400 >> [[1](https://mail.openjdk.java.net/pipermail/core-libs-dev/2021-March/075214.html)]. >> A CSR has also been drafted, and comments are welcome >> [[2](https://bugs.openjdk.java.net/browse/JDK-8264209)]. > > Naoto Sato has updated the pull request incrementally with one additional > commit since the last revision: > > Added comment to System.out/err init. Marked as reviewed by joehw (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/3419
Re: RFR: 8264208: Console charset API [v4]
On Tue, 13 Apr 2021 19:30:53 GMT, Joe Wang wrote: >> Although the code path is different, the logic to determine the encoding is >> not changed, as `sun.stdout/err.encoding` are only set if the VM is invoked >> from a terminal (in fact, there's a bug where they aren't set even in a >> terminal on unix env, which I fixed in this patch) which is the same >> condition in which `System.console()` returns the console. And for both >> cases, it will default to `Charset.defaultCharset()` if they are not >> available. >> >> Having said that, one regression test started to fail with this >> implementation change as follows: >> >> Exception in thread "main" java.util.ServiceConfigurationError: Locale >> provider adapter "CLDR"cannot be instantiated. >> at >> java.base/sun.util.locale.provider.LocaleProviderAdapter.forType(LocaleProviderAdapter.java:199) >> at >> java.base/sun.util.locale.provider.LocaleProviderAdapter.findAdapter(LocaleProviderAdapter.java:287) >> at >> java.base/sun.util.locale.provider.LocaleProviderAdapter.getAdapter(LocaleProviderAdapter.java:258) >> at java.base/java.text.NumberFormat.getInstance(NumberFormat.java:960) >> at >> java.base/java.text.NumberFormat.getNumberInstance(NumberFormat.java:518) >> at java.base/java.util.Scanner.useLocale(Scanner.java:1270) >> at java.base/java.util.Scanner.(Scanner.java:543) >> at java.base/java.util.Scanner.(Scanner.java:554) >> at TestConsole.main(TestConsole.java:54) >> Caused by: java.lang.reflect.InvocationTargetException >> at >> java.base/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance0(Native >> Method) >> at >> java.base/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:78) >> at >> java.base/jdk.internal.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45) >> at >> java.base/java.lang.reflect.Constructor.newInstanceWithCaller(Constructor.java:499) >> at >> java.base/java.lang.reflect.Constructor.newInstance(Constructor.java:480) >> at >> java.base/sun.util.locale.provider.LocaleProviderAdapter.forType(LocaleProviderAdapter.java:188) >> ... 8 more >> >> I haven't looked at it in detail but it seems that calling >> `System.console()` in `System.initPhase1()` is not allowed, as the module >> system is not there yet. So I reverted the implementation to the original >> and simply retained the description in `System.out/err` with a change to >> `determined by` to `equivalent to`. > > Thanks for the explanation and updates. It's a worthwhile exercise to attempt > to align things around the new method. A short note above line 2023 to record > this might be useful as well (sth. like it's eq to console != null ? > console.charset() : Charset.defaultCharset();). No need to create another > update if you decide to add the note. Thanks. Added some explanations as suggested. - PR: https://git.openjdk.java.net/jdk/pull/3419
Re: RFR: 8264208: Console charset API [v6]
> Please review the changes for the subject issue. This has been suggested in > a recent discussion thread for the JEP 400 > [[1](https://mail.openjdk.java.net/pipermail/core-libs-dev/2021-March/075214.html)]. > A CSR has also been drafted, and comments are welcome > [[2](https://bugs.openjdk.java.net/browse/JDK-8264209)]. Naoto Sato has updated the pull request incrementally with one additional commit since the last revision: Added comment to System.out/err init. - Changes: - all: https://git.openjdk.java.net/jdk/pull/3419/files - new: https://git.openjdk.java.net/jdk/pull/3419/files/cafde56a..9e323145 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=3419&range=05 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=3419&range=04-05 Stats: 3 lines in 1 file changed: 3 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/3419.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3419/head:pull/3419 PR: https://git.openjdk.java.net/jdk/pull/3419
Re: RFR: 8264208: Console charset API [v4]
On Tue, 13 Apr 2021 18:24:55 GMT, Naoto Sato wrote: >> src/java.base/share/classes/java/lang/System.java line 2020: >> >>> 2018: setIn0(new BufferedInputStream(fdIn)); >>> 2019: setOut0(newPrintStream(fdOut, cs)); >>> 2020: setErr0(newPrintStream(fdErr, cs)); >> >> It was getting from sun.stdout.encoding or sun.stderr.encoding. After the >> change, when there is no console, it would be set with >> Charset.defaultCharset(). Would that be an incompatible change? > > Although the code path is different, the logic to determine the encoding is > not changed, as `sun.stdout/err.encoding` are only set if the VM is invoked > from a terminal (in fact, there's a bug where they aren't set even in a > terminal on unix env, which I fixed in this patch) which is the same > condition in which `System.console()` returns the console. And for both > cases, it will default to `Charset.defaultCharset()` if they are not > available. > > Having said that, one regression test started to fail with this > implementation change as follows: > > Exception in thread "main" java.util.ServiceConfigurationError: Locale > provider adapter "CLDR"cannot be instantiated. > at > java.base/sun.util.locale.provider.LocaleProviderAdapter.forType(LocaleProviderAdapter.java:199) > at > java.base/sun.util.locale.provider.LocaleProviderAdapter.findAdapter(LocaleProviderAdapter.java:287) > at > java.base/sun.util.locale.provider.LocaleProviderAdapter.getAdapter(LocaleProviderAdapter.java:258) > at java.base/java.text.NumberFormat.getInstance(NumberFormat.java:960) > at > java.base/java.text.NumberFormat.getNumberInstance(NumberFormat.java:518) > at java.base/java.util.Scanner.useLocale(Scanner.java:1270) > at java.base/java.util.Scanner.(Scanner.java:543) > at java.base/java.util.Scanner.(Scanner.java:554) > at TestConsole.main(TestConsole.java:54) > Caused by: java.lang.reflect.InvocationTargetException > at > java.base/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance0(Native > Method) > at > java.base/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:78) > at > java.base/jdk.internal.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45) > at > java.base/java.lang.reflect.Constructor.newInstanceWithCaller(Constructor.java:499) > at > java.base/java.lang.reflect.Constructor.newInstance(Constructor.java:480) > at > java.base/sun.util.locale.provider.LocaleProviderAdapter.forType(LocaleProviderAdapter.java:188) > ... 8 more > > I haven't looked at it in detail but it seems that calling `System.console()` > in `System.initPhase1()` is not allowed, as the module system is not there > yet. So I reverted the implementation to the original and simply retained the > description in `System.out/err` with a change to `determined by` to > `equivalent to`. Thanks for the explanation and updates. It's a worthwhile exercise to attempt to align things around the new method. A short note above line 2023 to record this might be useful as well (sth. like it's eq to console != null ? console.charset() : Charset.defaultCharset();). No need to create another update if you decide to add the note. - PR: https://git.openjdk.java.net/jdk/pull/3419
Re: RFR: 8264208: Console charset API [v4]
On Tue, 13 Apr 2021 13:04:17 GMT, Alan Bateman wrote: > 1. I think method name "charset()" is too short. It's not called frequently. > This method name should explain functionality. As for this one, I am open for suggestions. I thought `consoel()` was concise, and analogous to `Charset.defaultCharset()`. - PR: https://git.openjdk.java.net/jdk/pull/3419
Re: RFR: 8264208: Console charset API [v4]
On Tue, 13 Apr 2021 02:34:15 GMT, Joe Wang wrote: >> Naoto Sato has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Reverted PrintStream changes > > src/java.base/share/classes/java/lang/System.java line 2020: > >> 2018: setIn0(new BufferedInputStream(fdIn)); >> 2019: setOut0(newPrintStream(fdOut, cs)); >> 2020: setErr0(newPrintStream(fdErr, cs)); > > It was getting from sun.stdout.encoding or sun.stderr.encoding. After the > change, when there is no console, it would be set with > Charset.defaultCharset(). Would that be an incompatible change? Although the code path is different, the logic to determine the encoding is not changed, as `sun.stdout/err.encoding` are only set if the VM is invoked from a terminal (in fact, there's a bug where they aren't set even in a terminal on unix env, which I fixed in this patch) which is the same condition in which `System.console()` returns the console. And for both cases, it will default to `Charset.defaultCharset()` if they are not available. Having said that, one regression test started to fail with this implementation change as follows: Exception in thread "main" java.util.ServiceConfigurationError: Locale provider adapter "CLDR"cannot be instantiated. at java.base/sun.util.locale.provider.LocaleProviderAdapter.forType(LocaleProviderAdapter.java:199) at java.base/sun.util.locale.provider.LocaleProviderAdapter.findAdapter(LocaleProviderAdapter.java:287) at java.base/sun.util.locale.provider.LocaleProviderAdapter.getAdapter(LocaleProviderAdapter.java:258) at java.base/java.text.NumberFormat.getInstance(NumberFormat.java:960) at java.base/java.text.NumberFormat.getNumberInstance(NumberFormat.java:518) at java.base/java.util.Scanner.useLocale(Scanner.java:1270) at java.base/java.util.Scanner.(Scanner.java:543) at java.base/java.util.Scanner.(Scanner.java:554) at TestConsole.main(TestConsole.java:54) Caused by: java.lang.reflect.InvocationTargetException at java.base/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method) at java.base/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:78) at java.base/jdk.internal.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45) at java.base/java.lang.reflect.Constructor.newInstanceWithCaller(Constructor.java:499) at java.base/java.lang.reflect.Constructor.newInstance(Constructor.java:480) at java.base/sun.util.locale.provider.LocaleProviderAdapter.forType(LocaleProviderAdapter.java:188) ... 8 more I haven't looked at it in detail but it seems that calling `System.console()` in `System.initPhase1()` is not allowed, as the module system is not there yet. So I reverted the implementation to the original and simply retained the description in `System.out/err` with a change to `determined by` to `equivalent to`. - PR: https://git.openjdk.java.net/jdk/pull/3419
Re: RFR: 8241306: Add SignatureMethodParameterSpec subclass for RSASSA-PSS params [v7]
> This enhancement contains the following code changes: > > 1. Create a new public API `javax/xml/crypto/dsig/spec/RSAPSSParameterSpec` > and remove the internal one. > 2. Update marshaling and unmarshaling code inside `DOMRSAPSSSignatureMethod` > so it understands extra fields in `PSSParameterSpec` and is aware of the > defaults in both directions. > 3. Update `DOMSignedInfo` so that secure validation can restrict > `DigestMethod` used inside `RSAPSSParameterSpec` > 4. Tests Weijun Wang has updated the pull request incrementally with one additional commit since the last revision: more spec clarification - Changes: - all: https://git.openjdk.java.net/jdk/pull/3181/files - new: https://git.openjdk.java.net/jdk/pull/3181/files/a28a8fa1..df55414c Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=3181&range=06 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=3181&range=05-06 Stats: 12 lines in 2 files changed: 0 ins; 2 del; 10 mod Patch: https://git.openjdk.java.net/jdk/pull/3181.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3181/head:pull/3181 PR: https://git.openjdk.java.net/jdk/pull/3181
Re: RFR: 8241306: Add SignatureMethodParameterSpec subclass for RSASSA-PSS params [v6]
On Tue, 13 Apr 2021 17:07:19 GMT, Sean Mullan wrote:
>> Weijun Wang has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> spec clarification
>
> src/java.xml.crypto/share/classes/javax/xml/crypto/dsig/SignatureMethod.java
> line 247:
>
>> 245: * as the signature algorithm, the default parameter as defined in
>> 246: * https://tools.ietf.org/html/rfc6931#section-2.3.9";>RFC
>> 6931 Section 2.3.9
>> 247: * is used and this default parameter will also be returned by the
>
> WE should mention/link to the type returned. Suggest breaking this into two
> sentences:
>
> `If the {@code params} parameter is {@code null} when calling {@link
> XMLSignatureFactory#newSignatureMethod} with {@code RSA_PSS} as the signature
> algorithm, the default parameter as defined in href="https://tools.ietf.org/html/rfc6931#section-2.3.9";>RFC 6931 Section
> 2.3.9 is used. This default parameter is represented as an {@link
> RSAPSSParameterSpec} type and returned by the {@link
> SignatureMethod#getParameterSpec()} method.`
OK, and some other tweaks. New commit pushed.
-
PR: https://git.openjdk.java.net/jdk/pull/3181
Re: RFR: 8264208: Console charset API [v5]
> Please review the changes for the subject issue. This has been suggested in > a recent discussion thread for the JEP 400 > [[1](https://mail.openjdk.java.net/pipermail/core-libs-dev/2021-March/075214.html)]. > A CSR has also been drafted, and comments are welcome > [[2](https://bugs.openjdk.java.net/browse/JDK-8264209)]. Naoto Sato has updated the pull request incrementally with one additional commit since the last revision: Reflected further review comments. - Changes: - all: https://git.openjdk.java.net/jdk/pull/3419/files - new: https://git.openjdk.java.net/jdk/pull/3419/files/68db1a79..cafde56a Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=3419&range=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=3419&range=03-04 Stats: 14 lines in 1 file changed: 6 ins; 2 del; 6 mod Patch: https://git.openjdk.java.net/jdk/pull/3419.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3419/head:pull/3419 PR: https://git.openjdk.java.net/jdk/pull/3419
Re: RFR: Release Note for JDK-8264968 Provide the support for specifying a signer in keytool -genkeypair command
Hi Sean, Updated with your comments. Thanks for the review. Tia-May > On Apr 13, 2021, at 6:39 AM, Sean Mullan wrote: > > Looks good, a couple of suggested rewordings in the second sentence: > > > The `-signer` and `-signerkeypass` options have been added to the > `-genkeypair` command of the `keytool` utility. The `-signer` option > specifies the keystore alias of a signer and the `-signerkeypass` option > specifies the password used to protect the signer’s private key. These > options allow `keytool -genkeypair` to sign the certificate using the > signer’s private key. This is especially useful for generating a certificate > with a key agreement algorithm as its public key algorithm. > > --Sean > > On 4/9/21 5:12 PM, Hai-May Chao wrote: >> Please review the release note for JDK-8264968: >> https://bugs.openjdk.java.net/browse/JDK-8264968 >> Thanks, >> Hai-May
Re: RFR: 8263779: SSLEngine reports NEED_WRAP continuously without producing any further output
On Mon, 12 Apr 2021 06:30:43 GMT, djelinski
wrote:
>> As described in the bug, by connecting the SSLEngine with a misbehaving peer
>> SSL implementation, it can get into a state where it calling `wrap` reports
>> getStatus == OK, getHandshakeStatus === NEED_WRAP but still doesn't produce
>> any further output. It happens when the output bound is not empty.
>>
>> The handshake status could have more precise status if the out bound. The
>> patch was confirmed by the bug submitter.
>
> src/java.base/share/classes/sun/security/ssl/TransportContext.java line 590:
>
>> 588: HandshakeStatus getHandshakeStatus() {
>> 589: if (!outputRecord.isEmpty()) {
>> 590: // If no handshaking, special case to wrap alters or
>
> Suggestion:
>
> // If not handshaking, special case to wrap alerts or
Thank you for the correction.
> src/java.base/share/classes/sun/security/ssl/TransportContext.java line 592:
>
>> 590: // If no handshaking, special case to wrap alters or
>> 591: // post-handshake messages.
>> 592: if (!isOutboundClosed()) {
>
> If I'm reading the
> [TransportContect#closeNotify](https://github.com/openjdk/jdk/blob/627ad9fe22a153410c14d0b2061bb7dee2c300af/src/java.base/share/classes/sun/security/ssl/TransportContext.java#L275)
> and
> [TransportContext#passiveInboundClose](https://github.com/openjdk/jdk/blob/627ad9fe22a153410c14d0b2061bb7dee2c300af/src/java.base/share/classes/sun/security/ssl/TransportContext.java#L524)
> correctly, non-empty output record with both inbound and outbound closed
> happens when we reply with our close_notify to peer's. Now we will return
> NOT_HANDSHAKING which appears to be wrong.
Good catch! I will have an update. Thank you for the code review.
-
PR: https://git.openjdk.java.net/jdk/pull/3292
Re: RFR: 8241306: Add SignatureMethodParameterSpec subclass for RSASSA-PSS params [v6]
On Tue, 13 Apr 2021 15:31:35 GMT, Weijun Wang wrote:
>> This enhancement contains the following code changes:
>>
>> 1. Create a new public API `javax/xml/crypto/dsig/spec/RSAPSSParameterSpec`
>> and remove the internal one.
>> 2. Update marshaling and unmarshaling code inside `DOMRSAPSSSignatureMethod`
>> so it understands extra fields in `PSSParameterSpec` and is aware of the
>> defaults in both directions.
>> 3. Update `DOMSignedInfo` so that secure validation can restrict
>> `DigestMethod` used inside `RSAPSSParameterSpec`
>> 4. Tests
>
> Weijun Wang has updated the pull request incrementally with one additional
> commit since the last revision:
>
> spec clarification
src/java.xml.crypto/share/classes/javax/xml/crypto/dsig/SignatureMethod.java
line 247:
> 245: * as the signature algorithm, the default parameter as defined in
> 246: * https://tools.ietf.org/html/rfc6931#section-2.3.9";>RFC
> 6931 Section 2.3.9
> 247: * is used and this default parameter will also be returned by the
WE should mention/link to the type returned. Suggest breaking this into two
sentences:
`If the {@code params} parameter is {@code null} when calling {@link
XMLSignatureFactory#newSignatureMethod} with {@code RSA_PSS} as the signature
algorithm, the default parameter as defined in https://tools.ietf.org/html/rfc6931#section-2.3.9";>RFC 6931 Section
2.3.9 is used. This default parameter is represented as an {@link
RSAPSSParameterSpec} type and returned by the {@link
SignatureMethod#getParameterSpec()} method.`
-
PR: https://git.openjdk.java.net/jdk/pull/3181
Integrated: 8265138: Simplify DerUtils::checkAlg
On Tue, 13 Apr 2021 14:36:46 GMT, Weijun Wang wrote: > This fix makes `DerUtils::checkAlg` simple and clear, no more raw `Object` > and ambiguous string arguments. This pull request has now been integrated. Changeset: 9cd5400d Author:Weijun Wang URL: https://git.openjdk.java.net/jdk/commit/9cd5400d Stats: 37 lines in 3 files changed: 3 ins; 1 del; 33 mod 8265138: Simplify DerUtils::checkAlg Reviewed-by: xuelei - PR: https://git.openjdk.java.net/jdk/pull/3467
Re: RFR: 8265138: Simplify DerUtils::checkAlg
On Tue, 13 Apr 2021 14:36:46 GMT, Weijun Wang wrote: > This fix makes `DerUtils::checkAlg` simple and clear, no more raw `Object` > and ambiguous string arguments. Marked as reviewed by xuelei (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/3467
Re: RFR: 8264152: javax/net/ssl/DTLS/RespondToRetransmit.java timed out
On Tue, 13 Apr 2021 13:19:17 GMT, Fernando Guallini wrote: > test/jdk/javax/net/ssl/DTLS/RespondToRetransmit.java has been seen to fail > intermittently. > The server side is binding to the wildcard/localhost address which has been a > source of instability in many tests. Binding to loopback address fixes the > intermittent failures. > > In addition, other changes were introduced in the tests to improve code > readability: > - Reduce duplication by reusing code > - Replace if statements with Switch expressions > - Make fields final when appropriate > - Convert ServerCallable and ClientCallable to records > - Replace Byte.valueOf with Byte.parseByte to avoid redundant boxing Marked as reviewed by xuelei (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/3466
Re: RFR: 8241306: Add SignatureMethodParameterSpec subclass for RSASSA-PSS params [v6]
> This enhancement contains the following code changes: > > 1. Create a new public API `javax/xml/crypto/dsig/spec/RSAPSSParameterSpec` > and remove the internal one. > 2. Update marshaling and unmarshaling code inside `DOMRSAPSSSignatureMethod` > so it understands extra fields in `PSSParameterSpec` and is aware of the > defaults in both directions. > 3. Update `DOMSignedInfo` so that secure validation can restrict > `DigestMethod` used inside `RSAPSSParameterSpec` > 4. Tests Weijun Wang has updated the pull request incrementally with one additional commit since the last revision: spec clarification - Changes: - all: https://git.openjdk.java.net/jdk/pull/3181/files - new: https://git.openjdk.java.net/jdk/pull/3181/files/ce1678ea..a28a8fa1 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=3181&range=05 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=3181&range=04-05 Stats: 67 lines in 5 files changed: 34 ins; 24 del; 9 mod Patch: https://git.openjdk.java.net/jdk/pull/3181.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3181/head:pull/3181 PR: https://git.openjdk.java.net/jdk/pull/3181
Re: RFR: 8241306: Add SignatureMethodParameterSpec subclass for RSASSA-PSS params [v5]
On Mon, 12 Apr 2021 15:25:09 GMT, Weijun Wang wrote: >> This enhancement contains the following code changes: >> >> 1. Create a new public API `javax/xml/crypto/dsig/spec/RSAPSSParameterSpec` >> and remove the internal one. >> 2. Update marshaling and unmarshaling code inside `DOMRSAPSSSignatureMethod` >> so it understands extra fields in `PSSParameterSpec` and is aware of the >> defaults in both directions. >> 3. Update `DOMSignedInfo` so that secure validation can restrict >> `DigestMethod` used inside `RSAPSSParameterSpec` >> 4. Tests > > Weijun Wang has updated the pull request incrementally with one additional > commit since the last revision: > > no RSAPSSParameterSpec::toString and some test comments New commit pushed. If you're OK with the change, I'll update the CSR as well. - PR: https://git.openjdk.java.net/jdk/pull/3181
Re: RFR: 8241306: Add SignatureMethodParameterSpec subclass for RSASSA-PSS params [v2]
On Tue, 13 Apr 2021 14:22:47 GMT, Sean Mullan wrote:
>> You are right that the overridable methods are elsewhere
>> (`XMLSignatureFactory::newSignatureMethod` and
>> `SignatureMethod::getParameterSpec`), but I still feel it a little strange
>> to move the default parameter of one particular algorithm to the general
>> interface `SignatureMethod` (this is similar to talking about EC curve names
>> in `KeyPairGenerator`). It seems more natural to talk about this inside a
>> class which is specific to the RSASSA-PSS algorithm and
>> `RSAPSSParameterSpec` is the only public API we can find. We can add
>> something like "specify null if you want a default spec" to
>> `XMLSignatureFactory::newSignatureMethod` but it does not have enough space
>> to describe "how default values are determined" for each algorithm.
>>
>> I understand `@implSpec` is for implementations of a class or a method, but
>> does not mean the words must be put inside that exact class and method?
>> Maybe not necessarily.
>>
>> As for making `@implSpec` more specific, at signing time, we can only either
>> provide a `RSAPSSParameterSpec` or not one, so there is only one default
>> value which is SHA256_RSA_MGF1. On the other hand, at validation time we
>> might be parsing a partial-filled `RSAPSSParams` node and that's where
>> default salt and default trailer field show up.
>>
>> Also about the return value of `SignatureMethod::getParameterSpec`, are you
>> suggesting that for RSASSA-PSS it must be non null? This can be specified
>> somewhere but we need to find a place. For the existing `HMACParameterSpec`,
>> the method returns null if none is set. Do we treat that as no spec at all
>> (but for RSASSA-PSS there is always one)?
>>
>> My current opinion is that we still document all these in
>> `RSAPSSParameterSpec` but be very clear that this part is for
>> `XMLSignatureFactory::newSignatureMethod` and that part is for
>> `SignatureMethod::getParameterSpec`, etc, etc.
>
> I think it is worth asking the TCK team about this. After further thought, I
> am not sure `implSpec` is correct because the implementation is not in the
> classes, it is in the provider. I now think it needs to be part of the API
> contract, so that all implementations are compliant. `SignatureMethod`
> already defines the RSA_PSS algorithm constant, so it doesn't seem so
> out-of-place to put the specification there, something like:
>
>
> /**
> * The http://www.w3.org/2007/05/xmldsig-more#rsa-pss";>
> * RSASSA-PSS signature method algorithm URI.
> *
> * If the parameter is not specified when calling
> `XMLSignatureFactory.newSignatureMethod` with
> * RSA_PSS as the signature algorithm, the default parameter is used, which
> uses SHA-256 as the
> * {@code DigestMethod}, MGF1 with SHA-256 as the
> * {@code MaskGenerationFunction}, 32 as {@code SaltLength}, and 1 as
> * {@code TrailerField}. This is equivalent to the parameter-less signature
> * method {@link SignatureMethod#SHA256_RSA_MGF1 SHA256_RSA_MGF1} as defined
> * in https://tools.ietf.org/html/rfc6931#section-2.3.10";>RFC 6931.
> This default
> * parameter is returned by the `getParameterSpec` method.
> *
> * @since 17
> */
> String RSA_PSS = "http://www.w3.org/2007/05/xmldsig-more#rsa-pss";;
>
> Forget my comment about making `implSpec` more specific, I think that is now
> implied by RFC 3161. I also think the above specification would take care of
> my returning null comment, as all implementations would need to comply. I
> just didn't want applications to have to have code that checks for null and
> then don't know whether that means it is the default parameters or something
> else since it was implementation specific.
Great, I forgot about this string. This *is* a proper place to put the text.
-
PR: https://git.openjdk.java.net/jdk/pull/3181
RFR: 8265138: Simplify DerUtils::checkAlg
This fix makes `DerUtils::checkAlg` simple and clear, no more raw `Object` and ambiguous string arguments. - Commit messages: - 8265138: Simplify DerUtils::checkAlg Changes: https://git.openjdk.java.net/jdk/pull/3467/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=3467&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8265138 Stats: 37 lines in 3 files changed: 3 ins; 1 del; 33 mod Patch: https://git.openjdk.java.net/jdk/pull/3467.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3467/head:pull/3467 PR: https://git.openjdk.java.net/jdk/pull/3467
Re: RFR: 8264681: Use the blessed modifier order in java.security [v2]
On Mon, 12 Apr 2021 09:32:13 GMT, Alex Blewitt wrote: >> 8264681: Use the blessed modifier order in java.security > > Alex Blewitt has updated the pull request incrementally with one additional > commit since the last revision: > > Removed upstream licensed code from commit I think all conversations have been resolved, we can integrate. - Marked as reviewed by shade (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/3338
Re: RFR: 8241306: Add SignatureMethodParameterSpec subclass for RSASSA-PSS params [v2]
On Mon, 12 Apr 2021 20:53:21 GMT, Weijun Wang wrote:
>> Ok, I understand now. I think `@implSpec` (and probably the `@implNote`) are
>> in the wrong class. `@implSpec` means the implementation of this class. But
>> this class is final and does not contain that logic. The logic of
>> specifying/returning the defaults is in the JDK (XMLDSig provider)
>> implementation of `SignatureMethod`. So I think it belongs there. In this
>> class, you could add a sentence/link to `SignatureMethod`, something like
>> "See SignatureMethod for how default values are determined when the
>> parameters are not specified."
>>
>> Also, I think the `@implSpec` needs to be more specific, and also cover the
>> cases where some, but not all of the parameters are specified and defaults
>> are then used. For this, you will need to be more general, as the default
>> salt length is based on what hash algorithm is being used.
>>
>> As for `@implNote`, this probably could use more discussion, but it might be
>> better to make this part of the specification. If some implementations can
>> return null, and others return defaults, it complicates the application's
>> logic. The RFC has clearly specified what the defaults should be, so maybe
>> the easiest thing to do is to make all implementations comply by also making
>> it part of the API contract, and hiding the XML details as to whether the
>> parameter was actually specified or not, which should not matter to
>> applications.
>
> You are right that the overridable methods are elsewhere
> (`XMLSignatureFactory::newSignatureMethod` and
> `SignatureMethod::getParameterSpec`), but I still feel it a little strange to
> move the default parameter of one particular algorithm to the general
> interface `SignatureMethod` (this is similar to talking about EC curve names
> in `KeyPairGenerator`). It seems more natural to talk about this inside a
> class which is specific to the RSASSA-PSS algorithm and `RSAPSSParameterSpec`
> is the only public API we can find. We can add something like "specify null
> if you want a default spec" to `XMLSignatureFactory::newSignatureMethod` but
> it does not have enough space to describe "how default values are determined"
> for each algorithm.
>
> I understand `@implSpec` is for implementations of a class or a method, but
> does not mean the words must be put inside that exact class and method? Maybe
> not necessarily.
>
> As for making `@implSpec` more specific, at signing time, we can only either
> provide a `RSAPSSParameterSpec` or not one, so there is only one default
> value which is SHA256_RSA_MGF1. On the other hand, at validation time we
> might be parsing a partial-filled `RSAPSSParams` node and that's where
> default salt and default trailer field show up.
>
> Also about the return value of `SignatureMethod::getParameterSpec`, are you
> suggesting that for RSASSA-PSS it must be non null? This can be specified
> somewhere but we need to find a place. For the existing `HMACParameterSpec`,
> the method returns null if none is set. Do we treat that as no spec at all
> (but for RSASSA-PSS there is always one)?
>
> My current opinion is that we still document all these in
> `RSAPSSParameterSpec` but be very clear that this part is for
> `XMLSignatureFactory::newSignatureMethod` and that part is for
> `SignatureMethod::getParameterSpec`, etc, etc.
I think it is worth asking the TCK team about this. After further thought, I am
not sure `implSpec` is correct because the implementation is not in the
classes, it is in the provider. I now think it needs to be part of the API
contract, so that all implementations are compliant. `SignatureMethod` already
defines the RSA_PSS algorithm constant, so it doesn't seem so out-of-place to
put the specification there, something like:
/**
* The http://www.w3.org/2007/05/xmldsig-more#rsa-pss";>
* RSASSA-PSS signature method algorithm URI.
*
* If the parameter is not specified when calling
`XMLSignatureFactory.newSignatureMethod` with
* RSA_PSS as the signature algorithm, the default parameter is used, which
uses SHA-256 as the
* {@code DigestMethod}, MGF1 with SHA-256 as the
* {@code MaskGenerationFunction}, 32 as {@code SaltLength}, and 1 as
* {@code TrailerField}. This is equivalent to the parameter-less signature
* method {@link SignatureMethod#SHA256_RSA_MGF1 SHA256_RSA_MGF1} as defined
* in https://tools.ietf.org/html/rfc6931#section-2.3.10";>RFC 6931.
This default
* parameter is returned by the `getParameterSpec` method.
*
* @since 17
*/
String RSA_PSS = "http://www.w3.org/2007/05/xmldsig-more#rsa-pss";;
Forget my comment about making `implSpec` more specific, I think that is now
implied by RFC 3161. I also think the above specification would take care of my
returning null comment, as all implementations would need to comply. I just
didn't want applications to have to have code that checks for null and then
don't know whether
Re: RFR: 8264681: Use the blessed modifier order in java.security [v2]
On Mon, 12 Apr 2021 09:32:13 GMT, Alex Blewitt wrote: >> 8264681: Use the blessed modifier order in java.security > > Alex Blewitt has updated the pull request incrementally with one additional > commit since the last revision: > > Removed upstream licensed code from commit What happens to this now? Do I need to wait for a re-review of the change or do I hit the (slash) integrate command now? - PR: https://git.openjdk.java.net/jdk/pull/3338
Re: RFR: Release Note for JDK-8264968 Provide the support for specifying a signer in keytool -genkeypair command
Looks good, a couple of suggested rewordings in the second sentence: The `-signer` and `-signerkeypass` options have been added to the `-genkeypair` command of the `keytool` utility. The `-signer` option specifies the keystore alias of a signer and the `-signerkeypass` option specifies the password used to protect the signer’s private key. These options allow `keytool -genkeypair` to sign the certificate using the signer’s private key. This is especially useful for generating a certificate with a key agreement algorithm as its public key algorithm. --Sean On 4/9/21 5:12 PM, Hai-May Chao wrote: Please review the release note for JDK-8264968: https://bugs.openjdk.java.net/browse/JDK-8264968 Thanks, Hai-May
RFR: 8264152: javax/net/ssl/DTLS/RespondToRetransmit.java timed out
test/jdk/javax/net/ssl/DTLS/RespondToRetransmit.java has been seen to fail intermittently. The server side is binding to the wildcard/localhost address which has been a source of instability in many tests. Binding to loopback address fixes the intermittent failures. In addition, other changes were introduced in the tests to improve code readability: - Reduce duplication by reusing code - Replace if statements with Switch expressions - Make fields final when appropriate - Convert ServerCallable and ClientCallable to records - Replace Byte.valueOf with Byte.parseByte to avoid redundant boxing - Commit messages: - renamed constants field - fix intermittent time out, refactor Changes: https://git.openjdk.java.net/jdk/pull/3466/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=3466&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8264152 Stats: 300 lines in 3 files changed: 78 ins; 130 del; 92 mod Patch: https://git.openjdk.java.net/jdk/pull/3466.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3466/head:pull/3466 PR: https://git.openjdk.java.net/jdk/pull/3466
Re: RFR: 8264208: Console charset API [v4]
On Tue, 13 Apr 2021 12:54:51 GMT, Ichiroh Takiguchi wrote: > 1. I think method name "charset()" is too short. It's not called frequently. > This method name should explain functionality. > 2. Sometimes stderr may be redirected to stdout by shell. Why do we need to > set different encodings for these two (sun.stdout.encoding and > sun.stderr.encoding) ? Console's class description already covers this "If the virtual machine is started from an interactive command line without redirecting the standard input and output streams then ...". The existing reader and writer methods use the same charset. - PR: https://git.openjdk.java.net/jdk/pull/3419
Re: RFR: 8264208: Console charset API [v4]
On Mon, 12 Apr 2021 23:01:24 GMT, Naoto Sato wrote: >> Please review the changes for the subject issue. This has been suggested in >> a recent discussion thread for the JEP 400 >> [[1](https://mail.openjdk.java.net/pipermail/core-libs-dev/2021-March/075214.html)]. >> A CSR has also been drafted, and comments are welcome >> [[2](https://bugs.openjdk.java.net/browse/JDK-8264209)]. > > Naoto Sato has updated the pull request incrementally with one additional > commit since the last revision: > > Reverted PrintStream changes I have 2 concerns: 1. I think method name "charset()" is too short. It's not called frequently. This method name should explain functionality. 2. Sometimes stderr may be redirected to stdout by shell. Why do we need to set different encodings for these two (sun.stdout.encoding and sun.stderr.encoding) ? - PR: https://git.openjdk.java.net/jdk/pull/3419
Re: RFR: 8048199: Replace anonymous inner classes with lambdas, where applicable, in JNDI [v3]
On Tue, 13 Apr 2021 10:04:19 GMT, Conor Cleary wrote: >> ### Description >> This fix is part of a previous effort to both cleanup/modernise JNDI code, >> the details of which can be seen in >> [JDK-8048091](https://bugs.openjdk.java.net/browse/JDK-8048091). A number >> JNDI methods under `java.naming` use Anonymous Inner Classes in cases where >> only a single object unique to the requirements of the method is used. The >> issues these occurrences of AICs cause are highlighted below. >> >> - AICs, in the cases concerned with this fix, are used where only one >> operation is required. While AICs can be useful for more complex >> implementations (using interfaces, multiple methods needed, local fields >> etc.), Lambdas are better suited here as they result in a more readable and >> concise solution. >> >> ### Fixes >> - Where applicable, occurrences of AICs were replaced with lambdas to >> address the issues above resulting primarily in more readable/concise code. > > Conor Cleary has updated the pull request incrementally with one additional > commit since the last revision: > > 8048199: Cleaner syntak in getContextClassLoader Marked as reviewed by dfuchs (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/3416
Re: RFR: 8048199: Replace anonymous inner classes with lambdas, where applicable, in JNDI [v3]
On Tue, 13 Apr 2021 10:04:19 GMT, Conor Cleary wrote: >> ### Description >> This fix is part of a previous effort to both cleanup/modernise JNDI code, >> the details of which can be seen in >> [JDK-8048091](https://bugs.openjdk.java.net/browse/JDK-8048091). A number >> JNDI methods under `java.naming` use Anonymous Inner Classes in cases where >> only a single object unique to the requirements of the method is used. The >> issues these occurrences of AICs cause are highlighted below. >> >> - AICs, in the cases concerned with this fix, are used where only one >> operation is required. While AICs can be useful for more complex >> implementations (using interfaces, multiple methods needed, local fields >> etc.), Lambdas are better suited here as they result in a more readable and >> concise solution. >> >> ### Fixes >> - Where applicable, occurrences of AICs were replaced with lambdas to >> address the issues above resulting primarily in more readable/concise code. > > Conor Cleary has updated the pull request incrementally with one additional > commit since the last revision: > > 8048199: Cleaner syntak in getContextClassLoader Marked as reviewed by aefimov (Committer). - PR: https://git.openjdk.java.net/jdk/pull/3416
Re: RFR: 8048199: Replace anonymous inner classes with lambdas, where applicable, in JNDI [v2]
On Tue, 13 Apr 2021 10:00:51 GMT, Conor Cleary wrote: >> Good idea, also would fit in with the style of the method just after, >> `priviligedHasNext()` as that also uses a functional interface. > > Changes included as described in commit > [5d6ecd3](https://github.com/openjdk/jdk/pull/3416/commits/5d6ecd31eb6ed2a63f17b2e798e83b91cb720075) Here again this is not strictly equivalent but AFAIK `Thread::currentThread` doesn't require any permission, so this should be OK. - PR: https://git.openjdk.java.net/jdk/pull/3416
Re: RFR: 8048199: Replace anonymous inner classes with lambdas, where applicable, in JNDI [v2]
On Tue, 13 Apr 2021 09:34:15 GMT, Conor Cleary wrote:
>> src/java.naming/share/classes/javax/naming/ldap/StartTlsRequest.java line
>> 223:
>>
>>> 221: */
>>> 222: private final ClassLoader getContextClassLoader() {
>>> 223: PrivilegedAction pa = () ->
>>> Thread.currentThread().getContextClassLoader();
>>
>> We can use here an instance method reference to beautify code a little bit
>> more:
>> ```PrivilegedAction pa =
>> Thread.currentThread()::getContextClassLoader;```
>
> Good idea, also would fit in with the style of the method just after,
> `priviligedHasNext()` as that also uses a functional interface.
Changes included as described in commit
[5d6ecd3](https://github.com/openjdk/jdk/pull/3416/commits/5d6ecd31eb6ed2a63f17b2e798e83b91cb720075)
-
PR: https://git.openjdk.java.net/jdk/pull/3416
Re: RFR: 8048199: Replace anonymous inner classes with lambdas, where applicable, in JNDI [v3]
> ### Description > This fix is part of a previous effort to both cleanup/modernise JNDI code, > the details of which can be seen in > [JDK-8048091](https://bugs.openjdk.java.net/browse/JDK-8048091). A number > JNDI methods under `java.naming` use Anonymous Inner Classes in cases where > only a single object unique to the requirements of the method is used. The > issues these occurrences of AICs cause are highlighted below. > > - AICs, in the cases concerned with this fix, are used where only one > operation is required. While AICs can be useful for more complex > implementations (using interfaces, multiple methods needed, local fields > etc.), Lambdas are better suited here as they result in a more readable and > concise solution. > > ### Fixes > - Where applicable, occurrences of AICs were replaced with lambdas to address > the issues above resulting primarily in more readable/concise code. Conor Cleary has updated the pull request incrementally with one additional commit since the last revision: 8048199: Cleaner syntak in getContextClassLoader - Changes: - all: https://git.openjdk.java.net/jdk/pull/3416/files - new: https://git.openjdk.java.net/jdk/pull/3416/files/840ea35c..5d6ecd31 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=3416&range=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=3416&range=01-02 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/3416.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3416/head:pull/3416 PR: https://git.openjdk.java.net/jdk/pull/3416
Re: RFR: 8048199: Replace anonymous inner classes with lambdas, where applicable, in JNDI [v2]
On Mon, 12 Apr 2021 16:44:16 GMT, Aleksei Efimov wrote:
>> Conor Cleary has updated the pull request incrementally with two additional
>> commits since the last revision:
>>
>> - Update copyright headers
>> - Tidied up lambdas
>
> src/java.naming/share/classes/javax/naming/ldap/StartTlsRequest.java line 223:
>
>> 221: */
>> 222: private final ClassLoader getContextClassLoader() {
>> 223: PrivilegedAction pa = () ->
>> Thread.currentThread().getContextClassLoader();
>
> We can use here an instance method reference to beautify code a little bit
> more:
> ```PrivilegedAction pa =
> Thread.currentThread()::getContextClassLoader;```
Good idea, also would fit in with the style of the method just after,
`priviligedHasNext()` as that also uses a functional interface.
-
PR: https://git.openjdk.java.net/jdk/pull/3416
