Re: [kitten] [nfsv4] draft-ietf-nfsv4-rpcsec-gssv3: request for review

[Benjamin Kaduk <[e-mail]>]

Hmm, this seems to have gotten rather long.  The most important part is 
near the top, just after the refresher on the RPCSEC_GSS protocol.

On Wed, 30 Jul 2014, Adamson, Andy wrote:

> Hello
>
> I spoke with Shawn Emery (the Kitten WG Chair) last week at IETF 90, and 
> he agreed that I should cross-post the RPCSEC_GSSv3 draft to the Kitten 
> WG as well as to the NFSv4 WG to solicit reviews.  Please review the 
> draft which adds two new RPCSEC GSS operations and is a normative 
> reference to draft-ietf-nfsv4-minorversion2.
>
> As we are working to finish draft-ietf-nfsv4-minorversion2 , please 
> submit your reviews by Aug 31, 2014.
>
> https://datatracker.ietf.org/doc/draft-ietf-nfsv4-rpcsec-gssv3

I'll start off with some big-picture items, and leave my more detailed 
comments/nitpicks to the end.

First off, RPCSEC_GSSv3 is a diff on top of RPCSEC_GSSv2, itself a diff on 
top of RPCSEC_GSSv1.  As a refresher [well, I had to go look it up.  Maybe 
it's a refresher for other people], RPCSEC works by running the GSS 
negotiation loop over RPC messages, with special RPCSEC control messages 
being passed on top of the "NULL" RPC until the context is estalblished.
Once the context is established, "real" data RPCs can be sent.  Three 
levels of protection are provided for the RPC bodies, none, integrity, and 
privacy.  The choice of level for this attempt at this RPC, as well as a 
sequence number unique to this transmission, are encoded into the 
"credential", a part of the RPC request header; there is a GSS MIC over 
the header, so the header (and protection level and sequence number) are 
always protected.  The request payload's encoding depends on the level; 
for none, it is unchanged.  For integrity, a MIC is taken over the 
concatenation of the sequence number and the request body, and the 
transmitted payload is the concatenation of that sequence number and 
request body, and the MIC.  For privacy, the same seqnum+body encoding is 
done, but GSS_Wrap is used instead of GSS_GetMIC, and the output of Wrap 
used as the payload.  The server keeps a window of sequence numbers and 
rejects replays and old ones; if packet loss occurs and a client must 
retransmit, the retransmit uses a new sequence number.

---

Multi-principal authentication

This draft proposes a multi-principal authentication scheme, restricted to 
just the case of a privileged client process on a machine combining the 
(privileged) host's credentials with the (unprivileged) user's credentials 
so as to take action on behalf of the user.  This is a similar combination 
to that we are using for RXGK_AFSCombineTokens (see 
draft-wilkinson-afs3-rxgk-afs), restricted to just a combination of host 
and user credentials, specifying which one is which.  I think this is the 
only well-understood scenario for compount authentication at the moment, 
and makes sense.

The key material from the host's GSS context are used unchanged for 
securing RPCs issued with the combined identity, but the opaque RPCSEC_GSS 
context handle is changed to indicate that it represents a "child" context 
which has the combined identity (and possibly other attributes as well, 
not relevant for multi-principal authentication).  The creation of the 
child context involves sending an authenticated RPC using the 
parent/host-credential 
context, containing body data including a random nonce and the MIC of that 
nonce using the "inner" context (i.e., the user's credentials).  The reply 
contains the same nonce, but the MIC in the reply is performed using the 
parent/host-credentials context.  The RPC to create the child context is 
not permitted over a plaintext channel, and requires either integrity 
protection, confidentiality+integrity protection, or channel binding to a 
secure channel.

However, I don't think this is strong enough; I think this scheme requires 
the "privacy" level of protection.  Otherwise, an attacker could replay 
the nonce+MIC and obtain an RPCSEC_GSS context that will authenticate as 
the user from the "inner" context, without actually proving that it 
possesses the user's credentials.  In RXGK_AFSCombineTokens, we are not 
using (opaque) GSS credentials and can explicitly combine the key material 
for a strong proof of possession.  GSS credentials are opaque, and the 
GSS-API does not really provide any primitives that seem applicable here. 
So, I would recommend requiring privacy protection for this call.

---

The union construct in 2.6.1 with the default case being an opaque is 
quite similar to the extensible union construct defined in 
draft-keiser-afs3-xdr-union; the only difference is that the encoding of 
the currently listed 'LABEL' and 'PRIVS' cases do not include a length 
field.  It might be nice to have afs3 and nfsv4 agree on a consistent type 
for the extensible union primitive, though I understand that this is 
unlikely to happen on the timescale you desire for the rpcsec-gssv3 draft.

---

It feels a little strange to be talking about adding a way to specify 
privileges/restrictions via "assertions" onto a GSS-based security scheme, 
since GSS is philosophically more of a "request and check" scheme for 
features.  This is not a realy problem, of course, it just takes some 
getting used to.  Actually, I guess it is still kind of "request and 
check", since the server only replies with the assertions it has accepted. 
Maybe this is indicative that the name could be more descriptive.

---

This draft makes no mention of the use of the 'critical' bit for 
structured privilege assertions, but does imply that the critical bit will 
apply to any future branches that are added to the rgss3_assertion_u. 
This strikes me as odd; at the very least it could say that the critical 
bit is ignored.  (That's my reading of what the behavior is supposed to 
be, from section 2.6.1.3.)

---

It seems like it would not adversely affect the document to move the 
definition of rgss3_chan_binding up to between rgss3_gss_mp_auth and the 
branches of rgss3_assertion, to match the layout of the rgss3_create_args 
structure.  Kind of minor, but helps the reader find things.

---

I think it's valid to assume that a successful call to GSS_GetMIC will 
never return a zero-length output, so the encoding of 
rgss3_create_{args,res} could be reduced in size by just using an opaque 
for their respective _chan_bind_mic fields, instead of including the extra 
pointer in the type.  I don't know if there's enough need for space to 
make this worth doing, though.  There is some potential value in being 
able to easily differentiate "not present" from zero-length.

---

Why is RPCSEC_GSS_BIND_CHANNEL marked as "not used" in the sample code on 
page 7?  It is not otherwise mentioned in the draft, and the abstract 
claims that RPCSEC_GSSv3 provides channel binding capabilities. 
Furthermore, it is permitted to use rpc_gss_svc_channel_prot to protect 
the RPCSEC_GSS_{CREATE,LIST} messages, which requires the prior use of 
RPCSEC_GSS_BIND_CHANNEL.  I see that an alternate scheme of channel 
binding is specified by this document, but it does not actively forbid the 
use of the RPCSEC_GSSv2 model, as written.

---

And now, the minutiae:


The fourth paragraph of section 1 (Introduction) refers to section 8 of 
draft-ietf-nfsv4-minorversion2-27 for Labeled NFS, but the IETF tools will 
only give me the -26, where Labeled NFS is section 9.  Not a big deal, but 
I figured I'd mention it.

The phrasing in section 2.3 that "RPCSEC_GSS version 3 MUST change the 
verifier" seems odd.  This is the specification of RPCSEC_GSSv3, just say 
what format is specified.  Note it as a change from the previous format if 
you must, but to say that "this document MUST do this thing that this 
document does" serves no purpose other than to confuse the reader.

In section 2.6, fourth paragraph, there's a missing space in "version2".

In 2.6.1.1, the following text is a little confusing:
    Thus a server may refuse to
    grant requested authority to a user acting alone (e.g., via an
    unprivileged user-space program), or to a client acting alone (e.g.
    when a client is acting on behalf of a user) but may grant requested
    authority to a client acting on behalf of a user if the server
    identifies the user and trusts the client.
It makes more sense when one reads "client" to mean "in-kernel NFS 
client", but would probably be more clear if the "client is acting on 
behalf of a user" is reworded, possibly to "on behalf of a single user, 
using only that user's credentials).  It looks like the "client" vs. 
"kernel service" distinction is fuzzy throughout the rest of the section, 
too.  I would prefer if the word "client" throughout the document referred 
only to "the RPC client", without any implications about whether the RPC 
client is a trusted (in-kernel) service or a program run by the user or 
anything else.  This document defines RPCSEC_GSSv3, not NFS; "client" 
should mean "RPC client", not "NFS client".

Still on multi-principal authentication, the text says "Other 
multi-principal parent and inner context handle uses might eventually make 
sense."  It seems like any such uses would require standards action to 
specify them; you might as well also say that they would be introduced in 
a new revision of the RPCSEC_GSS protocol (e.g., v4).

In:
    An inner RPCSEC_GSSv3 context handle that is bound to a parent
    RPCSEC_GSS context through multi-principal authentication MUST be
    treated by servers as authenticating the GSS-API initiator principal
    authenticated by the inner context handle's GSS-API security context.
In the first line, should that be "inner" or "child"?  I think "child", 
since the child context handle is what is used to authenticate RPCs, and 
the inner context handle is implicit.  The identity is from the inner 
handle, of course.

This form of multi-principal authentication is using a nonce+MIC to verify 
the validity of the assertion of the inner context.  You should give some 
guidance on the length of the nonce, especially if the nonce can be sent 
in plaintext.  (For similar things in afs3-rxgk, we use 20 octets. 
Apparently this is UUID-length, and I'm told there is some precedent for 
this choice.)

Please put a comma before "with" in "the same rgmp_nounce as was sent in 
the call data with the rgmp_nounce_mic created using the GSS-API security 
context associate with the parent handle."

The fourth text paragraph of section 2.6.1.2 refers to section 12.2.2 of 
draft-ietf-nfsv4-minorversion2-27; again, only -26 is currently available, 
and I cannot infer what section 12.2.2 is supposed to be from the context.

In the sixth paragraph of that same section, "can assert that label is a 
secret" should have either "that" or "the" (your preference) inserted 
prior to "label".

The last sentence of the penultimate paragraph of section 2.6.1.3 should 
be reworded for clarity, maybe something like "If the server receives a 
structured privilege assertion that fails to verify according to the 
requirements of the RPC application defined behavior, the assertion is 
rejected [...]"

It's probably worth noting in 2.6.1.4 that the GSS_GetMIC for the channel 
binding is done using the outer/parent RPCSEC_GSSv3 context, instead of 
just "the RPCSEC_GSSv3 context handle's GSS-API security context".  The 
channel binding may be performed in parallel with muti-principal 
authentication, so there may be multiple contexts in play.

The clause "if the client really wanted channel binding" is rather 
informal; you might want to adjust it to say something about "considered 
to be critical" or similar.

In the security considerations, "evaludated" has a typo (spurious 'd').

-Ben