Re: RFR: 8256895: Add support for RFC 8954: Online Certificate Status Protocol… [v2]
On Tue, 12 Jan 2021 19:18:18 GMT, Hai-May Chao wrote:
>> This enhancement adds support for the nonce extension in OCSP request
>> extensions by system property jdk.security.certpath.ocspNonce.
>>
>> Please review the CSR at:
>> https://bugs.openjdk.java.net/browse/JDK-8257766
>
> Hai-May Chao has updated the pull request incrementally with one additional
> commit since the last revision:
>
> update to use List.of() and typo changes
Changes requested by mullan (Reviewer).
src/java.base/share/classes/sun/security/provider/certpath/RevocationChecker.java
line 88:
> 86: boolean ocspNonce;
> 87: }
> 88: private RevocationProperties rp;
I think this field could be `final`.
src/java.base/share/classes/sun/security/provider/certpath/RevocationChecker.java
line 107:
> 105:
> 106: private void setDefaultNonce() {
> 107: byte[] nonce = null;
This variable looks like it is not used and can be removed.
src/java.base/share/classes/sun/security/provider/certpath/RevocationChecker.java
line 109:
> 107: byte[] nonce = null;
> 108:
> 109: // Set the nonce by default in OCSP request extension when the
> sytem property
Typo: s/sytem/system/
src/java.base/share/classes/sun/security/provider/certpath/RevocationChecker.java
line 113:
> 111: if (rp.ocspNonce) {
> 112: try {
> 113: setOcspExtensions(List.of(new
> OCSPNonceExtension(DEFAULT_NONCE_BYTES)));
I don't think we should use the `PKIXRevocationChecker.setOcspExtensions()` API
to add an OCSP Nonce extension. That API is intended to be called by
applications. I think the Nonce extension should be set by the implementation
only and not exposed via the standard API. Also, a nonce should be unique for
each OCSP request, but setting it here means that it could re-use the same
nonce for different OCSP requests.
I think a better place to create/add the OCSPExtension is in the `checkOCSP`
method, and the extension should be created each time that method is called (if
the system property is enabled), so a new nonce is created for each OCSP
request.
-
PR: https://git.openjdk.java.net/jdk/pull/2039
Re: RFR: 8256895: Add support for RFC 8954: Online Certificate Status Protocol… [v2]
On Tue, 12 Jan 2021 19:18:18 GMT, Hai-May Chao wrote: >> This enhancement adds support for the nonce extension in OCSP request >> extensions by system property jdk.security.certpath.ocspNonce. >> >> Please review the CSR at: >> https://bugs.openjdk.java.net/browse/JDK-8257766 > > Hai-May Chao has updated the pull request incrementally with one additional > commit since the last revision: > > update to use List.of() and typo changes Looks good. - Marked as reviewed by jnimeh (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/2039
Re: RFR: 8256895: Add support for RFC 8954: Online Certificate Status Protocol… [v2]
On Tue, 12 Jan 2021 20:10:34 GMT, Rajan Halade wrote:
>> Hai-May Chao has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> update to use List.of() and typo changes
>
> test/jdk/security/infra/java/security/cert/CertPathValidator/certification/SSLCA.java
> line 50:
>
>> 48: public static void main(String[] args) throws Exception {
>> 49:
>> 50: System.setProperty("jdk.security.certpath.ocspNonce", "true");
>
> Trying to understand this change - why do we need this change and only in
> couple of tests? Did these test fail with your change?
No, these test did not fail with my changes. These two tests are being changed
to set the system property jdk.security.certpath.ocspNonce=true in order to
send the nonce as the responders in these tests will take the nonce and return
it their responses, so we could test the benefit of having nonce extension set.
-
PR: https://git.openjdk.java.net/jdk/pull/2039
Re: RFR: 8256895: Add support for RFC 8954: Online Certificate Status Protocol… [v2]
On Tue, 12 Jan 2021 19:18:18 GMT, Hai-May Chao wrote:
>> This enhancement adds support for the nonce extension in OCSP request
>> extensions by system property jdk.security.certpath.ocspNonce.
>>
>> Please review the CSR at:
>> https://bugs.openjdk.java.net/browse/JDK-8257766
>
> Hai-May Chao has updated the pull request incrementally with one additional
> commit since the last revision:
>
> update to use List.of() and typo changes
test/jdk/security/infra/java/security/cert/CertPathValidator/certification/SSLCA.java
line 50:
> 48: public static void main(String[] args) throws Exception {
> 49:
> 50: System.setProperty("jdk.security.certpath.ocspNonce", "true");
Trying to understand this change - why do we need this change and only in
couple of tests? Did these test fail with your change?
-
PR: https://git.openjdk.java.net/jdk/pull/2039
Re: RFR: 8256895: Add support for RFC 8954: Online Certificate Status Protocol… [v2]
On Tue, 12 Jan 2021 16:26:11 GMT, Jamil Nimeh wrote:
>> Hai-May Chao has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> update to use List.of() and typo changes
>
> In general it looks pretty good. Just a couple small comments.
Thanks for the review, Jamil. I've updated with all of your comments.
> src/java.base/share/classes/sun/security/provider/certpath/OCSPNonceExtension.java
> line 126:
>
>> 124: * is set for this extension
>> 125: * @param incomingNonce The nonce data to be set for the extension.
>> This
>> 126: * must be a non-null array of at least one byte long and can
>> be upto
>
> typo: "upto" -> "up to"
fixed.
> src/java.base/share/classes/sun/security/provider/certpath/OCSPNonceExtension.java
> line 143:
>
>> 141: // RFC 8954, section 2.1: the length of the nonce MUST be at
>> least 1 octet
>> 142: // and can be up to 32 octets.
>> 143: if (incomingNonce.length > 0 && incomingNonce.length <=32) {
>
> nit: space after the <= to be consistent with style elsewhere
fixed.
> src/java.base/share/classes/sun/security/provider/certpath/RevocationChecker.java
> line 118:
>
>> 116: tmpExtensions = new ArrayList();
>> 117: tmpExtensions.add(nonceExt);
>> 118: setOcspExtensions(tmpExtensions);
>
> It seems like you could collapse 113 - 118 into something like:
> setOcspExtensions(List.of(new OCSPNonceExtension(DEFAULT_NONCE_BYTES)));
>
> At the very least, it looks like you could do away with 113, since you
> immediately change the value of the tmpExtensions reference on 116.
collapsing done.
-
PR: https://git.openjdk.java.net/jdk/pull/2039
Re: RFR: 8256895: Add support for RFC 8954: Online Certificate Status Protocol… [v2]
> This enhancement adds support for the nonce extension in OCSP request > extensions by system property jdk.security.certpath.ocspNonce. > > Please review the CSR at: > https://bugs.openjdk.java.net/browse/JDK-8257766 Hai-May Chao has updated the pull request incrementally with one additional commit since the last revision: update to use List.of() and typo changes - Changes: - all: https://git.openjdk.java.net/jdk/pull/2039/files - new: https://git.openjdk.java.net/jdk/pull/2039/files/5ade73d4..26254d59 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=2039=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=2039=00-01 Stats: 10 lines in 2 files changed: 0 ins; 5 del; 5 mod Patch: https://git.openjdk.java.net/jdk/pull/2039.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/2039/head:pull/2039 PR: https://git.openjdk.java.net/jdk/pull/2039
