[Freeipa-devel] [PATCHES] Fix getkeytab operation

[Nathaniel McCallum]

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

Also, shouldn't the functions in ipa_asn1.[ch] get a prefix? Maybe
ipa_asn1_?

I'd love to see some function documentation in ipa_asn1.h. At the least,
this should cover allocation semantics.

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?

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

Nathaniel