Subject: [Gen-art] Gen-ART LC Review of draft-ietf-forces-model-extension-03

From: Ben Campbell <[email-masked]>
Date: Thu, 7 Aug 2014 17:51:18 -0500

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-forces-model-extension-03
Reviewer: Ben Campbell
Review Date: 2014-08-08
IETF LC End Date: 2014-09-09

Summary: Nearly ready for publication as a proposed standard, but there are some issues that should be considered first, and some editorial issues that might also be worth consideration.

Note 1: I am not an expert in xml schema specifications. I hope assume others have reviewed the schema, and mechanically verified them. 

Note 2: I notice that the Adrian's AD review commented about the lack of working group review, while the shepherd writeup comments that the working group had a solid consensus on this draft. On the surface, those comments seem to conflict. (I don't expect the author to take any action on this)

Major issues: 

-- IDNits points out that this draft may need the pre-RFC5378 boilerplate, due to content from RFC 5812. It does appear to include substantial amounts of XML schema from 5812. While the publication date of 5812 post-dates RFC5378, there were many revisions of the draft form that pre-date 5378 by some time. So unless the author has gotten permission from all 5812 authors (and I note that author list changed several times prior to publication), this draft and any resulting RFC may well need the boilerplate.

Minor issues:

-- section 2.3, 3rd paragraph:

The 2119 language in this paragraph seems more like description of procedures. You really only need 2119 language when you want to constrain some choice an implementor might make. Also, in the two cases of "MUST be ignored", please consider using active voice? (That is, who or what must ignore it?)

-- section 2.4, 2nd paragraph:

This also seems like a description of general procedures that doesn't really need 2119 language.

-- section 2.4, bulleted paragraph:

Is this similar to saying that eventBecomesEqualTo effectively becomes an eventChanged after achieving the target value? Once the value changes from the target by V or more, does it reset the becomesEqualTo trigger?

-- 2.6, 2nd paragraph: "... derivedFrom will always select the latest version."

What if a newer version of the parent is created after the child is defined? Can it cause backwards compatibility issues if the parent class changes out from under the child?

-- section 6:

The assertion that the changes in this draft have no effect on security is a bit of a red flag. This draft introduces new kinds of properties and events that can be expressed, as well as a change to the inheritance model. Are you sure none of those have a security impact? It would at least be good to mention, for each of these changes, why you think it does not affect the prior model security considerations.

Nits/editorial comments: 

-- Abstract, 2nd paragraph: 

Please expand LFP on first use.
s/ "... update RFC5812 ..." / "... updates RFC5812 ..."

-- section 1, first paragraph:

Please expand "FEs" on first use . (I know you did in the abstract, but the body and abstract should each be able to stand alone.)

-- section 1, 2nd paragraph, last sentence

Sentence appears to be missing one or more commas.

-- section 2.1, 2nd paragraph: " ... can be seen in the OpenFlow switch 1.1 ..."

I assume you mean the OpenFlow1.1 specification, not any particular switch.

-- 2.1, paragraph 5:

The proximity of the description of figure 2 and the beginning of figure 1 may be confusing. I suggest moving figure 1 to immediately after it's description in the first paragraph of the section.

-- section 2.2, 2nd paragraph: "... allows optional to add ..."

I suggest "allows the optional addition" or "allows the option to add"

-- section 2.2, paragraph after Figure 4: "Additionally this document appends to the declaration of the AtomicType from Figure 5 to Figure 6 to allow default values to Atomic datatypes."

Sentence is difficult to parse.

-- section 2.3, 1st paragraph: "

2nd sentence is awkward. It seems to agree with previous sentence in spite of "however". Also, what is antecedent of it's?

-- section 2.3, 2nd paragraph, first sentence: " With this extension is it allowed to define..."

I suggest "This extension allows the definition ..."

-- section 2.4, first paragraph:" ... when the value is exactly ..."

s / is / becomes

-- section 2.4, last paragraph:

I can't parse "take into account to use". Do you mean "consider using..."?

-- section 2.5, 1st paragarph:

By "current model", do you mean prior to this update? Keep in mind that once this is published, the updated version will be the current one. Also, "sent/received" should be "sent and received" or "sent or received".

-- 2.5, 2nd paragraph:

s / defines / specifies 

Also, what does it mean to "target LFB properties". Do you mean "refer to..."?

-- Figure 13, caption:

Missing "of" after "disallowing". Or consider "New XML to disallow..."

-- 2.6, 1st paragraph:

should "... LFB class may inherit." be "LPB class inherits."?

" ...  different versions of an LFB class exists."

s / exists / exist

Also, should that be different versions of the _parent_ class?

-- 2.7, 2nd paragraph:

Consider active voice. e.g., "This document introduces..."

---

From: "Haleplidis Evangelos" <[email-masked]>
Date: Sat, 16 Aug 2014 01:43:17 +0300

Greetings Ben,

I was slow to respond as I am currently on vacations.

Thank you very much for your extended review on the document. Please see
inline.

I have prepared all the necessary changes and the only thing that remains is
the boilerplate issue (and perhaps the security issues). When we resolve
this I can publish the new version.

Regards,
Evangelos Haleplidis.

> Summary: Nearly ready for publication as a proposed standard, but there
> are some issues that should be considered first, and some editorial
> issues that might also be worth consideration.
> 
> Note 1: I am not an expert in xml schema specifications. I hope assume
> others have reviewed the schema, and mechanically verified them.

[ΕΗ] I don't know if anyone else has verified the schema, but I have
verified it with verification tools and created sample xml libraries.

> Note 2: I notice that the Adrian's AD review commented about the lack
> of working group review, while the shepherd writeup comments that the
> working group had a solid consensus on this draft. On the surface,
> those comments seem to conflict. (I don't expect the author to take any
> action on this)
> 
> Major issues:
> 
> -- IDNits points out that this draft may need the pre-RFC5378
> boilerplate, due to content from RFC 5812. It does appear to include
> substantial amounts of XML schema from 5812. While the publication date
> of 5812 post-dates RFC5378, there were many revisions of the draft form
> that pre-date 5378 by some time. So unless the author has gotten
> permission from all 5812 authors (and I note that author list changed
> several times prior to publication), this draft and any resulting RFC
> may well need the boilerplate.
> 

[ΕΗ] Thank you for catching this as I have very little experience with this
kind of issues.
As I have not asked permission from the RFC5812 authors, what should be the
proper way to resolve this issue? 
Ask for permission or use a previous boilerplate?

> The 2119 language in this paragraph seems more like description of
> procedures. You really only need 2119 language when you want to
> constrain some choice an implementor might make. Also, in the two cases
> of "MUST be ignored", please consider using active voice? (That is, who
> or what must ignore it?)
> 

[ΕΗ] In this specific case (I will reword it) the MUST is a constraint that
the implementer has to conform for interoperability. Otherwise the model
would mean something different from one implementer to the other, e.g. for
A's implementation the struct's component may have an access type of
"read-write" but for B it may mean "read-only". The MUST there defines that
when you define a struct component's access type, it MUST override the whole
struct's access type.

> really need 2119 language.

[ΕΗ] Again I think that the MUST is something that is needed for
interoperability issues. The BecomesEqualTo MUST generate only one event
when the component reaches that specific value and not while it is there. 

> Is this similar to saying that eventBecomesEqualTo effectively becomes
> an eventChanged after achieving the target value? Once the value
> changes from the target by V or more, does it reset the becomesEqualTo
> trigger?
> 

[ΕΗ] No, it does not effectively become an eventChanged. The difference is
that it will only be triggered ONCE when the value is changed and not
continuously.
And yes, it resets the BecomesEqualTo trigger.

> What if a newer version of the parent is created after the child is
> defined? Can it cause backwards compatibility issues if the parent
> class changes out from under the child?
> 

[ΕΗ] Thank you for catching this. You're right, this may create backwards
compatibility issues.
This was an error on my part while writing the update of the draft. My
initial response was to use the first version (see
http://www.ietf.org/mail-archive/web/forces/current/msg04838.html) so it
would have been "select the first version".

I have made the necessary correction and made the text better. It now reads
as follows:
"However if the version is omitted then the derivedFrom will always specify
the first version of the parent LFB class, which usually is version 1.0."

Using always the first will mean something that is always there, and
resolves any ambiguities.

> -- section 6:
> 
> The assertion that the changes in this draft have no effect on security
> is a bit of a red flag. This draft introduces new kinds of properties
> and events that can be expressed, as well as a change to the
> inheritance model. Are you sure none of those have a security impact?
> It would at least be good to mention, for each of these changes, why
> you think it does not affect the prior model security considerations.
> 

[ΕΗ] I am no security expert, but I agree with the security considerations
of RFC 5812 and have referred that the same applies to this document.
As we didn't make any major changes in the model, but simply added a few
more items, namely a new event, some new properties and a way to define
optional access types and complex metadata. 
These are constructs and do not add anything controversial to the initial
model. Now in regards to the inheritance model, we didn't add something new,
we simply clarified an issue that existed by introducing the version of the
derived from class. 

However, as I said I'm no security expert (and I don't know if you're one),
if a security expert could comment on this, it could be helpful.

> Nits/editorial comments:

[ΕΗ] Thank you for all the nits/editorial comments. All have been taken into
account.

---

From: Ben Campbell <[email-masked]>
Date: Tue, 19 Aug 2014 18:02:02 -0500

Hi, thanks for the response. Comments inline. I've removed sections that don't seem to need further comment.

> verified it with verification tools and created sample xml libraries.

Works for me!

[...]

> Ask for permission or use a previous boilerplate?

Asking permission is the best choice, because it would allow you to publish this one with the "fully complies" boilerplate. That has the advantage of allowing any future IETF works incorporate content from it without having the same issue.

But that may be a tricky thing to do. I note that the drafts leading up to the publication of RFC5812 rotate through a number of authors at different times. It may be a bit tricky to figure out the proper list of authors to ask permission from. Therefore I would not object if you went with the pre-5378 boilerplate. Details are in section 6.d.iii. of http://trustee.ietf.org/license-info/IETF-TLP-4.htm .

If you are using xml2rfc, the tool can insert the boilerplate for you. For details, see http://xml2rfc.ietf.org and scroll down to the light-green box that talks about the IETF Trust Legal Provisions boilerplate.

> when you define a struct component's access type, it MUST override the whole

In this particular case, it's not clear to me if the MUST actually constrains a choice vs being a statement of fact. If you believe it to be the former then I am okay with it. The rewording might help.

> that it will only be triggered ONCE when the value is changed and not

You might consider something like:

" The event is triggered only when the value of the targeted component becomes equal to the event condition value. Implementations MUST NOT generate subsequent events while the targeted component’s value remains equal to the event condition’s value."

[...]

> resolves any ambiguities.

Okay.

> if a security expert could comment on this, it could be helpful.

No, I am not one.  Hopefully this will get a SecDir review as well. But that sort of review usually goes better if the Security Consideration section shows your reasoning, along the lines of listing the high-level types of changes, and for each, why it has no new security impact. Your response contains more of that sort of thing; it might help to add it (or parts of it) to the draft. 

I was a bit concerned that the default version for inheritance could be an issue, but you addressed that elsewhere.

[...]

---

From: "Haleplidis Evangelos" <[email-masked]>
Date: Thu, 21 Aug 2014 01:00:38 +0300

Greetings Ben,

Thank you for your response. Please see inline.

Regards,
Evangelos Haleplidis.

> From: Ben Campbell [mailto:[email-masked]]
> 

[ΕΗ] Same here! :)

> >> substantial amounts of XML schema from 5812. While the publication
> >> date of 5812 post-dates RFC5378, there were many revisions of the
> >> draft form that pre-date 5378 by some time. So unless the author has
> >> gotten permission from all 5812 authors (and I note that author list
> >> changed several times prior to publication), this draft and any
> >> resulting RFC may well need the boilerplate.
> >>
> >
> > [ΕΗ] Thank you for catching this as I have very little experience
> with
> > this kind of issues.
> > As I have not asked permission from the RFC5812 authors, what should
> > be the proper way to resolve this issue?
> Asking permission is the best choice, because it would allow you to
> publish this one with the "fully complies" boilerplate. That has the
> advantage of allowing any future IETF works incorporate content from it
> without having the same issue.
> 
> But that may be a tricky thing to do. I note that the drafts leading up
> to the publication of RFC5812 rotate through a number of authors at
> different times. It may be a bit tricky to figure out the proper list
> of authors to ask permission from. Therefore I would not object if you
> went with the pre-5378 boilerplate. Details are in section 6.d.iii. of
> http://trustee.ietf.org/license-info/IETF-TLP-4.htm .
> 	
> If you are using xml2rfc, the tool can insert the boilerplate for you.
> For details, see http://xml2rfc.ietf.org and scroll down to the light-
> green box that talks about the IETF Trust Legal Provisions boilerplate.
> 

[ΕΗ] I discussed with Joel with regards to the copyright issues.
The short answer is that this document draws directly from RFC5812 and
relies on RFC5812 for such issues (as it uses the same boilerplate).

Is this satisfactory?

