Re: RFR (+CSR) 8201627: Kerberos sequence number issues

2018-04-26 Thread Valerie Peng 

Sure, should be fine...
Valerie

On 4/25/2018 9:48 PM, Weijun Wang wrote:

I filed https://bugs.openjdk.java.net/browse/JDK-8202300 but might not have 
time to make it into JDK 11.

--Max


On Apr 26, 2018, at 12:06 AM, Weijun Wang  wrote:

I'll keep using int32 (at least in this fix), both Java and MIT krb5 contain 
these words

* Workaround implementation incompatibilities by not generating
* initial sequence numbers greater than 2^30

So bad thing could only happen after 2^30 messages.

--Max


On Apr 25, 2018, at 10:38 PM, Weijun Wang  wrote:

It's complicated. Looks like MIT krb5 uses a uint32 for old etypes (DES, 3DES, 
RC4) and a uint64 for new ones (AES) [1][2].

I'll do more experiments.

Thanks
Max

[1] 
https://github.com/krb5/krb5/blob/master/src/lib/gssapi/generic/util_seqstate.c#L76
[2] 
https://github.com/krb5/krb5/blob/master/src/lib/gssapi/krb5/init_sec_context.c#L825


On Apr 24, 2018, at 8:55 PM, Wang Weijun  wrote:

RFC 4120 5.5.1 has

seq-number
This optional field includes the initial sequence number to be used by the 
KRB_PRIV or KRB_SAFE messages when sequence numbers are used to detect replays. 
(It may also be used by application specific messages.) When included in the 
authenticator, this field specifies the initial sequence number for messages 
from the client to the server. When included in the AP-REP message, the initial 
sequence number is that for messages from the server to the client. When used 
in KRB_PRIV or KRB_SAFE messages, it is incremented by one after each message 
is sent. Sequence numbers fall in the range 0 through 2^32 - 1 and wrap to zero 
following the value 2^32 - 1.


If it wraps, then it’s 4 bytes.

I will read more on it.

Thanks
Max


在 2018年4月24日，18:08，Valerie Peng  写道：

Hi Max,

Most changes look good. I have only some comments and questions (see below):

- InitSecContextToken.java, line 62: bad -> unrecognized?
- According to the class javadoc of various Token classes and Kerberos spec, 
the sequence number is 8-byte integer from header byte 8-15, but java int have 
only 4 bytes. The current code seems to assume the first 4 bytes of the 
sequence number are always 0. For the sake of compliance and max 
inter-operability, maybe we should use long to store the sequence number?

CSR looks good to me.
Thanks,
Valerie




On 4/18/2018 10:40 AM, Weijun Wang wrote:
Please take a review of this fix:

webrev: http://cr.openjdk.java.net/~weijun/8201627/webrev.00/
CSR: https://bugs.openjdk.java.net/browse/JDK-8201814

Basically we fix some bugs and introduce a system property so we can interop 
with everyone.

Thanks
Max

Re: RFR: ChaCha20 and ChaCha20/Poly1305 Cipher implementations

2018-04-26 Thread Jamil Nimeh 

Agreed.  I will make that change.

--Jamil


On 04/26/2018 11:20 AM, Sean Mullan wrote:

On 4/26/18 1:36 PM, Jamil Nimeh wrote:
Will do.  I made some wording changes there to move away from 
"96-bit" to "12-byte" also since that probably is more useful to 
developers.


One other comment is that I think the get methods of 
ChaCha20ParameterSpec should be final. There should be no reason to 
override those methods and it will help ensure that the contract of 
those methods are not being overridden by a subclass incorrectly.


--Sean

Re: RFR: ChaCha20 and ChaCha20/Poly1305 Cipher implementations

2018-04-26 Thread Sean Mullan 

On 4/26/18 1:36 PM, Jamil Nimeh wrote:
Will do.  I made some wording changes there to move away from "96-bit" 
to "12-byte" also since that probably is more useful to developers.


One other comment is that I think the get methods of 
ChaCha20ParameterSpec should be final. There should be no reason to 
override those methods and it will help ensure that the contract of 
those methods are not being overridden by a subclass incorrectly.


--Sean

Re: RFR: ChaCha20 and ChaCha20/Poly1305 Cipher implementations

2018-04-26 Thread Sean Mullan 

