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

[Bradford Wetmore]

Last call:

    http://cr.openjdk.java.net/~wetmore/8061842/webrev.03/
Changes from .02:

added null check for cryptoPolicyProperty
renamed javaHomePolicyPath
tweaked exception wording "files in"
added test for empty string ("").


On 8/24/2016 6:22 PM, Weijun Wang wrote:
+        Path javaHomePath = Paths.get(javaHomeProperty, "conf",
"security",
+                "policy").normalize();

This is not javaHomePath, but policyPath.

Renamed as javaHomePolicyPath.

You added "cryptoPolicyProperty" to some exceptions, but the exception
titles are "Unexpected jurisdiction policy filename found: " and
"Couldn't parse jurisdiction policy files: ". cryptoPolicyProperty is
not file(s). Maybe "... file(s) in " + cryptoPolicyProperty?

Sure.  Sounds good.

Tony wrote:
> So by having no crypto.policy defined we have no JCA?

With no crypto.policy, that shuts down the JceSecurity init, and the app 
will exit with an ExceptionInInitializerError, like it does if the 
policy files are missing today.

Thanks Sean for pointing out my new NPE I just introduced! ("Maybe add a 
null check to be cleaner?")  Drat!  I would add a new test case, but 
Security.setProperty() doesn't allow for "null."

> Does that mean no
> operations at all (No MessageDigest, etc) or no restrictable crypto
> ops?

Recall that our JCA/JCE Permissions model is based on positive 
permission model checks.  It would mean no CryptoPermissions would be 
created, thus any operation that needs a CryptoPermissions (implies()) 
would fail.  So if you were somehow able to get the system to have no 
policy, there would be no Permissions granted to do any JCE operations.


As an aside, it would only fail JCE (Cipher/Mac/KeyAgreement), JCA would 
be unaffected as it doesn't use permissions.

> Since we know a limited number of countries have import issues, can we
> make no crypto.policy property defined as unlimited policy?  Defining
> the property would be for only limiting the access.  We could get rid of
> the unlimited policy file and just ship a limited policy file.

I'm very hesitant to do that, we need to be limited by default.

On 8/25/2016 6:56 AM, Sean Coffey wrote:
> 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 ?

Yes.

> 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.

We're doing the same thing with "tbd" during the provider slot 
assignment. I'd like to keep it somewhat similar.

Brad



No other comment.

Thanks
Max

On 8/25/2016 8: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