Integrated: 8272396: mismatching debug output streams
On Thu, 12 Aug 2021 19:43:43 GMT, Xue-Lei Andrew Fan wrote: > Please review this simple code clean up. In RSAKeyExchange, a same debug log > message is broken apart into System.out and System.err. The SSLLogger should > be used for SSL debug log dumpling in JDK. > > Simple update and code clean up only, no new regression test. This pull request has now been integrated. Changeset: 6b8b160e Author:Xue-Lei Andrew Fan URL: https://git.openjdk.java.net/jdk/commit/6b8b160e374a4a566d193a594d9a228646e8e067 Stats: 2 lines in 1 file changed: 0 ins; 1 del; 1 mod 8272396: mismatching debug output streams Reviewed-by: mullan - PR: https://git.openjdk.java.net/jdk/pull/5105
Re: RFR: 8272396: mismatching debug output streams
On Thu, 12 Aug 2021 19:43:43 GMT, Xue-Lei Andrew Fan wrote: > Please review this simple code clean up. In RSAKeyExchange, a same debug log > message is broken apart into System.out and System.err. The SSLLogger should > be used for SSL debug log dumpling in JDK. > > Simple update and code clean up only, no new regression test. Marked as reviewed by mullan (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/5105
Re: RFR: 8271566: DSA signature length value is not accurate in P11Signature [v2]
On Fri, 13 Aug 2021 17:11:45 GMT, Martin Balao wrote: >> As described in JDK-8271566 [1], this patch proposal is intended to fix a >> problem that arises when using DSA keys that have a 256-bits (or larger) G >> parameter for signatures (either signing or verifying). There were some >> incorrect assumptions and hard-coded length values in the code before. >> Please note that, for example, the tuple (2048, 256) for DSA is valid >> according to FIPS PUB 186-4. >> >> Beyond the specific issues in signatures, I decided to provide a broader >> solution and enable key parameter retrieval for other key types (EC, DH) >> when possible. This is, when the key is not sensitive. One thing that I >> should note here is that token keys (those that have the CKA_TOKEN attribute >> equal to 'true') are considered sensitive in this regard, at least by the >> NSS Software Token implementation. I don't have access to other vendor >> implementations but if there is any concern, we can adjust the constraint to >> NSS-only. However, I'm not sure which use-case would require to get private >> keys out of a real token, weakening its security. I'd be more conservative >> here and not query the values if not sure that it will succeed. >> >> No regressions found in jdk/sun/security/pkcs11. A new test added: >> LargerDSAKey. >> >> -- >> [1] - https://bugs.openjdk.java.net/browse/JDK-8271566 > > Martin Balao has updated the pull request incrementally with one additional > commit since the last revision: > > P11Key static inner classes refactorings. I made some class refactoring in P11Key to achieve the following goals: * Make the classes hierarchy more clear. It was a bit odd to see P11PrivateKey and P11(RSA,DSA,DH,EC)PrivateKey unrelated in terms of "the latter is a more specific instance of the former" * We now have P11(RSA,DSA,DH,EC)PrivateKey inheriting from P11PrivateKey * This allowed to promote the 'encoded' field which is shared across all of them * Keep the API as before * For an external-package client, a non-sensitive private key is seen as a (RSA,DSA,DH,EC)PrivateKey as before. Thus, the result of calling methods that return key values or parameters should be non-null as expected * A sensitive private key will be of the PrivateKey visible type for an external-package client. Thus, it's not possible to get key values or parameters from there. Note that calling the methods that return the encoded key or format will keep returning null in these cases as before. * The difference now is that a package client (sun.security.pkcs11 internal class) can access key parameters for sensitive keys because the visible type for them is (RSA,DSA,DH,EC)PrivateKeyInternal * The key parameter information is now available and may be useful to several P11 classes in the future, in addition to P11Signature which is using it now to have an exact estimate of a DSA signature length * There was boiler-plate code with P11(RSA,DSA,DH,EC)PublicKey classes because there is an overlap with key attributes fetched for private keys, and the fetching machinery was duplicated. We now have a single way of getting key attributes. * In the P11RSAPrivateKey case (RSA CRT), we save one call to the native PKCS#11 library because all attributes are obtained at once, instead of delaying the fetch of CKA_MODULUS and CKA_PRIVATE_EXPONENT to a later point. We do not add additional PKCS#11 calls in any case. * Several improvements specific to RSA key classes. Despite being a bit different than DSA, DH and EC because of the existence of CRT and non-CRT keys; they are now better aligned and duplicated code was removed. The refactorings implied removing fields from private but serializable classes. In example, P11DSAPublicKey does not longer have the fields 'y' and 'params'. My understanding is that this a serial compatibility breaker and a CSR would be needed. I can address that but wish you could have a look at the proposal before, so we come to something stable first. No test regressions observed in jdk/sun/security/pkcs11 category. - PR: https://git.openjdk.java.net/jdk/pull/4961
Re: RFR: 8271566: DSA signature length value is not accurate in P11Signature [v2]
> As described in JDK-8271566 [1], this patch proposal is intended to fix a > problem that arises when using DSA keys that have a 256-bits (or larger) G > parameter for signatures (either signing or verifying). There were some > incorrect assumptions and hard-coded length values in the code before. Please > note that, for example, the tuple (2048, 256) for DSA is valid according to > FIPS PUB 186-4. > > Beyond the specific issues in signatures, I decided to provide a broader > solution and enable key parameter retrieval for other key types (EC, DH) when > possible. This is, when the key is not sensitive. One thing that I should > note here is that token keys (those that have the CKA_TOKEN attribute equal > to 'true') are considered sensitive in this regard, at least by the NSS > Software Token implementation. I don't have access to other vendor > implementations but if there is any concern, we can adjust the constraint to > NSS-only. However, I'm not sure which use-case would require to get private > keys out of a real token, weakening its security. I'd be more conservative > here and not query the values if not sure that it will succeed. > > No regressions found in jdk/sun/security/pkcs11. A new test added: > LargerDSAKey. > > -- > [1] - https://bugs.openjdk.java.net/browse/JDK-8271566 Martin Balao has updated the pull request incrementally with one additional commit since the last revision: P11Key static inner classes refactorings. - Changes: - all: https://git.openjdk.java.net/jdk/pull/4961/files - new: https://git.openjdk.java.net/jdk/pull/4961/files/b02826b1..ce7c79ed Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=4961&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=4961&range=00-01 Stats: 802 lines in 3 files changed: 428 ins; 188 del; 186 mod Patch: https://git.openjdk.java.net/jdk/pull/4961.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4961/head:pull/4961 PR: https://git.openjdk.java.net/jdk/pull/4961
Re: RFR: 8266182: Automate manual steps listed in the test jdk/sun/security/pkcs12/ParamsTest.java [v6]
On Fri, 13 Aug 2021 09:47:53 GMT, Abdul Kolarkunnu
wrote:
>> ParamsTest is an interop test between keytool <-> openssl. There are some
>> manual steps listed in jdk/sun/security/pkcs12/params/README to perform
>> after the execution of jtreg execution. So this test is to perform that
>> manual steps.
>
> Abdul Kolarkunnu has updated the pull request incrementally with one
> additional commit since the last revision:
>
> 8266182: Automate manual steps listed in the test
> jdk/sun/security/pkcs12/ParamsTest.java
Thanks for the change. The fallback to `testWithJavaCommands()`-only is fine.
Some comments.
test/jdk/sun/security/pkcs12/KeytoolOpensslInteropTest.java line 91:
> 89: opensslPath = OpensslArtifactFetcher.fetchOpenssl();
> 90: }
> 91: if (opensslPath != null && verifyOpensslVerion(opensslPath)) {
The code here is a little different from your description above. If user
provides a non-1.1.* openssl through `test.openssl.path` then you won't go look
into elsewhere.
Also, `getSystemOpensslPath()` and `OpensslArtifactFetcher.fetchOpenssl()`
already ensure version is 1.1.* so the check above is duplicated.
test/jdk/sun/security/pkcs12/KeytoolOpensslInteropTest.java line 614:
> 612: }
> 613:
> 614: private static boolean verifyOpensslVerion(String path) {
Oops, a typo in the method name. `s/Verion/Version/`.
test/jdk/sun/security/pkcs12/KeytoolOpensslInteropTest.java line 618:
> 616: ProcessTools.executeCommand(path, "version")
> 617: .shouldHaveExitValue(0)
> 618: .shouldMatch("1.1.*");
Precisely the regex should be `1\.1\..*`, or maybe you can simply call
`.shouldContain("1.1.")`.
test/lib/jdk/test/lib/artifacts/OpensslArtifactFetcher.java line 24:
> 22: */
> 23:
> 24: package jdk.test.lib.artifacts.openssl;
The file is not in this package. Somehow I don't think if it's not worth
creating a new package for it. Do you expect we will have more classes here?
test/lib/jdk/test/lib/artifacts/OpensslArtifactFetcher.java line 35:
> 33: public class OpensslArtifactFetcher {
> 34:
> 35: public static String fetchOpenssl() {
Can we rename this to `fetchOpenssl1.1.1`? Someday we will need to download
other versions. If so, you can probably move `getSystemOpensslPath`,
`verifyOpensslVersion`, and `System.getProperty("test.openssl.path")` here as
well. It's quite likely that they will be used together.
-
PR: https://git.openjdk.java.net/jdk/pull/4413
RFR: 8270344: Session resumption errors
Corner case where a session resumption can fail if the TLS server changes supported protocol versions in relation to a cached SSLSession. This is primarily an issue where the legacy TLS version is used in place of the newer "supported_versions" TLS extension. - Commit messages: - 8270344: Session resumption errors Changes: https://git.openjdk.java.net/jdk/pull/5110/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=5110&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8270344 Stats: 192 lines in 2 files changed: 192 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/5110.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5110/head:pull/5110 PR: https://git.openjdk.java.net/jdk/pull/5110
Re: RFR: 8267844: Replace Integer/Long.valueOf() with Integer/Long.parse*() where applicable [v2]
On Fri, 13 Aug 2021 13:16:04 GMT, SkateScout
wrote:
> OK even if we keep out the edge case in the try block the
> "parseLong(nm.substring(index), radix)" could be replaced as already
> mentioned with parseLong(nm. index, nm.length(), radix)
I think it already was:
https://github.com/openjdk/jdk/blob/master/src/java.base/share/classes/java/lang/Long.java#L1287
> and in the catch block the idea to throw an "nice" error can be misleading.
> since -02147483648 for example would become -2147483648 because the radix is
> 8.
Yes, it's slightly misleading that the radix specifier gets cropped out, but
the error message does include the radix information so it's not a bug
technically:
jshell> Integer.decode("-01234567890123");
| Exception java.lang.NumberFormatException: For input string:
"-1234567890123" under radix 8
> Since per Radix only one String is possible to get through would if not be
> faster and more clear to check (compare) if it is the matching string and
> return the correct value else throw the error. This is also easy to read and
> even if is on the edge avoid substring , concationation and reparsing.
It might be a bit faster for that one non-exceptional accepted input, sure. It
could also incur a cost on the fast path due increasing the weight of the code.
You'd need to carefully measure that the added logic and checks doesn't cause
any trouble elsewhere.
-
PR: https://git.openjdk.java.net/jdk/pull/5068
Re: RFR: 8267844: Replace Integer/Long.valueOf() with Integer/Long.parse*() where applicable [v2]
On Tue, 10 Aug 2021 21:06:00 GMT, Сергей Цыпанов wrote: >> The code in `Integer.decode()` and `Long.decode()` might allocate two >> instances of Integer/Long for the negative values less than -127: >> >> Integer result; >> >> result = Integer.valueOf(nm.substring(index), radix); >> result = negative ? Integer.valueOf(-result.intValue()) : result; >> >> To avoid this we can declare 'result' as `int` and use `Integer.parseInt()` >> method. Same applicable for `Long` and some other classes. > > Сергей Цыпанов has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains five additional > commits since the last revision: > > - 8267844: Add benchmarks for Integer/Long.decode() > - 8267844: Rid useless substring when calling Integer/Long.parse*() > - Merge branch 'master' into 8267844 > - Merge branch 'master' into 8267844 > - 8267844: Replace Integer/Long.valueOf() with Integer/Long.parse*() where > applicable OK even if we keep out the edge case in the try block the "parseLong(nm.substring(index), radix)" could be replaced as already mentioned with parseLong(nm. index, nm.length(), radix) and in the catch block the idea to throw an "nice" error can be misleading. since -02147483648 for example would become -2147483648 because the radix is 8. Since per Radix only one String is possible to get through would if not be faster and more clear to check (compare) if it is the matching string and return the correct value else throw the error. This is also easy to read and even if is on the edge avoid substring , concationation and reparsing. - PR: https://git.openjdk.java.net/jdk/pull/5068
Re: RFR: 8266182: Automate manual steps listed in the test jdk/sun/security/pkcs12/ParamsTest.java [v4]
On Mon, 9 Aug 2021 12:54:40 GMT, Weijun Wang wrote:
>> Abdul Kolarkunnu has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> 8266182: Automate manual steps listed in the test
>> jdk/sun/security/pkcs12/ParamsTest.java
>
> test/jdk/sun/security/pkcs12/KeytoolOpensslInteropTest.java line 48:
>
>> 46: import java.nio.file.Files;
>> 47: import java.nio.file.Path;
>> 48: import java.nio.file.Paths;
>
> IntelliJ IDEA shows quite some imports are useless.
Removed all unused imports
> test/jdk/sun/security/pkcs12/KeytoolOpensslInteropTest.java line 100:
>
>> 98: OutputAnalyzer output = ProcessTools.executeCommand(
>> 99: opensslPath, "version")
>> 100: .shouldHaveExitValue(0);
>
> This looks like a good time to ensure the version is 1.1.* or at least 1.* (I
> haven't tested with 1.0.* versions). Also, when trying out with 3.0.0, there
> are only 2 failures (line 119 generating weak file failed without -legacy.
> line 479 succeeded with a warning).
ok, added version check ProcessTools.executeCommand(path,
"version").shouldHaveExitValue(0).shouldMatch("1.1.*");
I tested with 1.0.2a version, test fails because some of the info messages are
different.
eg: 'MAC: sha256, Iteration 1' missing from stdout/stderr
1.0.2a produces output as "MAC Iteration 1" . ": sha256" is not there.
-
PR: https://git.openjdk.java.net/jdk/pull/4413
Re: RFR: 8266182: Automate manual steps listed in the test jdk/sun/security/pkcs12/ParamsTest.java [v4]
On Mon, 9 Aug 2021 13:04:03 GMT, Weijun Wang wrote:
>> Abdul Kolarkunnu has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> 8266182: Automate manual steps listed in the test
>> jdk/sun/security/pkcs12/ParamsTest.java
>
> test/jdk/sun/security/pkcs12/KeytoolOpensslInteropTest.java line 88:
>
>> 86: testWithJavaCommands();
>> 87: testWithOpensslCommands(opensslPath);
>> 88: } else {
>
> I still think it's better to succeed with a warning when the openssl binary
> cannot be found. IMHO it's a little unfriendly to force people setting up a
> system property to let the test pass. It's also dependent on runner machines
> and the user might have to adjust their scripts or launcher all the time. I
> would rather keep the old test/data if I want to ensure the test gets run
> everywhere.
>
> Also, it might also help if the test can search for openssl, maybe simply
> from `/usr/bin/openssl` or `/usr/local/bin/openssl`, but this means you might
> need to check for the version.
>
> Third, is it OK to let the system property pointing to the binary itself
> directly? When I was trying out this test I was using the openssl I built and
> it's not in a `bin` sub-directory.
Ok, changed openssl selection flow as below:
1. Check whether test.openssl.path is set and it's the the preferred
version(1.1.*) of openssl
2. If above property doesn't set, then look for already installed openssl
(version 1.1.*) in system path /usr/bin/openssl or /usr/local/bin/openssl
3. if above is also not available try to download openssl from artifactory
If any of above 3 succeeds then perform all tests, otherwise skip all openssl
command dependent tests.
-
PR: https://git.openjdk.java.net/jdk/pull/4413
Re: RFR: 8266182: Automate manual steps listed in the test jdk/sun/security/pkcs12/ParamsTest.java [v4]
On Mon, 9 Aug 2021 13:10:05 GMT, Weijun Wang wrote: >> Abdul Kolarkunnu has updated the pull request incrementally with one >> additional commit since the last revision: >> >> 8266182: Automate manual steps listed in the test >> jdk/sun/security/pkcs12/ParamsTest.java > > test/jdk/sun/security/pkcs12/KeytoolOpensslInteropTest.java line 580: > >> 578: return SecurityTools.keytool(s); >> 579: } >> 580: > > I feel the lines below should go to a test library. Moved fetchOpenssl() API to test/lib/jdk/test/lib/artifacts/OpensslArtifactFetcher.java - PR: https://git.openjdk.java.net/jdk/pull/4413
Re: RFR: 8266182: Automate manual steps listed in the test jdk/sun/security/pkcs12/ParamsTest.java [v6]
> ParamsTest is an interop test between keytool <-> openssl. There are some > manual steps listed in jdk/sun/security/pkcs12/params/README to perform after > the execution of jtreg execution. So this test is to perform that manual > steps. Abdul Kolarkunnu has updated the pull request incrementally with one additional commit since the last revision: 8266182: Automate manual steps listed in the test jdk/sun/security/pkcs12/ParamsTest.java - Changes: - all: https://git.openjdk.java.net/jdk/pull/4413/files - new: https://git.openjdk.java.net/jdk/pull/4413/files/cad8f2d3..0c1ede14 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=4413&range=05 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=4413&range=04-05 Stats: 0 lines in 7 files changed: 0 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/4413.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4413/head:pull/4413 PR: https://git.openjdk.java.net/jdk/pull/4413
Re: RFR: 8266182: Automate manual steps listed in the test jdk/sun/security/pkcs12/ParamsTest.java [v5]
> ParamsTest is an interop test between keytool <-> openssl. There are some > manual steps listed in jdk/sun/security/pkcs12/params/README to perform after > the execution of jtreg execution. So this test is to perform that manual > steps. Abdul Kolarkunnu has updated the pull request incrementally with one additional commit since the last revision: 8266182: Automate manual steps listed in the test jdk/sun/security/pkcs12/ParamsTest.java - Changes: - all: https://git.openjdk.java.net/jdk/pull/4413/files - new: https://git.openjdk.java.net/jdk/pull/4413/files/fe17653f..cad8f2d3 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=4413&range=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=4413&range=03-04 Stats: 543 lines in 9 files changed: 464 ins; 50 del; 29 mod Patch: https://git.openjdk.java.net/jdk/pull/4413.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4413/head:pull/4413 PR: https://git.openjdk.java.net/jdk/pull/4413
Re: RFR: 8267844: Replace Integer/Long.valueOf() with Integer/Long.parse*() where applicable [v2]
On Thu, 12 Aug 2021 21:26:08 GMT, SkateScout
wrote:
> Hi,
> i check Long.java line 1301...1311 and i am not sure if this code is really
> good.
>
> 1. If negative is false the whole part ends with two times the same
> substring and this implies the same error.
>
> 2. If negative is true and we get an error than we can throw the error
> without an second parse step or we can use the substring from the first round.
>
> 3. Also as mentioned above the parseLong(text,radix) should be changed to
> parseLong(seq, beginIndex, endIndex, radix)
>this would avoid at least in the positive case the substring at all.
>
> 4. The same points are with Integer as well.
>try {
>result = parseLong(nm.substring(index), radix);
>result = negative ? -result : result;
>} catch (NumberFormatException e) {
>// If number is Long.MIN_VALUE, we'll end up here. The next line
>// handles this case, and causes any genuine format error to be
>// rethrown.
>String constant = negative ? ("-" + nm.substring(index))
>: nm.substring(index);
>result = parseLong(constant, radix);
>}
The pre-existing logic here is iffy, but I think it is correct.
For Integer: If negative is true, then parsing "2147483648" (Integer.MAX_VALUE
+ 1) would throw, be reparsed as "-2147483648" and then be accepted as
Integer.MIN_VALUE. This is the only case that should be non-exceptional, but it
should also be exceedingly rare in practice. For negative values it improves
the error messages a bit to add the "-" and reparse.
Improving the catch clauses with `parseLong(CharSequence, int, int, int)` and
adding an `if (!negative) throw e` case to the catch could theoretically
improve performance of parsing the MIN_VALUE edge case and repeated decoding of
malformed positive numbers, but these are rare or exceptional cases where we
should favor simplicity over optimal performance
-
PR: https://git.openjdk.java.net/jdk/pull/5068
Re: RFR: 8267844: Replace Integer/Long.valueOf() with Integer/Long.parse*() where applicable [v2]
On Tue, 10 Aug 2021 21:06:00 GMT, Сергей Цыпанов
wrote:
>> The code in `Integer.decode()` and `Long.decode()` might allocate two
>> instances of Integer/Long for the negative values less than -127:
>>
>> Integer result;
>>
>> result = Integer.valueOf(nm.substring(index), radix);
>> result = negative ? Integer.valueOf(-result.intValue()) : result;
>>
>> To avoid this we can declare 'result' as `int` and use `Integer.parseInt()`
>> method. Same applicable for `Long` and some other classes.
>
> Сергей Цыпанов has updated the pull request with a new target base due to a
> merge or a rebase. The incremental webrev excludes the unrelated changes
> brought in by the merge/rebase. The pull request contains five additional
> commits since the last revision:
>
> - 8267844: Add benchmarks for Integer/Long.decode()
> - 8267844: Rid useless substring when calling Integer/Long.parse*()
> - Merge branch 'master' into 8267844
> - Merge branch 'master' into 8267844
> - 8267844: Replace Integer/Long.valueOf() with Integer/Long.parse*() where
> applicable
Hi,
i check Long.java line 1301...1311 and i am not sure if this code is really
good.
1) If negative is false the whole part ends with two times the same substring
and this implies the same error.
2) If negative is true and we get an error than we can throw the error without
an second parse step or we can use the substring from the first round.
3) Also as mentioned above the parseLong(text,radix) should be changed to
parseLong(seq, beginIndex, endIndex, radix)
this would avoid at least in the positive case the substring at all.
4) The same points are with Integer as well.
try {
result = parseLong(nm.substring(index), radix);
result = negative ? -result : result;
} catch (NumberFormatException e) {
// If number is Long.MIN_VALUE, we'll end up here. The next line
// handles this case, and causes any genuine format error to be
// rethrown.
String constant = negative ? ("-" + nm.substring(index))
: nm.substring(index);
result = parseLong(constant, radix);
}
-
PR: https://git.openjdk.java.net/jdk/pull/5068
