Re: RFR 8233222: Clarify system property usage in KerberosPrincipal instantiation

[Sean Mullan]

Looks good.

--Sean

On 11/21/19 8:27 PM, Weijun Wang wrote:

I rewrite the change into

+     * <p>Note that when this class or any other Kerberos-related class is
+     * initially loaded and initialized, it may read and cache the default
+     * realm from the Kerberos configuration file or via the
+     * java.security.krb5.realm system property (the value will be empty if
+     * no default realm is specified), such that any subsequent calls to set
+     * or change the default realm by setting the java.security.krb5.realm
+     * system property may be ignored.

I kept the "via" word because it's used in all other places. The "the value will be 
empty" description might be still not precise but IMO it should be clear.

I'll dup the same paragraph in the other constructor.

Here is the webrev:

    https://cr.openjdk.java.net/~weijun/8233222/webrev.00

Thanks,
Max

On Nov 22, 2019, at 5:29 AM, Sean Mullan <sean.mul...@oracle.com> wrote:

On 11/14/19 10:41 PM, Weijun Wang wrote:

Please review the change below:
*diff --git 
a/src/java.security.jgss/share/classes/javax/security/auth/kerberos/KerberosPrincipal.java
 
b/src/java.security.jgss/share/classes/javax/security/auth/kerberos/KerberosPrincipal.java*
*--- 
a/src/java.security.jgss/share/classes/javax/security/auth/kerberos/KerberosPrincipal.java*
*+++ 
b/src/java.security.jgss/share/classes/javax/security/auth/kerberos/KerberosPrincipal.java*
@@ -106,10 +106,19 @@
       *
       * <p>If the input name does not contain a realm, the default realm
       * is used. The default realm can be specified either in a Kerberos
-     * configuration file or via the java.security.krb5.realm
+     * configuration file or via the {@systemproperty java.security.krb5.realm}
       * system property. For more information, see the
       * {@extLink security_guide_jgss_tutorial Kerberos Requirements}.
-     * Additionally, if a security manager is
+     *
+     * <p>Please note that when this class or any other Kerberos-related class
+     * is initially loaded and initialized, it might read the default realm
+     * from the Kerberos configuration file or via the
+     * java.security.krb5.realm system property. The default realm is cached
+     * (even if there is none) and any calls to subsequently set or change
+     * the default realm by setting the java.security.krb5.realm system
+     * property might be ignored.

I would probably remove "Please" and use "may" instead of "might". I also think you can 
remove "via". The last sentence sounds like caching is a requirement, so maybe reword as:

"Note that when this class or any other Kerberos-related class is initially loaded 
and initialized, it may read and cache the default realm (even if there is none) from the 
Kerberos configuration file or the java.security.krb5.realm system property, such that 
any subsequent calls to set or change the default realm by setting the 
java.security.krb5.realm system property may be ignored."

A user may not know what you mean by "even if there is none" so I think you may 
need to explain that in more detail, perhaps in an additional sentence.

+     *
+     * <p>Additionally, if a security manager is
       * installed, a {@link ServicePermission} must be granted and the service
       * principal of the permission must minimally be inside the
       * {@code KerberosPrincipal}'s realm. For example, if the result of
Here, the "Kerberos-related" class could be KeyTab as shown in this bug or 
something else like JAAS login configured with a Krb5LoginModule, or a JGSS call that 
touched the Kerberos 5 mechanism.
I used several "might" because this is just a hint but not a specified behavior 
and I don't want to restrict any evolution of the underlying implementation. For the same 
reason there is no test, although it's quite easy to trigger such a case. I've added a 
noreg-doc label.
Also, I assume there is no need for a CSR. This is not about compatibility or 
any new specification.

I would probably double-check with Joe. This could be considered a bit more 
than just a specification clarification.

Also, I assume you would make the same change to both of the KerberosPrincipal 
ctors?

Lastly, I think you should change the title of the bug to better reflect what fix you are 
proposing, maybe "Clarify KerberosPrincipal behavior when java.security.krb5 system 
property is set or changed."

--Sean