[AFS3-std] draft-wilkinson-afs3-rxgk-afs-01 comments

[Andrew Deason]

Recently a few people reviewed the draft-wilkinson-afs3-rxgk-afs-01 doc
together, and I've compiled the notes here. I've broken it down into
trivial stuff, section 10, and less trivial stuff (not all comments in
this last section are 'big', but I tried to just include stuff that
could actually change the meaning of the spec). This email contains the
trivial and less trivial; nontrivial discussion on section 10 will
follow in a separate email, because I think there's a lot to talk about
wrt it.

A few of these comments use the first person, but many of these were not
really written by me personally. But if you wanna argue about them or
whatever, I'm sure the author will see it here :) (I haven't been
keeping track of which comments came from whom.)

Anyway:


Trivial stuff:

Preamble:

 -- I don't think "Networking Working Group" is appropriate, since this doesn't
 involve a protocol handled by that WG.

1:

 -- The last sentence is a comma splice; it should either be two separate
 sentences, or it should use a semicolon or something.
 
1.1:

 -- 3rd sentence should read "one, or more, cells," or perhaps "one (or more)
 cells" or "one cell (or more)" or "at least one cell"

 -- The 2nd paragraph should reference the VVL spec

 -- s/syncronised/synchronised/ (assuming UK spelling is fine)

 -- In the last sentence, I would say "synchronized via" instead of
 "synchronized by"

1.2:

 -- In the last sentence, 'which' should have a comma preceding it, or should
 be changed to a 'that'.

2:

 -- Missing trailing period.

3:

 -- s/Clients which wish/Clients that wish/

3.1:

 -- In the first sentence, it seems like this is providing a CombineTokens
 variant that is AFS-specific, not a more generalized CombineTokens.

 -- In the first paragraph after the AFSCombineTokens argument descriptions:
 "[...] with a security level of 1 (integrity) or more." should say "or higher"
 (or "or greater")
 
 -- In the 2nd paragraph after the AFSCombineTokens argument descriptions;
 "Recommendations on keying cache managers are contained below". This should
 instead have a specific reference for the mentioned section.

 -- The 2nd paragraph after AFSCombineTokens is missing a trailing period.
 
 -- 4th paragraph after AFSCombineTokens, "Clients using a printed token (see
 below)" needs a reference to a specific section.

 -- 4th paragraph after AFSCombineTokens: missing trailing period.

 -- In the last paragraph, it seems like it would be helpful if this reference
 specified a specific section in the referenced doc.

4.2:

 -- 1st paragraph: sentences 3 and 4: s/which/that/

 -- End of the first paragraph: "The management of per-server keys is discussed
 in more detail below" should reference a specific section.

 -- 2nd paragraph, last sentence: "old key" should be plural

 -- Last paragraph, last sentence: missing period.

4.3:

 -- The first paragraph, "expressed by the following XDR" should be "expressed
 by the following RPC-L"

 -- 'enctype' definition is missing a trailing period.

 -- In the K0 definition, "see the rxgk specification for details" should
 point to a specific section, and have an actual reference

 -- 'level' definition is missing a trailing period.

 -- 'lifetime' definition is missing a trailing period.

 -- 'bytelife': s/byes/bytes/

 -- 'bytelife' definition is missing a trailing period.

 -- 'identities' definition is missing a trailing period.

6.1:

 -- In the first sentence, it should read "that material MAY be used to key
 the client"

7.1:

 -- Should this be "Token Printing" not "Ticket Printing"?

8:

 -- In the second paragraph: "The vlserver should then note the rx security
 layer used in registration" perhaps should say "MUST then note". Or maybe just
 reword this like: "The vlserver then infers rxgk support from the rx security
 layer used in registration".

9:

 -- First paragraph, last sentence: s/who MAY decided/who MAY decide/

 -- Missing period at the end of the first paragraph

 -- Missing colon at the end of the 2nd paragraph

 -- Second paragraph: s/AFS3/AFS-3/

 -- secIndex: missing period.

 -- keyDataRequest: s/For rxgk it is, the/For rxgk, it is the/

 -- keyDataRequest: missing period.

 -- keyDataResponse: s/For rxgk it is the/For rxgk, it is the/

 -- keyDataResponse: missing period.

 -- In the 2nd paragraph after the VL_RegisterAddrsAndKey definition: "If there
 is no common encryption type, then the server must fail the request" should
 say "then the server MUST fail the request"

 -- Antepenultimate paragraph: missing period.

 -- Last paragraph, last sentence: missing period.

10:

 -- End of the 1st paragraph: missing period.

 -- End of the 2nd paragraph: missing colon.

 -- Penultimate paragraph: missing period.

12.2:

 -- Last sentence in the section is missing a period.