On 4/26/18 11:57 AM, Sean Mullan wrote:
The ChaCha20ParameterSpec.java file should have an @since 11 annotation 
on it.


Also:

  65 if (nonce.length == 12) {
  66 this.nonce = nonce.clone();
  67 } else {
  68 throw new IllegalArgumentException(
  69 "Nonce must be 96-bits in length");
  70 }

You should clone nonce before you check the length and check the length 
on the copy, not the parameter passed in. Also, you should use 
NONCE_LENGTH instead of 12 since it is already defined as a constant in 
the class.


--Sean



--Sean

On 3/26/18 3:08 PM, Jamil Nimeh wrote:

Hello all,

This is a request for review for the ChaCha20 and ChaCha20-Poly1305 
cipher implementations.  Links to the webrev and the JEP which 
outlines the characteristics and behavior of the ciphers are listed 
below.


http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.01/
http://openjdk.java.net/jeps/329

Thanks,
--Jamil

Re: RFR: ChaCha20 and ChaCha20/Poly1305 Cipher implementations

2018-04-26 Thread Jamil Nimeh 
The NONCE_LENGTH I fixed last night in response to JVT comments.  I will do the 
clone also.  Thanks for the comments!


--Jamil
 Original message From: Sean Mullan  
Date: 4/26/18  10:22 AM  (GMT-08:00) To: Jamil Nimeh 
, OpenJDK Dev list  
Subject: Re: RFR: ChaCha20 and ChaCha20/Poly1305 Cipher implementations 
On 4/26/18 11:57 AM, Sean Mullan wrote:
> The ChaCha20ParameterSpec.java file should have an @since 11 annotation 
> on it.

Also:

   65 if (nonce.length == 12) {
   66 this.nonce = nonce.clone();
   67 } else {
   68 throw new IllegalArgumentException(
   69 "Nonce must be 96-bits in length");
   70 }

You should clone nonce before you check the length and check the length 
on the copy, not the parameter passed in. Also, you should use 
NONCE_LENGTH instead of 12 since it is already defined as a constant in 
the class.

--Sean

> 
> --Sean
> 
> On 3/26/18 3:08 PM, Jamil Nimeh wrote:
>> Hello all,
>>
>> This is a request for review for the ChaCha20 and ChaCha20-Poly1305 
>> cipher implementations.  Links to the webrev and the JEP which 
>> outlines the characteristics and behavior of the ciphers are listed 
>> below.
>>
>> http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.01/
>> http://openjdk.java.net/jeps/329
>>
>> Thanks,
>> --Jamil

Re: RFR: ChaCha20 and ChaCha20/Poly1305 Cipher implementations

2018-04-26 Thread Jamil Nimeh 
Will do.  I made some wording changes there to move away from "96-bit" to 
"12-byte" also since that probably is more useful to developers.


--Jamil
 Original message From: Sean Mullan  
Date: 4/26/18  8:57 AM  (GMT-08:00) To: Jamil Nimeh , 
OpenJDK Dev list  Subject: Re: RFR: ChaCha20 and 
ChaCha20/Poly1305 Cipher implementations 
The ChaCha20ParameterSpec.java file should have an @since 11 annotation 
on it.

--Sean

On 3/26/18 3:08 PM, Jamil Nimeh wrote:
> Hello all,
> 
> This is a request for review for the ChaCha20 and ChaCha20-Poly1305 
> cipher implementations.  Links to the webrev and the JEP which outlines 
> the characteristics and behavior of the ciphers are listed below.
> 
> http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.01/
> http://openjdk.java.net/jeps/329
> 
> Thanks,
> --Jamil

Re: RFR: ChaCha20 and ChaCha20/Poly1305 Cipher implementations

2018-04-26 Thread Sean Mullan 
The ChaCha20ParameterSpec.java file should have an @since 11 annotation 
on it.


--Sean

On 3/26/18 3:08 PM, Jamil Nimeh wrote:

Hello all,

This is a request for review for the ChaCha20 and ChaCha20-Poly1305 
cipher implementations.  Links to the webrev and the JEP which outlines 
the characteristics and behavior of the ciphers are listed below.


http://cr.openjdk.java.net/~jnimeh/reviews/8153028/webrev.01/
http://openjdk.java.net/jeps/329

Thanks,
--Jamil