ipa-kdb: support KDB DAL version 6.1 #410

abbra · 2017-01-24T07:11:16Z

DAL version 6.0 removed support for a callback to free principal.
This broke KDB drivers which had complex e_data structure within
the principal structure. As result, FreeIPA KDB driver was leaking
memory with DAL version 6.0 (krb5 1.15).

DAL version 6.1 added a special callback for freeing e_data structure.
See details at krb5/krb5#596

Restructure KDB driver code to provide this callback in case
we are built against DAL version that supports it. For DAL version
prior to 6.0 use this callback in the free_principal callback to
tidy the code.

https://fedorahosted.org/freeipa/ticket/6619

On Fedora the required interface is available in krb5-1.15-5.fc26 package.

sumit-bose · 2017-01-24T08:47:18Z

Are there any plans how to handle 6.0? Should configure at least show a warning if KRB5_KDB_DAL_MAJOR_VERSION == 6 but no free e_data callback was found?

Should the .min_ver in the kdb_function_table be set to '1' if there is a free e_data callback?

abbra · 2017-01-24T09:37:18Z

Thanks for the suggestions. I've updated the configure check to explicitly warn when both .free_principal and .free_principal_e_data are missing. DAL version 5 had .free_principal and we do not support any other DAL versions yet, so this should be enough.

I also merged definitions of the kdb_function_table for both DAL versions by adding corresponding initializers in the right places wrapped with the #ifdef-s. I think it will be better than the current duplication, considering we need to support three different API variations.

simo5 · 2017-01-24T13:48:26Z

abbra, we should also change how spec deps work
I asked @RHarwood to add a provides that is the dal version number
we should stop having a dep on the krb5 major version number and instead have a dependecy on this provide

simo5 · 2017-01-24T13:53:28Z

Also I know you can use ifdefs to avoid copy&pasting large parts of the structure initialization but I would prefer 3 separate full inits based only on ifdefs on the DAL version numbers.
in pseudo:
if v5:
vtable = { ... }
elif v6.0:
vtable = { ... }
elid v6.1:
vtable = { ... }
else:
error!

Those tables cannot change so using ifdefs in them can only risk to introduce bugs in one of the versions rather than help reduce code duplication.

abbra · 2017-01-24T14:01:40Z

@simo5 spec dependencies are separate from the code -- the spec will not help on Debian, for example.
We need both the spec dependencies and the proper checks in the code.

simo5 · 2017-01-24T14:48:14Z

Doesn't kdb.h also export a MINOR version to test against ?

simo5 · 2017-01-24T14:55:18Z

I checked and can't find it ... facepalm

abbra · 2017-01-24T14:57:50Z

No, no minor DAL version. That's why I had to resort to structure member checks in autoconf.

frozencemetery · 2017-01-24T19:54:59Z

There was talk of exporting a minor dal version but I think upstream explicitly doesn't want it.

freeipa.spec.in should be modified if I understand correctly; otherwise this looks good.

abbra · 2017-02-06T12:41:25Z

I split the tables into separate ones and also made independent #if/#endif blocks for them. Finally, I added a spec file guard to force using 1.15-5 or later version on Fedora 26 or later.

simo5

It'd be nice to use the provide added to the Fedora spec file, otherwise LGTM

simo5 · 2017-02-06T13:31:29Z

freeipa.spec.in

@@ -57,8 +57,12 @@ Source0:        freeipa-%{version}.tar.gz
 BuildRoot:      %{_tmppath}/%{name}-%{version}-%{release}-root-%(%{__id_u} -n)

 BuildRequires:  openldap-devel
+%if 0%{?fedora} > 25
+BuildRequires: krb5-devel >= 1.15-5


@frozencemetery introduced a provides that expicitly marks DAL versions, can we use that provide instead of a version number ?

Field is named krb5-kdb-version, and 1.15.5 sets it to 6.1.

@simo5 @frozencemetery unfortunately, the provide of "krb5-kdb-version = 6.1" is on krb5-libs, not on krb5-devel, so I cannot do a buildrequires dependency this way.

abbra · 2017-02-07T08:46:13Z

@simo5 @frozencemetery unfortunately, the provide of "krb5-kdb-version = 6.1" is on krb5-libs, not on krb5-devel, so I cannot do a buildrequires dependency this way.

simo5 · 2017-02-07T09:48:09Z

@frozencemetery Should we provide krb5-kdb-version-devel from krb5-devel ?

frozencemetery · 2017-02-07T18:08:23Z

@simo5 @abbra I'll move it over, but it won't break anything to pull in krb5-devel and krb5-kdb-version as far as I can tell since you're getting krb5-libs pulled in anyway.

abbra · 2017-02-07T19:10:37Z

Updated the spec file and the commit message.

frozencemetery

Nice, you're faster than the fedora builders. krb5-1.15-6 has it as part of krb5-devel.

simo5 · 2017-02-07T19:29:36Z

freeipa.spec.in

+# DAL version change may cause code crash or memory leaks, it is better to fail early.
+%if 0%{?fedora} > 25
+BuildRequires: krb5-devel >= 1.15-5
+BuildRequires: krb5-kdb-version = 6.1


Shouldn't we add this to Requires ad well ?

