Static use of java.security.MessageDigest is not thread-safe. A symptom of this bug would be "random" 401 responses to Digest Authentication challenges. The static declaration: protected static MessageDigest md5Helper; The use of this idiom in the DigestAuthenticator (and RealmBase) class means that all threads are effectively using the same instance of the MessageDigest. Highly concurrent testing showed this defect. In reviewing the java docs for MessageDigest, there is nothing that indicates that it is thread-safe. There are two solutions, either create a wrapper class which synchronizes access, or create new instances when needed. For performance reasons it would probably be a good idea to create a wrapper class as creating new instances can be expensive.
I looked in MessageDigest, and indeed, it is not thread safe. Problems are very unlikely to occur, though.
As far as I can see DigestAuthenticator only ever wants a digested AND encoded String given a String. It might simplify the code to factor out the synchronize+digest+encode into another utility method in MD5Encoder that accepts single String parameter and returns an encoded MD5 digest String result. This method would synchronize on its MessageDigest. Addition to MD5Encoder could be something like: private static MessageDigest md5Helper; static { try { if (md5Helper == null) md5Helper = MessageDigest.getInstance("MD5"); } catch (NoSuchAlgorithmException e) { throw new IllegalStateException(); } } public String digestAndEncode(String p) { synchronized (md5Helper) { return encode(md5Helper.digest(p.getBytes())); } } Sorry for lack of a proper patch but Ive not got proper setup at moment.
Checked nightly build and there are other places where this bug could rear it's ugly head: 1. RealmBase 2. org.apache.catalina.servlets.WebdavServlet
Created attachment 13686 [details] RealmBase patch
The issue is rare, but possible, and I like this patch because it's small, and safe. The performance impact is negligible. So I'll go ahead and commit it for RealmBase -- thanks for submitting it. WebDAV is a bit of a different story: it's not nearly as much of a concern, just a little lightweight implementation not subject to highly concurrent use. Done for 5.0.31, doing 5.5.6 next.