[Freeipa-devel] [PATCHES] Fix getkeytab operation

[Simo Sorce]

On Tue, 18 Nov 2014 11:23:42 -0500
Nathaniel McCallum <npmccallum at redhat.com> wrote:

> On Tue, 2014-11-18 at 00:01 -0500, Simo Sorce wrote:
> > Hello team,
> > 
> > Recently Alexander opened the following bug:
> > https://fedorahosted.org/freeipa/ticket/4718
> > 
> > The problem with this code is that manual ASN.1 encoding is fragile
> > and too prone to error, and I found various issues while
> > investigating the bug. So I decided to give a shot at replacing the
> > fragile manual code with more robust code autogenerated by the
> > asn1c compiler.
> > 
> > While working on replacing the code with the autogenerated one I
> > discovered additional encoding issues of which the following ticket
> > represent only the most evident:
> > https://fedorahosted.org/freeipa/ticket/4728
> > 
> > I found numerous encoding errors which basically made the getkeytab
> > control we implemented in both the client and the server not respect
> > the encoding we actually defined with ASN.1 notation here:
> > http://www.freeipa.org/page/V4/Keytab_Retrieval
> > 
> > While digging and testing replacement functions is also became
> > evident that the getkeytab feature was only partially working and
> > that the bug in 4718 was not just a minor error, but cannot ever
> > work on existing servers as there are compounding bugs that would
> > prevent using the getkeytab protocol to ever work if the user
> > specified enctypes via ipa-getkeytab instead of just requesting the
> > server's defaults.
> > 
> > Because of this and because it was just too hard *and* useless to
> > try to be compatible with existing broken clients and servers the
> > new code breaks compatibility for correctness of implementation.
> > 
> > The break in compatibility is mitigated by the fact that
> > ipa-getkeytab always falls back to the old setkeytab control in
> > case of error, so normal operations will not be disrupted. The only
> > feature that will not work if you have a buggy client or a buggy
> > server is the keytab retrieval option, as that feature is only
> > available with the new control. Given we have only recently
> > introduced CLI and UI to actually enable the use of the retrieval
> > option and given the fact 4.x has not yet hit major distribution
> > stable releases I think that this patchset is acceptable as long as
> > we can land it in 4.1.2 and/or in an immediately following patch
> > release (also in 4.0.x possibly) so that it can land as a zero day
> > upgrade for Fedora (and an upgrade for Debian).
> > 
> > If you have any questions, please shoot.
> > 
> > This code is fully tested by me on top of master. I think it should
> > apply directly on 4.1.2 and 4.0.x with none or minor changes.
> 
> Patch 0001:
> Typo in the commit message. Otherwise LGTM.
> 
> Patch 0002:
> I would strongly prefer that generated code live in its own directory
> and static library. Then the wrapper around that code should make its
> own library and link to the library for the generated code.
> 
> Something like:
> * asn1/asn1c/libasn1c.a
> * asn1/libipaasn1.a

Although I can see the logic, it sounds a little bit extreme to build a
convenience library for a convenience library ... what's the gain ?
The ipa_asn1.c code is intimately tied to the autogenerated code anyway.

> Also, shouldn't the functions in ipa_asn1.[ch] get a prefix? Maybe
> ipa_asn1_?

uhmm maybe we should indeed.
any other opinion ?

> I'd love to see some function documentation in ipa_asn1.h. At the
> least, this should cover allocation semantics.

Yeah, I added a comment or two in the .c file but did not make it
into .h file, I'll fix that.

> Are there any changes in KeytabModule itself from the previous
> implementation? It looks like yes. NewKeys.enctypes for instance used
> to be SEQUENCE OF Int16, but is now SEQUENCE OF INTEGER. Isn't the
> Kerberos enctype Int32? Why not use this?

INTEGER is really all we need.

> Same with GKReply.newkvno (new_kvno), KrbKey.key, KrbKey.salt, etc.
> 
> It seems to me like you're trying to resist pulling in the Kerberos
> ASN.1 module. If this is the only reason, it seems to me like we'll
> probably need it eventually anyway and we can just configure the
> compiler to drop the unused symbols.

It would be a lot of code we do not need.
I could import just the Int32 definition, but why ?
INTEGER works fine for our purposes (we use system defined integers so
it is limited to a 'long').

> But maybe I'm missing something.
> 
> Why does lber.h get added to ipa_krb5.h? Aren't we trying to get rid
> of lber with this patch?

Because the header file uses struct berval in a function I am not
touching, so it need the include to avoid compile warnings, eventually
we may change things around to improve minor details, but this patch is
blocking for Fedora 21 so I would like to avoid anything that is not a
hard blocker.

Simo.


-- 
Simo Sorce * Red Hat, Inc * New York