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. > > 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 _______________________________________________ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel