[AFS3-std] rxgk implementation notes

[Benjamin Kaduk]

Thanks for the detailed replies.

On Thu, 28 Feb 2013, Simon Wilkinson wrote:

>
> On 28 Feb 2013, at 22:05, Benjamin Kaduk wrote:
>
>> We still have some places that refer to or imply an ordering of 
>> security levels, such as "this MUST NOT be less than the security level 
>> originally negotiated"; the OpenAFS rxkad implementation has this sort 
>> of assumption baked in, in that the client and server track a "minimum 
>> security level" in the security object private data, and make numeric 
>> comparisons against that value.  We used to have a channel-bound 
>> security level, and conceivably one may get added in the future.  It 
>> would be nice to either settle on "a token only supports exactly the 
>> level it was negotiated for" or some better ordering on the values of 
>> security levels.  Having the client and server explicitly track all 
>> allowable security levels would be somewhat annoying.
>
> This is definitely an issue - in my implementation, we just use the 
> various numeric values to provide an ordering in much the same way as 
> rxkad. Whilst this is abstracted out from the rest of the code, I agree 
> that in a world where we have many other security levels this will be a 
> bit of a nightmare.

I'm a bit curious how things would fail if we required an exact match for 
the security level.  I don't think there are many places in the openafs 
codebase that change the level, so it seems likely that almost everything 
is using a single level anyway.

>> Having a mixture of RXGK_Data and plain XDR opaque types gets a bit 
>> annoying; in some places I end up having one variable of each type that 
>> alias the same storage.
>
> This is purely an implementation issue. There's no reason that RXGK_Data 
> can't be a plain XDR opaque type. In fact, if you're using OpenAFS's 
> rxgen properly, they can be. In my implementation, the header from the 
> .xg file has just:
>
>    typedef struct rx_opaque RXGK_Data;

Hmm, that would probably be convenient.  More motivation to write rxgen.1!

>> It's hard to get the logic right for when to terminate a GSS 
>> negotiation loop, especially when the same C variable is used to hold 
>> major/minor status information from both the client and server calls. 
>> I still owe us an update to the text in the document on this matter.
>
> The negotiation loop logic is exactly the same as that in OpenSSH when 
> using GSSAPI - it seems to me to be a pretty standard GSSAPI negotiaton 
> loop.

My point was more "everyone seems to have trouble getting the negotiation 
loop right the first time they write a GSS application"; there's nothing 
particularly special about our case (other than the current text in the 
document being wrong).

>> Having publicly-visible routines that take a gssapi context or other 
>> gssapi type is pretty annoying for the code structure, as it makes 
>> gssapi.h a prerequisite for the rxgk header.  This is rather 
>> inevitable, but it may prove convenient to split that stuff off into a 
>> separate header/library only used by the negotiation service.
>
> I don't think there's any need for any part of the rxgk API to take 
> GSSAPI types. The YFS implementation definitely does not - the whole 
> GSSAPI internals are completely hidden from the caller.

I agree that there's no need.  Looking more carefully, the only routine I 
currently export that takes a gss type is the one to take a GSS security 
context and produce the master key k0; that could easily move to a private 
header.

>> Additionally, the gssapi libraries shipped with some OSes do not 
>> support the pseudo-random functions that we need.  This is just 
>> something that implementations will have to deal with.
>
> There are ways (some pretty, some less pretty) of working around this.

I guess we'll talk about that on openafs-devel when the time comes.

>> It takes a fair amount of code to prepare the several structures that 
>> we specify as opaque or RXGK_Data typed objects that are the XDR 
>> encoding of some more detailed type.  Manually specifying bit layouts 
>> would be differently annoying though, I'm sure.
>
> Specifying them as XDR does specify bit layouts. It's up to the 
> implementor whether to encode them using the XDR functions, or by 
> manually bit twiddling. In either case, the XDR specs within the draft 
> do provide an exact account of which bits should be present on the wire.

Sure.

>> We say "rxgk challenges simply contain some versioning information and 
>> a random nonce selected by the server", though the current RPC-L is 
>> just a nonce.  It sort of seems like it may be worth adding a version 
>> field in case the challenge/response protocol ever needs to be rev'd.
>
> An earlier version of the draft did contain versioning information. 
> However, until we have extended unions, that versioning information is 
> useless - because any attempt to decode a structure with a form other 
> than that expected by the receiver will produce a marshalling error. The 
> only way (currently) to take advantage of a version number is to peek at 
> the structure without decoding it, and then use the version byte to 
> decide which structure decoder to use. This seemed to provide no 
> additional elegance compared to making the decision based on the encoded 
> length of the structure, and so the versioning byte was removed. If (and 
> when) we standardise extended unions, this is definitely something that 
> could be revisited for a later version of rxgk.

Oh, right, I remember now.

> At the moment, I think the reference to versioning information should 
> just be removed.

Okay, I'll do that.

>> Endianness conversion routines for 64-bit quantities (i.e., rxgkTime) 
>> do not seem to be universally available; an incorrect hand-rolled 
>> implementation could lead to interoperability issues.
>
> These are specified as part of XDR, and are used throughout AFS - 
> providing both ends of the connection are using correct XDR 
> implementations, this shouldn't be an issue.

We have to manually generate NBO for the seed for the PRF+ used to 
generate the transport key; I suppose we could leverage the XDR encoder 
for parts of this but it seemed easier to just use htonl for the 32-bit 
fields.

>> It's a bit awkward to include call number information in the 
>> authenticator (read: feels like a layering violation), and it's not 
>> clear to me what benefit is gained from doing so.  I vaguely remember 
>> seeing some discussion of this in an archive somewhere, but can't find 
>> it now.
>
> I don't have a reference to hand, but the importance of this is 
> discussed in one of the CITI RX security papers. Essentially, it removes 
> an avenue by which an attacker can hijack an encrypted connection. Much 
> of the design of rxgk (and the modifications to rxkad) come from the 
> security work done by CITI on AFS-3. If you haven't read their RX papers 
> already, I would strongly recommend doing so.

Thanks for the pointer, I have not read those papers.

>>  All I see looking nowis discussion of using the maxcalls information 
>> as a way to migrate to having more than 4 calls per connection, which 
>> doesn't sound familiar. We also don't mention whether call numbers in 
>> use are checked as part of verifying the authenticator -- I do not 
>> currently do so.
>
> You should just provide the call number vector to the RX layer, which 
> will take care of checking the call numbers within it (OpenAFS's RX 
> currently just sets up the connection according to the provided 
> authenticator, which will cause non-matching calls to fail as they are 
> encountered)

Oh, hmm, I guess rxi_SetCallNumberVector is exported.

>> Strictly speaking, this affects the rxgk-afs document, but including 
>> the client UUID in the authenticator's appdata field is pretty awkward. 
>> This field "contains the UUID of the client", which sounds like it 
>> should be the cache manager's idea of the UUID, but getting that from 
>> the rx layer seems to be a layering violation,
>
> This is based on work that Tom Keiser was doing - 
> draft-tkeiser-rxrpc-sec-clear provides a detailed account of his 
> motivations.

Okay, I'll take a look.

>> and we may have situations (e.g., bos) where an rxgk client is not 
>> running a cache manager.
>
> The intent of this document was that you would only be using rxgk-afs 
> with fileserver-like protocols. That is, specifically not bos.

Hmm, I guess we can make an optional uuid field in the client security 
object and only fill a uuid in the appdata if it's present.

-Ben