RFR: 8218021: Have jarsigner preserve posix permission attributes

[Seán Coffey]

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.coffey at 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.coffey at 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.coffey at 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.Andersen at 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.Andersen at oracle.com
>>>
>>>
>>>