Community
Participate
Working Groups
Build Identifier: jetty-distribution-7.0.1.v20091125 PropertyFileLoginModule reads in and caches the password file the first time it is used. This means that when the password file is changed later, jetty has to be restarted to pick up the new passwords. HashLoginService contains logic to re-load the file based on passed properties. Both classes could share common code which detects changes in the password file and reloads the file as necessary, or reload after a timeout (as is done in HashLoginService now). Reproducible: Always Steps to Reproduce: 1. Create a JAAS Config with PropertyFileLoginModule 2. Start Jetty, try to authenticate 3. Modify passwords file 4. try to authenticate with new user / password
actually I am thinking that the PropertyFileLoginModule ought to just use an instance of the HashLoginService to authenticate against. They both seem to be based on property files (same format taking a quick peek). The security system in jetty7 is built around LoginService and IdentityService interfaces so with that in mind, another option would be to have a LoginService and IdentityService based jaas login module so you could then mix and match the services you need in one place. If that existed then we could leverage the existing services in o.e.j.security and effectively replace the PropertyFileLoginModule, the JDBCLoginModule and the the LdapLoginModule once I finish up and migrate some of the ldap work I have been working on.
Created attachment 201813 [details] Unfinished(!) patch Hi Jesse, this patch is NOT finished. I had some issues with a strange behaviour of the JAAS LoginModule which seems to reinstantiate the LoginModule on every login/auth request. I think the problem is in JAASLoginService line 212: LoginContext loginContext = new LoginContext(_loginModuleName, subject, callbackHandler); The loginContext can probably be reused (instance variable) and this will resolve the problem. But I need to learn more about JAAS to make sure that this is really the case. I also didn't make it to verify what the old behaviour was as previously changes to the realm.properties file have definetly not been reread. HashLoginService contains some problem with PropertyUserStore. It doesn't rescan the realm.properties file for some reason. I started some debugging on it, but didn't find anything. Guess it's something obvious. Note that the code for HashLoginService especially has not been beautified yet. So a quick code review and probably some more testing would be good. Sorry, that I didn't make it before my holiday. :( I hate to leave unfinished stuff before holiday, but I really have to hurry now. Cheers, Thomas
http://download.oracle.com/javase/6/docs/api/javax/security/auth/login/LoginContext.html says: "A LoginContext should not be used to authenticate more than one Subject. A separate LoginContext should be used to authenticate each different Subject." So we shouldn't keep a login context around between uses, as each use can be for a different Subject (user). Jan (In reply to comment #2) > Created attachment 201813 [details] > Unfinished(!) patch > > Hi Jesse, > > this patch is NOT finished. I had some issues with a strange behaviour of the > JAAS LoginModule which seems to reinstantiate the LoginModule on every > login/auth request. > > I think the problem is in JAASLoginService line 212: > LoginContext loginContext = new LoginContext(_loginModuleName, subject, > callbackHandler); > > The loginContext can probably be reused (instance variable) and this will > resolve the problem. But I need to learn more about JAAS to make sure that this > is really the case. > > I also didn't make it to verify what the old behaviour was as previously > changes to the realm.properties file have definetly not been reread. > > HashLoginService contains some problem with PropertyUserStore. It doesn't > rescan the realm.properties file for some reason. I started some debugging on > it, but didn't find anything. Guess it's something obvious. > > Note that the code for HashLoginService especially has not been beautified yet. > So a quick code review and probably some more testing would be good. Sorry, > that I didn't make it before my holiday. :( I hate to leave unfinished stuff > before holiday, but I really have to hurry now. > > Cheers, > Thomas
Ok, that's what I expected. You know what...I will take the laptop with me and have it finished until monday. I'm into the topic now and it'll be way less work for me than for you as I just worked on it. Will keep you updated here.
Created attachment 201871 [details] Finished patch Hey Jan, Jesse, please find attached the finished patch. Would be good if you can do a quick code review and retest as well. I've tested both JAAS and HashLoginService with different settings and changed the properties file a couple of times. Seems to work fine. Cheers, Thomas
Thomas, I can't seem to apply the patch to the latest jetty-7 head: [2080] git apply -v ~/Desktop/323311.patch /home/janb/Desktop/323311.patch:44: trailing whitespace. * /home/janb/Desktop/323311.patch:45: trailing whitespace. * /home/janb/Desktop/323311.patch:192: trailing whitespace. * /home/janb/Desktop/323311.patch:632: trailing whitespace. Checking patch jetty-plus/src/main/java/org/eclipse/jetty/plus/jaas/spi/PropertyFileLoginModule.java... Checking patch jetty-security/src/main/java/org/eclipse/jetty/security/HashLoginService.java... error: while searching for: protected void doStart() throws Exception { super.doStart(); if (getRefreshInterval() > 0) { _scanner = new Scanner(); _scanner.setScanInterval(getRefreshInterval()); List<File> dirList = new ArrayList<File>(1); dirList.add(_configResource.getFile()); _scanner.setScanDirs(dirList); _scanner.setFilenameFilter(new FilenameFilter() { public boolean accept(File dir, String name) { File f = new File(dir, name); try { if (f.compareTo(_configResource.getFile()) == 0) return true; } catch (IOException e) { return false; } return false; } }); _scanner.addListener(new BulkListener() { public void filesChanged(List filenames) throws Exception { if (filenames == null) return; if (filenames.isEmpty()) return; if (filenames.size() == 1 && filenames.get(0).equals(_config)) loadUsers(); } public String toString() { return "HashLoginService$Scanner"; } }); _scanner.setReportExistingFilesOnStartup(false); _scanner.setRecursive(false); _scanner.start(); } } error: patch failed: jetty-security/src/main/java/org/eclipse/jetty/security/HashLoginService.java:179 error: jetty-security/src/main/java/org/eclipse/jetty/security/HashLoginService.java: patch does not apply Checking patch jetty-security/src/main/java/org/eclipse/jetty/security/PropertyUserStore.java... Checking patch jetty-util/src/main/java/org/eclipse/jetty/util/Scanner.java... Checking patch jetty-security/src/main/java/org/eclipse/jetty/security/HashLoginService.java... error: while searching for: @Override protected void loadUsers() throws IOException { //TODO: Consider refactoring MappedLoginService to not have to override with unused methods } /* ------------------------------------------------------------ */ error: patch failed: jetty-security/src/main/java/org/eclipse/jetty/security/HashLoginService.java:116 error: jetty-security/src/main/java/org/eclipse/jetty/security/HashLoginService.java: patch does not apply Checking patch jetty-security/src/main/java/org/eclipse/jetty/security/MappedLoginService.java... Checking patch jetty-security/src/main/java/org/eclipse/jetty/security/PropertyUserStore.java... Checking patch jetty-plus/src/main/java/org/eclipse/jetty/plus/jaas/JAASLoginService.java... Is the patch against the most recent jetty-7? Jan
Hi Jan, just had a look at it. I think you've fixed this other bug in HashLoginService for file update detection. I will have a look and try to reapply my changes to the latest version and provide a new patch for you. I can't promise when I'm able to do it. I think this evening should be fine. Cheers, Thomas
Created attachment 202767 [details] New Patches based on trunk created in a branch. Hi, I've refactored PropertyUserStore to be able to store UserIdentities. Now it can be used as some kind of listenerservice or to store/retrieve UserIdentities directly. PropertyFileLoginModule is now refactored to use the latter functionality of PropertyUserStore. Seems to work well on my tests. Cheers, Thomas
Thomas, I have flagged the latest patch set as +iplog and have committed them to aptly named branch '323311' I removed the additions to the IdentityService and DefaultIdentityService and moved that logic into the loadUsers() since it was only being used in one place and I didn't like the direct usage of KnownUser in the default impl there. aside from that, at first blush this looks like what we are looking for though. I would like you to review the branch and make sure everything came across cleanly, it was a bit of a bear to apply anyway, no need to generate patches for this functionality again, if you need to generate another one do it against that branch let me know what you think of the branch and I can merge it over to trunk Jan, you can review on branch if you like
http://git.eclipse.org/c/jetty/org.eclipse.jetty.project.git/log/?h=323311
Hi Jesse, like your changes. As already stated I didn't like touching the IdentityService as well. You refactored PropertyFileLoginModule not to be an implementation of UserListener anymore, thus we don't need the listener functionality in UserPropertyStore atm. and could remove it to slim the code. However I'm fine leaving it in as an Option for everybody else. I didn't retest, just review your changes. Looks good to me. If you want me to retest, let me know. Cheers, Thomas
