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.

Sign up for GitHub

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

8179502: Enhance OCSP, CRL and Certificate Fetch Timeouts #2747

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

8179502: Enhance OCSP, CRL and Certificate Fetch Timeouts #2747

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
df460a6
Backport 2836c34b64e4626e25c86a53e5bef2bf32f95d2e
alexeybakhtin Jul 28, 2024
e73f8c1
Change default readtimeout to CONNECT_TIMEOUT
alexeybakhtin Nov 12, 2024
65e02f9
Remove unused DEFAULT_READ_TIMEOUT constant
alexeybakhtin Nov 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
64 changes: 63 additions & 1 deletion src/java.base/share/classes/sun/security/action/GetPropertyAction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 1998, 2021, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 1998, 2023, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
Expand Down Expand Up @@ -162,6 +162,68 @@ public Properties run() {
}
}

/**
* Convenience method for fetching System property values that are timeouts.
* Accepted timeout values may be purely numeric, a numeric value
* followed by "s" (both interpreted as seconds), or a numeric value
* followed by "ms" (interpreted as milliseconds).
*
* @param prop the name of the System property
* @param def a default value (in milliseconds)
* @param dbg a Debug object, if null no debug messages will be sent
*
* @return an integer value corresponding to the timeout value in the System
* property in milliseconds. If the property value is empty, negative,
* or contains non-numeric characters (besides a trailing "s" or "ms")
* then the default value will be returned. If a negative value for
* the "def" parameter is supplied, zero will be returned if the
* property's value does not conform to the allowed syntax.
*/
public static int privilegedGetTimeoutProp(String prop, int def, Debug dbg) {
if (def < 0) {
def = 0;
}

String rawPropVal = privilegedGetProperty(prop, "").trim();
if (rawPropVal.length() == 0) {
return def;
}

// Determine if "ms" or just "s" is on the end of the string.
// We may do a little surgery on the value so we'll retain
// the original value in rawPropVal for debug messages.
boolean isMillis = false;
String propVal = rawPropVal;
if (rawPropVal.toLowerCase().endsWith("ms")) {
propVal = rawPropVal.substring(0, rawPropVal.length() - 2);
isMillis = true;
} else if (rawPropVal.toLowerCase().endsWith("s")) {
propVal = rawPropVal.substring(0, rawPropVal.length() - 1);
}

// Next check to make sure the string is built only from digits
if (propVal.matches("^\\d+$")) {
try {
int timeout = Integer.parseInt(propVal);
return isMillis ? timeout : timeout * 1000;
} catch (NumberFormatException nfe) {
if (dbg != null) {
dbg.println("Warning: Unexpected " + nfe +
" for timeout value " + rawPropVal +
". Using default value of " + def + " msec.");
}
return def;
}
} else {
if (dbg != null) {
dbg.println("Warning: Incorrect syntax for timeout value " +
rawPropVal + ". Using default value of " + def +
" msec.");
}
return def;
}
}

