[Freeipa-devel] [PATCHES] Fix getkeytab operation

[Nathaniel McCallum]

On Tue, 2014-11-18 at 12:40 -0500, Simo Sorce wrote:
> 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.

Moving the files to a new directory and creating a new libtool library
can hardly be called "extreme." It would probably take 5 minutes of
work. It probably took you longer to respond to this email. :)

I think it adds a great deal of readability to a new programmer
approaching this code. And that, for me, is worth it.

> > 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.

That would be automatically generated once and probably never touched
again.

> 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').

It works and is easier, at the expense of readability (and perhaps some
subtle bugs later for the other types). It really just seems odd to me
to define a protocol for transferring Kerberos types while avoiding the
use of Kerberos types solely to avoid automatically generating some
code.

> > 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.

Makes sense. This was mostly just a curious oddity.

Nathaniel