Looks fine to me. Xuelei
On Sep 21, 2011, at 3:28 PM, Weijun Wang <weijun.w...@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.w...@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 ---------- >>>>>>>> >>>>>>>> >>>> >>
