Thanks for the review Max. All edits made bar the
"Event.clearReportListener(Event.ReporterCategory.POSIXPERMS);"
suggested edit. That's already in a finally block.
latest webrev:
https://cr.openjdk.java.net/~coffeys/webrev.8218021.v5/webrev/
I plan to push once I have a clean test run.
regards,
Sean.
On 01/07/2020 03:02, Weijun Wang wrote:
---------------------
src/java.base/share/classes/sun/security/provider/certpath/DistributionPointFetcher.java
@@ -248,7 +248,7 @@ private static X509CRL getCRL(URIName name) throws
CertStoreException {
debug.println("Trying to fetch CRL from DP " + uri);
}
Event.report("event.crl.check", uri.toString());
Event.report(Event.ReporterCategory.CRLCHECK,"event.crl.check",
uri.toString());
Please add a whitespace after CRLCHECK.
---------------------
src/java.base/share/classes/sun/security/provider/certpath/OCSP.java
@@ -234,7 +234,7 @@ static OCSPResponse check(List<CertId> certIds, URI
responderURI,
debug.println("connecting to OCSP service at: " + url);
}
Event.report("event.ocsp.check", url.toString());
Event.report(Event.ReporterCategory.CRLCHECK,"event.ocsp.check",
url.toString());
White space after CRLCHECK.
---------------------
src/jdk.jartool/share/classes/jdk/security/jarsigner/JarSigner.java
int perms = JUZFA.getPosixPerms(ze);
if (!posixPermsDetected && perms != -1) {
posixPermsDetected = true;
Event.report(Event.ReporterCategory.POSIXPERMS, "true");
The method signature is "report(category, type, values...)". There is no need to define a type or value here but I feel like more normal to call 'report(POSIXPERMS, "detected")' because the 2nd argument is type instead of value.
---------------------
src/jdk.jartool/share/classes/sun/security/tools/jarsigner/Main.java
@@ -1054,7 +1063,7 @@ private void displayMessagesAndResult(boolean isSigning) {
notYetValidCert || chainNotValidated || hasExpiredCert ||
hasUnsignedEntry || signerSelfSigned || (legacyAlg != 0) ||
(disabledAlg != 0) || aliasNotInStore || notSignedByAlias ||
tsaChainNotValidated || permsDetected ||
I'd prefer move the permsDetected check and the if block for it out of this big block, since it's not a fatal warning and just informational. You can move it into "if (hasExpiringCert....".
---------------------
src/jdk.jartool/share/classes/sun/security/tools/jarsigner/Main.java
@@ -1805,6 +1820,7 @@ void signJar(String jarName, String alias)
fos.close();
}
Event.clearReportListener(Event.ReporterCategory.POSIXPERMS);
Maybe in a final block?
---------------------
test/jdk/sun/security/util/Resources/Usages.java
@@ -75,7 +75,7 @@
"(?m)rb[ \\n]*\\.getString[ \\n]*\\([ \\n]*\"(.*?)\"\\)");
static Pattern EVENT_OCSP_CRL = Pattern.compile(
"Event\\.report\\(.*,\"(.*?)\",");
If you agree to add a whitespace in the calls in OSCP.java and DistributionFetcher.java, you might want to add a whitespace (or "\s*") here as well.
Thanks,
Max
On Jun 30, 2020, at 9:51 PM, Seán Coffey <sean.cof...@oracle.com> wrote:
Thanks Lance.
During the CSR review, a suggestion was made to have jarsigner preserve such
attributes by default. Warnings about these attributes will also be added
during signing and verify operations (if detected).
webrev: https://cr.openjdk.java.net/~coffeys/webrev.8218021.v4/webrev/
regards,
Sean.
On 22/06/2020 17:17, Lance Andersen wrote:
HI Sean,
Looks OK based on our exchanges. Thank you for your time on this one!
Best
Lance
On Jun 22, 2020, at 7:22 AM, Seán Coffey <sean.cof...@oracle.com> wrote:
Thanks Lance.
I've updated the patch with some extra offline feedback from yourself and Max.
A new warning is printed with use of the new flag. A warning is also printed
when file posix permissions are detected on resources being signed. Test
updated for that also.
https://cr.openjdk.java.net/~coffeys/webrev.8218021.v3/webrev/
regards,
Sean.
On 12/06/2020 17:05, Lance Andersen wrote:
Hi Sean,
I think your changes look fine so all good FMPOV.
Best
Lance
On Jun 12, 2020, at 6:21 AM, Seán Coffey <sean.cof...@oracle.com> wrote:
Hi,
I'd like to reboot this jarsigner enhancement request[1]. I've removed the
problem references to zip file name extensions. Instead, there's a new JDK
implementation specific jarsigner option: -keepposixperms
https://bugs.openjdk.java.net/browse/JDK-8218021
https://cr.openjdk.java.net/~coffeys/webrev.8218021.v2/webrev/
regards,
Sean.
[1] http://mail.openjdk.java.net/pipermail/security-dev/2020-January/021141.html
<oracle_sig_logo.gif>
Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering
1 Network Drive
Burlington, MA 01803
lance.ander...@oracle.com
<oracle_sig_logo.gif>
Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering
1 Network Drive
Burlington, MA 01803
lance.ander...@oracle.com
