[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-security-dev
Subject:    Re: RFR 8233222: KeyTab.getInstance().exists() has unspecified side effects preventing KerberosPrinc
From:       Sean Mullan <sean.mullan () oracle ! com>
Date:       2019-11-21 21:29:46
Message-ID: 615f6401-1fa9-4b16-b6a9-4ba53f622374 () oracle ! com
[Download RAW message or body]

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
[prev in list] [next in list] [prev in thread] [next in thread]