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

[Xuelei Fan]

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

>>>
>>> 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();

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)?

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