Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

client install: do not assume /etc/krb5.conf.d exists #623

Closed
wants to merge 1 commit into from
Closed

client install: do not assume /etc/krb5.conf.d exists #623

wants to merge 1 commit into from

Conversation

HonzaCholasta
Copy link
Contributor

@HonzaCholasta HonzaCholasta commented Mar 20, 2017

Add includedir /etc/krb5.conf.d to /etc/krb5.conf only if
/etc/krb5.conf.d exists.

This fixes client install on platforms which do not have /etc/krb5.conf.d.

https://pagure.io/freeipa/issue/6589

@tiran
Copy link
Member

tiran commented Mar 20, 2017

I'd rather create /etc/krb5.conf.d than to make the line conditional.

@HonzaCholasta
Copy link
Contributor Author

There is no reason to, the directory is not owned by us and we don't use it for anything anyway (see ticket triage for relevant discussion).

@puiterwijk
Copy link

Would you be upgrading the krb5.conf after people upgrade krb5-libs to include the new includedir then?
Since that's what would happen if you don't change the krb5.conf and people update to a krb5-libs that has the includedir.

I've had to help a lot of people that ended up with configuration files lacking krb5.conf.d due to ipa-client setups (and other company configs, but at least that's limited to people working at companies giving broken krb5 configs).

]

if os.path.exists(paths.COMMON_KRB5_CONF_DIR):
opts += [

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why using opts += here, while the rest of the code uses opts.extend?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because I didn't notice that.

@HonzaCholasta
Copy link
Contributor Author

@puiterwijk, upgrade will be handled by krb5 itself, see https://bugzilla.redhat.com/show_bug.cgi?id=1431198.

@lslebodn
Copy link
Contributor

lslebodn commented Mar 27, 2017

FYI: /etc/krb5.conf.d is not default include directory. It is fedora/el7 specific.

debian testing has MIT kerberos 1.15 and /etc/krb5.conf.d does not exist there as is not included in /etc/krb5.conf.

So +1 for @HonzaCholasta approach.

@tiran
Copy link
Member

tiran commented Mar 28, 2017

The ipa-certauth plugin now starts to rely on the existence of /etc/krb5.conf.d:

%config(noreplace) %{_sysconfdir}/krb5.conf.d/ipa-certauth

Practicality beats purity, let's make /etc/krb5.conf.d part of the offical FreeIPA configuation settings on all IPA enrolled systems.

@lslebodn
Copy link
Contributor

lslebodn commented Mar 28, 2017 via email

@tiran
Copy link
Member

tiran commented Mar 28, 2017

Practicality beats purity

Let's define /etc/krb5.conf.d as part of our API and don't waste more time on shaving yet another yak. @tjaalton (Debian/Ubuntu maintainer) said

fine by me

@frozencemetery
Copy link
Contributor

(Note: a standard directory in distributions that freeipa could use would be provided by the krb5 maintainer, not the freeipa maintainer.)

Add `includedir /etc/krb5.conf.d` to /etc/krb5.conf only if
/etc/krb5.conf.d exists.

Do not rely on /etc/krb5.conf.d to enable the certauth plugin.

This fixes install on platforms which do not have /etc/krb5.conf.d.

https://pagure.io/freeipa/issue/6589
@frozencemetery
Copy link
Contributor

Adding on to my previous comment: I've talked with the Debian maintainers, and they plan to add the same includedir after the Stretch release. So, eventually (for some values of eventually) we won't have to worry about this.

@HonzaCholasta
Copy link
Contributor Author

@frozencemetery, this is not for the sake of Debian. We will still have to worry about this for operating systems which are not Fedora- or Debian-based.

@martbab martbab self-assigned this Jun 28, 2017
@martbab martbab added ack Pull Request approved, can be merged pushed Pull Request has already been pushed labels Jun 28, 2017
@martbab
Copy link
Contributor

martbab commented Jun 28, 2017

master:

  • d5fc0dd install: do not assume /etc/krb5.conf.d exists

@martbab martbab closed this Jun 28, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ack Pull Request approved, can be merged pushed Pull Request has already been pushed
Projects
None yet
6 participants