Code Review Needed: 7031830: bad_record_mac failure on TLSv1.2 enabled connection with SSLEngine

[Xuelei Fan]

Both (JDK 7u2 and JDK 8) look fine to me. Thanks for the update.

Xuelei

On 10/18/2011 8:41 AM, Brad Wetmore wrote:
> (Max/Valerie/Sean, I am hoping to get this into 7u2, thus need a second
> codereview.  The code itself is pretty straightforward.)
> 
> Xuelei wrote:
>> The change of SSLEngineImpl looks fine to me.
>>
>> In the new regression test:
> 
> Changes made.  Thanks for the feedback, Xuelei.  I tweaked things a bit
> even more in the test case only.  Now, I alternate between the client
> and the server initiating the closing.
> 
> http://cr.openjdk.java.net/~wetmore/7031830/
> 
> See the .01 versions.
> 
> Brad
> 
> 
> 
> 
> On 10/14/2011 7:23 PM, Xuelei Fan wrote:
>> The change of SSLEngineImpl looks fine to me.
>>
>> In the new regression test:
>>
>> A. class name
>>
>> 28  * @run main/othervm SSLEngineTemplate
>> 29  *
>> 30  *     SunJSSE does not support dynamic system properties, no way to
>> re-use
>> 31  *     system properties in samevm/agentvm mode.
>>
>> I think the class name should be SSLEngineBadBufferArrayAccess.
>> - 28  * @run main/othervm SSLEngineTemplate
>> + 28  * @run main/othervm SSLEngineBadBufferArrayAccess
>>
>> B. a minor comment, reuse the serverEngine.wrap() logic.
>> Is it possible to remove line 286 ~ 297, and have
>> "while(!isEngineClosed())" handle the close notify? So that the server
>> engine would be able to handle the client close_notify response.
>>
>>     boolean exchangeHasDone = false;
>>     while (!isEngineClosed()) {
>> 283   if (!exchangeHasDone&&  clientMsg.length == serverIn.position()) {
>>           // sanity check
>>           ...
>>           serverEngine.closeOutbound();
>>           exchangeHasDone = true.
>>       }
>>     } catch (...) {
>>
>>
>> Xuelei
>>
>> On 10/15/2011 8:52 AM, Brad Wetmore wrote:
>>> Hi Xuelei,
>>>
>>> I need code reviews for the bug I mentioned to you earlier.
>>>
>>> 7031830: bad_record_mac failure on TLSv1.2 enabled connection with
>>> SSLEngine
>>>
>>> The MAC calculation was summing the wrong data range when using
>>> non-direct byte buffers and TLS1.1/1.2.
>>>
>>> The new regression test will now interop-test SSLEngine with SSLSockets
>>> using both direct and non-direct ByteBuffers, over SSLv3, TLSv1,
>>> TLSv1.1, and TLSv1.2.
>>>
>>> http://cr.openjdk.java.net/~wetmore/7031830/
>>>
>>> I plan to push this to both JDK 8 and 7u2, so there are 2 webrevs there.
>>>   They should be the same.
>>>
>>> Brad
>>