Re: [HOKEY] AD review of draft-ietf-hokey-erp-aak

[Zhen Cao <[e-mail]>]

Thanks a lot.  I will work out the revision with my co-authors soon.

On Thu, Jan 12, 2012 at 6:17 PM, Stephen Farrell
<[e-mail]> wrote:
>
> Hiya,
>
> A few notes below but mostly those changes look like they
> should be fine.
>
> Any idea when the new I-D will pop out? Be good to get this
> moving along...
>
>

>
> I think if you added more text to explain the hierarchy that'd
> be good for the reader. But if you say that's known to folks who
> are familiar with ERP I can live with that. Good to add the
> KDFs all right.
>
okay.


>> #17, p8 - Is the sequence number TLV called "Sequence"? If not,
>> then are you allowed spaces in TLV names?
>>
>> [CZ]: In this draft, the sequence number is defined as TV format
>> rather than TLV format.
>> In the sequence number TV format, 8 bit type for sequence number will
>> be followed by 16 bit
>> Sequence number value.
>
>
> I was really asking about whether or not TV or TLV names can be
> anything of it there's a rule or convention e.g. not include
> spaces in those names. (Now that I look I see you also have some
> TV names with a "/" character in them). I guess that's a bit
> of a nit though.
>
> But now that you mention it, where does it say that the sequence
> number TV value is 16 bits long? It does say that for the SEQ
> field but maybe needs repeating.

That's the same with ERP, but we will also mention it in our document.


>> [CZ]: In the request (i.e.,EAP-Initiate/Re-auth) message to the
>> server, only CAP-Identifier needs to be present
>> In the response (i.e.,EAP-Finish/Re-auth) message from the server, all
>> the sub TLV including CAP-Identifier and pMSK-lifetime and Cryptosuit
>> should be present in the message.
>
>
> I think that needs to be specified here. So adding that would
> be good (unless its there already and I missed it;-)

Thanks.

>> #26, p10 - From when is the lifetime of the pMSK calculated? (Same
>> for the pRK-lifetime field.) Are these signed or unsigned 32-bit
>> values?
>>
>> [CZ]: Upon receiving EAP-Initiate message from the peer, the server
>> begin to calculate pRK,pMSK.
>> Upon receiving EAP-Finish message, the peer begin to calculate PMSK,pRK.
>
>
> Ok. I think you need to say that. And also specify if the 32 bits
> are unsigned or signed. (Signed probably makes no sense really but
> good to say that.) Note that time_t is signed so there's an easy
> bug for coders to trip over here.
>
> I think there's no harm here in large values, right? Since 2^32 seconds
> is 136 years there could be a DoS if something were just stored that
> long but I believe that's not a problem. Correct me if I'm wrong.

Correct. unsigned 32 bits. Thank you.

>> #31, p10 - The list of cryptosuites desription says "The server
>> SHOULD include this if the...was not acceptable and is being
>> rejected." That's pretty ambiguous. What's the exception to the
>> SHOULD? Does "is being rejected" mean that this MUST NOT be present
>> when things worked ok?
>>
>> [CZ]: No. You have some misunderstanding to the cryptosuite descrption.
>> We define Cryptosuite field in both EAP-initiate message and EAP-Finish
>> message,
>> We also allow carry Crytosuite List TLV( Type is 5) in both
>> EAP-Initiate message and EAP-Finish message.
>> If Cryptosuite included in the Cryptosuite field of EAP-Initiate
>> message is rejected by the server, the server can indicate to the peer
>> the cryptosuit list the server supports using cryptosuite list TLV.
>> Does this clarify?
>
>
> Yep. Good to say that in the spec (unless its there and I missed
> it as usual:-)

okay.

>
>> #33, p11 - Section 7 says a new message "should" be defined - who's
>> gonna do that where? Seems odd that its not here.
>>
>> [CZ]: Could be removed if you think odd.
>
>
> I think it is odd. Is this something that's needed though? If
> so, who's doing the work or in what document is the required
> message defined? What are the consequences of that not being
> defined?

okay, will remove this.