Remove non-sensical kdestroy on https stop #468

simo5 · 2017-02-15T09:59:19Z

This kdestroy runs as root and wipes root's own ccachs ...
this is totally inappropriate.

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

Signed-off-by: Simo Sorce simo@redhat.com

tiran · 2017-02-15T10:00:20Z

ipaplatform/redhat/tasks.py

@@ -451,8 +451,7 @@ def configure_httpd_service_ipa_conf(self):
            dict(
                KRB5CC_HTTPD=paths.KRB5CC_HTTPD,
                KDCPROXY_CONFIG=paths.KDCPROXY_CONFIG,
-                IPA_HTTPD_KDCPROXY=paths.IPA_HTTPD_KDCPROXY,
-                POST='-{kdestroy} -A'.format(kdestroy=paths.KDESTROY)
+                IPA_HTTPD_KDCPROXY=paths.IPA_HTTPD_KDCPROXY


please leave the trailing comma

martbab · 2017-02-15T15:18:29Z

I would rather keep kdestroy there, but only really purge the apache ccache explicitly:

--- a/ipaplatform/redhat/tasks.py
+++ b/ipaplatform/redhat/tasks.py
@@ -452,7 +452,8 @@ class RedHatTaskNamespace(BaseTaskNamespace):
                 KRB5CC_HTTPD=paths.KRB5CC_HTTPD,
                 KDCPROXY_CONFIG=paths.KDCPROXY_CONFIG,
                 IPA_HTTPD_KDCPROXY=paths.IPA_HTTPD_KDCPROXY,
-                POST='-{kdestroy} -A'.format(kdestroy=paths.KDESTROY)
+                POST='-{kdestroy} -c {ccache}'.format(
+                    kdestroy=paths.KDESTROY, ccache=paths.KRB5CC_HTTPD)
             )
         )

Otherwise we will bump into regressions caused by stale HTTPD ccaches left over when backing up/restoring IPA installation. I would demonstrate it on a failing backup/restore tests but they are completely messed up right now.

tiran · 2017-02-15T15:25:02Z

Why do we back up ccache in the first place?

martbab · 2017-02-15T15:34:19Z

We do not backup ccache, we back up apache keytab.

During restore into installer server we back up old Kerberos keys, but without any mechanism to purge the new apache ccache acquired during the installation of new server you would end up with key mismatch and nothing would work until the ccache expires.

As to why a) we backup Kerberos keys, and b) support restoring into running IPA server that is beyond me.

rcritten · 2017-02-15T15:39:20Z

If you don't backup the keytab then how do you expect to bring the server back up? Fetch new keys for all services?

Full restore is very clearly documented as a recovery from complete failure in which case the restored master is the only one so there should be no mismatch between what was backed-up and what was restored.

martbab · 2017-02-15T15:42:31Z

@rcritten can you please re-read my comment very slowly? I wrote that we do backup keytabs.

martbab · 2017-02-15T15:50:09Z

And indeed I can reproduce the original failure reported in https://fedorahosted.org/freeipa/ticket/5296 with this PR.

If I manually remove apache ccache (kdestroy -c /var/run/httpd/ipa/krbcache/krb5ccache) I can use the framework again.

rcritten · 2017-02-15T15:54:23Z

Rudeness is not necessary.

You said:

"As to why a) we backup Kerberos keys, and b) support restoring into running IPA server that is beyond me."

The reason for a) is to restore an exact copy of what was backed up.

As for b, the idea of restoring into a running IPA server to replace the existing install with a new one is something I discuss in some detail at http://www.freeipa.org/page/V3/Backup_and_Restore and outline the ton of problems associated with it. It was never intended to be supported due to these issues.

tiran · 2017-02-15T15:57:46Z

I'm with @rcritten .

If we need to clean up / remove some files during a restore, then these clean-ups should be handled by ipa-restore. The service files are IMHO the wrong place.

pvoborni · 2017-02-15T16:15:55Z

And AFAIK b) is not supported. @martbab , does something indicate otherwise?

martbab · 2017-02-15T16:17:38Z

@rcritten I apologize for sounding rude. I misread your comment and interpreted it differently than intended.

That said, if the restore to a running IPA server is not intended to be supported, why do we have a number of tests for this scenario? I have tried to find some discussion in the design page you posted but did not find any discussion of restore into running server, only the steps taken.

