[Freeipa-devel] [PATCHES] Fix getkeytab operation

Tue Nov 18 20:01:15 UTC 2014

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

As I see it, we're setting out a new precedent. All new ASN.1 code will
take this route (which is, indeed, better). So while it is small now, it
won't stay small forever. Being that we are in the business of routinely
handling ASN.1 stuff, this seems to me like a sensible architecture for
the future.

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