RFR: 8061842: Package jurisdiction policy files as something other than JAR

[Sean Coffey]

Looks good Brad. One comment :

You might hit an NPE here but I guess the Exception handling in the 
parent call would handle it :

JceSecurity.java :
  254         if (Paths.get(cryptoPolicyProperty).getNameCount() != 1) {

Maybe add a null check to be cleaner ?

I missed the "crypto.policy=crypto.policydir-tbd" approach in 1st 
revision. Isn't it going to confuse people studying the code ? This 
value gets replaced at build time - right ? What about changing it to 
something like :
"crypto.policy=$crypto.policydir-tbd //replaced at build time"

and having the make files search for that string instead ? I think it 
makes the intent more obvious.

Regards,
Sean.

On 25/08/2016 01:21, Bradford Wetmore wrote:
> Max/SeanC/SeanM,
>
> The latest update:
>
>     http://cr.openjdk.java.net/~wetmore/8061842/webrev.02/
>
> On 8/17/2016 9:26 PM, Wang Weijun wrote:
>> Before this change, you require default policy in neither export nor 
>> import to be empty but do not care about the getMinimum() result. In 
>> this change, you make sure the final result is not empty. I assume 
>> this is a fix?
>
> I made the change to allow for our traditional (default) export/import 
> mechanism, but other additional styles could be added/used.  Since we 
> no longer sign, distros are free to edit, add and/or remove files.  
> But before doing any JCE operation, the environment needs to grant 
> something, otherwise there are no perms and no JCE available.
>
>> 283                     // Did we find a default perms?
>>
>> What does this line mean?
>
> I've moved to the right position in the file.  I meant did we find a 
> default perms file, vs an exempt.
>
>> 296                         // This should never happen
>>
>> But you can still print out the file name.
>
> I'm concerned that the exception might print out the entire path 
> instead of just the filename, which would include java.home and 
> probably should not be made available.
>
>> Can you rename policydir-tbd to something else? I am afraid it will 
>> be confused with policy.url.1 etc.
>
> Changed to:  crypto.policydir-tbd?
>
>> The original README.TXT in unlimited says "are exportable from the 
>> United States." and you have "is exportable." now. Is this intended? 
>> (IANAL)
>
> Changed.
>
>> TestUnlimited.java:
>> 45 "// Use the AES are the test Cipher", you mean "Use AES as the 
>> test Cipher"?
>> 51 "throw new Exception ("Unlimited policy is NOT active");". No 
>> space before "(".
>
> Fixed.
>
> Sean Mullan wrote:
>
>  > What about setting the default value to "limited"? And then this
>  > would only be changed to "unlimited" if the build --enable-unlimited-
>  > crypto option is specified?
>
> I could, but I'm concerned that a build with 
> --enabled-unlimited-crypto would expect that the compiled-in version 
> default would also be unlimited and would be surprised with limited.
>
> Upon Max's suggestion above, I've changed the name of the marker to 
> "crypto.policy=crypto.policydir-tbd."  Does that work for you?
>
>  > Instead of throwing an exception here, I wonder if it would make more
>  > sense to assume a default value of "limited" if the property is not
>  > set or is empty.
>
> We could, but see above.
>
> Sean Coffey wrote:
>
> > Please include the exception 'e' in your last exception here.
>
> Again, I'm concerned about outputting java.home, so I'm just going to 
> output the final directory name.
>
> > 3. Test case.
> >
> > The TestUnlimited.java testcase seems to be lacking. Do you want to
> > test other values for crypto.policy ? 'limited' would be one.
> > Throwing in some dummy value would also be good so that the exception
> > handling code gets exercised.
>
> Done.
>
>  * @run main/othervm TestUnlimited limited fail
>  * @run main/othervm TestUnlimited unlimited pass
>  * @run main/othervm TestUnlimited NosuchDir exception
>  * @run main/othervm TestUnlimited . exception
>  * @run main/othervm TestUnlimited /tmp/unlimited exception
>  * @run main/othervm TestUnlimited ../policy/unlimited exception
>  * @run main/othervm TestUnlimited ./unlimited exception
>
> > It needs to be run in ovm mode since you're setting a Security
> > property.
>
> Yes, good catch.
>
> Brad
>