No. krb5-kdb-version is in krb5-devel now, requiring krb5-devel to installed system is not needed. However, when building the package we'll get krb5 >= 1.15-5 due to 'krb5-kdb-version = 6.1' BuildRequire and, as result, Requires(post): krb5-server >= %{krb5_base_version} will be set which will force use of 1.15.

Granted, this could be not enough but released version of F26 will have krb5-server 1.15-6 or later, so I think it is OK.

DAL version 6.0 removed support for a callback to free principal. This broke KDB drivers which had complex e_data structure within the principal structure. As result, FreeIPA KDB driver was leaking memory with DAL version 6.0 (krb5 1.15). DAL version 6.1 added a special callback for freeing e_data structure. See details at krb5/krb5#596 Restructure KDB driver code to provide this callback in case we are built against DAL version that supports it. For DAL version prior to 6.0 use this callback in the free_principal callback to tidy the code. Use explicit KDB version dependency in Fedora 26+ via BuildRequires. With new DAL version, freeipa package will fail to build and we'll have to add a support for new DAL version explicitly. https://fedorahosted.org/freeipa/ticket/6619

abbra · 2017-02-13T07:47:59Z

I've rebased against master and added responses to inline comments in the PR.

simo5

LGTM

MartinBasti · 2017-02-15T13:24:27Z

Fixed upstream
master:
https://fedorahosted.org/freeipa/changeset/593ea7da9a732647052cb56c08ad367e40be3912

abbra force-pushed the krb5-1-15-DAL-changes branch from e4c0982 to a6c0013 Compare January 24, 2017 09:34

abbra force-pushed the krb5-1-15-DAL-changes branch from a6c0013 to 0972231 Compare February 6, 2017 12:37

simo5 reviewed Feb 6, 2017

View reviewed changes

abbra force-pushed the krb5-1-15-DAL-changes branch 2 times, most recently from 1d1fcf7 to a08f150 Compare February 7, 2017 19:09

frozencemetery approved these changes Feb 7, 2017

View reviewed changes

simo5 reviewed Feb 7, 2017

View reviewed changes

abbra force-pushed the krb5-1-15-DAL-changes branch from a08f150 to 2bbe7c1 Compare February 13, 2017 07:38

simo5 approved these changes Feb 13, 2017

View reviewed changes

simo5 added the ack Pull Request approved, can be merged label Feb 14, 2017

MartinBasti added the pushed Pull Request has already been pushed label Feb 15, 2017

MartinBasti closed this Feb 15, 2017

tkrizek mentioned this pull request Mar 16, 2017

[ipa-4-4] ipa-kdb: support KDB DAL version 6.1 #606

Closed

ipa-kdb: support KDB DAL version 6.1 #410

ipa-kdb: support KDB DAL version 6.1 #410

abbra commented Jan 24, 2017 •

edited by MartinBasti

sumit-bose commented Jan 24, 2017

abbra commented Jan 24, 2017

simo5 commented Jan 24, 2017

simo5 commented Jan 24, 2017

abbra commented Jan 24, 2017

simo5 commented Jan 24, 2017

simo5 commented Jan 24, 2017

abbra commented Jan 24, 2017

frozencemetery commented Jan 24, 2017

abbra commented Feb 6, 2017

simo5 left a comment

simo5 Feb 6, 2017

frozencemetery Feb 6, 2017

abbra Feb 7, 2017

abbra commented Feb 7, 2017

simo5 commented Feb 7, 2017

frozencemetery commented Feb 7, 2017 •

edited

abbra commented Feb 7, 2017

frozencemetery left a comment

simo5 Feb 7, 2017

abbra Feb 13, 2017

abbra commented Feb 13, 2017

simo5 left a comment

MartinBasti commented Feb 15, 2017

ipa-kdb: support KDB DAL version 6.1 #410

ipa-kdb: support KDB DAL version 6.1 #410

Conversation

abbra commented Jan 24, 2017 • edited by MartinBasti

sumit-bose commented Jan 24, 2017

abbra commented Jan 24, 2017

simo5 commented Jan 24, 2017

simo5 commented Jan 24, 2017

abbra commented Jan 24, 2017

simo5 commented Jan 24, 2017

simo5 commented Jan 24, 2017

abbra commented Jan 24, 2017

frozencemetery commented Jan 24, 2017

abbra commented Feb 6, 2017

simo5 left a comment

Choose a reason for hiding this comment

simo5 Feb 6, 2017

Choose a reason for hiding this comment

frozencemetery Feb 6, 2017

Choose a reason for hiding this comment

abbra Feb 7, 2017

Choose a reason for hiding this comment

abbra commented Feb 7, 2017

simo5 commented Feb 7, 2017

frozencemetery commented Feb 7, 2017 • edited

abbra commented Feb 7, 2017

frozencemetery left a comment

Choose a reason for hiding this comment

simo5 Feb 7, 2017

Choose a reason for hiding this comment

abbra Feb 13, 2017

Choose a reason for hiding this comment

abbra commented Feb 13, 2017

simo5 left a comment

Choose a reason for hiding this comment

MartinBasti commented Feb 15, 2017

abbra commented Jan 24, 2017 •

edited by MartinBasti

frozencemetery commented Feb 7, 2017 •

edited