Re: [nfsv4] AD review of draft-ietf-nfsv4-rpcrdma-cm-pvt-data

[Chuck Lever <[e-mail]>]

Hi Magnus-

> On Nov 5, 2019, at 4:46 AM, Magnus Westerlund <[e-mail]> wrote:
> 
> On Mon, 2019-11-04 at 11:36 -0500, Chuck Lever wrote:
>> Hi Magnus, thanks for your review and moving forward with the AD write-up.
>> 
>>> On Nov 4, 2019, at 9:56 AM, Magnus Westerlund <
>>> magnus.westerlund=[e-mail]> wrote:
>>> 
>>> Hi,
>>> 
>>> I have started my AD review. It was such a short document that I thought I
>>> read through it to figure out what more I need to read. So I have not yet
>>> read like RFC 8166. So from a context of having little context understanding
>>> the below are my comments and questions. I think it may be faster if you
>>> help answering them then I read a lot of documents and still have question
>>> marks. 
>>> 
>>> 
>>> 	1. So this private data field is only existing in Infinite Band
>>> Connection manger? So what this document is defining a structure to a field
>>> which currently don’t have any structure but are part of a protocol defined
>>> by another body? 
>>> 		a. Have this work been formally agreed with IBTA? 
>>> 		b. Or is the relationship to the connection manager used to
>>> establish the RPC over RDMA different, if that is the case it needs to be
>>> better explained. 
>> 
>> The private data field is for use by Upper Layer Protocols. It's existence
>> is defined by the IBTA, but not its content. It's opaque to the CM, hence
>> the name "private".
>> 
>> Both iSER and iWARP itself (both IETF-defined) can place data in this field.
>> That's why there
>> is a 32-bit format ID field: if that doesn't match the constant defined in
>> this document, that means some other ULP is using the private data field,
>> and its contents are then ignored by RPC-over-RDMA.
> 
> So with new text as proposed in other email to clarify that this extensions
> apply to several different protocols will help making it clear that this is
> useful for IETF to define and not stepping on anyone's particular toes. 
> 
>> 
>> 
>>> 	3. Section 5: Is there only going to be one object of private data
>>> forever in the relevant communication? If not, why isn’t this a well-
>>> specified TLV so that unknow type entries can be skipped to the nest object?
>> 
>> As I understand it, the private data field is used by one ULP at a time.
>> I'm not aware of a TLV. There are already existing ULP implementations
>> that do not assume a TLV, so that might be a tough sell.
>> 
> 
> That helps a bit, but basically what we end up with is that the ULP need to
> cordinate between the end-point about which private data format(s) it will use.
> And if is to include multiple formats it need to know that the peer can delimit
> all of them. That is what I see as the issue. Maybe this assumption and
> responsibility of the ULP can be made clearer in the document. 
> 
>> 
>>> 	4. Even if there ever is going to be one entry, can you be more formal
>>> in description of what is a valid message using another format identifier,
>>> is that only the 32-bit field of the format Identifier, or also the version.
>> 
>> I can cite the use of this field by other protocols, if that would help.
> 
> As I said yes. 
>> 
>> 
>>> 	5. Isn’t the version field just unnecessary? If one needs version using
>>> another format identifier is more easy than the identifier + version
>>> construct.
>> 
>> It's possibly superfluous. But:
>> 
>> - the two fields serve different purposes, even though they could be combined.
>> - there's already an implementation that uses both fields.
> 
> So, I understand that your down the road on this one and you don't need to
> implement any change. But, it was a reflection I had after commenting on issue
> 4). 
> 
>> 
>> 
>>> 	6. Section 6: The IANA consideration is confusing. If the specification
>>> for a CM private data format requires IESG approval, then why have the
>>> expert review policy. In that case one can simply use the IESG approval
>>> policy instead. However, requiring any type of IESG approval here appear to
>>> be a way to high bar. I think expert review is likely appropriate, but the
>>> guidance to the Experts needs to be clearer. For example what happens if
>>> IBTA want to use this to add some format?
>> 
>> I would be OK with Expert Review only, but:
>> 
>> 1. I will need suggestions about expert guidance. Will look at other RFCs.
> 
> Basically the WG can specify what requirements you consider needed for a
> registration request. Publicly availble specification, contact information, what
> criterias the expert should make judgment on. Like if there are sensitive
> information or not. Etc. 
> 
>> 2. IMO WG consensus is needed on the replacement text of this section.
> 
> Sure, and if the WG want to choice IESG approval or Specification Required as
> policy for the registry that is also fine with me. However, I am currently not
> fine with the strange mix that is currently documented. 
> 
>> 
>> 
>>> 	7. Section 7: The security consideration. I am missing any discussion
>>> about the security requirements that the actual extension. It appears that
>>> integrity and source authenticity are required for safe operation of these
>>> two extensions.
>> 
>> Integrity is a property of RDMA Reliable Connection transports.
>> 
>> Not clear about the source authenticity requirement. Can you elaborate?
> 
> What I am trying to say, is that security consideration sections should make a
> security requirement analysis. And what I can see this private data when
> delivered to receiver, the receiver need to know that the data was not modified
> and it needs to know from whom it came. Thus integrity and source
> authentication. Then you can simply add a statement that the protocol you know
> to carry this field will provide that security service. 
> 
> The other aspect to consider is really if one can get any impact on the peer by
> being malicous in setting these fields. For example can I get the peer entity to
> consume more resources? To my understanding the answer is partially yes, but not
> without having to similarily promise to allocate resources. And if I don't do
> things will fail. And the point is that there is other methods that will be more
> efficient to do Denile of Service attacks on the peer in the protocol. 
> 
> It is usually good to document what aspects of attacks that has been considered
> and what level of an issue they are on.

The latest:

https://chucklever.github.io/i-d-rpcrdma-cm-pvt-data/draft-ietf-nfsv4-rpcrdma-cm-pvt-data.html

And a convenience URL for diffing with -04:

https://tools.ietf.org/rfcdiff?url1=https://tools.ietf.org/id/draft-ietf-nfsv4-rpcrdma-cm-pvt-data.txt&url2=https://chucklever.github.io/i-d-rpcrdma-cm-pvt-data/draft-ietf-nfsv4-rpcrdma-cm-pvt-data.txt


I've tried to address the following issues that you called out after your
attempt at an AD write-up of -04:

- Now a Proposed Standard updating RFC 8166
- Replaced the stale link to the IB specification
- Additional text explaining precedents and the purpose of the Format Identifier
- Revised the IANA Considerations section to remove "specification required" language
- Revised the Security Considerations section

I can take this further if you would like more changes before the next steps.
Otherwise I can provide a draft-ietf-nfsv4-rpcrdma-cm-pvt-data-05.xml for you
to submit manually.


--
Chuck Lever