@tiran I tend to agree with you now. It seemed like a good idea to purge ccaches in the unit file when we switched from KEYRING: to FILE: for apache. However the restore use-case is not the only one which can result into stale ccache, I can also think about requesting new Apache keytab, restarting the service and be left with a stale ccache and key mismatch again.

MartinBasti · 2017-02-15T16:24:49Z

@pvoborni this is the way how it this tested by QA, so that's why I added this kind of test to upstream. I disagree that b) is not supported. It is just emulation fo case when user ruined kerberos keytabs and service configuration and the one needs to restore backup on the installed server.

simo5 · 2017-02-16T15:14:25Z

@MartinBasti the unit files are the wrong place to destroy ccaches, especially given they run as a different user (root) and may not have access to destroy stuff when we start using KCM.
If we need clear ccaches then we need a different plan, please reopen the original bug, and push this PR to fix the impeding issue.

MartinBasti · 2017-02-16T15:23:04Z

@simo5 any ideas how this should be fixed? We cannot push this patch without additional fix of removing outdated ccache because it will cause permanent fail of CI for backup/restore and it will mask real issues.

simo5 · 2017-02-16T15:53:41Z

If this is about backup/restore, add a kdestroy ccache in the restore scripts, making sue it su - apache first

MartinBasti · 2017-02-16T16:02:11Z

how about @martbab comment?

#468 (comment)

However the restore use-case is not the only one which can result into stale ccache, I can also think about requesting new Apache keytab, restarting the service and be left with a stale ccache and key mismatch again.

simo5 · 2017-02-16T16:16:04Z

If you request a new keytab you should clean up the cacche ?
If we have a way to run the post exec command as the right user and with the right /tmp (httpd unit file uses namepaced /tmp) we could keep this code in the unit file I guess, although it would be wasteful in most cases when ccache does not change...

martbab · 2017-02-17T07:45:56Z

Could we use just keep the post command as "kdestroy -c {apache_ccache_path}"? Or is everything chrooted into name-spaced /tmp and we can not access the ccache file from within the unit file?

abbra · 2017-02-17T09:44:55Z

Yes, when namespaced /tmp is used, unit file does not have any view into that.

tiran · 2017-02-17T10:45:06Z

How about we use systemd PrivateTmp for temporary files? It is not only more secure but it also automatically removes all temporary files when the service is stopped: https://www.freedesktop.org/software/systemd/man/systemd.exec.html#PrivateTmp=

abbra · 2017-02-17T10:50:11Z

@tiran we do use PrivateTmp already. This is not about PrivateTmp, though, because we don't store credentials caches in a private tmp.

tiran · 2017-02-17T11:04:49Z

That's my point. Why is the ccache file not stored in PrivateTmp? The ccache can be removed at any time. It doesn't have to be retained.

PrivateTmp solves the issue for us. We just have to figure out how to combine tmpfiles.d with PrivateTmp to create /var/run/apache in private temp.

simo5 · 2017-02-17T13:45:51Z

I guess we can simply set KRB5CCNAME=/tmp/krb5_httpd in the unit file and we should be ok then.
@martbab or @mbasti, can you try that ?
If it solves your scenario we can change this PR to replace the POST with that Env in the unit file.

simo5 · 2017-02-17T13:53:21Z

Uhm I just tried setting KRB5CCNAME=/tmp/krb5_httpd in my install and ... I found out we do not actually generate an httpd ccache, so why are we trying to destroy the ccache again ?
Anyway, I am going to add this Environment line to the unit file just in case, so that we can address the issue if we ever need a ccache.

martbab · 2017-02-17T14:16:01Z

install/share/ipa-httpd.conf.template

@@ -1,7 +1,7 @@
 # Do not edit. Created by IPA installer.

 [Service]
+Environment=KRB5CCNAME=/tmp/krb5-httpd


I would rather revive paths.KRB5CC_HTTPD and substitute it to the template than having magic strings.

I also noticed that KRB5CC_HTTPD was left over in ipaplatform.debian.paths module.

ok I'll do that

Thank you let's wait for Travis CI.

This kdestroy runs as root and wipes root's own ccachs ... this is totally inappropriate. Use a file ccache that ends up in the private tmp, so that if the service is restarted the file is automatically removed. https://fedorahosted.org/freeipa/ticket/6673 Signed-off-by: Simo Sorce <simo@redhat.com>

