Subject: Re: [Gen-art] Gen-art LC review of draft-ietf-dime-ovli-08 - with summary

From: Steve Donovan <[email-masked]>
Date: Tue, 04 Aug 2015 07:23:39 -0500

Elwyn,

Please see my responses inline.

Regards,

Steve

On 7/23/15 11:32 AM, Elwyn Davies wrote:
> I am the assigned Gen-ART reviewer for this draft. For background on
> Gen-ART, please see the FAQ at
>
> <http://wiki.tools.ietf.org/area/gen/trac/wiki/GenArtfaq>.
>
> Please resolve these comments along with any other Last Call comments
> you may receive.
>
> Document: draft-ietf-dime-ovli-08.txt
> Reviewer: Elwyn Davies
> Review Date: 2015/07/23
> IETF LC End Date: 2015/07/27
> IESG Telechat date: (if known) -
>
> Summary: Ready with nits.  A very well written document - thanks. Some 
> minor issues with out-of-date or OBE'd statements that need a bit of 
> tweaking.
SRD> Thanks, we had a good group of people working on it.
>
> Major issues:
>
> Minor issues:
> s4.2:
> Is there a need for and consequently how would one either cancel the 
> current abatement algorithm or select a different one?  I guess this 
> might happen if the reacting node that was running the algorithm went 
> away or itself became overloaded.  However I I have no idea whether 
> this is a reasonable question!  OK.. well I see in s5.1.1 that the 
> abatement algorithm can be changed... a pointer in s4.2 would be 
> useful.  But could it be turned off?
SRD> OK.
>
> Appendix A.3:  The statements in this appendix are not 'future proof'. 
> Referring to ongoing actions of the DIME WG will have little value in 
> the future.  Maybe something like "There is an expectation that <these 
> developments> will occur and can be integrated with the features 
> described in this document."
SRD> Good suggestion.
>
> Appendix C.1, para 1: There are some tense and past/future issues in 
> this section - It is now mid 2015 and this para refers in the future 
> to 'end of 2014'.  Please rewrite to reflect current actualité and in 
> a way that will be relevant when this is published as an RFC.
SRD> I propose removing Appendix C in its entirety.
>
> Appendix C.5:
>> C.5.  No New Vulnerabilities
>>
>>     The working group believes that DOIC is compliant with the
>>     requirement to avoid introducing new vulnerabilities. However, this
>>     requirement may warrant an early security expert review.
> Hmm!  I fear that it would difficult to consider this draft 'ready' if 
> there is no reasonable consensus that it hasn't introduced any new 
> vulnerabilities.  Has the security expert review actually happened?
SRD> No, a security expert review has not happened, other then Stephen's 
review.
>
>> REQ 13: The solution MUST NOT introduce substantial additional work
>>             for a node in an overloaded state.  For example, a
>>             requirement for an overloaded node to send overload
>>             information every time it received a new request would
>>             introduce substantial work.
>>
>>             *Not Compliant*. DOIC does in fact encourage an overloaded
>>             node to send an OLR in every response.  The working group
>>             that other mechanisms to ensure that every relevant node
>>             receives an OLR would create even more work. ****[Note: This
>>             needs discussion.]****
> Has this discussion occurred?  Does the WG have consensus that this 
> derogation from the requirements is acceptable?
SRD> Yes, there is consensus on the mechanism as defined.
>
> Nits/editorial comments:
> General: s/i.e./i.e.,/ g; s/e.g./e.g.,/g
> General: Consistent use of "non-supporting" vs "non supporting" (lots 
> of places)
SRD> Ok.
>
> s2, Host-Routed Requests:
> Expand acronym AVP (first use).
SRD> Ok.
>
> s5.2.1.1:
>>     A host-type OCS entry is identified by the pair of application-id 
>> and
>>     the node's DiameterIdentity.
>>
>>     A realm-type OCS entry is identified by the pair of application-id
>>     and realm.
> The application-id, DiameterIdentity and realm presumably come from 
> the various messages taht are being piggybacked on.  A reference to 
> the relevant section of the RFC that gives definitions of these (and 
> which messages to get them from) would be helpful.  Later... this 
> issue is partially resolved by s7.3.  A pointer to that section would 
> be desirable.  s7.3 needs a reference to where the relevant messages 
> are defined.
SRD> I agree that a pointer to section 7.3 would be helpful.
>
> s5.2.1.4, last para:
>>     A reporting node MUST keep an OCS entry with a validity duration of
>>     zero ("0") for a period of time long enough to ensure that any non-
>>     expired reacting node's OCS entry created as a result of the 
>> overload
>>     condition in the reporting node is deleted.
> Is it possible to offer any guidance on what constitutes 'long enough'?
SRD> Long enough is based on when all of the expiration timers in OLRs 
sent by the reporting node have expired.  This is, in essence, what it 
already says.
>
> s5.2.2 (in Note):
>>   Any extension
>>        that defines a new abatement treatment must also defined the
>>        interaction of the new abatement treatment with existing
>>        treatments.
> s/defined/define/
SRD> Agreed.
>
> s5.3, paras 2, 3, 5,  7  (but probably not the MUST NOT in para 6):
> In line with the text in the previous comment, I think s/MUST/must/, 
> s/MAY/may/ - they are not things the protocol does
SRD> I'm agreeable to this change if the rest of the DIME group is ok.
>
> s7:
> Pointers are needed to the RFC sections that define the data types and 
> the AVP syntax definition structure used in this section.
SRD> This is in the table in section 7.8.
>
> s7.3 - see comments on s5.2.1.1 above
SRD> See above comment.
>
> s8, para 6: s/Diameter identity/DiameterIdentity/ (possibly)
SRD> I'm don't see the need for the change.  It's already talking about 
the Destination-Host AVP, which carries the DiameterID.  This is just 
prose for saying that.
>
> s9.2:  It doesn't seem obvious to me that the values are supposed to 
> have names in the registry.  Surely s/b
>   (e.g.)
>     Feature Vector Entry Name
>     Feature Vector Entry Value
>     Specification...
SRD> I'm open to guidance on this.  I don't have a lot of experience in 
defining IANA requirements.
>
> s10.3, last para:
>>     Requirement 28 [RFC7068] indicates that the overload control 
>> solution
>>     cannot assume that all Diameter nodes in a network are trusted, and
>>     that malicious nodes not be allowed to take advantage of the 
>> overload
>>     control mechanism to get more than their fair share of service.
> I can't parse the last phrase of this (from "and that malicious..." 
> onwards.  I know what you mean..
> Suggest s/trusted, and that/trusted.  It also requires that/
SRD> Ok.
>
> s10.4, last para:
>>     At the time of this writing, the DIME working group is studying
>>     requirements for adding end-to-end security features
>>     [I-D.ietf-dime-e2e-sec-req] to Diameter.
> This statement has pretty much been OBE'd.  the draft is in WG last 
> call.  I take it we can assume that the draft knows what is being 
> specified: thus
>> These features, when they
>>     become available, might make it easier to establish trust in non-
>>     adjacent nodes for overload control purposes.
> It should be possible to make a more concrete statement here.
SRD> The work on e2e security is still in a requirements stage. As such, 
we don't yet know what is being specified.
>
>
>
> App C.1, para 1: s/normative references/a normative  reference/
SRD> I am proposing removing all of appendix C.
>
> App D.1, para 1: s/identity/entity/ (I think)
SRD> Agreed.

