On Tue, 18 Nov 2014 14:28:08 -0500 Nathaniel McCallum <npmccal...@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 <npmccal...@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 _______________________________________________ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel