8179503: Java should support GET OCSP calls #1760

jnimeh · 2020-12-13T19:18:42Z

This enhancement allows the underlying OCSP subsystem to submit OCSP requests using HTTP GET when the request is less than or equal to 255 bytes as documented in RFC 6960 and clarified in RFC 5019.

Progress

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

Issue

JDK-8179503: Java should support GET OCSP calls

Reviewers

Xue-Lei Andrew Fan (@XueleiFan - Reviewer)

Download

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

bridgekeeper · 2020-12-13T19:19:32Z

👋 Welcome back jnimeh! 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 · 2020-12-13T19:20:51Z

@jnimeh 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.

mlbridge · 2020-12-13T19:24:00Z

Webrevs

XueleiFan · 2020-12-31T04:28:43Z

src/java.base/share/classes/sun/security/provider/certpath/OCSP.java

+                responderURI.toString());
+
+        URL url;
+        HttpURLConnection con;


I was just wondering, if the new httpclient APIs could be used here? Maybe it is a out of the scope of this update.

It probably could be done, but HttpURLConnection has been working fine so I didn't see the need to change it.

HttpURLConnection run in block mode, which could be a problem for high volume environment. But definitely, it is not the purpose of this update.

XueleiFan · 2020-12-31T04:42:47Z

src/java.base/share/classes/sun/security/provider/certpath/OCSP.java

