[Freeipa-devel] [PATCHES] Fix getkeytab operation

[Simo Sorce]

On Tue, 18 Nov 2014 14:28:08 -0500
Nathaniel McCallum <npmccallum at redhat.com> wrote:

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

Ok, it's not the work that is extreme, it is just the additional
layering that is a bit silly imo, as the asn1/libipaasn1.a would just
be a very thin wrapper, the generated Makefile will probably be bigger
then the wrapper it compiles :)
but ok.

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

Ok I'll take a stab at it now.

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

The irony did not escape me when I had to make the change, and was
actually expecting a question about it during review :)

Simo.

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