[AFS3-std] Re: afs3-rxgk-updates for 03
Andrew Deason
adeason@sinenomine.net
Mon, 29 Oct 2012 16:57:35 -0500
Here are comments on the individual commits. Most of these are just
"+1"s, but having them there helped me keep track :) Anyway:
On Thu, 25 Oct 2012 09:58:03 -0400 (EDT)
Benjamin Kaduk <kaduk@MIT.EDU> wrote:
> commit 820690138e8371e6da14dc616c237f7e5e89c648
> Author: Ben Kaduk <kaduk@mit.edu>
> Date: Wed Oct 24 14:57:40 2012 -0400
>
> Move RX reference to first use
+1
> commit 1165728030cae0a91cb2f72e9822c707902dc438
> Author: Ben Kaduk <kaduk@mit.edu>
> Date: Wed Oct 24 15:00:04 2012 -0400
>
> Edit for grammar and punctuation
+1, but I'm not really looking at this terribly closely, of course :)
> commit 5f4cc9b355e46d8ad939d823b4f2071967b2a8fe
> Author: Ben Kaduk <kaduk@mit.edu>
> Date: Wed Oct 24 15:17:56 2012 -0400
>
> Use title case
+1
> commit 6394ddf4e1f43e8b54c602b2bffb88d26dfe77f9
> Author: Ben Kaduk <kaduk@mit.edu>
> Date: Wed Oct 24 15:23:02 2012 -0400
>
> Grammar fix
+1
> commit f17765371ed350dd7342a242fe0628d4680ac0fc
> Author: Ben Kaduk <kaduk@mit.edu>
> Date: Wed Oct 24 15:35:52 2012 -0400
>
> Grammar and parallelization tweaks
+1
> commit 7c9a51aa1bb9afd3c18f715b181949eb3f5860e6
> Author: Ben Kaduk <kaduk@mit.edu>
> Date: Wed Oct 24 15:54:14 2012 -0400
>
> RFC 2119 police
+1
> commit 39a100e02a1c7074de1cdd80d40dd36e17175f8b
> Author: Ben Kaduk <kaduk@mit.edu>
> Date: Wed Oct 24 16:37:31 2012 -0400
>
> Use fewer pronouns
+1
> commit aeb115c9a2a12b0623c65a2b705002694fa80754
> Author: Ben Kaduk <kaduk@mit.edu>
> Date: Wed Oct 24 16:58:34 2012 -0400
>
> Make better use of named values
+1
> commit 8e0451de7dbdc3abb335bffc79e30d7c51d6c78b
> Author: Ben Kaduk <kaduk@mit.edu>
> Date: Wed Oct 24 17:17:42 2012 -0400
>
> The value zero is special for (byte)lifetime
+1. My previous "objections" on this I don't think really matter. I
didn't see what the point of the MUST restriction was, since the
lifetimes are not strictly adhered to anyway. I don't know if any text
really needs to change, but it helps me to think of this like:
The connection has one advisory lifetime. The server and client both
pick an advisory lifetime, and the lifetime for the connection is just
the stricter of the two lifetimes chosen. So the server MUST choose one
at least as strict as the client specified, just to accurately represent
what the connection lifetime actually is, even if the server is not
bound to obey it. (Similarly goes for byte-life.)
> commit 051c46a6d806e0ce4eab737ff91dc14b34b77375
> Author: Ben Kaduk <kaduk@mit.edu>
> Date: Wed Oct 24 17:43:10 2012 -0400
>
> The token need not be an encrypted blob
I think this text makes the rest of the document a little unclear if the
token is not an encrypted blob. I'm not sure if it's worth it to bother,
but a sentence or two could be added... something like:
For the remainder of this document, we will assume that the token
contains an encrypted copy of the user identifier and session key, for
concision. Any section referring to token decryption or encryption is
still applicable to applications not encrypting data inside tokens. It
is assumed that such applications will conceptually "decrypt" a token by
looking up the token in a local token table, and will "encrypt" a token
by associating the token value with a token table entry in the server.
> commit ddfa58a6e558139c7d889813bfd8730de05f6d55
> Author: Ben Kaduk <kaduk@mit.edu>
> Date: Wed Oct 24 18:06:59 2012 -0400
>
> Add internal cross-references
[...]
> @@ -422,7 +426,8 @@ enum RXGK_Level {
> the client stores the current timestamp as an rxgk_Time
> (start_time for the rest of this discussion), and
> then uses this, along with other connection information, to derive a
> - transport key from the current user's master key.</t>
> + transport key from the current user's master key
> + (<xref target="derivation" />).</t>
[...]
> @@ -519,7 +524,8 @@ enum RXGK_Level {
>
> <list style="hanging" hangIndent="6">
> <t hangText="start_time:">The time since the Unix epoch
> - (1970-01-01 00:00:00Z), expressed as an rxgkTime.</t>
> + (1970-01-01 00:00:00Z), expressed as an rxgkTime
> + (<xref target="time" />).</t>
I think e.g. (see <xref target="derivation" />) looks a little better
for some outputs, since iirc sometimes the xref is just translated into
e.g. [2] with a link.
> commit 74bc8de3886728c5ace1a28a4c0eacf0c2d68275
> Author: Ben Kaduk <kaduk@mit.edu>
> Date: Wed Oct 24 22:22:10 2012 -0400
>
> Use RXGK_Levels more appropriately
[...]
> @@ -403,7 +403,9 @@ enum RXGK_Level {
> </t>
> <t>To reduce the potential for denial of service attacks, servers
> SHOULD only offer the CombineTokens operation to clients connecting
> - over an rxgk secured connection.</t>
> + over an rxgk secured connection. The RXGK_Level of the rxgk
> + connection does not affect the resiliance against denial of
> + service attacks.</t>
I find the purpose of that last sentence ("don't require any particular
RXGK_Level") not immediately clear from that text. This is minor, but
possible suggested text:
- over an rxgk secured connection.</t>
+ over an rxgk secured connection. The server SHOULD accept
+ connections with any RXGK_Level, since the RXGK_Level of the rxgk
+ connection does not affect the resiliance against denial of
+ service attacks.
> commit 53bfa6afea3d4ecb5161c35dd3a6272c17feb109
> Author: Ben Kaduk <kaduk@mit.edu>
> Date: Wed Oct 24 22:40:28 2012 -0400
>
> Add a couple more references
+1
> commit 2b2773b5f8b94a1594dc4ef588242164e153fb89
> Author: Ben Kaduk <kaduk@mit.edu>
> Date: Wed Oct 24 22:53:52 2012 -0400
>
> Catch up to ticket->token rename
+1
> commit 2f004d75d2d385cc3fc8b5be7b36c15aa93725b8
> Author: Ben Kaduk <kaduk@mit.edu>
> Date: Wed Oct 24 23:01:27 2012 -0400
>
> Add a bit more about 'clear' vs. 'integrity'
+1
> commit 32e778474933dda0847c06c3911af44e0b83efb8
> Author: Ben Kaduk <kaduk@mit.edu>
> Date: Wed Oct 24 23:08:42 2012 -0400
>
> RXGK_StartParams is not made of lists
+1
> commit 87b53d0f24296da5b479ad6dc0351a15b4ac9d6a
> Author: Ben Kaduk <kaduk@mit.edu>
> Date: Wed Oct 24 23:13:20 2012 -0400
>
> An empty token is zero-length, not NULL
+1
> commit c23fc51c268fefc460b224a12b63cd9a9672b538
> Author: Ben Kaduk <kaduk@mit.edu>
> Date: Wed Oct 24 23:21:29 2012 -0400
>
> Describe the GSSNegotiate errorcode field
I wasn't quite sure on what kind of errors are expected here. If they
are what you wrote in this commit, then I think it makes sense to make
this application-specific, and in rxgk-afs we specify a registry to
specify values (or if we want to keep it implementation-private or
something, that's an option, too).
But if this is intended to contain GSS error codes... aren't those
standard? (and don't we need more than one 4-byte int for that?)
> commit 13a2d01b722969da997f1878ad176991fb0ffabc
> Author: Ben Kaduk <kaduk@mit.edu>
> Date: Wed Oct 24 23:26:49 2012 -0400
>
> Clarify token expiry
For krb5-based tokens, does this have any relevance for renewable
tickets? That is, if our expiration time is in 10 hours, but we are
renewable for 7 days, we want this field to specify the 'expiration
time' in 7 days from now, not 10 hours, correct? Or does that just
result in an entirely new connection because the token is effectively
entirely new? (I feel like this is obvious, but after reading this text
for a while I tend to get confused easily... :)
> commit ae442489148fc1c304d385713c442644fee259de
> Author: Ben Kaduk <kaduk@mit.edu>
> Date: Wed Oct 24 23:33:39 2012 -0400
>
> Remind that gss_pseudo_random is deterministic
+1
> commit 0309471e70db7a363892cc345e0e591409c46ac2
> Author: Ben Kaduk <kaduk@mit.edu>
> Date: Wed Oct 24 23:46:36 2012 -0400
>
> Clarify CombineTokens' least-permissiveness
The 'special case' here seems unnecessarily awkward. We can just say
that lifetime, bytelife, and any application-specific data should be
combined so the result is the most restrictive of the two. The other
fields (enctype, level, expiration) should be set to the minimum of the
input tokens.
Also, should the combined token enctype really be set to the minimum of
the input tokens? Does the order of enctypes based on integer value
really mean anything? (should this maybe be "most preferable" enctype
based on the input enctypes, or something?)
> commit ec2362e3bc9f4f567e3d60e3234e344c9d5abccb
> Author: Ben Kaduk <kaduk@mit.edu>
> Date: Wed Oct 24 23:53:01 2012 -0400
>
> Wordsmith
+1
--
Andrew Deason
adeason@sinenomine.net