+        URL url;
+        HttpURLConnection con;
+        String encodedGetReq = responderURI.toString() + "/" +
+                URLEncoder.encode(Base64.getMimeEncoder(0, new byte[0]).


I did not get the point to use MIME encoder. Is the basic base64 encoder sufficient here?

Force of habit. I'm so used to using getMimeEncoder for other structures that need line breaks so I just stuck with it. From the docs it looks like the basic one might work just fine. The encoder returned by Base64.getUrlEncoder() definitely doesn't give me the encoding I want, which is why I went with the MIME encoder and then pushed it through URLEncoder.encode() to get the proper HTTP path string value. I'll try it with the Basic base64 encoder, since that will simplify the code a bit.

XueleiFan · 2020-12-31T04:58:50Z

src/java.base/share/classes/sun/security/provider/certpath/OCSP.java

+                URLEncoder.encode(Base64.getMimeEncoder(0, new byte[0]).
+                        encodeToString(bytes), "UTF-8");
+
+        if (encodedGetReq.length() <= 255) {


If the request less than 256, the GET method will be used. RFC 6960 declare this as a "MAY" feature ("To enable HTTP caching, small requests ... MAY be submitted using GET"). Although RFC 5019 declare it as a "MUST" feature ("When sending requests that are less than or equal to 255 bytes ... clients MUST use the GET method"), but RFC 5109 is released before RFC 6960. I'm not very sure if there is interop issues that a server may not accept the "Get" method for some reasons. I don't worry about it too much, but just for sure you have considered the cases.

I've tried the GET code with various public OCSP responders as well as a few things like OpenSSL's ocsp command (1.1.1d) and Dogtag 10. There is the potential for some compatibility issues, but I think it's pretty small. For instance, OpenSSL's 1.0.2 and earlier's ocsp command running in daemon mode does not accept GET requests at all. But with 1.1.0 and onward, GET is fully supported. I don't think that specifically would be an issue in any large scale deployment - I doubt those large-scale implementations use something like openssl ocsp. There are some clients like the OCSP stapling subsystem in Nginx that will use GETs for small requests also, so I would hope that server-side support would be pretty widespread by now (HTTP GET was even in RFC 2560).

Hm, let's see if there is an interop issue in practice, before we think about a workaround. Thank you!

XueleiFan · 2020-12-31T05:06:02Z

src/java.base/share/classes/sun/security/provider/certpath/OCSP.java

+        if (contentLength == -1) {
+            contentLength = Integer.MAX_VALUE;
+        }
+


Is there a leak if the URL connection is not closed?

From looking at the javadoc for URLConnection, I think the close has to happen on the Input/OutputStreams. I have the OutputStream inside a try-with-resources block. But I think I need to do the same with the InputStream so both are closed at the end of the transaction.

I general consider to use HttpURLConnection.disconnect() and I/O close. Per the HttpURLConnection class specification, "Each HttpURLConnection instance is used to make a single request but the underlying network connection to the HTTP server may be transparently shared by other instances. Calling the close() methods on the InputStream or OutputStream of an HttpURLConnection after a request may free network resources associated with this instance but has no effect on any shared persistent connection. Calling the disconnect() method may close the underlying socket if a persistent connection is otherwise idle at that time.".

In this case I don't think there's a shared persistent connection going on. The URLConnection object's entire lifecycle is within this one method and for one connection only. But I do see your point here. Maybe a try-finally with the disconnect inside the finally might be cleaner instead of try-with-resources around each stream. I'll give that a spin and see what happens.

…e resources

XueleiFan

Looks good to me.

openjdk · 2020-12-31T17:33:40Z

@jnimeh 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:

8179503: Java should support GET OCSP calls

Reviewed-by: xuelei

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 1 new commit pushed to the master branch:

8435f0d: 8258525: Some existing tests should use /nodynamiccopyright/ instead of the standard header

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
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.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

jnimeh · 2020-12-31T18:03:19Z

/integrate

openjdk · 2020-12-31T18:04:17Z

@jnimeh Since your change was applied there has been 1 commit pushed to the master branch:

8435f0d: 8258525: Some existing tests should use /nodynamiccopyright/ instead of the standard header

Your commit was automatically rebased without conflicts.

Pushed as commit f5ee356.

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

jnimeh added 2 commits December 13, 2020 11:06

8179503: Java should support GET OCSP calls

040a428

Merge

32a0987

openjdk bot added the rfr Pull request is ready for review label Dec 13, 2020

openjdk bot added the security security-dev@openjdk.org label Dec 13, 2020

jnimeh added 2 commits December 29, 2020 18:32

Change IO to use readExactlyNBytes method

dc83d10

Merge

d63cb0c

XueleiFan reviewed Dec 31, 2020

View reviewed changes

Use basic Base64 encoder and use HttpUrlConnection.disconnect to clos…

8f0cfeb

…e resources

XueleiFan approved these changes Dec 31, 2020

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Dec 31, 2020

openjdk bot closed this Dec 31, 2020

openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Dec 31, 2020

jnimeh deleted the JDK-8179503 branch January 1, 2021 18:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8179503: Java should support GET OCSP calls #1760

8179503: Java should support GET OCSP calls #1760

jnimeh commented Dec 13, 2020 •

edited by openjdk bot

Loading

bridgekeeper bot commented Dec 13, 2020

openjdk bot commented Dec 13, 2020

mlbridge bot commented Dec 13, 2020 •

edited

Loading

XueleiFan Dec 31, 2020

jnimeh Dec 31, 2020

XueleiFan Dec 31, 2020

XueleiFan Dec 31, 2020

jnimeh Dec 31, 2020

XueleiFan Dec 31, 2020

jnimeh Dec 31, 2020

XueleiFan Dec 31, 2020

XueleiFan Dec 31, 2020

jnimeh Dec 31, 2020

XueleiFan Dec 31, 2020

jnimeh Dec 31, 2020

XueleiFan left a comment

openjdk bot commented Dec 31, 2020

jnimeh commented Dec 31, 2020

openjdk bot commented Dec 31, 2020

8179503: Java should support GET OCSP calls #1760

8179503: Java should support GET OCSP calls #1760

Conversation

jnimeh commented Dec 13, 2020 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Download

bridgekeeper bot commented Dec 13, 2020

openjdk bot commented Dec 13, 2020

mlbridge bot commented Dec 13, 2020 • edited Loading

Webrevs

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

XueleiFan left a comment

Choose a reason for hiding this comment

openjdk bot commented Dec 31, 2020

jnimeh commented Dec 31, 2020

openjdk bot commented Dec 31, 2020

jnimeh commented Dec 13, 2020 •

edited by openjdk bot

Loading

mlbridge bot commented Dec 13, 2020 •

edited

Loading