Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8256895: Add support for RFC 8954: Online Certificate Status Protocol (OCSP) Nonce Extension #2039

Closed
wants to merge 7 commits into from

Conversation

@haimaychao
Copy link
Contributor

@haimaychao haimaychao commented Jan 11, 2021

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


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8256895: Add support for RFC 8954: Online Certificate Status Protocol (OCSP) Nonce Extension

Reviewers

Download

$ git fetch https://git.openjdk.java.net/jdk pull/2039/head:pull/2039
$ git checkout pull/2039

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Jan 11, 2021

👋 Welcome back hchao! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk openjdk bot added the rfr label Jan 11, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Jan 11, 2021

@haimaychao The following label will be automatically applied to this pull request:

  • security

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

@openjdk openjdk bot added the security label Jan 11, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Jan 11, 2021

Copy link
Member

@jnimeh jnimeh left a comment

In general it looks pretty good. Just a couple small comments.

@@ -121,12 +123,13 @@ public OCSPNonceExtension(byte[] incomingNonce) throws IOException {
* @param isCritical a boolean flag indicating whether the criticality bit
* is set for this extension
* @param incomingNonce The nonce data to be set for the extension. This
* must be a non-null array of at least one byte long.
* must be a non-null array of at least one byte long and can be upto
Copy link
Member

@jnimeh jnimeh Jan 12, 2021

typo: "upto" -> "up to"

Copy link
Contributor Author

@haimaychao haimaychao Jan 12, 2021

fixed.

if (incomingNonce.length > 0) {
// RFC 8954, section 2.1: the length of the nonce MUST be at least 1 octet
// and can be up to 32 octets.
if (incomingNonce.length > 0 && incomingNonce.length <=32) {
Copy link
Member

@jnimeh jnimeh Jan 12, 2021

nit: space after the <= to be consistent with style elsewhere

Copy link
Contributor Author

@haimaychao haimaychao Jan 12, 2021

fixed.


tmpExtensions = new ArrayList<Extension>();
tmpExtensions.add(nonceExt);
setOcspExtensions(tmpExtensions);
Copy link
Member

@jnimeh jnimeh Jan 12, 2021

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.

Copy link
Contributor Author

@haimaychao haimaychao Jan 12, 2021

collapsing done.

@haimaychao
Copy link
Contributor Author

@haimaychao haimaychao commented Jan 12, 2021

Thanks for the review, Jamil. I've updated with all of your comments.

@@ -47,6 +47,7 @@

public static void main(String[] args) throws Exception {

System.setProperty("jdk.security.certpath.ocspNonce", "true");
Copy link
Member

@rhalade rhalade Jan 12, 2021

Trying to understand this change - why do we need this change and only in couple of tests? Did these test fail with your change?

Copy link
Contributor Author

@haimaychao haimaychao Jan 12, 2021

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.

jnimeh
jnimeh approved these changes Jan 14, 2021
Copy link
Member

@jnimeh jnimeh left a comment

Looks good.

}
private RevocationProperties rp;
Copy link
Member

@seanjmullan seanjmullan Jan 14, 2021

I think this field could be final.

Copy link
Contributor Author

@haimaychao haimaychao Jan 15, 2021

No change made due to getting an error: cannot assign a value to final variable rp.

init(anchor, params);
}

private void setDefaultNonce() {
byte[] nonce = null;
Copy link
Member

@seanjmullan seanjmullan Jan 14, 2021

This variable looks like it is not used and can be removed.

Copy link
Contributor Author

@haimaychao haimaychao Jan 15, 2021

The setDefaultNonce() method is removed.

private void setDefaultNonce() {
byte[] nonce = null;

// Set the nonce by default in OCSP request extension when the sytem property
Copy link
Member

@seanjmullan seanjmullan Jan 14, 2021

Typo: s/sytem/system/

Copy link
Contributor Author

@haimaychao haimaychao Jan 15, 2021

The setDefaultNonce() method is removed as creating nonce is done in checkOCSP() method now.

// jdk.security.certpath.ocspNonce=true.
if (rp.ocspNonce) {
try {
setOcspExtensions(List.of(new OCSPNonceExtension(DEFAULT_NONCE_BYTES)));
Copy link
Member

@seanjmullan seanjmullan Jan 14, 2021

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.

Copy link
Contributor Author

@haimaychao haimaychao Jan 15, 2021

Creating the nonce is moved to checkOCSP() method.

@openjdk
Copy link

@openjdk openjdk bot commented Jan 15, 2021

@haimaychao This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8256895: Add support for RFC 8954: Online Certificate Status Protocol (OCSP) Nonce Extension

Reviewed-by: jnimeh, mullan

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 188 new commits pushed to the master branch:

  • 4f11ff3: 8259488: Shenandoah: Missing timing tracking for STW CLD root processing
  • 0785147: 8259949: x86 32-bit build fails when -fcf-protection is passed in the compiler flags
  • 5891509: 8259947: (fs) Optimize UnixPath.encode implementation
  • 69f90b5: 8259843: initialize dli_fname array before calling dll_address_to_library_name
  • 52ed2aa: 8259786: initialize last parameter of getpwuid_r
  • 70b5b31: 8257664: HTMLEditorKit: Wrong CSS relative font sizes
  • 0b01d69: 8260005: Shenandoah: Remove unused AlwaysTrueClosure in ShenandoahConcurrentRootScanner::roots_do()
  • 0529480: 8259867: Move encoding checks into ZipCoder
  • 7c32ffe: 8258383: vmTestbase/gc/g1/unloading/tests/unloading_compilation_level[1,2,3] time out without TieredCompilation
  • 9f21bb6: 8259983: do not use uninitialized expand_ms value in G1CollectedHeap::expand_heap_after_young_collection
  • ... and 178 more: https://git.openjdk.java.net/jdk/compare/a653928230c4153af04b1b32acd1af366eefa6ac...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@jnimeh, @seanjmullan) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

@openjdk openjdk bot added the ready label Jan 15, 2021
@haimaychao haimaychao changed the title 8256895: Add support for RFC 8954: Online Certificate Status Protocol… 8256895: Add support for RFC 8954: Online Certificate Status Protocol (OCSP) Nonce Extension Jan 15, 2021
}
} catch (IOException e) {
throw new CertPathValidatorException("Failed to create the default nonce " +
"in OCSP entensions");
Copy link
Member

@seanjmullan seanjmullan Jan 20, 2021

Typo: s/entensions/extensions/

Also, use the CertPathValidatorException(String, Throwable) ctor instead and pass the IOException as the 2nd parameter.

Copy link
Contributor Author

@haimaychao haimaychao Jan 20, 2021

Fixed.

try {
// create the 16-byte nonce by default
Extension nonceExt = new OCSPNonceExtension(DEFAULT_NONCE_BYTES);
tmpExtensions.add(nonceExt);
Copy link
Member

@seanjmullan seanjmullan Jan 20, 2021

I think you should add the OCSPNonce extension to the list of extensions that the application passed in, as there may be other extensions that have been specified and should be sent in the OCSP response, ex:

ocspExtensions.add(new OCSPNonceExtension(DEFAULT_NONCE_BYTES));

This means you don't need the tmpExtensions variable.

Copy link
Contributor Author

@haimaychao haimaychao Jan 20, 2021

During testing, I got the "java.base/java.util.Collections$UnmodifiableCollection.add(Collections.java:1062) exception with this line of change. I've changed to use a tmpExtensions variable when setting the OCSP nonce to the extension sets instead of modifying the ocspExtensions.

response = OCSP.check(Collections.singletonList(certId),
responderURI, issuerInfo, responderCert, null,
ocspExtensions, params.variant());
rp.ocspNonce ? tmpExtensions : ocspExtensions, params.variant());
Copy link
Member

@seanjmullan seanjmullan Jan 20, 2021

Here you can just pass in ocspExtensions since it will contain the nonce if the property has been set.

Copy link
Contributor Author

@haimaychao haimaychao Jan 20, 2021

No change as tmpExtensions is needed.

if (ocspExtensions.size() > 0) {
tmpExtensions = new ArrayList<Extension>(ocspExtensions);
} else {
tmpExtensions = new ArrayList<Extension>();
}

tmpExtensions.add(nonceExt);
Copy link
Member

@seanjmullan seanjmullan Jan 20, 2021

If you only need the nonce, you could use List.of and save a little bit of memory, ex:

                        if (ocspExtensions.size() > 0) {
                            tmpExtensions = new ArrayList<Extension>(ocspExtensions);
                            tmpExtensions.add(nonceExt);
                        } else {
                            tmpExtensions = List.of(nonceExt);
                        }

Copy link
Contributor Author

@haimaychao haimaychao Jan 20, 2021

Thanks for the review. Updated as suggested.

@@ -740,9 +746,43 @@ private void checkOCSP(X509Certificate cert,
null, -1);
}

List<Extension> tmpExtensions = Collections.<Extension>emptyList();
Copy link
Member

@seanjmullan seanjmullan Jan 20, 2021

One other comment - I think you can just set this to null.

Copy link
Contributor Author

@haimaychao haimaychao Jan 20, 2021

Fixed.

@haimaychao
Copy link
Contributor Author

@haimaychao haimaychao commented Jan 20, 2021

/integrate

@openjdk openjdk bot added the sponsor label Jan 20, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Jan 20, 2021

@haimaychao
Your change (at version b334a5d) is now ready to be sponsored by a Committer.

@seanjmullan
Copy link
Member

@seanjmullan seanjmullan commented Jan 20, 2021

/sponsor

@openjdk openjdk bot closed this Jan 20, 2021
@openjdk openjdk bot added the integrated label Jan 20, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Jan 20, 2021

@seanjmullan @haimaychao Since your change was applied there have been 188 commits pushed to the master branch:

  • 4f11ff3: 8259488: Shenandoah: Missing timing tracking for STW CLD root processing
  • 0785147: 8259949: x86 32-bit build fails when -fcf-protection is passed in the compiler flags
  • 5891509: 8259947: (fs) Optimize UnixPath.encode implementation
  • 69f90b5: 8259843: initialize dli_fname array before calling dll_address_to_library_name
  • 52ed2aa: 8259786: initialize last parameter of getpwuid_r
  • 70b5b31: 8257664: HTMLEditorKit: Wrong CSS relative font sizes
  • 0b01d69: 8260005: Shenandoah: Remove unused AlwaysTrueClosure in ShenandoahConcurrentRootScanner::roots_do()
  • 0529480: 8259867: Move encoding checks into ZipCoder
  • 7c32ffe: 8258383: vmTestbase/gc/g1/unloading/tests/unloading_compilation_level[1,2,3] time out without TieredCompilation
  • 9f21bb6: 8259983: do not use uninitialized expand_ms value in G1CollectedHeap::expand_heap_after_young_collection
  • ... and 178 more: https://git.openjdk.java.net/jdk/compare/a653928230c4153af04b1b32acd1af366eefa6ac...master

Your commit was automatically rebased without conflicts.

Pushed as commit 8b95d95.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants