Re: [netconf] Genart last call review of draft-ietf-netconf-keystore-29

[Kent Watsen <[e-mail]>]

Hi Reese,

Thank you for your review.
Please find below my responses to your comments.

Kent


> On Jan 22, 2024, at 8:36 PM, Reese Enghardt via Datatracker <[e-mail]> wrote:
> 
> Reviewer: Reese Enghardt
> Review result: Ready with Issues
> 
> I am the assigned Gen-ART reviewer for this draft. The General Area
> Review Team (Gen-ART) reviews all IETF documents being processed
> by the IESG for the IETF Chair.  Please treat these comments just
> like any other last call comments.
> 
> For more information, please see the FAQ at
> 
> <https://wiki.ietf.org/en/group/gen/GenArtFAQ>.
> 
> Document: draft-ietf-netconf-keystore-29
> Reviewer: Reese Enghardt
> Review Date: 2024-01-22
> IETF LC End Date: 2024-01-24
> IESG Telechat date: Not scheduled for a telechat
> 
> Summary: The document is clear and fairly concise, and seems useful. It is
> ready to be published after confirming that all important security
> considerations have been documented.
> 
> Major issues: None, assuming the use of clear text keys has been sufficiently
> discussed and its impacts carefully considered.
> 
> Minor issues:
> 
> I was surprised to see that the model allows to store keys in the clear. I
> assume that the WG has carefully considered the security implications here, and
> at least Section 4.2 says a key SHOULD be encrypted. Still, I wonder, if it's
> worth giving a bit of context in the Introduction, maybe saying why clear text
> keys are supported but that they SHOULD NOT be used unless there's no other way?

Whoops, my Security Considerations for the module left out this paragraph
added to all of the other drafts in the series:

	Please be aware that this YANG module uses groupings from
	other YANG modules that define nodes that may be considered
	sensitive or vulnerable in network environments.  Please
	review the Security Considerations for dependent YANG modules
	for information as to which nodes may be considered sensitive
	or vulnerable in network environments.

This is important because the “cleartext-key” and “cleartext-private-key”
leafs come from groupings defined in the crypto-types draft:

	https://datatracker.ietf.org/doc/html/draft-ietf-netconf-crypto-types-27#section-2.1.4.3
	https://datatracker.ietf.org/doc/html/draft-ietf-netconf-crypto-types-27#section-2.1.4.5

And that draft’s Security Considerations sections says:

	3.7. Cleartext Passwords and Keys

	The module contained within this document enables, only when specific
	"feature" statements are enabled, for the cleartext value of passwords
	and keys to be stored in the configuration database. Storing cleartext
	values for passwords and keys is NOT RECOMMENDED.

All good?



> Introduction:
> "This document intends to support existing practices; it does not intend to
> define new behavior for servers to implement." This document is a Proposed
> Standard, so I find the above sentence a bit confusing. If the point here is
> that many or most server implementations already support the features and
> conform to the behavior defined in this document, that's great. But otherwise I
> assume we do want the implementations to change so they support the features or
> conform to the behavior? If I got the intention of this sentence right, I
> suggest changing it to "This document is intended to reflect existing practices
> that many server implementations already support at the time of writing" or
> some such.

Adjusted per your recommendation.
Thanks to the suggested text.


> Section 5 (Security Considerations):
> Given the sensitive nature of the data stored using this model, I suggest
> another pass at this section following RFC 3552 (BCP 72): Are there any other
> possible attacks on confidentiality, integrity, authenticity, or availability,
> which are in scope for this section? Is there any specific threat model that
> needs to be considered? I also suggest naming some of the possible impacts of
> these attacks. Some possible impacts seem obvious, but I wonder if I'm missing
> any significant impacts because I'm not very familiar with the model.

IDK either.  But I can say that this draft has had two SecDir reviews.
I’m happy to email or get on a call to discuss with interested parties at anytime.


> Section 5.3:
> "access control mechanisms like NACM SHOULD be used to limit access to the
> key's secret data to only the most trusted authorized clients (e.g., an
> organization’s crypto officer)." Does "client" refer to a person here, rather
> than the RFC 6241 definition of client, which the document says it uses? If so,
> I suggest finding a different term here.

Client refers to RFC 6241’s definition of client.  But that client authenticates
itself to the server using credentials that may, e.g., belong to an organization’s
crypto officer. I did this:

	s/an organization’s/belonging to an organizations’s/???

Is it better?


> Nits/editorial comments:
> 
> Section 5.1:
> "The YANG module defined in this document defines a mechanism called a
> "keystore" that, by its name, suggests that it will protect its contents from
> unauthorized disclosure and modification." This might be semantics, but: In my
> mind, the name suggests that there's sensitive data, which implies that the
> data needs to be protected, with obvious consequences otherwise. Please
> consider rephrasing this sentence.

I get what you’re saying, but turning the sentence around like that seems to
misplace the emphasis, e.g., the draft isn’t about the data but the mechanism.
At least I couldn’t see how to do it.  Can you suggest specific replacement text?


Thanks again,
Kent