> >> constrain some choice an implementor might make. Also, in the two
> >> cases of "MUST be ignored", please consider using active voice?
> (That
> >> is, who or what must ignore it?)
> >>
> >
> > [ΕΗ] In this specific case (I will reword it) the MUST is a
> constraint
> > that the implementer has to conform for interoperability. Otherwise
> > the model would mean something different from one implementer to the
> > other, e.g. for A's implementation the struct's component may have an
> > access type of "read-write" but for B it may mean "read-only". The
> > MUST there defines that when you define a struct component's access
> > type, it MUST override the whole struct's access type.
> 
> In this particular case, it's not clear to me if the MUST actually
> constrains a choice vs being a statement of fact. If you believe it to
> be the former then I am okay with it. The rewording might help.
> 

[ΕΗ] I reworded it and provided also an example. The text now reads:

"When optional access type for components within a struct are defined, these
components's access type MUST override the access type of the struct. For
example if a struct has an access type of read-write but has a component
that is a read-only counter, the counter's access type MUST be read-only."

I believe that it is an implementation constraint as there are two
possibilities (override or not). With the "MUST" we constrain it to one
(override).

I also changed the two "it MUST be ignored" to "the access type MUST be
ignored" to better specify what "it" is.

> >> This also seems like a description of general procedures that
> doesn't
> >> really need 2119 language.
> >
> > interoperability issues. The BecomesEqualTo MUST generate only one
> > event when the component reaches that specific value and not while it
> is there.
> >
> >>
> >> Is this similar to saying that eventBecomesEqualTo effectively
> >> becomes an eventChanged after achieving the target value? Once the
> >> value changes from the target by V or more, does it reset the
> >> becomesEqualTo trigger?
> >>
> >
> > [ΕΗ] No, it does not effectively become an eventChanged. The
> > difference is that it will only be triggered ONCE when the value is
> > changed and not continuously.
> 
> " The event is triggered only when the value of the targeted component
> becomes equal to the event condition value. Implementations MUST NOT
> generate subsequent events while the targeted component?s value remains
> equal to the event condition?s value."
> 

[ΕΗ] Text changed to your suggestion. Thank you.

> >
> >> -- section 6:
> >>
> >> The assertion that the changes in this draft have no effect on
> >> security is a bit of a red flag. This draft introduces new kinds of
> >> properties and events that can be expressed, as well as a change to
> >> the inheritance model. Are you sure none of those have a security
> impact?
> > [ΕΗ] I am no security expert, but I agree with the security
> > considerations of RFC 5812 and have referred that the same applies to
> this document.
> > As we didn't make any major changes in the model, but simply added a
> > few more items, namely a new event, some new properties and a way to
> > define optional access types and complex metadata.
> > These are constructs and do not add anything controversial to the
> > initial model. Now in regards to the inheritance model, we didn't add
> > something new, we simply clarified an issue that existed by
> > introducing the version of the derived from class.
> >
> > However, as I said I'm no security expert (and I don't know if you're
> > one), if a security expert could comment on this, it could be
> helpful.
> >
> 
> No, I am not one.  Hopefully this will get a SecDir review as well. But
> that sort of review usually goes better if the Security Consideration
> section shows your reasoning, along the lines of listing the high-level
> types of changes, and for each, why it has no new security impact. Your
> response contains more of that sort of thing; it might help to add it
> (or parts of it) to the draft.
> 
> I was a bit concerned that the default version for inheritance could be
> an issue, but you addressed that elsewhere.
> 
> [...]=

[ΕΗ] Ok, added part of this. Now the security considerations read the
following:

This document adds only a few constructs to the initial model defined in
RFC5812, namely namely a new event, some new properties and a way to define
optional access types and complex metadata. These constructs do not change
the nature of the the initial model. In addition this document addresses and
clarifies an issue with the inheritance model by introducing the version of
the derivedFrom LFB class.
Thus the security considerations defined in RFC5812 applies to this document
as well as the changes proposed here are simply constructs to write XML
library definitions, as where in RFC5812 and have no effect on security
semantics with the protocol.

---

From: Ben Campbell <[email-masked]>
Date: Wed, 20 Aug 2014 17:21:55 -0500

> relies on RFC5812 for such issues (as it uses the same boilerplate).

Hrmm. So it does. I somehow had it in my head it had the older boilerplate. I must have gotten that from one of the draft versions So, never mind :-)

(It's interesting that IDNits apparently looked at the date of publication of the first 00 draft, not the RFC. I'm curious the history of what happened with RFCs that were in-process works and had changes in authorship at the time 5378 was published--but that's not this draft's problem and should probably happen in a bar discussion.)

