Here are some comments on afs3-rxgk-04. Many of these are editing notes, but I 
think this document needs further work with regards to the description of the 
GSSAPI handshake, and a fuller discussion of limits to the sizes of the various 
opaque data types is required.

Abstract
--------
> This document provides a general description of
> rxgk. A further document will provide details of integration with
> specific RX applications.

I think we're now at the point where this document provides information on how 
to integrate rxgk with generic RX applications - the only information in 
rxgk-afs is how to integrate it with the AFS-3 protocol family - so I think 
this could be clearer. Perhaps:

"This document provides a general description of rxgk and how to integrate it 
into generic RX applications. Application specific behaviour will
be described, as necessary, in future documents"

1. Introduction
---------------

> It builds on the Kerberos crypto framework

There should probably be an RFC3961 reference here.

3.1 Key Usage Values
--------------------

>   const RXGK_SERVER_ENC_TOKEN     = 1036;

Indentation is a bit messed up here

5. Token Format
---------------

> The token is completely opaque to the client, which just transmits it from 
> server to server

I had some trouble parsing "server to server" in this sentence. I wonder if 
"which just receives it from one server and passes it to another" might make 
the intent clearer.

6. Key Negotiation
------------------

>        const RXGK_MAXENCTYPES = 10;

Limiting to a maximum of 10 encryption types seems somewhat low, given that 
this is a hard limit that can't be negotiated around. Each encryption type only 
takes 4 bytes on the wire, so there doesn't seem to be a resource-based limit 
here. Given the sprawling proliferation of Kerberos encryption types, I can see 
somebody, at some point, wanting to use more than 10 here.

>        const RXGK_MAXLEVELS = 10;

Whilst we only support 3 levels today, again a hard limit of 10 seems low.

How about a limit of 255 for each of these?

>       const RXGK_MAXMIC = 16384;

By contrast, this seems large. The MIC is going to be the length of the chosen 
encryption type's checksum, plus a small amount of header material. For 
aes-256, that's a mere 28 octets (16 of CFX token, 12 of checksum). So, I think 
we could get away with something smaller here.

>       typedef opaque RXGK_Data<>;
[ ... ]
>           RXGK_Data token;

Given we're limiting everything else, should we also put some kind of upper 
limit on the size of the token? We do need to consider, though, that a token 
may contain non-Kerberos identities, and X509 (in particular) identities can be 
very large.

>       GSSNegotiate(IN RXGK_StartParams *client_start,
>            IN RXGK_Data *input_token_buffer,
>            IN RXGK_Data *opaque_in,
>            OUT RXGK_Data *output_token_buffer,
>            OUT RXGK_Data *opaque_out,
>            OUT unsigned int *gss_major_status,
>            OUT unsigned int *gss_minor_status,
>            OUT RXGK_Data *rxgk_info) = 1;

Same question here - RXGK_Data has no limits.

> This nonce SHOULD be at least 20 octets in length.

Is SHOULD strong enough here? Using a nonce that is too short will reduce the 
security of the resulting key.

> The GSS service name is application dependent.

We've described how to construct the acceptor name earlier in this section - 
perhaps refer back to that?

> The client then calls RXGK_GSSNegotiate, as defined above.

The client should only call RXGK_GSSNegotiate if the call to 
GSS_Init_sec_context succeeded

>  client_start  The client params structure detailed above.  This
>         should remain constant across the negotiation

"should" or SHOULD, or even MUST?

>   Each call to GSSNegotiate will return an output token from
>   GSS_Accept_sec_context() and/or an output opaque

I don't think that "and/or" is appropriate here. Output opaques are separate 
from the negotiation itself - they just serve as a means for the server to 
preserve state across multiple RPCs.

>         If the major status code from GSS_Init_sec_context() includes a
>         fatal error code, the negotiation loop is in an error condition
>         and terminates.

"includes a fatal error code" doesn't seem clearly defined. How about just 
"indicates a GSSAPI error"  

>         If the major status code is GSS_S_COMPLETE and
>         the mutual_state flag is not true, or the major status code is
>         GSS_S_COMPLETE and the conf_avail flag is not true, or the
>         major status code is GSS_S_COMPLETE and the integ_avail flag is
>         not true, the negotiation loop is in an error condition and
>         terminates.  

This could be expressed much more succinctly as:
"If the major status code is GSS_S_COMPLETE and the mutual_state, conf_avail 
flag and integ_avail flags are not all true, the negotiation loop is in an 
error condition and terminates"

>         If the major status code is GSS_S_COMPLETE and the
>         output token is zero length, this is a success condition and
>         the negotiation loop terminates (this cannot happen on the
>         first iteration of the loop).  Otherwise, if the major status
>         code does not include GSS_S_CONTINUE_NEEDED, the negotiation
>         loop is in an error condition and terminates.  If the major
>         status code includes GSS_S_CONTINUE_NEEDED, the output token is
>         sent to the server, per the next step.

This paragraph suggests that returning GSS_S_COMPLETE from 
GSS_Init_sec_context() and a non-empty output token is an error. All of the 
other GSS negotiation loops that I'm aware of permit this behaviour (the output 
token is simply sent to the server). This behaviour is also specifically 
permitted by RFC 4462.

>        If the most recent call to
>        GSS_Init_sec_context() returned a major status code including
>        GSS_S_CONTINUE_NEEDED and the output_token_buffer is zero
>        length, the negotiation loop is in error and terminates.

This doesn't need to be explicitly stated - if it is an error, then 
Gss_Init_sec_context() will handle it when we feed it the zero length token. 

Also:

When the server receives GSS_S_COMPLETE from Gss_Accept_sec_context, it also 
needs to check the context flags and make sure that at least conf_avail and 
integ_avail are true. It needs to do that before it constructs the ClientInfo 
structure.

I found the description of the loop hard going, and I know what it should do! 
I'm not sure how best to improve this, but as I've said before, I believe the 
language, and layout, of the description in the SSH GSSAPI RFC to be a good 
place to start.

>  If the returned value of conf_state is zero, the
>  negotiation has failed

It needs to be clearer that this is an output parameter from gss_unwrap. I 
started out grepping the document to work out where it was coming from!

7.3 RPC Definition
------------------

>       struct RXGK_TokenInfo {
>           int errorcode;

I'm not convinced that we need a separate error code here. Clients shouldn't be 
making negotiation decisions based upon the results of CombineTokens - a 
failure of this function should just be reported up to the user.

8.5 The Response
----------------

>   const RXGK_MAXAUTHENTICATOR = 1500  /* better fit in a packet! */

The normal maximum size of an RX payload is 1416 bytes, so an authenticator of 
1500 definitely won't fit in a packet! rxkad ends up taking liberties here, and 
just extends the maximum size of packet when the data it needs to transmit is 
too large, but that causes issues on networks that drop fragmented UDP. It 
would be good to avoid those here...

12.3 Nonce Lengths
------------------

>  The client_nonce is important for using the MIC of the StartParams
>   structure as a defense against man-in-the-middle attacks,

I'm not sure what this sentence is trying to say?

Cheers,

Simon

_______________________________________________
AFS3-standardization mailing list
[email protected]
http://lists.openafs.org/mailman/listinfo/afs3-standardization

Reply via email to