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

Weijun Wang weijun.wang at oracle.com
Wed Sep 21 07:28:16 UTC 2011 

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

More information about the security-dev mailing list