/**
* Convenience method for fetching System property values that are booleans.
*
Expand Down
46 changes: 27 additions & 19 deletions src/java.base/share/classes/sun/security/provider/certpath/OCSP.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,10 +26,7 @@

import java.io.IOException;
import java.io.OutputStream;
import java.net.URI;
import java.net.URL;
import java.net.HttpURLConnection;
import java.net.URLEncoder;
import java.net.*;
import java.security.cert.CertificateException;
import java.security.cert.CertPathValidatorException;
import java.security.cert.CertPathValidatorException.BasicReason;
Expand Down Expand Up @@ -72,13 +69,23 @@ public final class OCSP {
private static final Debug debug = Debug.getInstance("certpath");

private static final int DEFAULT_CONNECT_TIMEOUT = 15000;
private static final int DEFAULT_READ_TIMEOUT = 15000;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @alexeybakhtin and @GoeLin,

After e73f8c1, DEFAULT_READ_TIMEOUT is now unused and can be removed.

Apparently, this backward-compatibility problem went undetected in the original change. Please also note that the behavior is no longer aligned with the CSR (emphasis by me):

For all properties, existing and new, the proposed expanded syntax will conform to the following:

  • […]
  • As with the current behavior, non-numeric, non-decimal (e.g. hexadecimal values prepended by "0x", etc) values will be interpreted as illegal and will default to the 15 second timeout. The same is true for negative values.

com.sun.security.ocsp.readtimeout is now an exceptional case, it won't default to the 15 seconds timeout, but to com.sun.security.ocsp.timeout (which defaults to 15 seconds).

Should the CSR be adjusted? Otherwise, how do we ensure Oracle's backport behaves the same way? I don't have enough backporting experience, is it usually accepted to diverge from newer releases (≥ 21u) in a backport CSR?

Copy link
Author

@alexeybakhtin alexeybakhtin Nov 13, 2024 •
edited by bridgekeeper bot
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @franferrax
Thank you for the finding. I'll remove DEFAULT_READ_TIMEOUT
You are right, the CSR for update releases should be updated for com.sun.security.ocsp.readtimeout default value. I'll do it and ask you to review it from an engineering point of view.
I hope in this way, without backward compatibility, the patch will be accepted.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will take a look, but please note I'm not a Reviewer.

One more comment, I see that the following issues are not part of the backport. Although all of them are test-only changes (mostly tuning the timeout), we might also want to include these to avoid some instability.

The last one is an ongoing effort to try to fix JDK-8309754, so we could alternatively wait for that fix and then make all the 5 backports together.

Copy link
Author

@alexeybakhtin alexeybakhtin Nov 18, 2024 •
edited by bridgekeeper bot
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Backport PRs are created for test fixes


/**
* Integer value indicating the timeout length, in seconds, to be
* used for the OCSP check. A timeout of zero is interpreted as
* an infinite timeout.
* Integer value indicating the timeout length, in milliseconds, to be
* used for establishing a connection to an OCSP responder. A timeout of
* zero is interpreted as an infinite timeout.
*/
private static final int CONNECT_TIMEOUT = initializeTimeout();
private static final int CONNECT_TIMEOUT = initializeTimeout(
"com.sun.security.ocsp.timeout", DEFAULT_CONNECT_TIMEOUT);

/**
* Integer value indicating the timeout length, in milliseconds, to be
* used for reading an OCSP response from the responder. A timeout of
* zero is interpreted as an infinite timeout.
*/
private static final int READ_TIMEOUT = initializeTimeout(
"com.sun.security.ocsp.readtimeout", DEFAULT_READ_TIMEOUT);

