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

[Elwyn Davies <[e-mail]>]

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:
> 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.
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
>>
>> 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.
This seems to be consensus!
>
>>
>> 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.
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 :-)
>>
>>> 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.
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.
>>
>> 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.
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.
>>
>> 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.
Alright.  I'll live with this.
>>
>> 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.
At least take the WG out of the text.
>>
>>
>>
>> App C.1, para 1: s/normative references/a normative  reference/
> SRD> I am proposing removing all of appendix C.
OK
>
>>
>> App D.1, para 1: s/identity/entity/ (I think)
> SRD> Agreed.
>