code review request: 7089889: Krb5LoginModule.login() throws an exception if used without a keytab

[Xuelei Fan]

Looks fine to me.

Xuelei

On Sep 21, 2011, at 3:28 PM, Weijun Wang <weijun.wang at oracle.com> wrote:

> Webrev updated:
> 
>   http://cr.openjdk.java.net/~weijun/7089889/webrev.01/
> 
> Thanks
> Max
> 
> 
> On 09/21/2011 12:28 PM, Xuelei Fan wrote:
>> It's OK.
>> 
>> Xuelei
>> 
>> On 9/21/2011 12:26 PM, Weijun Wang wrote:
>>> 
>>> 
>>> On 09/21/2011 11:52 AM, Xuelei Fan wrote:
>>>> On 9/20/2011 1:38 PM, Weijun Wang wrote:
>>>>> Any more comment?
>>>>> 
>>>>> Thanks
>>>>> Max
>>>>> 
>>>>> On 09/16/2011 01:57 PM, Weijun Wang wrote:
>>>>>> 
>>>>>> 
>>>>>> On 09/16/2011 01:45 PM, Xuelei Fan wrote:
>>>>>>> My first impression about the fix:
>>>>>>> 1. Do you want to update getKeys() comments about isInitiator value?
>>>>>> 
>>>>>> OK. I'll simply remove the "(in JAAS words, isInitiator=true and
>>>>>> storeKey=true)" line.
>>>>>> 
>>>> Do you also need to remove "This is used when the client supplies
>>>> passowrd but need ...". I think it is the same as "in JAAS words, ...".
>>> 
>>> But that's where it's called. Maybe I should say more:
>>> 
>>>    This is used when the client supplies password but need keys
>>>    to act as an acceptor. It can be called when the state is INIT
>>>    and returned keys are generated with default salt, or, after
>>>    AS-REQ is performed, might be updated by KDC.
>>> 
>>>> 
>>>>>>> 
>>>>>>> 2. It seems that when isInitiator is true, getKeys() should throw
>>>>>>> exception if the state is not OK, right? If so, it looks like that the
>>>>>>> update of getKeys() will not throw ISE if the state is INIT. Does INIT
>>>>>>> state is enough in such case?
>>>>>> 
>>>>>> The KrbAsReqBuilder class does not know about isInitiator so it cannot
>>>>>> make any decision based on it. I'll leave that to its caller. When
>>>>>> username/password is provided, getKeys() can be called as soon as the
>>>>>> object is created, and it returns keys with default salt. If AS-REQ is
>>>>>> performed and state goes OK and the method is called again, the salt
>>>>>> might be updated in PA-DATA.
>>>>>> 
>>>> I'm not sure whether the getKeys() return the expected keys when the
>>>> state is INIT. It should be OK as it is an internal class, but as
>>>> requires that the caller always looks like:
>>>> 
>>>>      if (isInitiator) {
>>>>         // action to update the state to OK
>>>>      }
>>>>      encKeys = builder.getKeys();
>>> 
>>> Correct. In Krb5LoginModule, you can see
>>> 
>>>     if (isInitiator) {
>>>         // XXX Even if isInitiator=false, it might be
>>>         // better to do an AS-REQ so that keys can be
>>>         // updated with PA info
>>>         cred = builder.action().getCreds();
>>>     }
>>>     if (storeKey) {
>>>         encKeys = builder.getKeys();
>>>         // When encKeys is empty, the login actually fails.
>>>         // For compatibility, exception is thrown in commit().
>>>     }
>>> 
>>> This is the only place where this method is called.
>>> 
>>>> 
>>>> Is getKeys() only be called by Krb5LoginModule.java? If not, I think we
>>>> may need to rethink about the change of the behaviors. Otherwise, it is
>>>> OK. However, is it a little simpler to update builder.getKeys() to
>>>> builder.getKeys(boolean isInitiator)?
>>> 
>>> Yes. But I won't add automatic AS-REQ inside it. The argument will only
>>> be used for state check. Is that OK?
>>> 
>>> Thanks
>>> Max
>>> 
>>> 
>>>> 
>>>> Xuelei
>>>> 
>>>>>> Thanks
>>>>>> Max
>>>>>> 
>>>>>> 
>>>>>>> 
>>>>>>> Xuelei
>>>>>>> 
>>>>>>> On Sep 16, 2011, at 12:00 PM, Weijun Wang<weijun.wang at oracle.com>
>>>>>>> wrote:
>>>>>>> 
>>>>>>>> Hi All
>>>>>>>> 
>>>>>>>> Code changes at
>>>>>>>> 
>>>>>>>> http://cr.openjdk.java.net/~weijun/7089889/webrev.00/
>>>>>>>> 
>>>>>>>> KrbAsReqBuilder maintains a state enforcement: at the beginning it's
>>>>>>>> INIT, after doing an AS-REQ it's OK, and we used to only allow
>>>>>>>> getKeys() to be called when the state is OK. Now if an acceptor has
>>>>>>>> isInitiator=false, the OK state will never be reached. In most cases,
>>>>>>>> an acceptor uses a keytab so getKeys() is not called. However, if it
>>>>>>>> uses username/password (most likely in a peer-to-peer case and the
>>>>>>>> acceptor chooses not to login), getKeys() is needed and an exception
>>>>>>>> is thrown.
>>>>>>>> 
>>>>>>>> This code change makes getKeys() callable at both INIT and OK states.
>>>>>>>> In order to do this, the checkState() method now accepts varargs.
>>>>>>>> 
>>>>>>>> I'll backport this to 7u2, and since 7u2 is now in phase 2, I think I
>>>>>>>> need at least 2 reviewers.
>>>>>>>> 
>>>>>>>> Thanks
>>>>>>>> Max
>>>>>>>> 
>>>>>>>> -------- Original Message --------
>>>>>>>> 
>>>>>>>> *Change Request ID*: 7089889
>>>>>>>> 
>>>>>>>> *Synopsis*: Krb5LoginModule.login() throws an exception if used
>>>>>>>> without a keytab
>>>>>>>> 
>>>>>>>> 
>>>>>>>> === *Description*
>>>>>>>> ============================================================
>>>>>>>> FULL PRODUCT VERSION :
>>>>>>>> java version "1.7.0"
>>>>>>>> Java(TM) SE Runtime Environment (build 1.7.0-b147)
>>>>>>>> Java HotSpot(TM) Client VM (build 21.0-b17, mixed mode, sharing)
>>>>>>>> 
>>>>>>>> A DESCRIPTION OF THE PROBLEM :
>>>>>>>> The call to LoginContext.login() fails with an exception if the
>>>>>>>> LoginContext is configured to use Krb5LoginModule with the following
>>>>>>>> options:
>>>>>>>> 
>>>>>>>> storeKey="true"
>>>>>>>> useKeyTab="false"
>>>>>>>> isInitiator="false"
>>>>>>>> 
>>>>>>>> This appears to be a regression due to an attempt to solve bug
>>>>>>>> '6941083'.
>>>>>>>> 
>>>>>>>> The following source code is taken from
>>>>>>>> Kb5LoginModule.attemptAuthentication(false):
>>>>>>>> 
>>>>>>>> if (ktab == null) {
>>>>>>>> promptForPass(getPasswdFromSharedState);
>>>>>>>> builder = new KrbAsReqBuilder(principal, password);
>>>>>>>> if (isInitiator) {
>>>>>>>> // XXX Even if isInitiator=false, it might be
>>>>>>>> // better to do an AS-REQ so that keys can be
>>>>>>>> // updated with PA info
>>>>>>>> cred = builder.action().getCreds();
>>>>>>>> }
>>>>>>>> if (storeKey) {
>>>>>>>> encKeys = builder.getKeys();
>>>>>>>> // When encKeys is empty, the login actually fails.
>>>>>>>> // For compatibility, exception is thrown in commit().
>>>>>>>> }
>>>>>>>> }
>>>>>>>> 
>>>>>>>> This code path results builder.getGeys() being called while the
>>>>>>>> builder's state is 'INIT'.
>>>>>>>> The builder asserts via checkState() that the state is REQ_OK, hence
>>>>>>>> an exception.
>>>>>>>> 
>>>>>>>> This is a regression, as JRE6 and prior versions called
>>>>>>>> EncryptionKey.acquireSecretKeys()
>>>>>>>> to obtain the keys in this case.
>>>>>>>> 
>>>>>>>> REGRESSION. Last worked in version 6u26
>>>>>>>> 
>>>>>>>> STEPS TO FOLLOW TO REPRODUCE THE PROBLEM :
>>>>>>>> See the executable test case below. Program should be run on a
>>>>>>>> Windows machine that is joined to a domain. Replace 'REALM', 'KDC',
>>>>>>>> 'DOMAIN_USER' and 'DOMAIN_USER_PWD' as appropriate before running
>>>>>>>> 
>>>>>>>> EXPECTED VERSUS ACTUAL BEHAVIOR :
>>>>>>>> EXPECTED -
>>>>>>>> "Login succeeded" printed to console
>>>>>>>> ACTUAL -
>>>>>>>> "Login failed" printed to console.
>>>>>>>> Exception stacktrace printed to strerr
>>>>>>>> 
>>>>>>>> ERROR MESSAGES/STACK TRACES THAT OCCUR :
>>>>>>>> javax.security.auth.login.LoginException:
>>>>>>>> java.lang.IllegalStateException: Cannot get keys at REQ_OK state
>>>>>>>> at sun.security.krb5.KrbAsReqBuilder.checkState(Unknown Source)
>>>>>>>> at sun.security.krb5.KrbAsReqBuilder.getKeys(Unknown Source)
>>>>>>>> at
>>>>>>>> com.sun.security.auth.module.Krb5LoginModule.attemptAuthentication(Unknown
>>>>>>>> 
>>>>>>>> 
>>>>>>>> Source)
>>>>>>>> at com.sun.security.auth.module.Krb5LoginModule.login(Unknown Source)
>>>>>>>> at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
>>>>>>>> at sun.reflect.NativeMethodAccessorImpl.invoke(Unknown Source)
>>>>>>>> at sun.reflect.DelegatingMethodAccessorImpl.invoke(Unknown Source)
>>>>>>>> at java.lang.reflect.Method.invoke(Unknown Source)
>>>>>>>> at javax.security.auth.login.LoginContext.invoke(Unknown Source)
>>>>>>>> at javax.security.auth.login.LoginContext.access$000(Unknown Source)
>>>>>>>> at javax.security.auth.login.LoginContext$4.run(Unknown Source)
>>>>>>>> at javax.security.auth.login.LoginContext$4.run(Unknown Source)
>>>>>>>> at java.security.AccessController.doPrivileged(Native Method)
>>>>>>>> at javax.security.auth.login.LoginContext.invokePriv(Unknown Source)
>>>>>>>> at javax.security.auth.login.LoginContext.login(Unknown Source)
>>>>>>>> at LoginModuleExample.main(LoginModuleExample.java:43)
>>>>>>>> 
>>>>>>>> at javax.security.auth.login.LoginContext.invoke(Unknown Source)
>>>>>>>> at javax.security.auth.login.LoginContext.access$000(Unknown Source)
>>>>>>>> at javax.security.auth.login.LoginContext$4.run(Unknown Source)
>>>>>>>> at javax.security.auth.login.LoginContext$4.run(Unknown Source)
>>>>>>>> at java.security.AccessController.doPrivileged(Native Method)
>>>>>>>> at javax.security.auth.login.LoginContext.invokePriv(Unknown Source)
>>>>>>>> at javax.security.auth.login.LoginContext.login(Unknown Source)
>>>>>>>> at LoginModuleExample.main(LoginModuleExample.java:43)
>>>>>>>> 
>>>>>>>> REPRODUCIBILITY :
>>>>>>>> This bug can be reproduced always.
>>>>>>>> 
>>>>>>>> ---------- BEGIN SOURCE ----------
>>>>>>>> import java.util.HashMap;
>>>>>>>> import java.util.Map;
>>>>>>>> 
>>>>>>>> import javax.security.auth.callback.Callback;
>>>>>>>> import javax.security.auth.callback.CallbackHandler;
>>>>>>>> import javax.security.auth.callback.NameCallback;
>>>>>>>> import javax.security.auth.callback.PasswordCallback;
>>>>>>>> import javax.security.auth.login.AppConfigurationEntry;
>>>>>>>> import
>>>>>>>> javax.security.auth.login.AppConfigurationEntry.LoginModuleControlFlag;
>>>>>>>> 
>>>>>>>> import javax.security.auth.login.Configuration;
>>>>>>>> import javax.security.auth.login.LoginContext;
>>>>>>>> 
>>>>>>>> import com.sun.security.auth.module.Krb5LoginModule;
>>>>>>>> 
>>>>>>>> public class LoginModuleExample {
>>>>>>>> 
>>>>>>>> // Replace with the realm and KDC of the Windows domain.
>>>>>>>> private static final String REALM = "DEV-DEM.RECOMMIND.COM";
>>>>>>>> private static final String KDC =
>>>>>>>> "AU-DEV-DC01.dev-dem.recommind.com";
>>>>>>>> 
>>>>>>>> // Replace with the username and password of any account on the
>>>>>>>> // above domain. Account needs to be enabled and not locked out.
>>>>>>>> private static final String DOMAIN_USER = "sgr";
>>>>>>>> private static final String DOMAIN_USER_PWD = "0rodriguez)";
>>>>>>>> 
>>>>>>>> private static final String EXAMPLE_SERVER_LOGIN = "example-server";
>>>>>>>> 
>>>>>>>> /**
>>>>>>>> * @param args
>>>>>>>> */
>>>>>>>> public static void main(String[] args) {
>>>>>>>> 
>>>>>>>> System.setProperty("java.security.krb5.realm", REALM);
>>>>>>>> System.setProperty("java.security.krb5.kdc", KDC);
>>>>>>>> 
>>>>>>>> Configuration.setConfiguration(new CustomConfiguration());
>>>>>>>> 
>>>>>>>> try {
>>>>>>>> CallbackHandler handler = getUsernamePasswordHandler(DOMAIN_USER,
>>>>>>>> DOMAIN_USER_PWD);
>>>>>>>> 
>>>>>>>> LoginContext loginContext = new LoginContext(EXAMPLE_SERVER_LOGIN,
>>>>>>>> handler);
>>>>>>>> 
>>>>>>>> loginContext.login();
>>>>>>>> 
>>>>>>>> System.out.println("Login succeeded");
>>>>>>>> }
>>>>>>>> catch (Exception e) {
>>>>>>>> 
>>>>>>>> System.out.println("Login failed");
>>>>>>>> e.printStackTrace();
>>>>>>>> }
>>>>>>>> 
>>>>>>>> }
>>>>>>>> 
>>>>>>>> private static CallbackHandler getUsernamePasswordHandler(final
>>>>>>>> String username, final String password) {
>>>>>>>> 
>>>>>>>> final CallbackHandler handler = new CallbackHandler() {
>>>>>>>> 
>>>>>>>> public void handle(final Callback[] callback) {
>>>>>>>> for (int i = 0; i<   callback.length; i++) {
>>>>>>>> if (callback[i] instanceof NameCallback) {
>>>>>>>> final NameCallback nameCallback = (NameCallback) callback[i];
>>>>>>>> nameCallback.setName(username);
>>>>>>>> }
>>>>>>>> else if (callback[i] instanceof PasswordCallback) {
>>>>>>>> final PasswordCallback passCallback = (PasswordCallback) callback[i];
>>>>>>>> passCallback.setPassword(password.toCharArray());
>>>>>>>> }
>>>>>>>> }
>>>>>>>> }
>>>>>>>> };
>>>>>>>> 
>>>>>>>> return handler;
>>>>>>>> }
>>>>>>>> 
>>>>>>>> final static class CustomConfiguration extends Configuration {
>>>>>>>> 
>>>>>>>> @Override
>>>>>>>> public AppConfigurationEntry[] getAppConfigurationEntry(String
>>>>>>>> name) {
>>>>>>>> 
>>>>>>>> AppConfigurationEntry[] entries = new AppConfigurationEntry[0];
>>>>>>>> 
>>>>>>>> if (name.equals(EXAMPLE_SERVER_LOGIN)) {
>>>>>>>> String krbModule = Krb5LoginModule.class.getName();
>>>>>>>> LoginModuleControlFlag flag = LoginModuleControlFlag.REQUIRED;
>>>>>>>> Map<String, String>   options = new HashMap<String, String>();
>>>>>>>> 
>>>>>>>> options.put("storeKey", "true");
>>>>>>>> options.put("useKeyTab", "false");
>>>>>>>> options.put("isInitiator", "false");
>>>>>>>> 
>>>>>>>> AppConfigurationEntry entry = new AppConfigurationEntry(krbModule,
>>>>>>>> flag, options);
>>>>>>>> 
>>>>>>>> entries = new AppConfigurationEntry[] { entry };
>>>>>>>> }
>>>>>>>> 
>>>>>>>> return entries;
>>>>>>>> }
>>>>>>>> }
>>>>>>>> }
>>>>>>>> 
>>>>>>>> ---------- END SOURCE ----------
>>>>>>>> 
>>>>>>>> 
>>>> 
>>