Subject: [nfsv4] Review of draft-ietf-nfsv4.integrity-measurement-07

From: David Noveck <[email-masked]>
Date: Sat, 2 Nov 2019 07:35:21 -0400

*General Comments*

*Overall Evaluation*

This document is in good shape and I feel it is ready for Working Group
Last Call.  While I note some potential improvements below in *Per-section
Comments,* I feel these changes could reasonably be made as part of last
call rather then needing to be be done before that time.  However, if there
are plans for a -08, I feel it would simplify things if some of the
suggestions made here were considered for inclusion in -08

*Possible Overuse of the Word "MUST"*

I feel that the use of "MUST" within this document is not in accord with
the advice given in RFC2119 that these terms be used "sparingly".
 Although I haven't looked at each such use, where I suggest rewriting  for
other reasons, I would only use "MUST" where there is a special reason to
do so.

*Issues Related to Documentation of IMA Metadata Format*

There has been a lot of discussion/comment on the mailing list regarding
the fact this format is not documented in this draft.  Some of the
suggestions made in *Per-section Comments *are intended to make it clearer
why this has not been done.  While I feel this would be helpful, I have no
expectation it will resolve the objections that have been expressed, which
I don't fully understand.

While I do not think that this issue should pose any obstacle to
publication, it might make it hard for the working group to arrive at a
consensus for publication.  Nevertheless, I feel we need to move to WGLC
and try to resolve the issue in that context, as I feel that discussion on
the list so far has not been all that helpful.

*Per-section Comments*

*Abstract*

Suggest addition of the following sentence: "The format of the IMA Metadata
is not described in this document."

*1.1. The Linux Integrity Measurement Architecture*

In the first paragraph, suggest replacement of the phrase "local tinkering"
by "other local modifications"

*1.1.1. IMA Metadata*

The last paragraph of this section, while a correct description of the
current situation may give readers pause since it suggests the possibility
of an interoprerability nightmare.   Even though such a scenario is,
strictly speaking, out of scope, I believe it would be helpful to assure
people that steps are being taken to avoid it, even though those steps are
being taken by others.   For that reason, I am proposing replacing that
parapraph by the following two paragraphs.

The precise format of this metadata is currently determined by policies set
by the local security administrator.  The metadata and its format are opaque
to the mechanisms that store or transport it (i.e. filesystems). The
particulars of the PKI and the hash algorithm are set by local policy.   In
order to provide interoperability, there is a need to provide agreement on
these matters by an out-of-band mechanism so that the IMA data can be
recorgnized by all participating IMA subsystems.

The difficulties which arise when multiple formats are supported makes it
likely that a sinlgle format will be arrived at, avoiding the need for an
out-of-band agreement mechanism.   However the details of that format and
the means by which it will standardized remain uncertain at this tiime.

*3. Protocol Extension Considerations*

In the last clause of the last sentence of the last paragraph suggest
replacing "does not update [RFC7862] or [RFC7863]" by "updates neither
[RFC7862]
nor [RFC7863]"

*4.1.1. NFS4ERR_INTEGRITY (Error Code YYYYY)*

Suggest that the following additional paragraph be added to this section:

This error provides a means by which servers which implement an internal
apprasal mechanism may communicate to the client the fact that an appraisal
failure occurred, causing the requested operarion not be be performed.

*4.2.1. Reporting Server-Side IMA Appraisal Failures*

Suggest rewriting the first paragraph to read as follows:

An NFS server that implements an internal appraisal mechanism needs to be
able to report integrity-related failures to clients. In the absense of the
NFS4ERR_INTEGRITY error, described in Section 1.1,1, a server implementer
would be forced choose choose one of the status codes that were available
in the base NFS version 4.2 protocol, typically NFS4ERR_IO or NFS4ERR_ACCESS,
even though these code points have generic meanings that do iindicate an
integrity-related failure.

*4.3.2. Authorizing Updates to IMA Metadata*

As written, the second pragraph, does not make any provision for the
possibility that update of IMA Metadata would be supported for some but not
all of the file system on a particular server.   If it is desirable to
allow this possibility suggest rewriting this paragraph as follows:

If an NFS server implementation does not support modification of IMA metadata
via NFS within a particular file system, the server returns NFS4ERR_INVAL
to a SETATTR request within that file systen with the FATTR4_IMA attribute,
as specified by Section 5.5 of [RFC5661].

*4.5. Using NFS Attribute Fencing (VERIFY/NVERIFY)*

Suggest rewriting the second paragraph as follows:

When implementing these operations, the server is to use a simple
byte-by-byte comparison to evaluate whether the client-provided FATTR4_IMA
matches the FATTR4_IMA attribute associated with the target object. If the
server has a local IMA appraisal implementation, its failure MAY prevent
the use of the local FATTR4_IMA attribute value for the purpose of this
comparison. In the event that this happens, the appraisal failure is
indicated by returing  NFS4ERR_INTEGRITY if the client has indicated
support for IMA metadata, with NFS4ERR_ACCESS used otherwise.

*8. IANA Considerations*

Suggest replacing "has no" by "does not require any"

---

From: Chuck Lever <[email-masked]>
Date: Mon, 4 Nov 2019 10:23:28 -0500

Thanks for the review, Dave. I've created an issue against this document to record the necessary changes.

https://github.com/chucklever/i-d-integrity-measurement/issues/8

Inline comments below.

> On Nov 2, 2019, at 7:35 AM, David Noveck <[email-masked]> wrote:
> 
> General Comments
> 
> Overall Evaluation
> 
> This document is in good shape and I feel it is ready for Working Group Last Call.  While I note some potential improvements below in Per-section Comments, I feel these changes could reasonably be made as part of last call rather then needing to be be done before that time.  However, if there are plans for a -08, I feel it would simplify things if some of the suggestions made here were considered for inclusion in -08 
> 
> Possible Overuse of the Word "MUST"
> 
> I feel that the use of "MUST" within this document is not in accord with the advice given in RFC2119 that these terms be used "sparingly".   Although I haven't looked at each such use, where I suggest rewriting  for other reasons, I would only use "MUST" where there is a special reason to do so.

