You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomcat.apache.org by Brian Burch <br...@PingToo.com> on 2013/06/27 19:34:25 UTC
Tomcat 7.0.41 JNDIRealm revision 1491394
I eventually got round to integration testing of 7.0.41 yesterday and
was baffled to find I couldn't logon!
To cut a long debugging story short, revision 1491394 has a bug that was
introduced as part of the standardisation of our Base64 handling. I
wasn't sure whether I ought to open a new bug...
Here is the diff that works for me:
Index: java/org/apache/catalina/realm/JNDIRealm.java
===================================================================
--- java/org/apache/catalina/realm/JNDIRealm.java (revision 1491394)
+++ java/org/apache/catalina/realm/JNDIRealm.java (working copy)
@@ -1573,9 +1573,10 @@
password = password.substring(5);
md.reset();
md.update(credentials.getBytes(Charset.defaultCharset()));
- byte[] decoded = Base64.decodeBase64(md.digest());
+ byte[] digest = md.digest();
+ byte[] base64 = Base64.encodeBase64(digest);
String digestedPassword =
- new String(decoded, B2CConverter.ISO_8859_1);
+ new String(base64, B2CConverter.ISO_8859_1);
validated = password.equals(digestedPassword);
}
} else if (password.startsWith("{SSHA}")) {
BTW. The code is identical in trunk, so this patch works there too.
Thinks... pity some of this stuff doesn't have some lightweight unit tests.
Sorry to be a informal with this notification, but I thought timeliness
was more important than style!
Brian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: Tomcat 7.0.41 JNDIRealm revision 1491394
Posted by Brian Burch <br...@PingToo.com>.
On 27/06/13 21:03, Mark Thomas wrote:
> On 27/06/2013 19:23, Brian Burch wrote:
>
>> So.... will you deal with it from now on, or would you like me to open
>> bugs on tc7 and tc8?
>
> Fixed. It would be good if you could confirm the fix is correct.
I checked out the tc7 trunk. The source of JNDIRealm was exactly as I
expected to fix the bug. I replaced my patched catalina.jar with the
differently-sized version built from the trunk.
I confirm that authentication of SHA hashed passwords from an LDAP
directory is working again.
Brian
> Mark
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
> For additional commands, e-mail: dev-help@tomcat.apache.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: Tomcat 7.0.41 JNDIRealm revision 1491394
Posted by Mark Thomas <ma...@apache.org>.
On 27/06/2013 19:23, Brian Burch wrote:
> So.... will you deal with it from now on, or would you like me to open
> bugs on tc7 and tc8?
Fixed. It would be good if you could confirm the fix is correct.
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: Tomcat 7.0.41 JNDIRealm revision 1491394
Posted by Brian Burch <br...@PingToo.com>.
On 27/06/13 18:51, Konstantin Kolinko wrote:
> 2013/6/27 Brian Burch <br...@pingtoo.com>:
>> I eventually got round to integration testing of 7.0.41 yesterday and was
>> baffled to find I couldn't logon!
>>
>> To cut a long debugging story short, revision 1491394 has a bug that was
>> introduced as part of the standardisation of our Base64 handling. I wasn't
>> sure whether I ought to open a new bug...
>
> Your numbering is wrong, that revision is not ours. It was this one:
> http://svn.apache.org/viewvc?diff_format=l&view=revision&revision=1459346
Yes, I see what you mean and agree. I'm puzzled that my svn diff
reported the wrong revision, which is a pity because I put it in the new
thread subject and we will have to live with it unless you can
retrospectively edit the entries?
Thanks for finding the correct revision.
>
>> Here is the diff that works for me:
>>
>>
>> Index: java/org/apache/catalina/realm/JNDIRealm.java
>> ===================================================================
>> --- java/org/apache/catalina/realm/JNDIRealm.java (revision 1491394)
>> +++ java/org/apache/catalina/realm/JNDIRealm.java (working copy)
>> @@ -1573,9 +1573,10 @@
>> password = password.substring(5);
>> md.reset();
>>
>> md.update(credentials.getBytes(Charset.defaultCharset()));
>> - byte[] decoded = Base64.decodeBase64(md.digest());
>> + byte[] digest = md.digest();
>> + byte[] base64 = Base64.encodeBase64(digest);
>> String digestedPassword =
>> - new String(decoded, B2CConverter.ISO_8859_1);
>> + new String(base64, B2CConverter.ISO_8859_1);
>> validated = password.equals(digestedPassword);
>> }
>> } else if (password.startsWith("{SSHA}")) {
>>
>>
>
> In short, s/ decodeBase64 / encodeBase64 /.
Yes. I wanted to break the logic up because I needed to encode/decode my
test cases by hand!
I didn't trust my own external cribs and suspected I was digging into a
difference between the sun and openjdk digest engines.
Pencil/paper/hexadecimal calculator...
I didn't want to go round the loop of another source
change/build/deploy/test/debug/diff. You got the idea, just as I expected.
Imagine my surprise when I got 80% through the exercise and spotted the
"backwards" choice of Base64 method!
> It is fun that {MD5}&{SHA} branch and {SSHA} branch there use
> different approaches there
> (encoding the user-supplied password vs. decoding the stored one).
Actually, I can sort-of understand the person adding SSHA doing his/her
own thing and not wanting to change logic that is stable and works
(well, it did in 7.0.28!).
If we are looking for a vote, I prefer the way MD5/SHA does it. It takes
the user-supplied cleartext and digest/encodes it in a forward
direction, such that the directory hash is never exposed in situations
where the supplied value is incorrect.
>> BTW. The code is identical in trunk, so this patch works there too.
>>
>>
>> Thinks... pity some of this stuff doesn't have some lightweight unit tests.
>>
>> Sorry to be a informal with this notification, but I thought timeliness was
>> more important than style!
So.... will you deal with it from now on, or would you like me to open
bugs on tc7 and tc8?
Brian
>
> Best regards,
> Konstantin Kolinko
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
> For additional commands, e-mail: dev-help@tomcat.apache.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: Tomcat 7.0.41 JNDIRealm revision 1491394
Posted by Konstantin Kolinko <kn...@gmail.com>.
2013/6/27 Brian Burch <br...@pingtoo.com>:
> I eventually got round to integration testing of 7.0.41 yesterday and was
> baffled to find I couldn't logon!
>
> To cut a long debugging story short, revision 1491394 has a bug that was
> introduced as part of the standardisation of our Base64 handling. I wasn't
> sure whether I ought to open a new bug...
Your numbering is wrong, that revision is not ours. It was this one:
http://svn.apache.org/viewvc?diff_format=l&view=revision&revision=1459346
> Here is the diff that works for me:
>
>
> Index: java/org/apache/catalina/realm/JNDIRealm.java
> ===================================================================
> --- java/org/apache/catalina/realm/JNDIRealm.java (revision 1491394)
> +++ java/org/apache/catalina/realm/JNDIRealm.java (working copy)
> @@ -1573,9 +1573,10 @@
> password = password.substring(5);
> md.reset();
>
> md.update(credentials.getBytes(Charset.defaultCharset()));
> - byte[] decoded = Base64.decodeBase64(md.digest());
> + byte[] digest = md.digest();
> + byte[] base64 = Base64.encodeBase64(digest);
> String digestedPassword =
> - new String(decoded, B2CConverter.ISO_8859_1);
> + new String(base64, B2CConverter.ISO_8859_1);
> validated = password.equals(digestedPassword);
> }
> } else if (password.startsWith("{SSHA}")) {
>
>
In short, s/ decodeBase64 / encodeBase64 /.
It is fun that {MD5}&{SHA} branch and {SSHA} branch there use
different approaches there
(encoding the user-supplied password vs. decoding the stored one).
>
> BTW. The code is identical in trunk, so this patch works there too.
>
>
> Thinks... pity some of this stuff doesn't have some lightweight unit tests.
>
> Sorry to be a informal with this notification, but I thought timeliness was
> more important than style!
>
Best regards,
Konstantin Kolinko
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org