[Freeipa-devel] Design Review Keytab Retrieval

[Nathaniel McCallum]

On Fri, 2014-06-20 at 14:38 -0400, Simo Sorce wrote:
> On Fri, 2014-06-20 at 14:30 -0400, Nathaniel McCallum wrote:
> > On Fri, 2014-06-20 at 14:10 -0400, Simo Sorce wrote:
> > > On Fri, 2014-06-20 at 14:05 -0400, Nathaniel McCallum wrote:
> > > > On Mon, 2014-06-16 at 11:34 -0400, Simo Sorce wrote:
> > > > > Although the code is all done it would be nice to have a review of the
> > > > > feature, to see if it has all been captured:
> > > > > http://www.freeipa.org/page/V4/Keytab_Retrieval
> > > > 
> > > > I'm a bit confused about the behavior of enctypes in the Request.
> > > > 
> > > > "A list of enctypes is always necessary in input when a new keytab is
> > > > requested. However the list is filtered though the allowable enctypes
> > > > list and if nothing is left the operation is refused."
> > > > 
> > > > +1. However, the generated keys should be the set of allowed enctypes,
> > > > not the intersection between allowed and requested enctypes. This would
> > > > permit the later requesting of enctypes that were allowed at the time of
> > > > creation, but not requested.
> > > 
> > > Which you absolutely do not want.
> > > If your service understands only RC4-HMAC and you provide it with AES
> > > keys then communication from a third party client will fail because the
> > > KDC will give that client an AES encrypted ticket that the service does
> > > not know how to decrypt.
> > > 
> > > This is particularly important for old NFS Servers (like RHEL5) that
> > > understand only DES (sigh!)
> > > 
> > > > "If the getNew attribute is false, then the existing key is being
> > > > requested. In this case password and enctypes MUST NOT be set."
> > > > 
> > > > I don't get this. Shouldn't the return value of this request include
> > > > only the intersection between allowed and requested enctypes?
> > > 
> > > Not when you are retrieving existing keys. Only when you are creating
> > > new keys.
> > > 
> > > > There is no point in responding with enctypes the client has not requested.
> > > > And indeed, this provides extra data points to attack.
> > > 
> > > ??
> > > 
> > > > Having this proposed behavior also means you can remove OPTIONAL from
> > > > enctypes.
> > > 
> > > OPTIONAL is there because when you request existing keys it makes no
> > > sense to send a lit of enctypes, you get what the KDC has, the whole
> > > package, because you must be able to decrypt any ticket you get, getting
> > > a subset of keys would make no sense.
> > > 
> > > > So as it stands, enctypes currently controls what keys are generated. I
> > > > would prefer that enctypes controls what keys are returned. Am I missing
> > > > something?
> > > 
> > > I most definitely think you are :-)
> > 
> > Ah, right. This boils down to the KDC not having any way to know what
> > enctypes the destination principal supports. Thanks for clarifying that.
> > 
> > In this case, the ASN.1 provided is confusing. I think something like
> > this would be clearer and less error prone:
> > 
> > KeytabGetMessage ::= CHOICE {
> >     fetch        [0] Fetch,
> >     create       [1] Create
> >     reply        [2] Reply
> > }
> > 
> > Fetch ::= SEQUENCE {
> >     serviceIdentity [0] OCTET STRING
> > }
> > 
> > Create ::= SEQUENCE {
> >     serviceIdentity [0] OCTET STRING,
> >     enctypes        [1] SEQUENCE OF Int16,
> >     password        [2] OCTET STRING OPTIONAL
> > }
> > 
> > This would also, I think, result in a cleaner implementation where the
> > type of operation is logically built into the decoding step.
> 
> Well yes this looks better, but honestly, I think it is a bit late for
> this kind of feedback, given I have a working patchset, and asked months
> ago if people wanted to review the logical design.

You asked for a design review on Monday of this week. I am providing
that review. Do you want it or not?

> How strongly do you feel we absolutely need to change this and go
> through a new review cycle ?

This change would have very small impact on your patch set, but would be
much clearer for the future consumers of this protocol. Code can be
changed; protocols can't.

I am finishing the design doc now and will move to your patch set
shortly. I'd be happy to provide a quick code review for this change.

Nathaniel