[...]

> ignored" to better specify what "it" is.

This helps. 

For the record, my suggestion on more active voice was to say what must do the ignoring. But I think what you've got is good enough.

[...]

> semantics with the protocol.

You might consider adding something to say that the inheritance model change also does not change the security considerations. (Maybe it makes things better, by removing the potential for choosing a wrong parent class? Not sure if that's a security issue, unless there was some kind of parent-assertion attack.)

 It does seem like the inheritance change is a bona-fide extension, not just a clarification, since you added the version attribute.

---

From: "Haleplidis Evangelos" <[email-masked]>
Date: Fri, 22 Aug 2014 01:26:07 +0300

Greetings Ben,

Thank you very much for the review and the discussion.
I have made all the relevant changes and have submitted (just in time it
seems) the new version.

Regards,
Evangelos Haleplidis.

> -----Original Message-----
> Sent: Thursday, August 21, 2014 1:22 AM
> To: Haleplidis Evangelos
> Cc: [email-masked]; gen-
> [email-masked]; [email-masked]
> Subject: Re: Gen-ART LC Review of draft-ietf-forces-model-extension-03
> 
> 
> Hrmm. So it does. I somehow had it in my head it had the older
> boilerplate. I must have gotten that from one of the draft versions So,
> never mind :-)
> 
> (It's interesting that IDNits apparently looked at the date of
> publication of the first 00 draft, not the RFC. I'm curious the history
> of what happened with RFCs that were in-process works and had changes
> in authorship at the time 5378 was published--but that's not this
> draft's problem and should probably happen in a bar discussion.)
> 
> [...]
> 
> >>
> >> constrains a choice vs being a statement of fact. If you believe it
> >> to be the former then I am okay with it. The rewording might help.
> >>
> >
> > "When optional access type for components within a struct are
> defined,
> > these components's access type MUST override the access type of the
> > struct. For example if a struct has an access type of read-write but
> > has a component that is a read-only counter, the counter's access
> type MUST be read-only."
> >
> > possibilities (override or not). With the "MUST" we constrain it to
> > one (override).
> >
> > I also changed the two "it MUST be ignored" to "the access type MUST
> > be ignored" to better specify what "it" is.
> >
> 
> This helps.
> 
> For the record, my suggestion on more active voice was to say what must
> do the ignoring. But I think what you've got is good enough.
> 
> [...]
> 
> 
> >>
> >> No, I am not one.  Hopefully this will get a SecDir review as well.
> >> But that sort of review usually goes better if the Security
> >> Consideration section shows your reasoning, along the lines of
> >> listing the high-level types of changes, and for each, why it has no
> >> new security impact. Your response contains more of that sort of
> >> thing; it might help to add it (or parts of it) to the draft.
> >>
> >> I was a bit concerned that the default version for inheritance could
> >> be an issue, but you addressed that elsewhere.
> >>
> >> [...]=
> >
> > This document adds only a few constructs to the initial model defined
> > in RFC5812, namely namely a new event, some new properties and a way
> > to define optional access types and complex metadata. These
> constructs
> > do not change the nature of the the initial model. In addition this
> > document addresses and clarifies an issue with the inheritance model
> > by introducing the version of the derivedFrom LFB class.
> > Thus the security considerations defined in RFC5812 applies to this
> > document as well as the changes proposed here are simply constructs
> to
> > write XML library definitions, as where in RFC5812 and have no effect
> > on security semantics with the protocol.
> >
> 
> You might consider adding something to say that the inheritance model
> change also does not change the security considerations. (Maybe it
> makes things better, by removing the potential for choosing a wrong
> parent class? Not sure if that's a security issue, unless there was
> some kind of parent-assertion attack.)
> 
>  It does seem like the inheritance change is a bona-fide extension, not
> just a clarification, since you added the version attribute.=

---

From: Ben Campbell <[email-masked]>
Date: Wed, 27 Aug 2014 16:25:27 -0500

Hi,

This version addresses all of the concerns from my gen-art review.

Thanks,

Ben.

---

From: "Adrian Farrel" <[email-masked]>
Date: Thu, 28 Aug 2014 19:12:12 +0100

Ben,

Thanks for the time and effort.

Adrian

> From: ietf [mailto:[email-masked]] On Behalf Of Ben Campbell
> Sent: 27 August 2014 22:25
> To: Haleplidis Evangelos
> Cc: [email-masked]; [email-masked];
> [email-masked]