Jetty.project: HashLoginService silently ignores file:/ config paths from 9.3.x

[javabrett]

Found this in https://github.com/gretty-gradle-plugin/gretty where a test failed on latest 9.3.x and 9.4.x which passed on 9.2.x. The test sets-up a Realm using HashLoginService and is configured with a file path, which was of the form e.g. new HashLoginService("auth", "file:/path/to/jetty-realm.properties").

That works fine in Jetty 9.2.24.v20180105. In the latest 9.3 and 9.4 builds we've tested with (9.3.23.v20180228 and 9.4.9.v20180320), the test fails, and HashLoginService and the PropertyUserStore underneath it just silently ignore that file since they consider it does not exist. If the string passed to the HashLoginService is changed from URL to path form, e.g. /path/to/jetty-realm.properties, the test passes as the login service is properly configured.

According to https://github.com/eclipse/jetty.project/blob/jetty-9.4.x/jetty-security/src/main/java/org/eclipse/jetty/security/HashLoginService.java#L100 :

/* ------------------------------------------------------------ /
/*
* Load realm users from properties file.
*

* The property file maps usernames to password specs followed by an optional comma separated list of role names.
*

*
* @param config uri or url or path to realm properties file
*/
public void setConfig(String config)
{
_config=config;
}

... so URL or URI as string should be fine. The URL as string is valid (works immediately with file: removed).

PathResource.exists() works with a Path type and is what causes the file path to be rejected and ignored here: https://github.com/eclipse/jetty.project/blob/jetty-9.4.x/jetty-security/src/main/java/org/eclipse/jetty/security/PropertyUserStore.java#L255

Is this a real change, and was it documented, or is it a bug/regression?

[olamy]

@javabrett can you create a pull request with your changes?
Thanks!

[javabrett]

@olamy I don't yet have a code-change proposal for this. If I make one, I will be sure to share it here.

My current workaround is in client-code starting Jetty embedded, to ensure it does not pass file URLs in protocol form.

[janbartel]

@javabrett the commits for issue #1520 look they are working as expected. Using the head of jetty-9.4.x branch - or the latest 9.4.10.v20180503 - I can call new HashLoginService ("foo", "file:/this/is/the/file.txt") and it is able to load the users. Can you confirm which version of jetty you're making the report against, and also a code snippet demonstrating the problem would be good.

[javabrett]

Thanks @janbartel - problem shows-up in Gretty tests for 9.3.23.v20180228 and 9.4.9.v20180320 but works fine in 9.2.24.v20180105. I'll try 9.4.10.v20180503 now.

I'd like to build Jetty anyway, both 9.2 and 9.4 branches, and try adding some tests. Are the branch builds hosted in a public way currently so they can be reviewed? I notice there's a Jenkinsfile but haven't found so-far any project details about public builds.

[olamy]

@javabrett here https://jenkins.webtide.net/blue/organizations/jenkins/jetty.project/activity

[janbartel]

@javabrett actually I can't reproduce a problem with jetty-9.4.9 either, calling the HashLoginService constructor with a file: url as you originally posted. Can you provide the src to the test you're doing?

[javabrett]

Right, sorry this could be a false report. I may close it later today. Definitely don't spend further time on it.

I wrote unit tests for it today and could not reproduce it.

I suspect that this is going to boil down to some issue with changes from 9.2 to 9.3 as it relates to the eay Gretty inits the service.

If false report then sorry for the noise.

[javabrett]

I have some further analysis now and a unit test which passes on 9.2 and fails on 9.3 and 9.4.

Test-branches:

• 9.2 (passes): https://github.com/javabrett/jetty.project/tree/2565-HashLoginService-test-9-2 , https://github.com/javabrett/jetty.project/commit/8e64a03ada42f598efa808318c3e7c910e8527fc
• 9.3 (fails): https://github.com/javabrett/jetty.project/tree/2565-HashLoginService-test-9-3 , https://github.com/javabrett/jetty.project/commit/de91e87df0097c6668441ddf5326569b46aaa58f
• 9.4 (fails): https://github.com/javabrett/jetty.project/tree/2565-HashLoginService-test-9-4 , https://github.com/javabrett/jetty.project/commit/eb95b2a041aa77980728228abf41457311b71ea0

Test failing in Travis for 9.3: https://travis-ci.org/javabrett/jetty.project/builds/387623809#L8413

Debugging an execution for Gretty running a Jetty 9.3.23.v20180228 container:

• HashLoginService constructor is called with two string args: ("auth", "file:/Users/bsrandal/git/gretty/integrationTests/helloGrettySecure/security/jetty-realm.properties"). Note that the string path is in URL form.
• Later when the embedded container starts, HashLoginService.doStart(), not having an existing user store, creates a PropertyUserStore and passes the string path via setConfig(), which in 9.3 has changed to setConfigPath(). In 9.3 this will result in _configPath = new File(configFile).toPath() and a UnixPath, but note that the path is still a string in URL form.
• PropertyUserStore.doStart() ... loadUsers() ... getConfigSource(). In 9.2 this called Resource.newResource(_config) but in 9.3.x it calls _configResource = new PathResource(_configPath).
• Due to the misunderstood URL vs Path, a bad resource object is created and when checked for exists(), the mangled path file:///Users/bsrandal/git/gretty/integrationTests/helloGrettySecure/file:/Users/bsrandal/git/gretty/integrationTests/helloGrettySecure/security/jetty-realm.properties is not found.

It looks like the new HashLoginService default delegation to PropertyUserStore has a problem when it calls setConfigPath, assuming that this should be already in path-safe form. Maybe setConfig remains safe, but it is now hidden via the creation of the default PropertyUserStore.

Would someone be able to validate the tests and confirm the analysis? In Gretty we could either make-good the current workaround, which ensures we are in path and not URI/URL form before creating HashLoginService, but this does work in 9.2 so it seems like a change. Alternatively if setConfig() still works for URI/URL strings in 9.3 and 9.4 we can explicitly construct one and pass it to HashLoginService.

Regardless of any of the above, I would say that PropertyUserStore#loadUsers() silently ignoring when if (getConfigResource().exists()) fails is a bug - it should log at WARN at least, since this represents a bad security configuration-error. Let me know if you'll consider a PR for that.

[janbartel]

@javabrett just ran your test in both jetty-9.4.9 release and also head of jetty-9.4.x branch and it passes! Output is:

[INFO] Running org.eclipse.jetty.security.HashLoginServiceTest
2018-06-04 17:33:10.243:INFO::main: Logging initialized @618ms to org.eclipse.jetty.util.log.StdErrLog
config path string: file:///home/janb/src/jetty-eclipse/jetty-9.4.x/jetty-security/target/property-user-store-test/users.txt
config path string: file:/home/janb/src/jetty-eclipse/jetty-9.4.x/jetty-security/target/property-user-store-test/users.txt
config path string: /home/janb/src/jetty-eclipse/jetty-9.4.x/jetty-security/target/property-user-store-test/users.txt
[INFO] Tests run: 3, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.24 s - in org.eclipse.jetty.security.HashLoginServiceTest
[INFO] 
[INFO] Results:
[INFO] 
[INFO] Tests run: 3, Failures: 0, Errors: 0, Skipped: 0

I can't see why we would get different results for exactly the same test on the same release (in this case 9.4.9). Can you definitely confirm the failure on 9.4.9? Can you post the output of the test? Can you post the os and jvm version you're using?

[javabrett]

This investigation started because Gretty integration tests which run fine on latest 9.2.x releases fail on both 9.3.x and 9.4.x latest releases, and one of the tests which fails is in the realm/user-credentials area.

I wanted to get some unit tests running in an empirical fashion, preferably on Travis on some third-party server, and as part of the Jetty build, because I was seeing all kinds of variations building on macos, or in the Gretty integration tests, which might not be perfect. I know Jetty builds on Jenkins, and I've spent an unreasonable amount of time on a Travis build ... hit a couple of fragile tests and other problems, but all good now with some workarounds.

So I think I can definitively say now that the unit tests do pass for 9.2.x HEAD, 9.4.9, 9.4.10 and 9.4.x HEAD, and they fail for 9.3.23 and 9.3.x HEAD. Problems that Gretty is having with 9.4.x are either separate, or not yet exposed by the unit tests that I wrote.

Perhaps the 9.3.x unit tests are not valid - worth checking those, they are on various branches in my fork.

I don't know that this is important enough to try and fix for 9.3.x - over to you on that one. For Gretty I think we will apply a local patch/workaround for 9.3. Hopefully I can find the remaining problems for 9.4.

I maintain that the code which loads the realm file should log something at WARN level if it can't locate the file.

Thanks for your patience on this, there's been a lot of trouble getting Gretty tests working for 9.3 and 9.4 (they weren't originally running when I joined the project and still aren't on master, only 9.2 and 7/8 are).

[janbartel]

@javabrett thanks for keeping on delving into this. I agree that a bad or missing file should cause an exception - take a look at the PR I just raised.

[joakime]

This has been merged into jetty-9.4.x and will show up in next release.