/**
* Boolean value indicating whether OCSP client can use GET for OCSP
Expand Down Expand Up @@ -107,16 +114,13 @@ public final class OCSP {
* system property. If the property has not been set, or if its
* value is negative, set the timeout length to the default.
*/
private static int initializeTimeout() {
@SuppressWarnings("removal")
Integer tmp = java.security.AccessController.doPrivileged(
new GetIntegerAction("com.sun.security.ocsp.timeout"));
if (tmp == null || tmp < 0) {
return DEFAULT_CONNECT_TIMEOUT;
private static int initializeTimeout(String prop, int def) {
int timeoutVal =
GetPropertyAction.privilegedGetTimeoutProp(prop, def, debug);
if (debug != null) {
debug.println(prop + " set to " + timeoutVal + " milliseconds");
}
// Convert to milliseconds, as the system property will be
// specified in seconds
return tmp * 1000;
return timeoutVal;
}

private static boolean initializeBoolean(String prop, boolean def) {
Expand Down Expand Up @@ -277,16 +281,18 @@ public static byte[] getOCSPBytes(List<CertId> certIds, URI responderURI,
Base64.getEncoder().encodeToString(bytes), "UTF-8"));

if (USE_GET && encodedGetReq.length() <= 255) {
url = new URL(encodedGetReq.toString());
url = new URI(encodedGetReq.toString()).toURL();
con = (HttpURLConnection)url.openConnection();
con.setConnectTimeout(CONNECT_TIMEOUT);
con.setReadTimeout(READ_TIMEOUT);
con.setDoOutput(true);
con.setDoInput(true);
con.setRequestMethod("GET");
} else {
url = responderURI.toURL();
con = (HttpURLConnection)url.openConnection();
con.setConnectTimeout(CONNECT_TIMEOUT);
con.setReadTimeout(CONNECT_TIMEOUT);
con.setReadTimeout(READ_TIMEOUT);
con.setDoOutput(true);
con.setDoInput(true);
con.setRequestMethod("POST");
Expand Down Expand Up @@ -316,6 +322,8 @@ public static byte[] getOCSPBytes(List<CertId> certIds, URI responderURI,
return (contentLength == -1) ? con.getInputStream().readAllBytes() :
IOUtils.readExactlyNBytes(con.getInputStream(),
contentLength);
} catch (URISyntaxException urise) {
throw new IOException(urise);
} finally {
if (con != null) {
con.disconnect();
Expand Down
45 changes: 33 additions & 12 deletions src/java.base/share/classes/sun/security/provider/certpath/URICertStore.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2006, 2020, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2006, 2023, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
Expand Down Expand Up @@ -50,7 +50,8 @@
import java.util.Collections;
import java.util.List;
import java.util.Locale;
import sun.security.action.GetIntegerAction;

import sun.security.action.GetPropertyAction;
import sun.security.x509.AccessDescription;
import sun.security.x509.GeneralNameInterface;
import sun.security.x509.URIName;
Expand Down Expand Up @@ -127,8 +128,12 @@ class URICertStore extends CertStoreSpi {
// allowed when downloading CRLs
private static final int DEFAULT_CRL_READ_TIMEOUT = 15000;

// Default connect and read timeouts for CA certificate fetching (15 sec)
private static final int DEFAULT_CACERT_CONNECT_TIMEOUT = 15000;
private static final int DEFAULT_CACERT_READ_TIMEOUT = 15000;

/**
* Integer value indicating the connect timeout, in seconds, to be
* Integer value indicating the connect timeout, in milliseconds, to be
* used for the CRL download. A timeout of zero is interpreted as
* an infinite timeout.
*/
Expand All @@ -137,30 +142,44 @@ class URICertStore extends CertStoreSpi {
DEFAULT_CRL_CONNECT_TIMEOUT);

/**
* Integer value indicating the read timeout, in seconds, to be
* Integer value indicating the read timeout, in milliseconds, to be
* used for the CRL download. A timeout of zero is interpreted as
* an infinite timeout.
*/
private static final int CRL_READ_TIMEOUT =
initializeTimeout("com.sun.security.crl.readtimeout",
DEFAULT_CRL_READ_TIMEOUT);

/**
* Integer value indicating the connect timeout, in milliseconds, to be
* used for the CA certificate download. A timeout of zero is interpreted
* as an infinite timeout.
*/
private static final int CACERT_CONNECT_TIMEOUT =
initializeTimeout("com.sun.security.cert.timeout",
DEFAULT_CACERT_CONNECT_TIMEOUT);

/**
* Integer value indicating the read timeout, in milliseconds, to be
* used for the CA certificate download. A timeout of zero is interpreted
* as an infinite timeout.
*/
private static final int CACERT_READ_TIMEOUT =
initializeTimeout("com.sun.security.cert.readtimeout",
DEFAULT_CACERT_READ_TIMEOUT);

/**
* Initialize the timeout length by getting the specified CRL timeout
* system property. If the property has not been set, or if its
* value is negative, set the timeout length to the specified default.
*/
private static int initializeTimeout(String prop, int def) {
Integer tmp = GetIntegerAction.privilegedGetProperty(prop);
if (tmp == null || tmp < 0) {
return def;
}
int timeoutVal =
GetPropertyAction.privilegedGetTimeoutProp(prop, def, debug);
if (debug != null) {
debug.println(prop + " set to " + tmp + " seconds");
debug.println(prop + " set to " + timeoutVal + " milliseconds");
}
// Convert to milliseconds, as the system property will be
// specified in seconds
return tmp * 1000;
return timeoutVal;
}

/**
Expand Down Expand Up @@ -276,6 +295,8 @@ static CertStore getInstance(AccessDescription ad) {
connection.setIfModifiedSince(lastModified);
}
long oldLastModified = lastModified;
connection.setConnectTimeout(CACERT_CONNECT_TIMEOUT);
connection.setReadTimeout(CACERT_READ_TIMEOUT);
try (InputStream in = connection.getInputStream()) {
lastModified = connection.getLastModified();
if (oldLastModified != 0) {
Expand Down
Loading