code review request: 7089889: Krb5LoginModule.login() throws an exception if used without a keytab
Xuelei Fan
xuelei.fan at oracle.com
Wed Sep 21 04:28:55 UTC 2011
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