RFR: 8160655 Fix denyAfter and usage types for security properties

Sean Mullan sean.mullan at oracle.com
Wed Feb 8 17:50:53 UTC 2017


The update looks good.

--Sean

On 2/7/17 8:09 PM, Anthony Scarpino wrote:
> I believe all comments are addressed in the below link
>
> http://cr.openjdk.java.net/~ascarpino/8160655/webrev.02/
>
> Everything I didn't comment on inline below was because I hadn't posted
> an update-to-date webrev.01 at that time.
>
> Tony
>
> On 02/06/2017 12:17 PM, Sean Mullan wrote:
>> Hi Tony,
>>
>> Here are my comments on the latest webrev:
>>
>> * SignerInfo.java
>>
>>  355                 try {
>>  356                     JAR_DISABLED_CHECK.permits(digestAlgname,
>> cparams);
>>  357                 } catch (CertPathValidatorException e) {
>>  358                     throw new SignatureException(e.getMessage(), e);
>>  359                 }
>>
>> It's a little odd this throws a CPVE because there is no certificate
>> being checked here.
>
> In offline discussions this will have to remain for now.
>
>
>>
>> * AlgorithmChecker.java
>>
>> 278         String currSigAlg = ((X509Certificate)cert).getSigAlgName();
>>
>> Just use x509cert.getSigAlgName() instead.
>>
>> * SignatureFileVerifier.java
>>
>> 145         System.err.println("key = " + name + " == name " + name);
>>
>> leftover debug?
>>
>>  265     // Algorithms that have been checked if they are weak.
>>  266     private Map<String, Boolean> permittedAlgs= new HashMap<>();
>>  267     // TSA timestamp of signed jar.  The newest timestamp is used.
>>  If there was
>>  268     // no TSA timestamp used when signed, current time is used
>> ("null").
>>  269     private Timestamp timestamp = null;
>>
>> can you move these lines up to the top of the class where other
>> variables are declared?
>>
>>  // Algorithms that have been checked if they are weak.
>>  266     private Map<String, Boolean> permittedAlgs= new HashMap<>();
>>
>> It might be better to have a Map<String, List<String>> where the key is
>> the header (*-DIGEST*) and the value is a list of weak algs. This way
>> you could avoid the need for the getWeakAlgorithms method and just get
>> the weak list of algs out of the map directly. An empty list or null
>> value would indicate the header is ok.
>>
>> 302         /* Look for the latest timestamp in the signature block */
>>
>> s/the latest timestamp/the latest timestamp or no timestamp/
>
> I changed the comment in a way that I think is more descriptive.
>
>>
>> * java.security
>>
>> Missing spaces between words on lines 646-648 (ex: algorithmin)
>>
>> * Main.java
>>
>> 640     void  rey verifyJar(String jarName)
>>
>> typo?
>>
>> * PKIXExtendedParameters.java
>>
>> Update class description to reflect this contains more than a timestamp.
>>
>> --Sean
>>
>> On 1/30/17 6:57 PM, Anthony Scarpino wrote:
>>> On 01/23/2017 03:27 PM, Anthony Scarpino wrote:
>>>> Hi,
>>>>
>>>> I need a code review of this change that brings more detail constraints
>>>> checking and control to certpath and jar disabled algorithm Security
>>>> properties.
>>>>
>>>> http://cr.openjdk.java.net/~ascarpino/8160655/webrev/
>>>>
>>>> thanks
>>>>
>>>> Tony
>>>
>>> Updated review
>>>
>>> http://cr.openjdk.java.net/~ascarpino/8160655/webrev.01/
>>>
>>> It address the comments and has a different SignatureFileVerifier.java
>>>
>>> Tony
>>>
>



More information about the security-dev mailing list