martbab · 2017-02-22T11:59:30Z

I have also noticed that the ccache is not created there, strange. However I think it is better to explicitly specify file-based ccache anyway just to be one the safe side. Otherwise everything seems to work as expected, even ipa-restore to live server scenario.

martbab · 2017-02-22T14:51:13Z

Fixed upstream
master:
https://fedorahosted.org/freeipa/changeset/b8f304c66994ae82ea484a4e8bd057d4ccf1e6bd

martbab · 2017-02-22T14:53:52Z

@pvoborni should the fix go also into 4-4 branch? see https://fedorahosted.org/freeipa/ticket/6673#comment:3

tiran reviewed Feb 15, 2017

View reviewed changes

simo5 force-pushed the rootccache branch from 9bb27c8 to fc61943 Compare February 16, 2017 15:26

simo5 force-pushed the rootccache branch from fc61943 to f12cd62 Compare February 17, 2017 13:56

martbab reviewed Feb 17, 2017

View reviewed changes

simo5 force-pushed the rootccache branch from f12cd62 to 4cec750 Compare February 17, 2017 14:44

simo5 force-pushed the rootccache branch from 4cec750 to 7a82122 Compare February 17, 2017 14:46

martbab added ack Pull Request approved, can be merged pushed Pull Request has already been pushed labels Feb 22, 2017

martbab closed this Feb 22, 2017

Remove non-sensical kdestroy on https stop #468

Remove non-sensical kdestroy on https stop #468

simo5 commented Feb 15, 2017 •

edited by martbab

tiran Feb 15, 2017

martbab commented Feb 15, 2017

tiran commented Feb 15, 2017

martbab commented Feb 15, 2017

rcritten commented Feb 15, 2017

martbab commented Feb 15, 2017

martbab commented Feb 15, 2017

rcritten commented Feb 15, 2017

tiran commented Feb 15, 2017

pvoborni commented Feb 15, 2017

martbab commented Feb 15, 2017

MartinBasti commented Feb 15, 2017

simo5 commented Feb 16, 2017

MartinBasti commented Feb 16, 2017

simo5 commented Feb 16, 2017

MartinBasti commented Feb 16, 2017

simo5 commented Feb 16, 2017

martbab commented Feb 17, 2017

abbra commented Feb 17, 2017

tiran commented Feb 17, 2017

abbra commented Feb 17, 2017

tiran commented Feb 17, 2017

simo5 commented Feb 17, 2017

simo5 commented Feb 17, 2017

martbab Feb 17, 2017

simo5 Feb 17, 2017

martbab Feb 17, 2017

martbab commented Feb 22, 2017

martbab commented Feb 22, 2017

martbab commented Feb 22, 2017

Remove non-sensical kdestroy on https stop #468

Remove non-sensical kdestroy on https stop #468

Conversation

simo5 commented Feb 15, 2017 • edited by martbab

tiran Feb 15, 2017

Choose a reason for hiding this comment

martbab commented Feb 15, 2017

tiran commented Feb 15, 2017

martbab commented Feb 15, 2017

rcritten commented Feb 15, 2017

martbab commented Feb 15, 2017

martbab commented Feb 15, 2017

rcritten commented Feb 15, 2017

tiran commented Feb 15, 2017

pvoborni commented Feb 15, 2017

martbab commented Feb 15, 2017

MartinBasti commented Feb 15, 2017

simo5 commented Feb 16, 2017

MartinBasti commented Feb 16, 2017

simo5 commented Feb 16, 2017

MartinBasti commented Feb 16, 2017

simo5 commented Feb 16, 2017

martbab commented Feb 17, 2017

abbra commented Feb 17, 2017

tiran commented Feb 17, 2017

abbra commented Feb 17, 2017

tiran commented Feb 17, 2017

simo5 commented Feb 17, 2017

simo5 commented Feb 17, 2017

martbab Feb 17, 2017

Choose a reason for hiding this comment

simo5 Feb 17, 2017

Choose a reason for hiding this comment

martbab Feb 17, 2017

Choose a reason for hiding this comment

martbab commented Feb 22, 2017

martbab commented Feb 22, 2017

martbab commented Feb 22, 2017

simo5 commented Feb 15, 2017 •

edited by martbab