I have a different take on the use of compliance keywords. "sparingly" is a subjective and qualitative adverb, making it difficult to follow as a normative mandate. Instead, I use a compliance keyword (especially MUST) whenever there is an action where, if the implementations do not comply, the protocol will not interoperate. However, I will review the document to check all uses of MUST and other compliance keywords.

> Issues Related to Documentation of IMA Metadata Format
> 
> There has been a lot of discussion/comment on the mailing list regarding the fact this format is not documented in this draft.  Some of the suggestions made in Per-section Comments are intended to make it clearer why this has not been done.  While I feel this would be helpful, I have no expectation it will resolve the objections that have been expressed, which I don't fully understand.
> 
> While I do not think that this issue should pose any obstacle to publication, it might make it hard for the working group to arrive at a consensus for publication.  Nevertheless, I feel we need to move to WGLC and try to resolve the issue in that context, as I feel that discussion on the list so far has not been all that helpful.

My intention is to add an Appendix that contains an Informative description of the current IMA metadata formats. The purpose of this descriptionis to help implementers construct an appraiser.

I hope to have this complete and submitted as a fresh revision by Thanksgiving.

I have traded e-mails with the chairs of the RATS WG. The integrity-measurement document, which describes an end-to-end integrity scheme, is somewhat outside the scope of their charter. I'm hoping they will agree to review the next revision of this document.

> Per-section Comments
> 
> Abstract
> 
> Suggest addition of the following sentence: "The format of the IMA Metadata is not described in this document."
> 
> 1.1. The Linux Integrity Measurement Architecture
> 
> In the first paragraph, suggest replacement of the phrase "local tinkering" by "other local modifications"
> 
> 1.1.1. IMA Metadata
> 
> The last paragraph of this section, while a correct description of the current situation may give readers pause since it suggests the possibility of an interoprerability nightmare.   Even though such a scenario is, strictly speaking, out of scope, I believe it would be helpful to assure people that steps are being taken to avoid it, even though those steps are being taken by others.   For that reason, I am proposing replacing that parapraph by the following two paragraphs.
> 
> The precise format of this metadata is currently determined by policies set by the local security administrator.  The metadata and its format are opaque to the mechanisms that store or transport it (i.e. filesystems). The particulars of the PKI and the hash algorithm are set by local policy.   In order to provide interoperability, there is a need to provide agreement on these matters by an out-of-band mechanism so that the IMA data can be recorgnized by all participating IMA subsystems.
> 
> The difficulties which arise when multiple formats are supported makes it likely that a sinlgle format will be arrived at, avoiding the need for an out-of-band agreement mechanism.   However the details of that format and the means by which it will standardized remain uncertain at this tiime.  
> 
> 3. Protocol Extension Considerations
> 
> In the last clause of the last sentence of the last paragraph suggest replacing "does not update [RFC7862] or [RFC7863]" by "updates neither [RFC7862] nor [RFC7863]" 
> 
> 4.1.1. NFS4ERR_INTEGRITY (Error Code YYYYY)
> 
> This error provides a means by which servers which implement an internal appraisal mechanism may communicate to the client the fact that an appraisal failure occurred, causing the requested operation not to be performed. 

I have some trouble with the suggested replacement.

"implement an internal appraisal mechanism" I was thinking of allowing this status code to be used for any integrity failure, not just appraisal failures. Today, NFS does not have any mechanism to report data or metadata integrity issues to clients.

"causing the requested operation not to be performed." Does that guarantee alter the idempotency of operations? In other words, is it possible that the requested operation could be partially completed before an integrity failure is recognized? How about "A data integrity issue prevented the completion of the requested operation".

The document will need to update the tables that list which operations may return NFS4ERR_INTEGRITY.

> 4.2.1. Reporting Server-Side IMA Appraisal Failures
> 
> An NFS server that implements an internal appraisal mechanism needs to be able to report integrity-related failures to clients. In the absense of the NFS4ERR_INTEGRITY error, described in Section 1.1,1, a server implementer would be forced choose choose one of the status codes that were available in the base NFS version 4.2 protocol, typically NFS4ERR_IO or NFS4ERR_ACCESS, even though these code points have generic meanings that do iindicate an integrity-related failure.
> 
> 4.3.2. Authorizing Updates to IMA Metadata
> 
> As written, the second pragraph, does not make any provision for the possibility that update of IMA Metadata would be supported for some but not all of the file system on a particular server.   If it is desirable to allow this possibility suggest rewriting this paragraph as follows: 
> 
> If an NFS server implementation does not support modification of IMA metadata via NFS within a particular file system, the server returns NFS4ERR_INVAL to a SETATTR request within that file systen with the FATTR4_IMA attribute, as specified by Section 5.5 of [RFC5661].
> 
> 4.5. Using NFS Attribute Fencing (VERIFY/NVERIFY)
> 
> When implementing these operations, the server is to use a simple byte-by-byte comparison to evaluate whether the client-provided FATTR4_IMA matches the FATTR4_IMA attribute associated with the target object. If the server has a local IMA appraisal implementation, its failure MAY prevent the use of the local FATTR4_IMA attribute value for the purpose of this comparison. In the event that this happens, the appraisal failure is indicated by returing  NFS4ERR_INTEGRITY if the client has indicated support for IMA metadata, with NFS4ERR_ACCESS used otherwise.
> 
> 8. IANA Considerations
> 

--
Chuck Lever