Less trivial:

As a general comment, there is no section for the AFS registrar. Since we're
creating new RPCs, not only does this section need to exist, it also needs to
request that RPC code points be allocated.

2:

 -- "securityIndex" is not defined; there should be a reference to the Rx
    specification.

3:

 -- We don't specify the Rx service ID for the rxgk key negotation service,
    and neither does the rxgk document.
 
 -- This section states that GSSNegotiate should only be utilized with database
 servers, however it does not specify what should happen if not all database
 servers support rxgk and/or do not have the common keying material. I'd like
 to see additional language addressing the inverse/converse, i.e., are we
 requiring that _all_ db servers be upgraded prior to fileserver migration? If
 so, it probably should be noted that we are ruling out the possibility of
 future enchancements whereby departments can have RO clone dbservers receiving
 their "out of band" feeds under some other keying material.

3.1:

 -- Penultimate paragraph: "This is the only situation in which an rxgk capable
 client operating within an rxgk enabled cell may downgrade its choice of
 security layer."

 This seems excessively strong, since this seems to imply that a client MUST
 NOT downgrade in any other situation. But this behavior just seems to be a
 security recommendation, and violating this does not mean that an
 implementation is not following the rxgk protocol. I think this should say
 that a client SHOULD NOT downgrade in any other situation, and note that it
 is a security recommendation to never downgrade unless a vldb servers tell us
 it is okay.

4.1:

 -- This seems really vague about where this structure actually goes. I believe
 it is XDR-encoded, and then put in the opaque field for tokens from the rxgk
 specification. But currently this is not specified.

4.2:

 -- 2nd paragraph, last sentence: This seems like maybe something that the
 administrator should be doing, rather than the protocol implementor. Is RFC
 2119 language appropriate in this situation?

4.3:

 -- Again, this is a little unclear on where this structure actually goes. It
 is XDR-encoded, encrypted, and then the result goes in the TokenContainer
 encrypted_token field.

 -- The structure name is also confusing, since the rxgk specification defines
 a type called RXGK_Token, which is just an opaque blob.

 -- The definition of 'level' is vague; it should say it refers to a specific
 rxgk integrity level, and reference the section where they are defined

 -- startTime should probably be an rxgkTime. The definition also seems a
 little oddly worded; we should say that servers MUST reject attempts to start
 connections with that token after startTime

 -- For lifetime and bytelife, I find "This is an advisory limit" to be vague;
 if they can simply be ignored, we should say that adhering to and enforcing
 these limits is OPTIONAL.

5:

 -- I would find this clearer if we had an RPC-L definition with a structure,
 even if that structure just contains a single element, which is an afsUUID.

6:

 -- This section implies that an attack exists if we just use the users' tokens
 to fetch data, but it doesn't provide a lot of details. I would guess that the
 relevant attack is if a user poses as a fileserver, and can supply bad data
 to the victim client, while still encrypting the data. If so, this should be
 made more clear, but it also seems like this is always a problem for existing
 rxkad, and for all rxnull connections, which should be stated if true.

 It also seems a little odd that we appear to be using the CM-specific keying
 data to verify data integrity, and not something from the fileserver or
 cell-wide preshared key. If that's unavoidable, that's okay, but it seems a
 bit odd, unless I'm misunderstanding something.

6.1:

 -- 1st paragraph, last sentence: should this read "The client SHOULD
 frequently renew"? This seems like it would be helpful to say that the client
 SHOULD NOT renew more frequently than, say, lifetime/4.

7.1:

 -- "Printing" tokens appears to be a term invented by the document to discuss
 such tokens. I don't find this very clear, since the document just starts
 using the term; this should be mentioned, and the definition explicitly
 stated. This could probably just go at the end of section 7's preamble, since
 it already seems to be describing what printed tokens are.

9:

 -- We should have a reference to the VVL spec for VL_RegisterAddrs mentions

 -- In the definition for secIndex, for rxgk the value MUST be 4

10:

 -- Discussion of this section will follow in a different email

12.1:

 -- The SHOULD in the last sentence seems inappropriate; this is a section for
 security recommendations, and does not dictate whether or not something is
 actually following the protocol. This seems like something directed at an
 administrator, anyway.

12.2:

 -- I agree this is a problem with this approach, and makes me never want to
 use it. However, the section doesn't really give an indication of what to do
 about it. Should the automatic registration never be used? Or should it just
 be used by sites which have less concern for security, and the greater
 convenience of the automatic registration outweighs the downside? If so, it
 should be mentioned.

12.3:

 -- As mentioned in the notes for section 6, this implies some kind of attack
 on cached information, but it doesn't say what it is.

-- 
Andrew Deason
adeason@sinenomine.net