Re: [netmod] Benjamin Kaduk's No Objection on draft-ietf-netmod-factory-default-14: (with COMMENT)

[Benjamin Kaduk <[e-mail]>]

Hi Qin,

The new updates look good, thanks.

-Ben

On Sun, Apr 26, 2020 at 02:52:15AM +0000, Qin Wu wrote:
> Hi, Ben:
> -----邮件原件-----
> 发件人: Benjamin Kaduk [mailto:[e-mail]] 
> 发送时间: 2020年4月25日 3:37
> 收件人: Qin Wu <[e-mail]>
> 抄送: The IESG <[e-mail]>; [e-mail]; [e-mail]; [e-mail]; Kent Watsen <[e-mail]>
> 主题: Re: Benjamin Kaduk's No Objection on draft-ietf-netmod-factory-default-14: (with COMMENT)
> 
> On Thu, Apr 23, 2020 at 04:54:37AM +0000, Qin Wu wrote:
> > Hi, Ben:
> > Thanks for your valuable comments, see reply inline below.
> > -----邮件原件-----
> > 发件人: Benjamin Kaduk via Datatracker [mailto:[e-mail]]
> > 发送时间: 2020年4月23日 9:39
> > 收件人: The IESG <[e-mail]>
> > 抄送: [e-mail]; 
> > [e-mail]; [e-mail]; Kent Watsen 
> > <[e-mail]>; [e-mail]
> > 主题: Benjamin Kaduk's No Objection on 
> > draft-ietf-netmod-factory-default-14: (with COMMENT)
> > 
> > Benjamin Kaduk has entered the following ballot position for
> > draft-ietf-netmod-factory-default-14: No Objection
> > 
> > When responding, please keep the subject line intact and reply to all 
> > email addresses included in the To and CC lines. (Feel free to cut 
> > this introductory paragraph, however.)
> > 
> > 
> > Please refer to 
> > https://www.ietf.org/iesg/statement/discuss-criteria.html
> > for more information about IESG DISCUSS and COMMENT positions.
> > 
> > 
> > The document, along with other ballot positions, can be found here:
> > https://datatracker.ietf.org/doc/draft-ietf-netmod-factory-default/
> > 
> > 
> > 
> > ----------------------------------------------------------------------
> > COMMENT:
> > ----------------------------------------------------------------------
> > 
> > While many of the secdir reviewer's complaints stem from the YANG security considerations boilerplate, it still seems like it would be worth some form of response to the review.
> > 
> > [Qin]: You are correct, we authors also bring up the discussion on 
> > sec-review comment on YANG security consideration boilerplate to netmod list. I have sent my response to the sec-review, Thanks for kindly reminder.
> > 
> > Section 1
> > 
> >    This document defines a method to reset a server to its factory
> >    default content.  The reset operation may be used, e.g., when the
> >    existing configuration has major errors so re-starting the
> >    configuration process from scratch is the best option.
> > 
> >    A "factory-reset" RPC is defined.  When resetting a device, all
> >    previous configuration settings will be lost and replaced by the
> >    factory default content.
> > 
> > nit: these two paragraphs talk about the same thing, but the next paragraph is a different thing.  It may be better to combine these two in to a single paragraph.
> > [Qin]:The format of this section is to first introduce what method we proposed? And then introduce what this method look like, or two key components for this method, i.e., one new factory-reset RPC and one new factory datastore.
> 
> If the first pargaraph is trying to introduce everything, then it should mention both the RPC and the datastore.  Right now, I only see it talking about the RPC (well, the "reset operation"), and thus it does not seem like a general introduction as opposed to an introduction specific to the RPC.
> 
> [Qin]: I see your point, a few clarification: YANG data model will include RPC definition and datastore definition and A device MAY implement the "factory-reset" RPC without
>    implementing the "factory-default" datastore. In addition, we want to avoid duplicated text in both abstraction and introduction. Based on this clarification, I propose the following change:
> OLD TEXT:
> "
>    This document defines a method to reset a server to its factory
>    default content.  The reset operation may be used, e.g., when the
>    existing configuration has major errors so re-starting the
>    configuration process from scratch is the best option.
> 
>    A "factory-reset" RPC is defined.  When resetting a device, all
>    previous configuration settings will be lost and replaced by the
>    factory default content.
> 
>    A "factory-default" read-only datastore is defined, that contains the
>    data to replace the contents of implemented read-write conventional
>    configuration datastores at reset.  This datastore can also be used
>    in the <get-data> operation.
> "
> NEW TEXT:
> "
>    This document defines a YANG data model and associated mechanism to
>    reset a server to its factory default content.  This mechanism may be
>    used, e.g., when the existing configuration has major errors so re-
>    starting the configuration process from scratch is the best option.
> 
>    A "factory-reset" RPC is defined within the YANG data model.  When
>    resetting a device, all previous configuration settings will be lost
>    and replaced by the factory default content.
> 
>    In addition, an optional "factory-default" read-only datastore is
>    defined within the YANG data model, that contains the data to replace
>    the contents of implemented read-write conventional configuration
>    datastores at reset.  This datastore can also be used in the <get-
>    data> operation.
> "
> > I prefer to keep as it is. Maybe we could tweak the first paragraph a little bit as follows:
> > "
> >    This document defines a method to reset a server to its factory
> >    default content.  This method may be used, e.g., when the
> >    existing configuration has major errors so re-starting the
> >    configuration process from scratch is the best option.
> > "
> >    A "factory-default" read-only datastore is defined, that contains the
> >    data to replace the contents of implemented read-write conventional
> >    configuration datastores at reset.  [...]
> > 
> > Can I suggest instead:
> > 
> > % A "factory-default" read-only datastore is defined, that reflects what the % conventional read-write datastores would be overwritten with in the case of % a factory-reset operation.
> > [Qin]: Looks equivalent, but I think the original one is more clear.
> 
> To me the phrase "the data to replace the contents of [...] at reset" is awkward, but your opinion as author trumps mine, here.
> 
> > Section 2
> > 
> >                                                           All security
> >    sensitive data (i.e., private keys, passwords, etc.)  SHOULD be
> >    overwritten with zeros or a pattern before deletion.  [...]
> > 
> > I might suggest instead:
> > 
> > % When this process includes security-sensitive data such as cryptographic keys or passwords, it is RECOMMENDED to perform the deletion in a manner as  thorough as possible (e.g., overwriting the physical storage medium with zeros and/or random bits) to reduce the risk of the sensitive material being recoverable.
> > 
> > [Qin]: Sounds reasonable to me, thanks.
> > It's probably worth noting that since this is only dymanically generated files, any cryptographic keys that are part of the factory-installed image will be retained (such as an IDevID certificate).
> > [Qin]:If this is conclusion of the draft-ietf-anima-bootstrapping-keyinfra discussion, yes, will consider it.
> > Section 3
> > 
> >    Following the guidelines for defining Datastores in the appendix A of
> >    [RFC8342], this document introduces a new optional datastore resource
> >    named "factory-default" that represents a preconfigured initial
> >    configuration that can be used to initialize the configuration of a
> > 
> > nit/soapbox: "preconfigured initial configuration" feels like an awkward wording to me; perhaps "pre-set initial configuration" or "fixed initial configuration"?
> > 
> > [Qin]: I see they are equivalent, but I am happy to take your proposal.
> > 
> > Section 4
> > 
> >         description
> >           "This read-only datastore contains the factory default
> >           configuration for the device used to replace the contents
> >           of the read-write conventional configuration datastores
> >           during a 'factory-reset' RPC operation.";
> > 
> > nit: the grammar here is off; maybe "for the device that will be used"?
> > (Or some adaptation of my proposed text from earlier.)
> > [Qin]: Sounds good to me.
> > 
> > Section 6
> > 
> > If the factory-default configuration is an "open" one, then performing the reset could leave the device (and thus the network!) vulnerable to attack until it is properly configured.  The rtgdir reviewer's comments seem related to this.
> > 
> > An attacker that could somehow cause the factory-reset to be performed would cause the loss of running state/crypto keys that would potentially require a lot of operator effort to recover (in addition to the more immediate DoS issues).
> > 
> > There is some discussion in draft-ietf-anima-bootstrapping-keyinfra about attacks that are possible when a device is restored to its factory default state; it might be worth trying to incorporate some of that discussion in some manner (whether inline or by reference).
> > [Qin]: Okay and will consider it.
> 
> Thanks!
>  
> >    The "factory-reset" RPC can prevent any further management of the
> >    device if the session and client config are included in the factory
> >    default contents.
> > 
> > I'm not sure this is 100% correct.  If the factory default config overwrites this items, then yes, it will prevent further management.  But we also say to delete dynamic files from nonvoliatile storage, which at least to me seems like it could include this class of items and cause the same symptoms even if the configuration items in question are not included in the factory default contents.
> > [Qin] It seems your comment is related to Eric's. Overwriting happen before deletion, Overwriting can be used to prevent such symptom.
> 
> I don't think it's related to Éric's comment, so I assume my meaning was not clear.
> 
> I agree that if if session and client config are included in the factory default contents, this means the "factory-reset" RPC can prevent further management of the device.  I *also* think that there are more cases where the "factory-reset" RPC can prevent further management of the device (and that we should mention mention the possibility of that, since the current text is easy to read as saying that the listed case is the only such case).
> In particular, if the session and client config are *not* in the factory default contents, but are instead treated as dynamic files on the nonvoliatile storage, the RPC will overwrite such config and thus prevent further management of the device.
> 
> [Qin]:Thanks for your clarification, here is the proposed change:
> OLD TEXT:
> "
>    The "factory-reset" RPC can prevent any further management of the
>    device if the session and client config are included in the factory
>    default contents.
> "
> NEW TEXT:
> "
>    The "factory-reset" RPC can prevent any further management of the
>    device when the server is reset back to its factory default
>    condition, e.g., the session and client config are included in the
>    factory default contents or treated as dynamic files on the
>    nonvoliatile storage and overwritten by the the "factory-reset" RPC.
> "
> -Ben