---

From: "Ben Campbell" <[email-masked]>
Date: Tue, 04 Aug 2015 09:10:53 -0500

(as individual)

Hi,

I'm mostly leaving this to Steve, but I do want to touch on one of the 
comments, below:

On 4 Aug 2015, at 7:23, Steve Donovan wrote:

>> Hmm!  I fear that it would difficult to consider this draft 'ready' 
>> if there is no reasonable consensus that it hasn't introduced any new 
> SRD> No, a security expert review has not happened, other then 
> Stephen's review.

Also, the point of the statement that the "working group believes..." is 
to show working group consensus. The early review mention was merely to 
try to be extra diligent. And while an _early_ review didn't happen, I 
think the fact that Stephen is now the responsible AD serves a similar 
purpose.

Thanks!

Ben.

---

From: Elwyn Davies <[email-masked]>
Date: Wed, 05 Aug 2015 15:21:05 +1000

Hi, Steve and Ben.

Thanks for the responses.

I take it Appendix C is being fed to the bit bucket, which is fair 
enough, so no more issues there.

Otherwise it looks mostly done.   A couple of comments inline below.

I'll look out for a new version when I return from the Outback in a 
couple of weeks.

Cheers,
Elwyn
(just on his way to Darwin).

On 04/08/2015 22:23, Steve Donovan wrote:
>> Summary: Ready with nits.  A very well written document - thanks. 
>> Some minor issues with out-of-date or OBE'd statements that need a 
>> bit of tweaking.
So... how would one turn it off if that was allowed/wanted/possible?
>>
>> Appendix A.3:  The statements in this appendix are not 'future 
>> proof'. Referring to ongoing actions of the DIME WG will have little 
>> value in the future.  Maybe something like "There is an expectation 
>> that <these developments> will occur and can be integrated with the 
>> features described in this document."
> SRD> Good suggestion.
OK
> SRD> I propose removing Appendix C in its entirety.
This seems to be consensus!
Ben Campbell added:
> Also, the point of the statement that the "working group believes..." 
> is to show working group consensus. The early review mention was 
> merely to try to be extra diligent. And while an _early_ review didn't 
> happen, I think the fact that Stephen is now the responsible AD serves 
> a similar purpose.
>
> Thanks!
>
> Ben. 
Stephen is surely a security expert so we'll leave it to him :-)
> SRD> Yes, there is consensus on the mechanism as defined.
Fair enough.  Do you think a comment about this decision in the body 
would help?  Particularly to make sure that an overloaded node does the 
minimum possible.
>> Nits/editorial comments:
This is all pretty much agreed.. couple of points still below.
>>>     A host-type OCS entry is identified by the pair of 
>>> application-id and
>> Pointers are needed to the RFC sections that define the data types 
>> and the AVP syntax definition structure used in this section.
Sorry - this is just an internal pointer - I think there needs to be 
pointers to (?) parts of RFC 6733 where the data types (e.g. Unsigned64) 
and the AVP syntax used for the definitions themselves are defined.
> SRD> I'm don't see the need for the change.  It's already talking 
> about the Destination-Host AVP, which carries the DiameterID.  This is 
> just prose for saying that.
Alright.  I'll live with this.
> SRD> I'm open to guidance on this.  I don't have a lot of experience 
> in defining IANA requirements.
>>
>> s10.3, last para:
> SRD> The work on e2e security is still in a requirements stage. As 
> such, we don't yet know what is being specified.
At least take the WG out of the text.
> SRD> I am proposing removing all of appendix C.
OK

---

From: Steve Donovan <[email-masked]>
Date: Thu, 06 Aug 2015 11:17:14 -0500

Elwyn,

One question is answered below.  The remainder of your comments have 
been updated in the latest version of the draft (which will be uploaded 
soon).

Regards,

Steve
<snip>
>>> might happen if the reacting node that was running the algorithm 
>>> went away or itself became overloaded. However I I have no idea 
>>> whether this is a reasonable question!  OK.. well I see in s5.1.1 
>>> that the abatement algorithm can be changed... a pointer in s4.2 
>>> would be useful.  But could it be turned off?
>> SRD> OK.
SRD> While not suggested, the selected abatement algorithm can be 
changed on a transaction by transaction basis.  Once an OLR has been 
sent the reporting node would need to cancel the existing overload 
report in one transaction and then send a new overload report for the 
new algorithm in a subsequent overload report.

---

From: Jari Arkko <[email-masked]>
Date: Thu, 20 Aug 2015 00:13:01 +0300

Elwyn, thanks for your in-depth review. And Steve, thank you for reacting to the reviews and taking action.

Jari