Re: RFR: 8209935: Test to cover CodeSource.getCodeSigners() [v5]
> A Test updated to cover the getCodeSigners. Sibabrata Sahoo has updated the pull request incrementally with two additional commits since the last revision: - 8209935: Test to cover CodeSource.getCodeSigners() - 8209935: Test to cover CodeSource.getCodeSigners() - Changes: - all: https://git.openjdk.org/jdk/pull/8286/files - new: https://git.openjdk.org/jdk/pull/8286/files/512cb615..a0c1d070 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8286=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8286=03-04 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/8286.diff Fetch: git fetch https://git.openjdk.org/jdk pull/8286/head:pull/8286 PR: https://git.openjdk.org/jdk/pull/8286
Re: RFR: 8209935: Test to cover CodeSource.getCodeSigners() [v4]
On Thu, 9 Jun 2022 21:12:26 GMT, Sean Mullan wrote: >> Sibabrata Sahoo has updated the pull request incrementally with one >> additional commit since the last revision: >> >> 8209935: Test to cover CodeSource.getCodeSigners() > > test/jdk/java/security/CodeSource/CertsMatch.java line 27: > >> 25: * @test >> 26: * @bug 6299163 >> 27: * @summary REGRESSION: java.security.CodeSource#equals not symmetric > > I think you should remove this line about "REGRESSION" as the summary is > supposed to be about what the test is testing. Suggest changing this to > summarize what methods are being tested. Updated the summary. - PR: https://git.openjdk.org/jdk/pull/8286
Re: RFR: 8287178: IntegerModuloP::multiplicativeInverse returns 0 for 0 [v2]
On Thu, 9 Jun 2022 22:29:36 GMT, Jamil Nimeh wrote: >> Weijun Wang has updated the pull request incrementally with one additional >> commit since the last revision: >> >> verb > > src/java.base/share/classes/sun/security/util/math/IntegerModuloP.java line > 161: > >> 159: // This method is used in 2 cases: >> 160: // 1. To calculate the inverse of a number in ECDSAOperations, >> 161: //this number must be non zero (modulus p). > > Should that be "modulo p"? Other than that it looks good to me. Yes, it's a verb here. - PR: https://git.openjdk.org/jdk/pull/9115
Re: RFR: 8287178: IntegerModuloP::multiplicativeInverse returns 0 for 0 [v2]
On Thu, 9 Jun 2022 23:10:05 GMT, Weijun Wang wrote: >> Add comment to the method. > > Weijun Wang has updated the pull request incrementally with one additional > commit since the last revision: > > verb Thanks, LGTM. - Marked as reviewed by jnimeh (Reviewer). PR: https://git.openjdk.org/jdk/pull/9115
Re: RFR: 8287178: IntegerModuloP::multiplicativeInverse returns 0 for 0 [v2]
> Add comment to the method. Weijun Wang has updated the pull request incrementally with one additional commit since the last revision: verb - Changes: - all: https://git.openjdk.org/jdk/pull/9115/files - new: https://git.openjdk.org/jdk/pull/9115/files/36741bba..15ef85fe Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=9115=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=9115=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/9115.diff Fetch: git fetch https://git.openjdk.org/jdk pull/9115/head:pull/9115 PR: https://git.openjdk.org/jdk/pull/9115
Re: RFR: 8286779: javax.crypto.CryptoPolicyParser#isConsistent always returns 'true' [v2]
On Thu, 9 Jun 2022 21:00:55 GMT, Sean Mullan wrote:
>> Hai-May Chao has updated the pull request incrementally with two additional
>> commits since the last revision:
>>
>> - Inconsistent entries test
>> - Inconsistent entries test
>
> src/java.base/share/classes/javax/crypto/CryptoPolicyParser.java line 202:
>
>> 200: if (!processedPermissions.isEmpty()) {
>> 201: throw new ParsingException(st.lineno(), "Inconsistent
>> policy");
>> 202: }
>
> Instead of setting the `allPermEntryFound` flag, what if you instead put an
> entry for `CryptoAllPermission.ALG_NAME` in `processedPermissions` here. Then
> if there are more entries after this, I think `isConsistent` will catch it in
> the following code:
>
>
> if (processedPermissions.containsKey(CryptoAllPermission.ALG_NAME)) {
> return false;
> }
Yes, with the `allPermEntryFound` flag, the current fix would not require to
put the `javax.crypto.CryptoAllPermission` entry in `processedPermissions`. So
`processedPermissions` will be used to keep `javax.crypto.CryptoPermission`
entries and is updated by `isConsistent()`, and no need to deal with
`javax.crypto.CryptoAllPermission` entry. I’d like to keep it as-is if there is
no objection.
-
PR: https://git.openjdk.org/jdk/pull/8985
Re: RFR: 8287178: IntegerModuloP::multiplicativeInverse returns 0 for 0
On Thu, 9 Jun 2022 21:34:56 GMT, Weijun Wang wrote: > Add comment to the method. src/java.base/share/classes/sun/security/util/math/IntegerModuloP.java line 161: > 159: // This method is used in 2 cases: > 160: // 1. To calculate the inverse of a number in ECDSAOperations, > 161: //this number must be non zero (modulus p). Should that be "modulo p"? Other than that it looks good to me. - PR: https://git.openjdk.org/jdk/pull/9115
RFR: 8287178: IntegerModuloP::multiplicativeInverse returns 0 for 0
Add comment to the method. - Commit messages: - add comment Changes: https://git.openjdk.org/jdk/pull/9115/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=9115=00 Issue: https://bugs.openjdk.org/browse/JDK-8287178 Stats: 8 lines in 1 file changed: 7 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/9115.diff Fetch: git fetch https://git.openjdk.org/jdk pull/9115/head:pull/9115 PR: https://git.openjdk.org/jdk/pull/9115
Re: RFR: 8209935: Test to cover CodeSource.getCodeSigners() [v4]
On Wed, 8 Jun 2022 10:30:45 GMT, Sibabrata Sahoo wrote: >> A Test updated to cover the getCodeSigners. > > Sibabrata Sahoo has updated the pull request incrementally with one > additional commit since the last revision: > > 8209935: Test to cover CodeSource.getCodeSigners() Marked as reviewed by mullan (Reviewer). test/jdk/java/security/CodeSource/CertsMatch.java line 27: > 25: * @test > 26: * @bug 6299163 > 27: * @summary REGRESSION: java.security.CodeSource#equals not symmetric I think you should remove this line about "REGRESSION" as the summary is supposed to be about what the test is testing. Suggest changing this to summarize what methods are being tested. - PR: https://git.openjdk.org/jdk/pull/8286
Re: RFR: 8286779: javax.crypto.CryptoPolicyParser#isConsistent always returns 'true' [v2]
On Tue, 7 Jun 2022 20:52:33 GMT, Hai-May Chao wrote:
>> Please review a small fix in CryptoPolicyParser class that it should not
>> pass “processedPermissions” parameter by value.
>> Ran MACH5 tier1 and tier2 without failures.
>
> Hai-May Chao has updated the pull request incrementally with two additional
> commits since the last revision:
>
> - Inconsistent entries test
> - Inconsistent entries test
src/java.base/share/classes/javax/crypto/CryptoPolicyParser.java line 202:
> 200: if (!processedPermissions.isEmpty()) {
> 201: throw new ParsingException(st.lineno(), "Inconsistent
> policy");
> 202: }
Instead of setting the `allPermEntryFound` flag, what if you instead put an
entry for `CryptoAllPermission.ALG_NAME` in `processedPermissions` here. Then
if there are more entries after this, I think `isConsistent` will catch it in
the following code:
if (processedPermissions.containsKey(CryptoAllPermission.ALG_NAME)) {
return false;
}
-
PR: https://git.openjdk.org/jdk/pull/8985
RFR: 8271838: AmazonCA.java interop test fails
Updated test certificates to new from CA. I did a loop run of this test and don't see the intermittent failure anymore. - Commit messages: - 8271838: AmazonCA.java interop test fails Changes: https://git.openjdk.org/jdk/pull/9111/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=9111=00 Issue: https://bugs.openjdk.org/browse/JDK-8271838 Stats: 252 lines in 1 file changed: 7 ins; 50 del; 195 mod Patch: https://git.openjdk.org/jdk/pull/9111.diff Fetch: git fetch https://git.openjdk.org/jdk pull/9111/head:pull/9111 PR: https://git.openjdk.org/jdk/pull/9111
Re: RFR: 8277307: Pre shared key sent under both session_ticket and pre_shared_key extensions [v2]
On Thu, 2 Jun 2022 21:02:16 GMT, Daniel Jeliński wrote: >> Session ticket extension should only contain pre-TLS1.3 stateless session >> tickets; it should not be used for sending TLS1.3 pre-shared keys. > > Daniel Jeliński has updated the pull request incrementally with one > additional commit since the last revision: > > different check for TLS13 A little late to this review as it's already been pushed, but I would have suggested leaving the `return new SessionTicketSpec().getEncoded();` as it keeps the encapsulation more clear. Otherwise, it looks good. - PR: https://git.openjdk.org/jdk/pull/8922
RFR: 8288132: Update test artifacts in QuoVadis CA interop tests
Updated test artifacts. Test will continue to fail intermittently with what appears to be issue in CA infra. JDK-8277855 will track it. - Commit messages: - 8288132: Update test artifacts in QuoVadis CA interop tests Changes: https://git.openjdk.org/jdk/pull/9110/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=9110=00 Issue: https://bugs.openjdk.org/browse/JDK-8288132 Stats: 215 lines in 1 file changed: 36 ins; 0 del; 179 mod Patch: https://git.openjdk.org/jdk/pull/9110.diff Fetch: git fetch https://git.openjdk.org/jdk pull/9110/head:pull/9110 PR: https://git.openjdk.org/jdk/pull/9110
