Mailing-List: contact dev-help@tomcat.apache.org; run by ezmlm
Precedence: bulk
Reply-To: "Tomcat Developers List" <dev@tomcat.apache.org>
Received-SPF: softfail (athena.apache.org: transitioning domain of
 brian@pingtoo.com does not designate 217.154.193.216 as permitted sender)
Message-ID: <51CC8316.9060500@PingToo.com>
Date: Thu, 27 Jun 2013 19:23:18 +0100
From: Brian Burch <brian@PingToo.com>
User-Agent: Mozilla/5.0 (X11; Linux i686;
 rv:17.0) Gecko/20130623 Thunderbird/17.0.7
MIME-Version: 1.0
To: Tomcat Developers List <dev@tomcat.apache.org>
Subject: Re: Tomcat 7.0.41 JNDIRealm revision 1491394
References: <51CC77A1.1030108@PingToo.com>
 <CABzHfVnvq_iVboKpzV-GTT3ftw4gucccjoFhoLWdaBg6x9PaQA@mail.gmail.com>
In-Reply-To: 
 <CABzHfVnvq_iVboKpzV-GTT3ftw4gucccjoFhoLWdaBg6x9PaQA@mail.gmail.com>
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit

On 27/06/13 18:51, Konstantin Kolinko wrote:
> 2013/6/27 Brian Burch <brian@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