[P2PSIP] AD evaluation: draft-ietf-p2psip-share-07

[Alissa Cooper <[e-mail]>]

I have reviewed this document in preparation for IETF last call. I have a number of comments and questions that need to be resolved before last call can be initiated. I’ve also included some nits below that should be resolved together with last call comments.

Given the nature of this document, I’d like for the shepherd to request an early SECDIR review after the comments below have been resolved so that the authors and WG can receive security feedback before the document progresses to IESG evaluation.


== Substantive comments and questions ==

= Section 3.1 =

I think this section requires clarification. 

How is the index value supposed to be initialized? Is it supposed to be chosen at random or set to 0 (or 1, as in the figure)?

I don’t understand how this mechanism relates to how SSRCs are chosen. In fact RFC 3550 doesn’t specify a particular algorithm to use, but merely provides one example. Furthermore, I don’t see how the collision probably for the array index value, which selects the least significant three bytes from a cryptographically random Node-Id that must be 16 bytes or longer, would be the same as for a randomly chosen 32-bit integer. Could you explain?

= Section 5 =

Are variable resource names expected to be UTF-8 strings? I think somewhere in this section the internationalization expectations for these strings need to be specified.

= Section 5.3 =

(1)
I think this section needs to specify normative requirements on the pattern construction to avoid duplicative or substring names as described in 5.1

(2)
"Configurations in this overlay document MUST adhere in syntax and semantic of names as defined by the context of use. For example, syntax restrictions apply when using P2PSIP[I-D.ietf-p2psip-sip], while a more general naming is feasible in plain RELOAD."

I don’t understand what the normative requirement is here or why it is needed. How is “the context of use” defined? Shouldn’t it be up to the specific protocol documents to define the required syntax and semantics for specific usages (e.g., the way draft-ietf-p2psip-sip does)?

(3)
"In the absence of a <variable-resource-names> element for a Kind using the USER-CHAIN-ACL access policy (see Section 6.6 <https://tools.ietf.org/html/draft-ietf-p2psip-share-07#section-6.6>), implementors SHOULD assume this default value."

Why is this SHOULD and not MUST? Shouldn’t implementations conservatively assume that variable names are not allowed unless explicitly specified?

(4)
"If no pattern is defined for a Kind or the "enable" attribute is false, allowable Resource Names are restricted to the username of the signer for Shared Resource.”

I think this needs to account for an error condition where the pattern does not meet the pattern construction requirements, e.g.:

""If no pattern is defined for a Kind, if the "enable" attribute is false, or if the regular expression does not meet the requirements specified in this section, the allowable Resource Names are restricted to the username of the signer for Shared Resource.”

= Section 6.2 =

For privacy reasons, wouldn’t it be better to overwrite every entry in a subtree when the root of the subtree gets overwritten? Otherwise the list of users who were given write access may remain long after their access has been revoked.

= Section 6.3 =

How strings are to be compared (e.g., as binary objects or whatever it is) needs to be normatively specified.

It is confusing to use normative language only in step 5 here. I would suggest either normatively defining each action or not using SHALL here.

= Section 6.6 =

"Otherwise, the value MUST be written if the certificate of the signer contains a username that matches to one of the variable resource name pattern (c.f. Section 5 <https://tools.ietf.org/html/draft-ietf-p2psip-share-07#section-5>) specified in the configuration document"

It seems to me that matching the pattern is not sufficient — isn’t it the case that both the user and domain portions of the user name in the certificate need to match the user and domain name portions present in the resource name? In general, the document seems to be missing discussion of the implications of having the user name and the resource name diverge. I think this affects every operation that involves comparing the two (or the Resource-Id, right?).

I’m also unclear about why policy for allowing access to shared resources is being strictly coupled with policy for allowing variable resource names. Might there be cases where it makes sense to authorize one but not the other? 

= Section 8.2 =

This section misses the threat of a misbehaving peer who is delegated write access — that seems like an important case to cover.

= Section 8.3 =

By “publicly readable” do you mean “readable by any node in the overlay”? Admission to the overlay would still be access controlled, correct?

= Section 9.2 =

What is the significance of 17, other than that it is in the unassigned range?


== Nits ==

= Section 1 =

The reference to I-D.ietf-p2psip-disco should be removed given that the document is several years old and not expected to advance as far as I know.

s/from one authorized to another (previously unauthorized) user/from one authorized user to another (previously unauthorized) user/

= Section 2 =

s/the peer-to-peer SIP concepts draft [I-D.ietf-p2psip-concepts <https://tools.ietf.org/html/draft-ietf-p2psip-share-07#ref-I-D.ietf-p2psip-concepts>]/[I-D.ietf-p2psip-concepts <https://tools.ietf.org/html/draft-ietf-p2psip-share-07#ref-I-D.ietf-p2psip-concepts>]/

= Section 3.1 =

s/Append an 8 bit long short individual index value/Append an 8-bit individual index value/

= Section 4.1 =

s/an Access Control including/an Access Control List including/

= Section 5.1 =

Same comment about I-D.ietf-p2psip-disco <https://tools.ietf.org/html/draft-ietf-p2psip-share-07#ref-I-D.ietf-p2psip-disco> as in Section 1.