Hi,
I have been working on sssd being able to flush hosts from it's cache.
Here is an initial patch to add the options to the cli of sss_cache.
I have noticed that there are some methods already in src/db/sysdb_ssh.c like sysdb_update_ssh_known_host_expire: Does that seem like the right function to call to expire a host?
Advice is appreciated.
On Thu, May 01, 2014 at 01:35:49PM +0930, William Brown wrote:
Hi,
I have been working on sssd being able to flush hosts from it's cache.
Here is an initial patch to add the options to the cli of sss_cache.
I have noticed that there are some methods already in src/db/sysdb_ssh.c like sysdb_update_ssh_known_host_expire: Does that seem like the right function to call to expire a host?
Advice is appreciated.
-- William Brown william@firstyear.id.au
Hi William,
thanks a lot for the submission!
The way I read the SSH responder, calling sysdb_update_ssh_known_host_expire() would be the right thing to do, but I wonder if that would be enough or whether we'd need to call out to the ssh responder to force writing out a new hosts file.
I've put Honza who wrote the SSH responder originally to CC, I'm sure he'll know :-)
On 2.5.2014 13:57, Jakub Hrozek wrote:
On Thu, May 01, 2014 at 01:35:49PM +0930, William Brown wrote:
Hi,
I have been working on sssd being able to flush hosts from it's cache.
Here is an initial patch to add the options to the cli of sss_cache.
I have noticed that there are some methods already in src/db/sysdb_ssh.c like sysdb_update_ssh_known_host_expire: Does that seem like the right function to call to expire a host?
Advice is appreciated.
-- William Brown william@firstyear.id.au
Hi William,
thanks a lot for the submission!
The way I read the SSH responder, calling sysdb_update_ssh_known_host_expire() would be the right thing to do, but I wonder if that would be enough or whether we'd need to call out to the ssh responder to force writing out a new hosts file.
What you want to do is remove the entry from the cache, right? If so, then sysdb_update_ssh_known_host_expire() is not the right thing to call.
Why do you want to force writing out a new hosts file? The file should never be used without sss_ssh_knownhostsproxy, which already takes care of rewriting it.
I've put Honza who wrote the SSH responder originally to CC, I'm sure he'll know :-)
The way I read the SSH responder, calling sysdb_update_ssh_known_host_expire() would be the right thing to do, but I wonder if that would be enough or whether we'd need to call out to the ssh responder to force writing out a new hosts file.
What you want to do is remove the entry from the cache, right? If so, then sysdb_update_ssh_known_host_expire() is not the right thing to call.
What is the right thing to call? I would like to be able to expire either a single host or all hosts ...
Why do you want to force writing out a new hosts file? The file should never be used without sss_ssh_knownhostsproxy, which already takes care of rewriting it.
Okay, that's good to know. I'll assume it's re-written every time it's accessed then?
On 8.5.2014 02:53, William Brown wrote:
The way I read the SSH responder, calling sysdb_update_ssh_known_host_expire() would be the right thing to do, but I wonder if that would be enough or whether we'd need to call out to the ssh responder to force writing out a new hosts file.
What you want to do is remove the entry from the cache, right? If so, then sysdb_update_ssh_known_host_expire() is not the right thing to call.
What is the right thing to call? I would like to be able to expire either a single host or all hosts ...
I'm afraid there is no right thing to call ATM, as there is no support for cache invalidation in SSH host code. I guess you'll have to implement it yourself.
Off the top of my head, you need to properly set SYSDB_CACHE_EXPIRE in sysdb_store_ssh_host, return only unexpired hosts in sysdb_get_ssh_known_hosts, make sysdb_search_ssh_hosts public and create sysdb_set_ssh_host_attr (sysdb_update_ssh_hosts does the same, so you can modify and rename it instead of writing a completely new function).
Why do you want to force writing out a new hosts file? The file should never be used without sss_ssh_knownhostsproxy, which already takes care of rewriting it.
Okay, that's good to know. I'll assume it's re-written every time it's accessed then?
That's right.
I'm afraid there is no right thing to call ATM, as there is no support for cache invalidation in SSH host code. I guess you'll have to implement it yourself.
Off the top of my head, you need to properly set SYSDB_CACHE_EXPIRE in sysdb_store_ssh_host, return only unexpired hosts in sysdb_get_ssh_known_hosts, make sysdb_search_ssh_hosts public and create sysdb_set_ssh_host_attr (sysdb_update_ssh_hosts does the same, so you can modify and rename it instead of writing a completely new function).
Please find attached a patch that adds the functionality. Note that ssh_hosts don't use the dataExpire attribute, they use a different attribute, which is why I opted to call ssh_known_host_expire.
On 26.5.2014 03:41, William wrote:
I'm afraid there is no right thing to call ATM, as there is no support for cache invalidation in SSH host code. I guess you'll have to implement it yourself.
Off the top of my head, you need to properly set SYSDB_CACHE_EXPIRE in sysdb_store_ssh_host, return only unexpired hosts in sysdb_get_ssh_known_hosts, make sysdb_search_ssh_hosts public and create sysdb_set_ssh_host_attr (sysdb_update_ssh_hosts does the same, so you can modify and rename it instead of writing a completely new function).
Please find attached a patch that adds the functionality. Note that ssh_hosts don't use the dataExpire attribute, they use a different attribute, which is why I opted to call ssh_known_host_expire.
I'm sorry, but that's completely wrong. The ssh_known_host_expire attribute does *not* denote cache entry expiration, it denotes known_hosts entry expiration, hence my request to add support for SYSDB_CACHE_EXPIRE. Unless you do that, I can't ACK.
On Mon, 2014-05-26 at 09:19 +0200, Jan Cholasta wrote:
On 26.5.2014 03:41, William wrote:
I'm afraid there is no right thing to call ATM, as there is no support for cache invalidation in SSH host code. I guess you'll have to implement it yourself.
Off the top of my head, you need to properly set SYSDB_CACHE_EXPIRE in sysdb_store_ssh_host, return only unexpired hosts in sysdb_get_ssh_known_hosts, make sysdb_search_ssh_hosts public and create sysdb_set_ssh_host_attr (sysdb_update_ssh_hosts does the same, so you can modify and rename it instead of writing a completely new function).
Please find attached a patch that adds the functionality. Note that ssh_hosts don't use the dataExpire attribute, they use a different attribute, which is why I opted to call ssh_known_host_expire.
I'm sorry, but that's completely wrong. The ssh_known_host_expire attribute does *not* denote cache entry expiration, it denotes known_hosts entry expiration, hence my request to add support for SYSDB_CACHE_EXPIRE. Unless you do that, I can't ACK.
Here is the fixed patch as per your advice. I am now adding the SYSDB_CACHE_EXPIRE attribute to ssh_hosts on add, the update attr function is now exposed, and I added some helpers to keep inline with some of the other sysdb functions.
I noticed in my testing that when a host had been expired, it wasn't put into known_hosts (Which is correct), but that the sss_ssh_knownproxy or other process wasn't updating the actual sysdb entries with the new key. Where is the refresh of an expired host taking place, or rather, where should it be taking place? I suspect it is in the freeipa provider code somewhere ...
On Thu, May 29, 2014 at 10:30:26AM +0930, William wrote:
On Mon, 2014-05-26 at 09:19 +0200, Jan Cholasta wrote:
On 26.5.2014 03:41, William wrote:
I'm afraid there is no right thing to call ATM, as there is no support for cache invalidation in SSH host code. I guess you'll have to implement it yourself.
Off the top of my head, you need to properly set SYSDB_CACHE_EXPIRE in sysdb_store_ssh_host, return only unexpired hosts in sysdb_get_ssh_known_hosts, make sysdb_search_ssh_hosts public and create sysdb_set_ssh_host_attr (sysdb_update_ssh_hosts does the same, so you can modify and rename it instead of writing a completely new function).
Please find attached a patch that adds the functionality. Note that ssh_hosts don't use the dataExpire attribute, they use a different attribute, which is why I opted to call ssh_known_host_expire.
I'm sorry, but that's completely wrong. The ssh_known_host_expire attribute does *not* denote cache entry expiration, it denotes known_hosts entry expiration, hence my request to add support for SYSDB_CACHE_EXPIRE. Unless you do that, I can't ACK.
Here is the fixed patch as per your advice. I am now adding the SYSDB_CACHE_EXPIRE attribute to ssh_hosts on add, the update attr function is now exposed, and I added some helpers to keep inline with some of the other sysdb functions.
I noticed in my testing that when a host had been expired, it wasn't put into known_hosts (Which is correct), but that the sss_ssh_knownproxy or other process wasn't updating the actual sysdb entries with the new key. Where is the refresh of an expired host taking place, or rather, where should it be taking place? I suspect it is in the freeipa provider code somewhere ...
Honza, does this patch version look any better to you?
Hi,
I'm terribly sorry for the late reply.
On 18.6.2014 17:53, Jakub Hrozek wrote:
On Thu, May 29, 2014 at 10:30:26AM +0930, William wrote:
On Mon, 2014-05-26 at 09:19 +0200, Jan Cholasta wrote:
On 26.5.2014 03:41, William wrote:
I'm afraid there is no right thing to call ATM, as there is no support for cache invalidation in SSH host code. I guess you'll have to implement it yourself.
Off the top of my head, you need to properly set SYSDB_CACHE_EXPIRE in sysdb_store_ssh_host, return only unexpired hosts in sysdb_get_ssh_known_hosts, make sysdb_search_ssh_hosts public and create sysdb_set_ssh_host_attr (sysdb_update_ssh_hosts does the same, so you can modify and rename it instead of writing a completely new function).
Please find attached a patch that adds the functionality. Note that ssh_hosts don't use the dataExpire attribute, they use a different attribute, which is why I opted to call ssh_known_host_expire.
I'm sorry, but that's completely wrong. The ssh_known_host_expire attribute does *not* denote cache entry expiration, it denotes known_hosts entry expiration, hence my request to add support for SYSDB_CACHE_EXPIRE. Unless you do that, I can't ACK.
Here is the fixed patch as per your advice. I am now adding the SYSDB_CACHE_EXPIRE attribute to ssh_hosts on add, the update attr function is now exposed, and I added some helpers to keep inline with some of the other sysdb functions.
Could you add a sssd.conf option for the cache timeout?
I noticed in my testing that when a host had been expired, it wasn't put into known_hosts (Which is correct), but that the sss_ssh_knownproxy or other process wasn't updating the actual sysdb entries with the new key. Where is the refresh of an expired host taking place, or rather, where should it be taking place? I suspect it is in the freeipa provider code somewhere ...
That's right, it's in src/providers/ipa/ipa_hostid.c. All it does is call sysdb_store_ssh_host, which is where the refresh should take place.
Honza, does this patch version look any better to you?
Besides the above, there are some whitespace errors:
0001-Allow-sss_cache-to-expire-sshKnownHosts.patch:60: trailing whitespace. errno_t 0001-Allow-sss_cache-to-expire-sshKnownHosts.patch:121: trailing whitespace. int 0001-Allow-sss_cache-to-expire-sshKnownHosts.patch:220: trailing whitespace.
0001-Allow-sss_cache-to-expire-sshKnownHosts.patch:296: trailing whitespace.
warning: 4 lines add whitespace errors.
Honza
On Tue, 2014-06-24 at 10:21 +0200, Jan Cholasta wrote:
Hi,
I'm terribly sorry for the late reply.
That's okay, I understand how it can be.
Could you add a sssd.conf option for the cache timeout?
Done. The option exists, and I updated the man page (?), and the default option for this. However, I can't quite work out how to access confdb inside of ipa_hostid.c when it calls sysdb_store_ssh_host.
Besides the above, there are some whitespace errors:
0001-Allow-sss_cache-to-expire-sshKnownHosts.patch:60: trailing whitespace. errno_t 0001-Allow-sss_cache-to-expire-sshKnownHosts.patch:121: trailing whitespace. int 0001-Allow-sss_cache-to-expire-sshKnownHosts.patch:220: trailing whitespace.
0001-Allow-sss_cache-to-expire-sshKnownHosts.patch:296: trailing whitespace.
All 4 fixed, and I (hope) haven't added any more.
Thanks for your help.
On Sun, 2014-06-29 at 20:22 +0930, William wrote:
On Tue, 2014-06-24 at 10:21 +0200, Jan Cholasta wrote:
Hi,
I'm terribly sorry for the late reply.
That's okay, I understand how it can be.
Could you add a sssd.conf option for the cache timeout?
Done. The option exists, and I updated the man page (?), and the default option for this. However, I can't quite work out how to access confdb inside of ipa_hostid.c when it calls sysdb_store_ssh_host.
Besides the above, there are some whitespace errors:
0001-Allow-sss_cache-to-expire-sshKnownHosts.patch:60: trailing whitespace. errno_t 0001-Allow-sss_cache-to-expire-sshKnownHosts.patch:121: trailing whitespace. int 0001-Allow-sss_cache-to-expire-sshKnownHosts.patch:220: trailing whitespace.
0001-Allow-sss_cache-to-expire-sshKnownHosts.patch:296: trailing whitespace.
All 4 fixed, and I (hope) haven't added any more.
Thanks for your help.
Helps if I attach the patch.
On 29.6.2014 13:14, William wrote:
On Sun, 2014-06-29 at 20:22 +0930, William wrote:
On Tue, 2014-06-24 at 10:21 +0200, Jan Cholasta wrote:
Hi,
I'm terribly sorry for the late reply.
That's okay, I understand how it can be.
Could you add a sssd.conf option for the cache timeout?
Done. The option exists, and I updated the man page (?), and the default option for this.
Thanks. Could you please rename the option to "entry_cache_ssh_host_timeout", so that it's consistent with the rest of the cache timeout options?
However, I can't quite work out how to access confdb
inside of ipa_hostid.c when it calls sysdb_store_ssh_host.
I guess you can store the value in sss_domain_info, like the rest of the cache timeouts. See confdb_get_domain_internal.
Besides the above, there are some whitespace errors:
0001-Allow-sss_cache-to-expire-sshKnownHosts.patch:60: trailing whitespace. errno_t 0001-Allow-sss_cache-to-expire-sshKnownHosts.patch:121: trailing whitespace. int 0001-Allow-sss_cache-to-expire-sshKnownHosts.patch:220: trailing whitespace.
0001-Allow-sss_cache-to-expire-sshKnownHosts.patch:296: trailing whitespace.
All 4 fixed, and I (hope) haven't added any more.
Thanks for your help.
Helps if I attach the patch.
It certainly does :)
Why are there changes to DBus code in the patch?
On Mon, 2014-07-07 at 13:41 +0200, Jan Cholasta wrote:
On 29.6.2014 13:14, William wrote:
On Sun, 2014-06-29 at 20:22 +0930, William wrote:
On Tue, 2014-06-24 at 10:21 +0200, Jan Cholasta wrote:
Hi,
I'm terribly sorry for the late reply.
That's okay, I understand how it can be.
Could you add a sssd.conf option for the cache timeout?
Done. The option exists, and I updated the man page (?), and the default option for this.
Thanks. Could you please rename the option to "entry_cache_ssh_host_timeout", so that it's consistent with the rest of the cache timeout options?
However, I can't quite work out how to access confdb
inside of ipa_hostid.c when it calls sysdb_store_ssh_host.
I guess you can store the value in sss_domain_info, like the rest of the cache timeouts. See confdb_get_domain_internal.
Besides the above, there are some whitespace errors:
0001-Allow-sss_cache-to-expire-sshKnownHosts.patch:60: trailing whitespace. errno_t 0001-Allow-sss_cache-to-expire-sshKnownHosts.patch:121: trailing whitespace. int 0001-Allow-sss_cache-to-expire-sshKnownHosts.patch:220: trailing whitespace.
0001-Allow-sss_cache-to-expire-sshKnownHosts.patch:296: trailing whitespace.
All 4 fixed, and I (hope) haven't added any more.
Thanks for your help.
Helps if I attach the patch.
It certainly does :)
Why are there changes to DBus code in the patch?
I guess it's because of https://fedorahosted.org/sssd/ticket/2344
One must be careful what he adds to the commit before the ticket is fixed.
On (07/07/14 13:41), Jan Cholasta wrote:
On 29.6.2014 13:14, William wrote:
On Sun, 2014-06-29 at 20:22 +0930, William wrote:
On Tue, 2014-06-24 at 10:21 +0200, Jan Cholasta wrote:
Hi,
I'm terribly sorry for the late reply.
That's okay, I understand how it can be.
Could you add a sssd.conf option for the cache timeout?
Done. The option exists, and I updated the man page (?), and the default option for this.
Thanks. Could you please rename the option to "entry_cache_ssh_host_timeout", so that it's consistent with the rest of the cache timeout options?
However, I can't quite work out how to access confdb
inside of ipa_hostid.c when it calls sysdb_store_ssh_host.
I guess you can store the value in sss_domain_info, like the rest of the cache timeouts. See confdb_get_domain_internal.
Besides the above, there are some whitespace errors:
0001-Allow-sss_cache-to-expire-sshKnownHosts.patch:60: trailing whitespace. errno_t 0001-Allow-sss_cache-to-expire-sshKnownHosts.patch:121: trailing whitespace. int 0001-Allow-sss_cache-to-expire-sshKnownHosts.patch:220: trailing whitespace.
0001-Allow-sss_cache-to-expire-sshKnownHosts.patch:296: trailing whitespace.
All 4 fixed, and I (hope) haven't added any more.
Thanks for your help.
Helps if I attach the patch.
It certainly does :)
Why are there changes to DBus code in the patch?
It is well known bug in codegen script https://fedorahosted.org/sssd/ticket/2344
Just do not include this file into new version of patch :-)
LS
Thanks. Could you please rename the option to "entry_cache_ssh_host_timeout", so that it's consistent with the rest of the cache timeout options?
However, I can't quite work out how to access confdb
inside of ipa_hostid.c when it calls sysdb_store_ssh_host.
I guess you can store the value in sss_domain_info, like the rest of the cache timeouts. See confdb_get_domain_internal.
Helps if I attach the patch.
It certainly does :)
Again, I have taken your advice and implemented these changes. I don't see any dbus related changes in my patch, so I hope that this is too your requirements.
Any comments and advice welcome.
On 11.7.2014 03:35, William wrote:
Thanks. Could you please rename the option to "entry_cache_ssh_host_timeout", so that it's consistent with the rest of the cache timeout options?
However, I can't quite work out how to access confdb
inside of ipa_hostid.c when it calls sysdb_store_ssh_host.
I guess you can store the value in sss_domain_info, like the rest of the cache timeouts. See confdb_get_domain_internal.
Helps if I attach the patch.
It certainly does :)
Again, I have taken your advice and implemented these changes. I don't see any dbus related changes in my patch, so I hope that this is too your requirements.
I do see some dbus changes: src/responder/ifp/ifp_iface_generated.c
As Pavel and Lukáš pointed out earlier, these changes should not be included in the patch, as they are a result of a bug in dbus codegen script.
Any comments and advice welcome.
The confdb argument in sysdb_store_ssh_host is not needed anymore.
On Tue, Jul 15, 2014 at 03:57:14PM +0200, Jan Cholasta wrote:
I do see some dbus changes: src/responder/ifp/ifp_iface_generated.c
As Pavel and Lukáš pointed out earlier, these changes should not be included in the patch, as they are a result of a bug in dbus codegen script.
btw to solve this, you can just do the following in the branch where you develop the patch: git reset --mixed HEAD~1 git checkout src/responder/ifp/ifp_iface_generated.c git commit -a
On Tue, 2014-07-15 at 15:57 +0200, Jan Cholasta wrote:
On 11.7.2014 03:35, William wrote:
Thanks. Could you please rename the option to "entry_cache_ssh_host_timeout", so that it's consistent with the rest of the cache timeout options?
However, I can't quite work out how to access confdb
inside of ipa_hostid.c when it calls sysdb_store_ssh_host.
I guess you can store the value in sss_domain_info, like the rest of the cache timeouts. See confdb_get_domain_internal.
Helps if I attach the patch.
It certainly does :)
Again, I have taken your advice and implemented these changes. I don't see any dbus related changes in my patch, so I hope that this is too your requirements.
I do see some dbus changes: src/responder/ifp/ifp_iface_generated.c
As Pavel and Lukáš pointed out earlier, these changes should not be included in the patch, as they are a result of a bug in dbus codegen script.
Any comments and advice welcome.
The confdb argument in sysdb_store_ssh_host is not needed anymore.
Both issues have been fixed.
Here is the patch once more.
On Tue, 2014-07-15 at 15:57 +0200, Jan Cholasta wrote:
On 11.7.2014 03:35, William wrote:
Thanks. Could you please rename the option to "entry_cache_ssh_host_timeout", so that it's consistent with the rest of the cache timeout options?
However, I can't quite work out how to access confdb
inside of ipa_hostid.c when it calls sysdb_store_ssh_host.
I guess you can store the value in sss_domain_info, like the rest of the cache timeouts. See confdb_get_domain_internal.
Helps if I attach the patch.
It certainly does :)
Again, I have taken your advice and implemented these changes. I don't see any dbus related changes in my patch, so I hope that this is too your requirements.
I do see some dbus changes: src/responder/ifp/ifp_iface_generated.c
As Pavel and Lukáš pointed out earlier, these changes should not be included in the patch, as they are a result of a bug in dbus codegen script.
Any comments and advice welcome.
The confdb argument in sysdb_store_ssh_host is not needed anymore.
Ignore that last patch, I messed up and didn't include a .h file. Here is the fixed patch.
I also rebased onto master, and I get:
/bin/sh ./libtool --tag=CC --mode=link gcc -Wall -Wshadow -Wstrict-prototypes -Wpointer-arith -Wcast-qual -Wcast-align -Wwrite-strings -Wundef -Werror-implicit-function-declaration -Winit-self -fno-strict-aliasing -std=gnu99 -g -O2 -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -D_LARGEFILE64_SOURCE -module -avoid-version -Wl,--version-script,./src/sss_client/autofs/sss_autofs.exports -o libsss_autofs.la -rpath /srv/sssd/lib/sssd/modules src/sss_client/common.lo src/sss_client/autofs/sss_autofs.lo -lpthread -ldl libtool: link: gcc -shared -fPIC -DPIC src/sss_client/.libs/common.o src/sss_client/autofs/.libs/sss_autofs.o -lpthread -ldl -O2 -Wl,--version-script -Wl,./src/sss_client/autofs/sss_autofs.exports -Wl,-soname -Wl,libsss_autofs.so -o .libs/libsss_autofs.so gcc: error: src/sss_client/.libs/common.o: No such file or directory make[2]: *** [libsss_autofs.la] Error 1 make[2]: Leaving directory `/home/a1176360/development/sssd' make[1]: *** [all-recursive] Error 1 make[1]: Leaving directory `/home/a1176360/development/sssd' make: *** [all] Error 2
Doesn't appear to be related to anything I have changed I don't think ...
On (16/07/14 11:46), William wrote:
On Tue, 2014-07-15 at 15:57 +0200, Jan Cholasta wrote:
On 11.7.2014 03:35, William wrote:
Thanks. Could you please rename the option to "entry_cache_ssh_host_timeout", so that it's consistent with the rest of the cache timeout options?
However, I can't quite work out how to access confdb
inside of ipa_hostid.c when it calls sysdb_store_ssh_host.
I guess you can store the value in sss_domain_info, like the rest of the cache timeouts. See confdb_get_domain_internal.
Helps if I attach the patch.
It certainly does :)
Again, I have taken your advice and implemented these changes. I don't see any dbus related changes in my patch, so I hope that this is too your requirements.
I do see some dbus changes: src/responder/ifp/ifp_iface_generated.c
As Pavel and Lukáš pointed out earlier, these changes should not be included in the patch, as they are a result of a bug in dbus codegen script.
Any comments and advice welcome.
The confdb argument in sysdb_store_ssh_host is not needed anymore.
Ignore that last patch, I messed up and didn't include a .h file. Here is the fixed patch.
I also rebased onto master, and I get:
/bin/sh ./libtool --tag=CC --mode=link gcc -Wall -Wshadow -Wstrict-prototypes -Wpointer-arith -Wcast-qual -Wcast-align -Wwrite-strings -Wundef -Werror-implicit-function-declaration -Winit-self -fno-strict-aliasing -std=gnu99 -g -O2 -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE -D_LARGEFILE64_SOURCE -module -avoid-version -Wl,--version-script,./src/sss_client/autofs/sss_autofs.exports -o libsss_autofs.la -rpath /srv/sssd/lib/sssd/modules src/sss_client/common.lo src/sss_client/autofs/sss_autofs.lo -lpthread -ldl libtool: link: gcc -shared -fPIC -DPIC src/sss_client/.libs/common.o src/sss_client/autofs/.libs/sss_autofs.o -lpthread -ldl -O2 -Wl,--version-script -Wl,./src/sss_client/autofs/sss_autofs.exports -Wl,-soname -Wl,libsss_autofs.so -o .libs/libsss_autofs.so gcc: error: src/sss_client/.libs/common.o: No such file or directory make[2]: *** [libsss_autofs.la] Error 1 make[2]: Leaving directory `/home/a1176360/development/sssd' make[1]: *** [all-recursive] Error 1 make[1]: Leaving directory `/home/a1176360/development/sssd' make: *** [all] Error 2
Doesn't appear to be related to anything I have changed I don't think ...
You forgot to change usage of sysdb_store_ssh_host in sysdb_ssh-tests. tests cannot be compiled. (make check)
CC src/tests/sysdb_ssh_tests-sysdb_ssh-tests.o src/tests/sysdb_ssh-tests.c:179:43: error: too few arguments to function call, expected 6, have 5 data->attrs); ^ src/db/sysdb_ssh.h:32:1: note: 'sysdb_store_ssh_host' declared here errno_t ^ 1 error generated.
From 29cdcbd9a20cfbd72c5a8d103f58e3f153887d73 Mon Sep 17 00:00:00 2001 From: William B william@adelaide.edu.au Date: Wed, 16 Jul 2014 11:45:02 +0930 Subject: [PATCH] Allow sss_cache tool to flush known hosts cache
src/confdb/confdb.h | 2 ++ src/config/etc/sssd.api.conf | 1 + src/db/sysdb_ssh.c | 58 +++++++++++++++++++++++++++++++++++++++--- src/db/sysdb_ssh.h | 17 ++++++++++++- src/man/po/sssd-docs.pot | 17 +++++++++++++ src/providers/ipa/ipa_hostid.c | 2 +- src/tools/sss_cache.c | 54 +++++++++++++++++++++++++++++++++++---- 7 files changed, 140 insertions(+), 11 deletions(-)
//snip
diff --git a/src/man/po/sssd-docs.pot b/src/man/po/sssd-docs.pot index df0456d..a4fce38 100644 --- a/src/man/po/sssd-docs.pot +++ b/src/man/po/sssd-docs.pot @@ -1140,6 +1140,23 @@ msgstr "" msgid "Default: 180" msgstr ""
+#. type: Content of: <reference><refentry><refsect1><refsect2><variablelist><varlistentry><term> +#: sssd.conf.5.xml:878 +msgid "ssh_known_hosts_expire (integer)" +msgstr ""
+#. type: Content of: <reference><refentry><refsect1><refsect2><variablelist><varlistentry><listitem><para> +#: sssd.conf.5.xml:881 +msgid "" +"How many seconds to keep a host ssh key after refresh. IE how long to cache " +"the host key for." +msgstr ""
+#. type: Content of: <reference><refentry><refsect1><refsect2><variablelist><varlistentry><listitem><para> +#: sssd.conf.5.xml:885 +msgid "Default: 31536000 (1 Year)" +msgstr ""
#. type: Content of: <reference><refentry><refsect1><refsect2><title> #: sssd.conf.5.xml:893 msgid "PAC responder configuration options"
We do not update pot files directly. Could you edit xml file src/man/sssd.conf.5.xml ?
LS
Doesn't appear to be related to anything I have changed I don't think ...
You forgot to change usage of sysdb_store_ssh_host in sysdb_ssh-tests. tests cannot be compiled. (make check)
CC src/tests/sysdb_ssh_tests-sysdb_ssh-tests.o src/tests/sysdb_ssh-tests.c:179:43: error: too few arguments to function call, expected 6, have 5 data->attrs); ^ src/db/sysdb_ssh.h:32:1: note: 'sysdb_store_ssh_host' declared here errno_t ^ 1 error generated.
Did I? I'll check that, and fix it if that's the case.
From 29cdcbd9a20cfbd72c5a8d103f58e3f153887d73 Mon Sep 17 00:00:00 2001 From: William B william@adelaide.edu.au Date: Wed, 16 Jul 2014 11:45:02 +0930 Subject: [PATCH] Allow sss_cache tool to flush known hosts cache
src/confdb/confdb.h | 2 ++ src/config/etc/sssd.api.conf | 1 + src/db/sysdb_ssh.c | 58 +++++++++++++++++++++++++++++++++++++++--- src/db/sysdb_ssh.h | 17 ++++++++++++- src/man/po/sssd-docs.pot | 17 +++++++++++++ src/providers/ipa/ipa_hostid.c | 2 +- src/tools/sss_cache.c | 54 +++++++++++++++++++++++++++++++++++---- 7 files changed, 140 insertions(+), 11 deletions(-)
//snip
diff --git a/src/man/po/sssd-docs.pot b/src/man/po/sssd-docs.pot index df0456d..a4fce38 100644 --- a/src/man/po/sssd-docs.pot +++ b/src/man/po/sssd-docs.pot @@ -1140,6 +1140,23 @@ msgstr "" msgid "Default: 180" msgstr ""
+#. type: Content of: <reference><refentry><refsect1><refsect2><variablelist><varlistentry><term> +#: sssd.conf.5.xml:878 +msgid "ssh_known_hosts_expire (integer)" +msgstr ""
+#. type: Content of: <reference><refentry><refsect1><refsect2><variablelist><varlistentry><listitem><para> +#: sssd.conf.5.xml:881 +msgid "" +"How many seconds to keep a host ssh key after refresh. IE how long to cache " +"the host key for." +msgstr ""
+#. type: Content of: <reference><refentry><refsect1><refsect2><variablelist><varlistentry><listitem><para> +#: sssd.conf.5.xml:885 +msgid "Default: 31536000 (1 Year)" +msgstr ""
#. type: Content of: <reference><refentry><refsect1><refsect2><title> #: sssd.conf.5.xml:893 msgid "PAC responder configuration options"
We do not update pot files directly. Could you edit xml file src/man/sssd.conf.5.xml ?
Okay, I'll revert the pot changes, and update the xml. Thanks for that.
On 16.7.2014 04:16, William wrote:
On Tue, 2014-07-15 at 15:57 +0200, Jan Cholasta wrote:
On 11.7.2014 03:35, William wrote:
Thanks. Could you please rename the option to "entry_cache_ssh_host_timeout", so that it's consistent with the rest of the cache timeout options?
However, I can't quite work out how to access confdb
inside of ipa_hostid.c when it calls sysdb_store_ssh_host.
I guess you can store the value in sss_domain_info, like the rest of the cache timeouts. See confdb_get_domain_internal.
Helps if I attach the patch.
It certainly does :)
Again, I have taken your advice and implemented these changes. I don't see any dbus related changes in my patch, so I hope that this is too your requirements.
I do see some dbus changes: src/responder/ifp/ifp_iface_generated.c
As Pavel and Lukáš pointed out earlier, these changes should not be included in the patch, as they are a result of a bug in dbus codegen script.
Any comments and advice welcome.
The confdb argument in sysdb_store_ssh_host is not needed anymore.
Ignore that last patch, I messed up and didn't include a .h file. Here is the fixed patch.
This new patch seems to be based on some older version of the patch, it uses wrong option name, sysdb_update_ssh_host still has the confdb argument, etc.
Ignore that last patch, I messed up and didn't include a .h file. Here is the fixed patch.
This new patch seems to be based on some older version of the patch, it uses wrong option name, sysdb_update_ssh_host still has the confdb argument, etc.
Fixed, and fixed the documentation into the xml file, removed the references to the .pot file.
On 18.7.2014 06:41, William wrote:
Ignore that last patch, I messed up and didn't include a .h file. Here is the fixed patch.
This new patch seems to be based on some older version of the patch, it uses wrong option name, sysdb_update_ssh_host still has the confdb argument, etc.
Fixed, and fixed the documentation into the xml file, removed the references to the .pot file.
I don't see any of the requested fixes in the patch.
See attachment for fixed patch. I also did a few additional tweaks to make the patch work when SSSD is updated from a previous version and when building without SSH bits.
On Mon, Jul 21, 2014 at 01:55:20PM +0200, Jan Cholasta wrote:
On 18.7.2014 06:41, William wrote:
Ignore that last patch, I messed up and didn't include a .h file. Here is the fixed patch.
This new patch seems to be based on some older version of the patch, it uses wrong option name, sysdb_update_ssh_host still has the confdb argument, etc.
Fixed, and fixed the documentation into the xml file, removed the references to the .pot file.
I don't see any of the requested fixes in the patch.
See attachment for fixed patch. I also did a few additional tweaks to make the patch work when SSSD is updated from a previous version and when building without SSH bits.
Awesome thank you, I guess some of us need to review the patch now you're also an author. And thanks for keeping the attribution to William, but if you like you can also split the patch into two so you have credit for your work.
William, can you help us test the patch?
On (21/07/14 14:08), Jakub Hrozek wrote:
On Mon, Jul 21, 2014 at 01:55:20PM +0200, Jan Cholasta wrote:
On 18.7.2014 06:41, William wrote:
Ignore that last patch, I messed up and didn't include a .h file. Here is the fixed patch.
This new patch seems to be based on some older version of the patch, it uses wrong option name, sysdb_update_ssh_host still has the confdb argument, etc.
Fixed, and fixed the documentation into the xml file, removed the references to the .pot file.
I don't see any of the requested fixes in the patch.
See attachment for fixed patch. I also did a few additional tweaks to make the patch work when SSSD is updated from a previous version and when building without SSH bits.
Awesome thank you, I guess some of us need to review the patch now you're also an author. And thanks for keeping the attribution to William, but if you like you can also split the patch into two so you have credit for your work.
William, can you help us test the patch?
Or he can use sign-off if William agrees. https://www.kernel.org/doc/Documentation/SubmittingPatches Section 12
LS
On 21 Jul 2014, at 21:42, Lukas Slebodnik lslebodn@redhat.com wrote:
On (21/07/14 14:08), Jakub Hrozek wrote:
On Mon, Jul 21, 2014 at 01:55:20PM +0200, Jan Cholasta wrote:
On 18.7.2014 06:41, William wrote:
Ignore that last patch, I messed up and didn't include a .h file. Here is the fixed patch.
This new patch seems to be based on some older version of the patch, it uses wrong option name, sysdb_update_ssh_host still has the confdb argument, etc.
Fixed, and fixed the documentation into the xml file, removed the references to the .pot file.
I don't see any of the requested fixes in the patch.
See attachment for fixed patch. I also did a few additional tweaks to make the patch work when SSSD is updated from a previous version and when building without SSH bits.
Sorry about this: seems towards the end i messed up my gif repo as i rebased on the wrong branch, which led to this issue. Thanks for your patience however.
Awesome thank you, I guess some of us need to review the patch now you're also an author. And thanks for keeping the attribution to William, but if you like you can also split the patch into two so you have credit for your work.
William, can you help us test the patch?
Sure, happy to help.
Or he can use sign-off if William agrees. https://www.kernel.org/doc/Documentation/SubmittingPatches Section 12
Sorry, explain for me? Im relatively new to this process.
LS _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
On Mon, Jul 21, 2014 at 11:35:31PM +0930, William wrote:
On 21 Jul 2014, at 21:42, Lukas Slebodnik lslebodn@redhat.com wrote:
On (21/07/14 14:08), Jakub Hrozek wrote:
On Mon, Jul 21, 2014 at 01:55:20PM +0200, Jan Cholasta wrote:
On 18.7.2014 06:41, William wrote:
> Ignore that last patch, I messed up and didn't include a .h file. Here > is the fixed patch.
This new patch seems to be based on some older version of the patch, it uses wrong option name, sysdb_update_ssh_host still has the confdb argument, etc.
Fixed, and fixed the documentation into the xml file, removed the references to the .pot file.
I don't see any of the requested fixes in the patch.
See attachment for fixed patch. I also did a few additional tweaks to make the patch work when SSSD is updated from a previous version and when building without SSH bits.
Sorry about this: seems towards the end i messed up my gif repo as i rebased on the wrong branch, which led to this issue. Thanks for your patience however.
Awesome thank you, I guess some of us need to review the patch now you're also an author. And thanks for keeping the attribution to William, but if you like you can also split the patch into two so you have credit for your work.
William, can you help us test the patch?
Sure, happy to help.
Or he can use sign-off if William agrees. https://www.kernel.org/doc/Documentation/SubmittingPatches Section 12
Sorry, explain for me? Im relatively new to this process.
The patch would contain two lines in the metadata part, one that would list you as the author and another that would list Jan Cholasta as a co-author, so to say. The lines would look like:
From: William B william@adelaide.edu.au Signed-off-by: Jan Cholasta jcholast@redhat.com
It's mostly for tracking who should get his share of glory (or blame :-)) for the patch.
On Mon, 2014-07-21 at 17:15 +0200, Jakub Hrozek wrote:
On Mon, Jul 21, 2014 at 11:35:31PM +0930, William wrote:
On 21 Jul 2014, at 21:42, Lukas Slebodnik lslebodn@redhat.com wrote:
On (21/07/14 14:08), Jakub Hrozek wrote:
On Mon, Jul 21, 2014 at 01:55:20PM +0200, Jan Cholasta wrote:
On 18.7.2014 06:41, William wrote:
>> Ignore that last patch, I messed up and didn't include a .h file. Here >> is the fixed patch. > > This new patch seems to be based on some older version of the patch, it > uses wrong option name, sysdb_update_ssh_host still has the confdb > argument, etc.
Fixed, and fixed the documentation into the xml file, removed the references to the .pot file.
I don't see any of the requested fixes in the patch.
See attachment for fixed patch. I also did a few additional tweaks to make the patch work when SSSD is updated from a previous version and when building without SSH bits.
Sorry about this: seems towards the end i messed up my gif repo as i rebased on the wrong branch, which led to this issue. Thanks for your patience however.
Awesome thank you, I guess some of us need to review the patch now you're also an author. And thanks for keeping the attribution to William, but if you like you can also split the patch into two so you have credit for your work.
William, can you help us test the patch?
Sure, happy to help.
Or he can use sign-off if William agrees. https://www.kernel.org/doc/Documentation/SubmittingPatches Section 12
Sorry, explain for me? Im relatively new to this process.
The patch would contain two lines in the metadata part, one that would list you as the author and another that would list Jan Cholasta as a co-author, so to say. The lines would look like:
From: William B william@adelaide.edu.au Signed-off-by: Jan Cholasta jcholast@redhat.com
It's mostly for tracking who should get his share of glory (or blame :-)) for the patch.
Mostly blame I would think ;)
I'm happy for this to be the case, so if you want to sign-off and commit, you are welcome to. I do appreciate your patience in working through this with me.
William, can you help us test the patch?
I have tested this patch version now, and can confirm that it works as expected.
On 07/21/2014 02:08 PM, Jakub Hrozek wrote:
On Mon, Jul 21, 2014 at 01:55:20PM +0200, Jan Cholasta wrote:
On 18.7.2014 06:41, William wrote:
Ignore that last patch, I messed up and didn't include a .h file. Here is the fixed patch.
This new patch seems to be based on some older version of the patch, it uses wrong option name, sysdb_update_ssh_host still has the confdb argument, etc.
Fixed, and fixed the documentation into the xml file, removed the references to the .pot file.
I don't see any of the requested fixes in the patch.
See attachment for fixed patch. I also did a few additional tweaks to make the patch work when SSSD is updated from a previous version and when building without SSH bits.
Awesome thank you, I guess some of us need to review the patch now you're also an author.
While Jan was so kind to give me some hints on how to test patch properly, we found out a little bug. So Jan is going to prepare an updated patch.
I'll do the review then.
And thanks for keeping the attribution to William, but if you like you can also split the patch into two so you have credit for your work.
William, can you help us test the patch? _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Dne 4.8.2014 v 19:34 Pavel Reichl napsal(a):
On 07/21/2014 02:08 PM, Jakub Hrozek wrote:
On Mon, Jul 21, 2014 at 01:55:20PM +0200, Jan Cholasta wrote:
On 18.7.2014 06:41, William wrote:
Ignore that last patch, I messed up and didn't include a .h file. Here is the fixed patch.
This new patch seems to be based on some older version of the patch, it uses wrong option name, sysdb_update_ssh_host still has the confdb argument, etc.
Fixed, and fixed the documentation into the xml file, removed the references to the .pot file.
I don't see any of the requested fixes in the patch.
See attachment for fixed patch. I also did a few additional tweaks to make the patch work when SSSD is updated from a previous version and when building without SSH bits.
Awesome thank you, I guess some of us need to review the patch now you're also an author.
While Jan was so kind to give me some hints on how to test patch properly, we found out a little bug. So Jan is going to prepare an updated patch.
I'll do the review then.
I can't reproduce the issue, maybe something else was wrong in your setup?
For the record, I installed IPA in a VM and then ran these commands on it:
$ sss_ssh_knownhostsproxy --debug 10 $HOSTNAME [no errors] $ cat /var/lib/sss/pubconf/known_hosts [not empty] # systemctl stop ipa $ sss_ssh_knownhostsproxy --debug 10 $HOSTNAME [no errors] $ cat /var/lib/sss/pubconf/known_hosts [not empty] # sss_cache -H $ sss_ssh_knownhostsproxy --debug 10 $HOSTNAME [no errors] $ cat /var/lib/sss/pubconf/known_hosts [empty] # systemctl start ipa $ sss_ssh_knownhostsproxy --debug 10 $HOSTNAME [no errors] $ cat /var/lib/sss/pubconf/known_hosts [not empty]
Pavel had an empty /var/lib/sss/pubconf/known_hosts in the last step, I have it non-empty as expected.
(sorry for the delay, I was on leave)
On 08/25/2014 02:05 PM, Jan Cholasta wrote:
Dne 4.8.2014 v 19:34 Pavel Reichl napsal(a):
On 07/21/2014 02:08 PM, Jakub Hrozek wrote:
On Mon, Jul 21, 2014 at 01:55:20PM +0200, Jan Cholasta wrote:
On 18.7.2014 06:41, William wrote:
> Ignore that last patch, I messed up and didn't include a .h file. > Here > is the fixed patch. This new patch seems to be based on some older version of the patch, it uses wrong option name, sysdb_update_ssh_host still has the confdb argument, etc.
Fixed, and fixed the documentation into the xml file, removed the references to the .pot file.
I don't see any of the requested fixes in the patch.
See attachment for fixed patch. I also did a few additional tweaks to make the patch work when SSSD is updated from a previous version and when building without SSH bits.
Awesome thank you, I guess some of us need to review the patch now you're also an author.
While Jan was so kind to give me some hints on how to test patch properly, we found out a little bug. So Jan is going to prepare an updated patch.
I'll do the review then.
I can't reproduce the issue, maybe something else was wrong in your setup?
For the record, I installed IPA in a VM and then ran these commands on it:
$ sss_ssh_knownhostsproxy --debug 10 $HOSTNAME [no errors] $ cat /var/lib/sss/pubconf/known_hosts [not empty] # systemctl stop ipa $ sss_ssh_knownhostsproxy --debug 10 $HOSTNAME [no errors] $ cat /var/lib/sss/pubconf/known_hosts [not empty] # sss_cache -H $ sss_ssh_knownhostsproxy --debug 10 $HOSTNAME [no errors] $ cat /var/lib/sss/pubconf/known_hosts [empty] # systemctl start ipa $ sss_ssh_knownhostsproxy --debug 10 $HOSTNAME [no errors] $ cat /var/lib/sss/pubconf/known_hosts [not empty]
Pavel had an empty /var/lib/sss/pubconf/known_hosts in the last step, I have it non-empty as expected.
(sorry for the delay, I was on leave)
This test ran OK this time, so I think you are right.
I have some comments to the patch itself.
I'm not personally fan of big patches like this one. Could you at least separate the man page changes to separate patch - so we can ask some native speaker to review it.
Also new options in sss_cache -h and -H are not documented in man pages, could you document them?
Please see some minor comments inline:
From 31312ab877da0b7fe8da9ddd808345e3096dbcd0 Mon Sep 17 00:00:00 2001 From: William B william@adelaide.edu.au Date: Mon, 21 Jul 2014 11:13:25 +0200 Subject: [PATCH] Allow sss_cache tool to flush SSH hosts cache
src/confdb/confdb.c | 11 ++++++ src/confdb/confdb.h | 2 + src/config/SSSDConfigTest.py | 2 + src/config/etc/sssd.api.conf | 1 + src/db/sysdb_ssh.c | 65 +++++++++++++++++++++++++++---- src/db/sysdb_ssh.h | 15 ++++++++ src/man/sssd.conf.5.xml | 14 +++++++ src/providers/ipa/ipa_hostid.c | 4 +- src/tests/sysdb_ssh-tests.c | 1 + src/tools/sss_cache.c | 86 +++++++++++++++++++++++++++++++++++++----- 10 files changed, 183 insertions(+), 18 deletions(-)
diff --git a/src/confdb/confdb.c b/src/confdb/confdb.c index 15de961..e2d7e8d 100644 --- a/src/confdb/confdb.c +++ b/src/confdb/confdb.c @@ -1040,6 +1040,17 @@ static int confdb_get_domain_internal(struct confdb_ctx *cdb, goto done; }
- /* Override the ssh known hosts timeout, if specified */
- ret = get_entry_as_uint32(res->msgs[0], &domain->ssh_host_timeout,
- CONFDB_DOMAIN_SSH_HOST_CACHE_TIMEOUT,
entry_cache_timeout);
- if (ret != EOK) {
DEBUG(SSSDBG_FATAL_FAILURE,
"Invalid value for [%s]\n",
CONFDB_DOMAIN_SSH_HOST_CACHE_TIMEOUT);
goto done;
- }
- /* Set refresh_expired_interval, if specified */ ret = get_entry_as_uint32(res->msgs[0],
&domain->refresh_expired_interval, CONFDB_DOMAIN_REFRESH_EXPIRED_INTERVAL, diff --git a/src/confdb/confdb.h b/src/confdb/confdb.h index ba33ea5..e6c5c2d 100644 --- a/src/confdb/confdb.h +++ b/src/confdb/confdb.h @@ -175,6 +175,7 @@ #define CONFDB_DOMAIN_SERVICE_CACHE_TIMEOUT "entry_cache_service_timeout" #define CONFDB_DOMAIN_AUTOFS_CACHE_TIMEOUT "entry_cache_autofs_timeout" #define CONFDB_DOMAIN_SUDO_CACHE_TIMEOUT "entry_cache_sudo_timeout" +#define CONFDB_DOMAIN_SSH_HOST_CACHE_TIMEOUT "entry_cache_ssh_host_timeout" #define CONFDB_DOMAIN_PWD_EXPIRATION_WARNING "pwd_expiration_warning" #define CONFDB_DOMAIN_REFRESH_EXPIRED_INTERVAL "refresh_expired_interval" #define CONFDB_DOMAIN_OFFLINE_TIMEOUT "offline_timeout" @@ -232,6 +233,7 @@ struct sss_domain_info { uint32_t service_timeout; uint32_t autofsmap_timeout; uint32_t sudo_timeout;
uint32_t ssh_host_timeout;
uint32_t refresh_expired_interval; uint32_t subdomain_refresh_interval;
diff --git a/src/config/SSSDConfigTest.py b/src/config/SSSDConfigTest.py index 1450003..2d12bc0 100755 --- a/src/config/SSSDConfigTest.py +++ b/src/config/SSSDConfigTest.py @@ -505,6 +505,7 @@ class SSSDConfigTestSSSDDomain(unittest.TestCase): 'entry_cache_service_timeout', 'entry_cache_autofs_timeout', 'entry_cache_sudo_timeout',
'entry_cache_ssh_host_timeout', 'refresh_expired_interval', 'lookup_family_order', 'account_cache_expiration',
@@ -863,6 +864,7 @@ class SSSDConfigTestSSSDDomain(unittest.TestCase): 'entry_cache_service_timeout', 'entry_cache_autofs_timeout', 'entry_cache_sudo_timeout',
'entry_cache_ssh_host_timeout', 'refresh_expired_interval', 'account_cache_expiration', 'lookup_family_order',
diff --git a/src/config/etc/sssd.api.conf b/src/config/etc/sssd.api.conf index 5e5a928..5f995af 100644 --- a/src/config/etc/sssd.api.conf +++ b/src/config/etc/sssd.api.conf @@ -133,6 +133,7 @@ entry_cache_netgroup_timeout = int, None, false entry_cache_service_timeout = int, None, false entry_cache_autofs_timeout = int, None, false entry_cache_sudo_timeout = int, None, false +entry_cache_ssh_host_timeout = int, None, false refresh_expired_interval = int, None, false
# Dynamic DNS updates diff --git a/src/db/sysdb_ssh.c b/src/db/sysdb_ssh.c index 7dd98cf..bc8b1f6 100644 --- a/src/db/sysdb_ssh.c +++ b/src/db/sysdb_ssh.c @@ -23,6 +23,14 @@ #include "db/sysdb_ssh.h" #include "db/sysdb_private.h"
+static struct ldb_dn * +sysdb_ssh_host_dn(TALLOC_CTX *mem_ctx,
struct sss_domain_info *domain,
const char *name)
+{
- return sysdb_custom_dn(mem_ctx, domain, name, SSH_HOSTS_SUBDIR);
+}
static errno_t sysdb_update_ssh_host(struct sss_domain_info *domain, const char *name, @@ -45,6 +53,7 @@ errno_t sysdb_store_ssh_host(struct sss_domain_info *domain, const char *name, const char *alias,
int cache_timeout, time_t now, struct sysdb_attrs *attrs)
{ @@ -147,6 +156,14 @@ sysdb_store_ssh_host(struct sss_domain_info *domain, goto done; }
- ret = sysdb_attrs_add_time_t(attrs, SYSDB_CACHE_EXPIRE,
cache_timeout ? (now +
cache_timeout) : 0);
- if (ret) {
Please use form:
ret != EOK
as it is used in the rest of the module
DEBUG(SSSDBG_OP_FAILURE, "Could not set sysdb cache expire
ret, strerror(ret));
goto done;
- }
- ret = sysdb_update_ssh_host(domain, name, attrs); if (ret != EOK) { goto done;
@@ -175,6 +192,36 @@ done: return ret; }
One too many empty lines.
+errno_t +sysdb_set_ssh_host_attr(struct sss_domain_info *domain,
const char *name,
struct sysdb_attrs *attrs,
int mod_op)
+{
Is the empty line here intentional?
- errno_t ret;
- struct ldb_dn *dn;
- TALLOC_CTX *tmp_ctx;
- tmp_ctx = talloc_new(NULL);
- if (!tmp_ctx) {
return ENOMEM;
- }
- dn = sysdb_ssh_host_dn(tmp_ctx, domain, name);
- if (!dn) {
ret = ENOMEM;
goto done;
- }
- ret = sysdb_set_entry_attr(domain->sysdb, dn, attrs, mod_op);
We should check return value here, right?
+done:
- talloc_free(tmp_ctx);
- return ret;
+}
errno_t sysdb_update_ssh_known_host_expire(struct sss_domain_info *domain, const char *name, @@ -229,13 +276,13 @@ sysdb_delete_ssh_host(struct sss_domain_info *domain, return sysdb_delete_custom(domain, name, SSH_HOSTS_SUBDIR); }
-static errno_t +errno_t sysdb_search_ssh_hosts(TALLOC_CTX *mem_ctx, struct sss_domain_info *domain, const char *filter, const char **attrs,
struct ldb_message ***hosts,
size_t *num_hosts)
size_t *num_hosts,
struct ldb_message ***hosts)
{ errno_t ret; TALLOC_CTX *tmp_ctx; @@ -297,7 +344,7 @@ sysdb_get_ssh_host(TALLOC_CTX *mem_ctx, }
ret = sysdb_search_ssh_hosts(tmp_ctx, domain, filter, attrs,
&hosts, &num_hosts);
if (ret != EOK) { goto done; }&num_hosts, &hosts);
@@ -335,15 +382,19 @@ sysdb_get_ssh_known_hosts(TALLOC_CTX *mem_ctx, return ENOMEM; }
- filter = talloc_asprintf(tmp_ctx, "(%s>=%ld)",
SYSDB_SSH_KNOWN_HOSTS_EXPIRE, (long)now);
- filter = talloc_asprintf(tmp_ctx,
- "(&(|(!(%s=*))(%s=0)(%s>=%lld))(%s>=%lld))",
SYSDB_CACHE_EXPIRE,
SYSDB_CACHE_EXPIRE,
SYSDB_CACHE_EXPIRE, (long long)now + 1,
SYSDB_SSH_KNOWN_HOSTS_EXPIRE, (long
long)now + 1); if (!filter) { ret = ENOMEM; goto done; }
ret = sysdb_search_ssh_hosts(mem_ctx, domain, filter, attrs,
hosts, num_hosts);
num_hosts, hosts);
done: talloc_free(tmp_ctx); diff --git a/src/db/sysdb_ssh.h b/src/db/sysdb_ssh.h index e8aca77..960a13f 100644 --- a/src/db/sysdb_ssh.h +++ b/src/db/sysdb_ssh.h @@ -33,6 +33,7 @@ errno_t sysdb_store_ssh_host(struct sss_domain_info *domain, const char *name, const char *alias,
int cache_timeout, time_t now, struct sysdb_attrs *attrs);
@@ -42,11 +43,25 @@ sysdb_update_ssh_known_host_expire(struct sss_domain_info *domain, time_t now, int known_hosts_timeout);
+int +sysdb_set_ssh_host_attr(struct sss_domain_info *domain,
const char *name,
struct sysdb_attrs *attrs,
int mod_op);
errno_t sysdb_delete_ssh_host(struct sss_domain_info *domain, const char *name);
errno_t +sysdb_search_ssh_hosts(TALLOC_CTX *mem_ctx,
struct sss_domain_info *domain,
const char *filter,
const char **attrs,
size_t *num_hosts,
struct ldb_message ***hosts);
+errno_t sysdb_get_ssh_host(TALLOC_CTX *mem_ctx, struct sss_domain_info *domain, const char *name, diff --git a/src/man/sssd.conf.5.xml b/src/man/sssd.conf.5.xml index 27d22f4..bb631c3 100644 --- a/src/man/sssd.conf.5.xml +++ b/src/man/sssd.conf.5.xml @@ -1175,6 +1175,20 @@ fallback_homedir = /home/%u </listitem> </varlistentry>
<varlistentry condition="with_ssh">
<term>entry_cache_ssh_host_timeout (integer)</term>
<listitem>
<para>
How many seconds to keep a host ssh key after
refresh. IE how long to cache the host key
for.
</para>
<para>
Default: entry_cache_timeout
</para>
</listitem>
</varlistentry>
<varlistentry> <term>refresh_expired_interval (integer)</term> <listitem>
diff --git a/src/providers/ipa/ipa_hostid.c b/src/providers/ipa/ipa_hostid.c index 905454d..6b82335 100644 --- a/src/providers/ipa/ipa_hostid.c +++ b/src/providers/ipa/ipa_hostid.c @@ -300,8 +300,8 @@ hosts_get_done(struct tevent_req *subreq) goto done; }
- ret = sysdb_store_ssh_host(state->domain, state->name,
state->alias, now,
attrs);
- ret = sysdb_store_ssh_host(state->domain, state->name, state->alias,
- state->domain->ssh_host_timeout, now, attrs); if (ret != EOK) { goto done; }
diff --git a/src/tests/sysdb_ssh-tests.c b/src/tests/sysdb_ssh-tests.c index 6a4c219..46f5c47 100644 --- a/src/tests/sysdb_ssh-tests.c +++ b/src/tests/sysdb_ssh-tests.c @@ -175,6 +175,7 @@ static int test_sysdb_store_ssh_host(struct test_data *data) ret = sysdb_store_ssh_host(data->ctx->domain, data->hostname, data->alias,
- data->ctx->domain->ssh_host_timeout, now, data->attrs); return ret;
diff --git a/src/tools/sss_cache.c b/src/tools/sss_cache.c index 7cd5852..c72af4b 100644 --- a/src/tools/sss_cache.c +++ b/src/tools/sss_cache.c @@ -30,6 +30,7 @@ #include "db/sysdb.h" #include "db/sysdb_services.h" #include "db/sysdb_autofs.h" +#include "db/sysdb_ssh.h"
#define INVALIDATE_NONE 0 #define INVALIDATE_USERS 1 @@ -37,22 +38,36 @@ #define INVALIDATE_NETGROUPS 4 #define INVALIDATE_SERVICES 8 #define INVALIDATE_AUTOFSMAPS 16 +#define INVALIDATE_SSH_HOSTS 32
#ifdef BUILD_AUTOFS +#ifdef BUILD_SSH #define INVALIDATE_EVERYTHING (INVALIDATE_USERS | INVALIDATE_GROUPS | \ INVALIDATE_NETGROUPS | INVALIDATE_SERVICES | \
INVALIDATE_AUTOFSMAPS)
-#else
INVALIDATE_AUTOFSMAPS |
INVALIDATE_SSH_HOSTS ) +#else /* BUILD_SSH */ +#define INVALIDATE_EVERYTHING (INVALIDATE_USERS | INVALIDATE_GROUPS | \
INVALIDATE_NETGROUPS |
INVALIDATE_SERVICES | \
INVALIDATE_AUTOFSMAPS )
+#endif /* BUILD_SSH */ +#else /* BUILD_AUTOFS */ +#ifdef BUILD_SSH +#define INVALIDATE_EVERYTHING (INVALIDATE_USERS | INVALIDATE_GROUPS | \
INVALIDATE_NETGROUPS |
INVALIDATE_SERVICES | \
INVALIDATE_SSH_HOSTS )
+#else /* BUILD_SSH */ #define INVALIDATE_EVERYTHING (INVALIDATE_USERS | INVALIDATE_GROUPS | \
INVALIDATE_NETGROUPS |
INVALIDATE_SERVICES) -#endif
INVALIDATE_NETGROUPS |
INVALIDATE_SERVICES ) +#endif /* BUILD_SSH */ +#endif /* BUILD_AUTOFS */
enum sss_cache_entry { TYPE_USER=0, TYPE_GROUP, TYPE_NETGROUP, TYPE_SERVICE,
- TYPE_AUTOFSMAP
- TYPE_AUTOFSMAP,
- TYPE_SSH_HOST
};
static errno_t search_autofsmaps(TALLOC_CTX *mem_ctx, @@ -69,18 +84,21 @@ struct cache_tool_ctx { char *netgroup_filter; char *service_filter; char *autofs_filter;
char *ssh_host_filter;
char *user_name; char *group_name; char *netgroup_name; char *service_name; char *autofs_name;
char *ssh_host_name;
bool update_user_filter; bool update_group_filter; bool update_netgroup_filter; bool update_service_filter; bool update_autofs_filter;
bool update_ssh_host_filter;
};
errno_t init_domains(struct cache_tool_ctx *ctx, const char *domain); @@ -152,6 +170,9 @@ int main(int argc, const char *argv[]) skipped &= !invalidate_entries(tctx, dinfo, TYPE_AUTOFSMAP, tctx->autofs_filter, tctx->autofs_name);
skipped &= !invalidate_entries(tctx, dinfo, TYPE_SSH_HOST,
tctx->ssh_host_filter,
tctx->ssh_host_name); ret = sysdb_transaction_commit(sysdb); if (ret != EOK) {
@@ -328,6 +349,14 @@ static errno_t update_all_filters(struct cache_tool_ctx *tctx, return ret; }
- /* Update ssh host filter */
- ret = update_filter(tctx, dinfo, tctx->ssh_host_name,
tctx->update_ssh_host_filter, "(%s=%s)", false,
&tctx->ssh_host_filter);
- if (ret != EOK) {
return ret;
- }
- return EOK;
}
@@ -371,6 +400,15 @@ static bool invalidate_entries(TALLOC_CTX *ctx, type_string = "autofs map"; ret = search_autofsmaps(ctx, dinfo, filter, attrs, &msg_count, &msgs); break;
- case TYPE_SSH_HOST:
type_string = "ssh_host";
+#ifdef BUILD_SSH
ret = sysdb_search_ssh_hosts(ctx, dinfo,
filter, attrs, &msg_count, &msgs);
+#else /* BUILD_SSH */
ret = ENOSYS;
+#endif /* BUILD_SSH */
break;
}
if (ret != EOK) {
@@ -446,6 +484,14 @@ static errno_t invalidate_entry(TALLOC_CTX *ctx, ret = sysdb_set_autofsmap_attr(domain, name, sys_attrs, SYSDB_MOD_REP); break;
case TYPE_SSH_HOST:
+#ifdef BUILD_SSH
ret = sysdb_set_ssh_host_attr(domain, name,
sys_attrs,
SYSDB_MOD_REP); +#else /* BUILD_SSH */
ret = ENOSYS;
+#endif /* BUILD_SSH */
break; default: return EINVAL; }
@@ -530,6 +576,7 @@ errno_t init_context(int argc, const char *argv[], struct cache_tool_ctx **tctx) char *netgroup = NULL; char *service = NULL; char *map = NULL;
- char *ssh_host = NULL; char *domain = NULL; int debug = SSSDBG_DEFAULT; errno_t ret = EOK;
@@ -563,6 +610,12 @@ errno_t init_context(int argc, const char *argv[], struct cache_tool_ctx **tctx) { "autofs-maps", 'A', POPT_ARG_NONE, NULL, 'a', _("Invalidate all autofs maps"), NULL }, #endif /* BUILD_AUTOFS */ +#ifdef BUILD_SSH
{ "ssh_host", 'h', POPT_ARG_STRING, &ssh_host, 0,
_("Invalidate particular SSH host"), NULL },
{ "ssh_hosts", 'H', POPT_ARG_NONE, NULL, 'h',
_("Invalidate all SSH hosts"), NULL },
+#endif /* BUILD_SSH */ { "domain", 'd', POPT_ARG_STRING, &domain, 0, _("Only invalidate entries from a particular domain"), NULL }, POPT_TABLEEND @@ -594,6 +647,9 @@ errno_t init_context(int argc, const char *argv[], struct cache_tool_ctx **tctx) case 'a': idb |= INVALIDATE_AUTOFSMAPS; break;
case 'h':
idb |= INVALIDATE_SSH_HOSTS;
break; case 'e': idb = INVALIDATE_EVERYTHING; break;
@@ -608,7 +664,7 @@ errno_t init_context(int argc, const char *argv[], struct cache_tool_ctx **tctx) }
if (idb == INVALIDATE_NONE && !user && !group &&
!netgroup && !service && !map) {
!netgroup && !service && !map && !ssh_host) { BAD_POPT_PARAMS(pc, _("Please select at least one object to invalidate\n"), ret, fini);
@@ -665,14 +721,26 @@ errno_t init_context(int argc, const char *argv[], struct cache_tool_ctx **tctx) ctx->update_autofs_filter = true; }
- if (idb & INVALIDATE_SSH_HOSTS) {
ctx->ssh_host_filter = talloc_asprintf(ctx, "(%s=*)",
SYSDB_NAME);
ctx->update_ssh_host_filter = false;
- } else if (ssh_host) {
ctx->ssh_host_name = talloc_strdup(ctx, ssh_host);
ctx->update_ssh_host_filter = true;
- }
- if (((idb & INVALIDATE_USERS) && !ctx->user_filter) || ((idb & INVALIDATE_GROUPS) && !ctx->group_filter) || ((idb & INVALIDATE_NETGROUPS) && !ctx->netgroup_filter) || ((idb & INVALIDATE_SERVICES) && !ctx->service_filter) || ((idb & INVALIDATE_AUTOFSMAPS) && !ctx->autofs_filter) ||
(user && !ctx->user_name) || (group && !ctx->group_name) ||
(netgroup && !ctx->netgroup_name) || (map &&
!ctx->autofs_name) ||
(service && !ctx->service_name)) {
((idb & INVALIDATE_SSH_HOSTS) && !ctx->ssh_host_filter) ||
(user && !ctx->user_name) ||
(group && !ctx->group_name) ||
(netgroup && !ctx->netgroup_name) ||
(service && !ctx->service_name) ||
(map && !ctx->autofs_name) ||
(ssh_host && !ctx->ssh_host_name)) { DEBUG(SSSDBG_CRIT_FAILURE, "Construction of filters failed\n"); ret = ENOMEM; goto fini;
--
On (28/08/14 18:11), Pavel Reichl wrote:
On 08/25/2014 02:05 PM, Jan Cholasta wrote:
Dne 4.8.2014 v 19:34 Pavel Reichl napsal(a):
On 07/21/2014 02:08 PM, Jakub Hrozek wrote:
On Mon, Jul 21, 2014 at 01:55:20PM +0200, Jan Cholasta wrote:
On 18.7.2014 06:41, William wrote:
>>Ignore that last patch, I messed up and didn't include a .h file. >>Here >>is the fixed patch. >This new patch seems to be based on some older version of the >patch, it >uses wrong option name, sysdb_update_ssh_host still has the confdb >argument, etc. > Fixed, and fixed the documentation into the xml file, removed the references to the .pot file.
I don't see any of the requested fixes in the patch.
See attachment for fixed patch. I also did a few additional tweaks to make the patch work when SSSD is updated from a previous version and when building without SSH bits.
Awesome thank you, I guess some of us need to review the patch now you're also an author.
While Jan was so kind to give me some hints on how to test patch properly, we found out a little bug. So Jan is going to prepare an updated patch.
I'll do the review then.
I can't reproduce the issue, maybe something else was wrong in your setup?
For the record, I installed IPA in a VM and then ran these commands on it:
$ sss_ssh_knownhostsproxy --debug 10 $HOSTNAME [no errors] $ cat /var/lib/sss/pubconf/known_hosts [not empty] # systemctl stop ipa $ sss_ssh_knownhostsproxy --debug 10 $HOSTNAME [no errors] $ cat /var/lib/sss/pubconf/known_hosts [not empty] # sss_cache -H $ sss_ssh_knownhostsproxy --debug 10 $HOSTNAME [no errors] $ cat /var/lib/sss/pubconf/known_hosts [empty] # systemctl start ipa $ sss_ssh_knownhostsproxy --debug 10 $HOSTNAME [no errors] $ cat /var/lib/sss/pubconf/known_hosts [not empty]
Pavel had an empty /var/lib/sss/pubconf/known_hosts in the last step, I have it non-empty as expected.
(sorry for the delay, I was on leave)
This test ran OK this time, so I think you are right.
I have some comments to the patch itself.
I'm not personally fan of big patches like this one. Could you at least separate the man page changes to separate patch - so we can ask some native speaker to review it.
I am not big fan of splitting patches just for "man page changes"
LS
On Thu, Aug 28, 2014 at 06:24:23PM +0200, Lukas Slebodnik wrote:
This test ran OK this time, so I think you are right.
I have some comments to the patch itself.
I'm not personally fan of big patches like this one. Could you at least separate the man page changes to separate patch - so we can ask some native speaker to review it.
I am not big fan of splitting patches just for "man page changes"
me neither, backporting or cherry-picking is then harder.
Patches should do one logical change.
On 08/28/2014 06:24 PM, Lukas Slebodnik wrote:
On (28/08/14 18:11), Pavel Reichl wrote:
On 08/25/2014 02:05 PM, Jan Cholasta wrote:
Dne 4.8.2014 v 19:34 Pavel Reichl napsal(a):
On 07/21/2014 02:08 PM, Jakub Hrozek wrote:
On Mon, Jul 21, 2014 at 01:55:20PM +0200, Jan Cholasta wrote:
On 18.7.2014 06:41, William wrote: >>> Ignore that last patch, I messed up and didn't include a .h file. >>> Here >>> is the fixed patch. >> This new patch seems to be based on some older version of the >> patch, it >> uses wrong option name, sysdb_update_ssh_host still has the confdb >> argument, etc. >> > Fixed, and fixed the documentation into the xml file, removed the > references to the .pot file. I don't see any of the requested fixes in the patch.
See attachment for fixed patch. I also did a few additional tweaks to make the patch work when SSSD is updated from a previous version and when building without SSH bits.
Awesome thank you, I guess some of us need to review the patch now you're also an author.
While Jan was so kind to give me some hints on how to test patch properly, we found out a little bug. So Jan is going to prepare an updated patch.
I'll do the review then.
I can't reproduce the issue, maybe something else was wrong in your setup?
For the record, I installed IPA in a VM and then ran these commands on it:
$ sss_ssh_knownhostsproxy --debug 10 $HOSTNAME [no errors] $ cat /var/lib/sss/pubconf/known_hosts [not empty] # systemctl stop ipa $ sss_ssh_knownhostsproxy --debug 10 $HOSTNAME [no errors] $ cat /var/lib/sss/pubconf/known_hosts [not empty] # sss_cache -H $ sss_ssh_knownhostsproxy --debug 10 $HOSTNAME [no errors] $ cat /var/lib/sss/pubconf/known_hosts [empty] # systemctl start ipa $ sss_ssh_knownhostsproxy --debug 10 $HOSTNAME [no errors] $ cat /var/lib/sss/pubconf/known_hosts [not empty]
Pavel had an empty /var/lib/sss/pubconf/known_hosts in the last step, I have it non-empty as expected.
(sorry for the delay, I was on leave)
This test ran OK this time, so I think you are right.
I have some comments to the patch itself.
I'm not personally fan of big patches like this one. Could you at least separate the man page changes to separate patch - so we can ask some native speaker to review it.
I am not big fan of splitting patches just for "man page changes"
LS
I like small patches, each doing one think and one think only. But I know I can't get always what I want.
Still I believe that it would be a good policy to have man page updates in separate patch as we already have policy to ask native speaker to do review such changes. I think, it's a way more convenient for reviewer not to have search through the patch for man page changes and as a bonus he gets reviewed-by achievement. :-)
What are you reasons not to have separate patch?
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
On (28/08/14 18:37), Pavel Reichl wrote:
On 08/28/2014 06:24 PM, Lukas Slebodnik wrote:
On (28/08/14 18:11), Pavel Reichl wrote:
On 08/25/2014 02:05 PM, Jan Cholasta wrote:
Dne 4.8.2014 v 19:34 Pavel Reichl napsal(a):
On 07/21/2014 02:08 PM, Jakub Hrozek wrote:
On Mon, Jul 21, 2014 at 01:55:20PM +0200, Jan Cholasta wrote: >On 18.7.2014 06:41, William wrote: >>>>Ignore that last patch, I messed up and didn't include a .h file. >>>>Here >>>>is the fixed patch. >>>This new patch seems to be based on some older version of the >>>patch, it >>>uses wrong option name, sysdb_update_ssh_host still has the confdb >>>argument, etc. >>> >>Fixed, and fixed the documentation into the xml file, removed the >>references to the .pot file. >I don't see any of the requested fixes in the patch. > >See attachment for fixed patch. I also did a few additional tweaks to >make >the patch work when SSSD is updated from a previous version and when >building without SSH bits. Awesome thank you, I guess some of us need to review the patch now you're also an author.
While Jan was so kind to give me some hints on how to test patch properly, we found out a little bug. So Jan is going to prepare an updated patch.
I'll do the review then.
I can't reproduce the issue, maybe something else was wrong in your setup?
For the record, I installed IPA in a VM and then ran these commands on it:
$ sss_ssh_knownhostsproxy --debug 10 $HOSTNAME [no errors] $ cat /var/lib/sss/pubconf/known_hosts [not empty] # systemctl stop ipa $ sss_ssh_knownhostsproxy --debug 10 $HOSTNAME [no errors] $ cat /var/lib/sss/pubconf/known_hosts [not empty] # sss_cache -H $ sss_ssh_knownhostsproxy --debug 10 $HOSTNAME [no errors] $ cat /var/lib/sss/pubconf/known_hosts [empty] # systemctl start ipa $ sss_ssh_knownhostsproxy --debug 10 $HOSTNAME [no errors] $ cat /var/lib/sss/pubconf/known_hosts [not empty]
Pavel had an empty /var/lib/sss/pubconf/known_hosts in the last step, I have it non-empty as expected.
(sorry for the delay, I was on leave)
This test ran OK this time, so I think you are right.
I have some comments to the patch itself.
I'm not personally fan of big patches like this one. Could you at least separate the man page changes to separate patch - so we can ask some native speaker to review it.
I am not big fan of splitting patches just for "man page changes"
LS
I like small patches, each doing one think and one think only. But I know I can't get always what I want.
Still I believe that it would be a good policy to have man page updates in separate patch as we already have policy to ask native speaker to do review such changes. I think, it's a way more convenient for reviewer not to have search through the patch for man page changes and as a bonus he gets reviewed-by achievement. :-)
What are you reasons not to have separate patch?
https://lists.fedorahosted.org/pipermail/sssd-devel/2014-August/021296.html
LS
On 08/28/2014 07:13 PM, Lukas Slebodnik wrote:
On (28/08/14 18:37), Pavel Reichl wrote:
On 08/28/2014 06:24 PM, Lukas Slebodnik wrote:
On (28/08/14 18:11), Pavel Reichl wrote:
On 08/25/2014 02:05 PM, Jan Cholasta wrote:
Dne 4.8.2014 v 19:34 Pavel Reichl napsal(a):
On 07/21/2014 02:08 PM, Jakub Hrozek wrote: > On Mon, Jul 21, 2014 at 01:55:20PM +0200, Jan Cholasta wrote: >> On 18.7.2014 06:41, William wrote: >>>>> Ignore that last patch, I messed up and didn't include a .h file. >>>>> Here >>>>> is the fixed patch. >>>> This new patch seems to be based on some older version of the >>>> patch, it >>>> uses wrong option name, sysdb_update_ssh_host still has the confdb >>>> argument, etc. >>>> >>> Fixed, and fixed the documentation into the xml file, removed the >>> references to the .pot file. >> I don't see any of the requested fixes in the patch. >> >> See attachment for fixed patch. I also did a few additional tweaks to >> make >> the patch work when SSSD is updated from a previous version and when >> building without SSH bits. > Awesome thank you, I guess some of us need to review the patch now > you're also an author. While Jan was so kind to give me some hints on how to test patch properly, we found out a little bug. So Jan is going to prepare an updated patch.
I'll do the review then.
I can't reproduce the issue, maybe something else was wrong in your setup?
For the record, I installed IPA in a VM and then ran these commands on it:
$ sss_ssh_knownhostsproxy --debug 10 $HOSTNAME [no errors] $ cat /var/lib/sss/pubconf/known_hosts [not empty] # systemctl stop ipa $ sss_ssh_knownhostsproxy --debug 10 $HOSTNAME [no errors] $ cat /var/lib/sss/pubconf/known_hosts [not empty] # sss_cache -H $ sss_ssh_knownhostsproxy --debug 10 $HOSTNAME [no errors] $ cat /var/lib/sss/pubconf/known_hosts [empty] # systemctl start ipa $ sss_ssh_knownhostsproxy --debug 10 $HOSTNAME [no errors] $ cat /var/lib/sss/pubconf/known_hosts [not empty]
Pavel had an empty /var/lib/sss/pubconf/known_hosts in the last step, I have it non-empty as expected.
(sorry for the delay, I was on leave)
This test ran OK this time, so I think you are right.
I have some comments to the patch itself.
I'm not personally fan of big patches like this one. Could you at least separate the man page changes to separate patch - so we can ask some native speaker to review it.
I am not big fan of splitting patches just for "man page changes"
LS
I like small patches, each doing one think and one think only. But I know I can't get always what I want.
Still I believe that it would be a good policy to have man page updates in separate patch as we already have policy to ask native speaker to do review such changes. I think, it's a way more convenient for reviewer not to have search through the patch for man page changes and as a bonus he gets reviewed-by achievement. :-)
What are you reasons not to have separate patch?
https://lists.fedorahosted.org/pipermail/sssd-devel/2014-August/021296.html
LS
I have read that already no need to link messages a few minutes old.
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
On (28/08/14 19:19), Pavel Reichl wrote:
On 08/28/2014 07:13 PM, Lukas Slebodnik wrote:
On (28/08/14 18:37), Pavel Reichl wrote:
On 08/28/2014 06:24 PM, Lukas Slebodnik wrote:
On (28/08/14 18:11), Pavel Reichl wrote:
On 08/25/2014 02:05 PM, Jan Cholasta wrote:
Dne 4.8.2014 v 19:34 Pavel Reichl napsal(a): >On 07/21/2014 02:08 PM, Jakub Hrozek wrote: >>On Mon, Jul 21, 2014 at 01:55:20PM +0200, Jan Cholasta wrote: >>>On 18.7.2014 06:41, William wrote: >>>>>>Ignore that last patch, I messed up and didn't include a .h file. >>>>>>Here >>>>>>is the fixed patch. >>>>>This new patch seems to be based on some older version of the >>>>>patch, it >>>>>uses wrong option name, sysdb_update_ssh_host still has the confdb >>>>>argument, etc. >>>>> >>>>Fixed, and fixed the documentation into the xml file, removed the >>>>references to the .pot file. >>>I don't see any of the requested fixes in the patch. >>> >>>See attachment for fixed patch. I also did a few additional tweaks to >>>make >>>the patch work when SSSD is updated from a previous version and when >>>building without SSH bits. >>Awesome thank you, I guess some of us need to review the patch now >>you're also an author. >While Jan was so kind to give me some hints on how to test patch >properly, we found out a little bug. So Jan is going to prepare an >updated patch. > >I'll do the review then. I can't reproduce the issue, maybe something else was wrong in your setup?
For the record, I installed IPA in a VM and then ran these commands on it:
$ sss_ssh_knownhostsproxy --debug 10 $HOSTNAME [no errors] $ cat /var/lib/sss/pubconf/known_hosts [not empty] # systemctl stop ipa $ sss_ssh_knownhostsproxy --debug 10 $HOSTNAME [no errors] $ cat /var/lib/sss/pubconf/known_hosts [not empty] # sss_cache -H $ sss_ssh_knownhostsproxy --debug 10 $HOSTNAME [no errors] $ cat /var/lib/sss/pubconf/known_hosts [empty] # systemctl start ipa $ sss_ssh_knownhostsproxy --debug 10 $HOSTNAME [no errors] $ cat /var/lib/sss/pubconf/known_hosts [not empty]
Pavel had an empty /var/lib/sss/pubconf/known_hosts in the last step, I have it non-empty as expected.
(sorry for the delay, I was on leave)
This test ran OK this time, so I think you are right.
I have some comments to the patch itself.
I'm not personally fan of big patches like this one. Could you at least separate the man page changes to separate patch - so we can ask some native speaker to review it.
I am not big fan of splitting patches just for "man page changes"
LS
I like small patches, each doing one think and one think only. But I know I can't get always what I want.
Still I believe that it would be a good policy to have man page updates in separate patch as we already have policy to ask native speaker to do review such changes. I think, it's a way more convenient for reviewer not to have search through the patch for man page changes and as a bonus he gets reviewed-by achievement. :-)
What are you reasons not to have separate patch?
https://lists.fedorahosted.org/pipermail/sssd-devel/2014-August/021296.html
LS
I have read that already no need to link messages a few minutes old.
In this case, I don't understand why did you ask? (no reply needed :-)
LS
Because Jakubs' message came after I started to write my message and I didn't read it till I posted mine.
Still the discussion was between you and me, so it would be more discussion friendly if you at least had writen that you share Jakubs' concerns (which are valid from my POV but I'm not sure they beat my arguments).
----- Original Message ----- From: "Lukas Slebodnik" lslebodn@redhat.com To: "Development of the System Security Services Daemon" sssd-devel@lists.fedorahosted.org Sent: Thursday, August 28, 2014 11:02:46 PM Subject: Re: [SSSD] sss_cache flush ssh hosts list.
On (28/08/14 19:19), Pavel Reichl wrote:
On 08/28/2014 07:13 PM, Lukas Slebodnik wrote:
On (28/08/14 18:37), Pavel Reichl wrote:
On 08/28/2014 06:24 PM, Lukas Slebodnik wrote:
On (28/08/14 18:11), Pavel Reichl wrote:
On 08/25/2014 02:05 PM, Jan Cholasta wrote:
Dne 4.8.2014 v 19:34 Pavel Reichl napsal(a): >On 07/21/2014 02:08 PM, Jakub Hrozek wrote: >>On Mon, Jul 21, 2014 at 01:55:20PM +0200, Jan Cholasta wrote: >>>On 18.7.2014 06:41, William wrote: >>>>>>Ignore that last patch, I messed up and didn't include a .h file. >>>>>>Here >>>>>>is the fixed patch. >>>>>This new patch seems to be based on some older version of the >>>>>patch, it >>>>>uses wrong option name, sysdb_update_ssh_host still has the confdb >>>>>argument, etc. >>>>> >>>>Fixed, and fixed the documentation into the xml file, removed the >>>>references to the .pot file. >>>I don't see any of the requested fixes in the patch. >>> >>>See attachment for fixed patch. I also did a few additional tweaks to >>>make >>>the patch work when SSSD is updated from a previous version and when >>>building without SSH bits. >>Awesome thank you, I guess some of us need to review the patch now >>you're also an author. >While Jan was so kind to give me some hints on how to test patch >properly, we found out a little bug. So Jan is going to prepare an >updated patch. > >I'll do the review then. I can't reproduce the issue, maybe something else was wrong in your setup?
For the record, I installed IPA in a VM and then ran these commands on it:
$ sss_ssh_knownhostsproxy --debug 10 $HOSTNAME [no errors] $ cat /var/lib/sss/pubconf/known_hosts [not empty] # systemctl stop ipa $ sss_ssh_knownhostsproxy --debug 10 $HOSTNAME [no errors] $ cat /var/lib/sss/pubconf/known_hosts [not empty] # sss_cache -H $ sss_ssh_knownhostsproxy --debug 10 $HOSTNAME [no errors] $ cat /var/lib/sss/pubconf/known_hosts [empty] # systemctl start ipa $ sss_ssh_knownhostsproxy --debug 10 $HOSTNAME [no errors] $ cat /var/lib/sss/pubconf/known_hosts [not empty]
Pavel had an empty /var/lib/sss/pubconf/known_hosts in the last step, I have it non-empty as expected.
(sorry for the delay, I was on leave)
This test ran OK this time, so I think you are right.
I have some comments to the patch itself.
I'm not personally fan of big patches like this one. Could you at least separate the man page changes to separate patch - so we can ask some native speaker to review it.
I am not big fan of splitting patches just for "man page changes"
LS
I like small patches, each doing one think and one think only. But I know I can't get always what I want.
Still I believe that it would be a good policy to have man page updates in separate patch as we already have policy to ask native speaker to do review such changes. I think, it's a way more convenient for reviewer not to have search through the patch for man page changes and as a bonus he gets reviewed-by achievement. :-)
What are you reasons not to have separate patch?
https://lists.fedorahosted.org/pipermail/sssd-devel/2014-August/021296.html
LS
I have read that already no need to link messages a few minutes old.
In this case, I don't understand why did you ask? (no reply needed :-)
LS _______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
On Fri, Aug 29, 2014 at 03:53:55AM -0400, Pavel Reichl wrote:
Because Jakubs' message came after I started to write my message and I didn't read it till I posted mine.
Still the discussion was between you and me, so it would be more discussion friendly if you at least had writen that you share Jakubs' concerns (which are valid from my POV but I'm not sure they beat my arguments).
Please keep the discussion strictly technical, from both sides.
Dne 28.8.2014 v 18:11 Pavel Reichl napsal(a):
On 08/25/2014 02:05 PM, Jan Cholasta wrote:
Dne 4.8.2014 v 19:34 Pavel Reichl napsal(a):
On 07/21/2014 02:08 PM, Jakub Hrozek wrote:
On Mon, Jul 21, 2014 at 01:55:20PM +0200, Jan Cholasta wrote:
On 18.7.2014 06:41, William wrote:
>> Ignore that last patch, I messed up and didn't include a .h file. >> Here >> is the fixed patch. > This new patch seems to be based on some older version of the > patch, it > uses wrong option name, sysdb_update_ssh_host still has the confdb > argument, etc. > Fixed, and fixed the documentation into the xml file, removed the references to the .pot file.
I don't see any of the requested fixes in the patch.
See attachment for fixed patch. I also did a few additional tweaks to make the patch work when SSSD is updated from a previous version and when building without SSH bits.
Awesome thank you, I guess some of us need to review the patch now you're also an author.
While Jan was so kind to give me some hints on how to test patch properly, we found out a little bug. So Jan is going to prepare an updated patch.
I'll do the review then.
I can't reproduce the issue, maybe something else was wrong in your setup?
For the record, I installed IPA in a VM and then ran these commands on it:
$ sss_ssh_knownhostsproxy --debug 10 $HOSTNAME [no errors] $ cat /var/lib/sss/pubconf/known_hosts [not empty] # systemctl stop ipa $ sss_ssh_knownhostsproxy --debug 10 $HOSTNAME [no errors] $ cat /var/lib/sss/pubconf/known_hosts [not empty] # sss_cache -H $ sss_ssh_knownhostsproxy --debug 10 $HOSTNAME [no errors] $ cat /var/lib/sss/pubconf/known_hosts [empty] # systemctl start ipa $ sss_ssh_knownhostsproxy --debug 10 $HOSTNAME [no errors] $ cat /var/lib/sss/pubconf/known_hosts [not empty]
Pavel had an empty /var/lib/sss/pubconf/known_hosts in the last step, I have it non-empty as expected.
(sorry for the delay, I was on leave)
This test ran OK this time, so I think you are right.
I have some comments to the patch itself.
I'm not personally fan of big patches like this one. Could you at least separate the man page changes to separate patch - so we can ask some native speaker to review it.
Also new options in sss_cache -h and -H are not documented in man pages, could you document them?
Please see some minor comments inline:
Fixed everything, except:
- ret = sysdb_set_entry_attr(domain->sysdb, dn, attrs, mod_op);
We should check return value here, right?
We want to return this value directly, there's no need to check it (other sysdb_set_*_attr functions do this as well).
Updated patch attached.
On 08/29/2014 10:41 AM, Jan Cholasta wrote:
Dne 28.8.2014 v 18:11 Pavel Reichl napsal(a):
On 08/25/2014 02:05 PM, Jan Cholasta wrote:
Dne 4.8.2014 v 19:34 Pavel Reichl napsal(a):
On 07/21/2014 02:08 PM, Jakub Hrozek wrote:
On Mon, Jul 21, 2014 at 01:55:20PM +0200, Jan Cholasta wrote:
On 18.7.2014 06:41, William wrote: >>> Ignore that last patch, I messed up and didn't include a .h file. >>> Here >>> is the fixed patch. >> This new patch seems to be based on some older version of the >> patch, it >> uses wrong option name, sysdb_update_ssh_host still has the confdb >> argument, etc. >> > Fixed, and fixed the documentation into the xml file, removed the > references to the .pot file. I don't see any of the requested fixes in the patch.
See attachment for fixed patch. I also did a few additional tweaks to make the patch work when SSSD is updated from a previous version and when building without SSH bits.
Awesome thank you, I guess some of us need to review the patch now you're also an author.
While Jan was so kind to give me some hints on how to test patch properly, we found out a little bug. So Jan is going to prepare an updated patch.
I'll do the review then.
I can't reproduce the issue, maybe something else was wrong in your setup?
For the record, I installed IPA in a VM and then ran these commands on it:
$ sss_ssh_knownhostsproxy --debug 10 $HOSTNAME [no errors] $ cat /var/lib/sss/pubconf/known_hosts [not empty] # systemctl stop ipa $ sss_ssh_knownhostsproxy --debug 10 $HOSTNAME [no errors] $ cat /var/lib/sss/pubconf/known_hosts [not empty] # sss_cache -H $ sss_ssh_knownhostsproxy --debug 10 $HOSTNAME [no errors] $ cat /var/lib/sss/pubconf/known_hosts [empty] # systemctl start ipa $ sss_ssh_knownhostsproxy --debug 10 $HOSTNAME [no errors] $ cat /var/lib/sss/pubconf/known_hosts [not empty]
Pavel had an empty /var/lib/sss/pubconf/known_hosts in the last step, I have it non-empty as expected.
(sorry for the delay, I was on leave)
This test ran OK this time, so I think you are right.
I have some comments to the patch itself.
I'm not personally fan of big patches like this one. Could you at least separate the man page changes to separate patch - so we can ask some native speaker to review it.
Also new options in sss_cache -h and -H are not documented in man pages, could you document them?
Please see some minor comments inline:
Fixed everything, except:
- ret = sysdb_set_entry_attr(domain->sysdb, dn, attrs, mod_op);
We should check return value here, right?
We want to return this value directly, there's no need to check it (other sysdb_set_*_attr functions do this as well).
Updated patch attached.
Thanks,
<varlistentry condition="with_ssh">
<term>
<option>-h</option>,<option>--ssh-host</option>
- <replaceable>autofs-map</replaceable>
^^^^^
copy&paste mistake?
</term>
<listitem>
<para>
Invalidate SSH public keys of a specific host.
</para>
</listitem>
</varlistentry>
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
On 09/01/2014 02:27 PM, Pavel Reichl wrote:
On 08/29/2014 10:41 AM, Jan Cholasta wrote:
Dne 28.8.2014 v 18:11 Pavel Reichl napsal(a):
On 08/25/2014 02:05 PM, Jan Cholasta wrote:
Dne 4.8.2014 v 19:34 Pavel Reichl napsal(a):
On 07/21/2014 02:08 PM, Jakub Hrozek wrote:
On Mon, Jul 21, 2014 at 01:55:20PM +0200, Jan Cholasta wrote: > On 18.7.2014 06:41, William wrote: >>>> Ignore that last patch, I messed up and didn't include a .h >>>> file. >>>> Here >>>> is the fixed patch. >>> This new patch seems to be based on some older version of the >>> patch, it >>> uses wrong option name, sysdb_update_ssh_host still has the >>> confdb >>> argument, etc. >>> >> Fixed, and fixed the documentation into the xml file, removed the >> references to the .pot file. > I don't see any of the requested fixes in the patch. > > See attachment for fixed patch. I also did a few additional > tweaks to > make > the patch work when SSSD is updated from a previous version and > when > building without SSH bits. Awesome thank you, I guess some of us need to review the patch now you're also an author.
While Jan was so kind to give me some hints on how to test patch properly, we found out a little bug. So Jan is going to prepare an updated patch.
I'll do the review then.
I can't reproduce the issue, maybe something else was wrong in your setup?
For the record, I installed IPA in a VM and then ran these commands on it:
$ sss_ssh_knownhostsproxy --debug 10 $HOSTNAME [no errors] $ cat /var/lib/sss/pubconf/known_hosts [not empty] # systemctl stop ipa $ sss_ssh_knownhostsproxy --debug 10 $HOSTNAME [no errors] $ cat /var/lib/sss/pubconf/known_hosts [not empty] # sss_cache -H $ sss_ssh_knownhostsproxy --debug 10 $HOSTNAME [no errors] $ cat /var/lib/sss/pubconf/known_hosts [empty] # systemctl start ipa $ sss_ssh_knownhostsproxy --debug 10 $HOSTNAME [no errors] $ cat /var/lib/sss/pubconf/known_hosts [not empty]
Pavel had an empty /var/lib/sss/pubconf/known_hosts in the last step, I have it non-empty as expected.
(sorry for the delay, I was on leave)
This test ran OK this time, so I think you are right.
I have some comments to the patch itself.
I'm not personally fan of big patches like this one. Could you at least separate the man page changes to separate patch - so we can ask some native speaker to review it.
Also new options in sss_cache -h and -H are not documented in man pages, could you document them?
Please see some minor comments inline:
Fixed everything, except:
- ret = sysdb_set_entry_attr(domain->sysdb, dn, attrs, mod_op);
We should check return value here, right?
We want to return this value directly, there's no need to check it (other sysdb_set_*_attr functions do this as well).
Updated patch attached.
Thanks,
<varlistentry condition="with_ssh">
<term>
<option>-h</option>,<option>--ssh-host</option>
- <replaceable>autofs-map</replaceable>
^^^^^
copy&paste mistake?
</term>
<listitem>
<para>
Invalidate SSH public keys of a specific host.
</para>
</listitem>
</varlistentry>
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Could you also add ticket number (#2358) to the commit message? Thanks!
On (02/09/14 16:21), Pavel Reichl wrote:
On 09/01/2014 02:27 PM, Pavel Reichl wrote:
On 08/29/2014 10:41 AM, Jan Cholasta wrote:
Dne 28.8.2014 v 18:11 Pavel Reichl napsal(a):
On 08/25/2014 02:05 PM, Jan Cholasta wrote:
Dne 4.8.2014 v 19:34 Pavel Reichl napsal(a):
On 07/21/2014 02:08 PM, Jakub Hrozek wrote: >On Mon, Jul 21, 2014 at 01:55:20PM +0200, Jan Cholasta wrote: >>On 18.7.2014 06:41, William wrote: >>>>>Ignore that last patch, I messed up and didn't include a >>>>>.h file. >>>>>Here >>>>>is the fixed patch. >>>>This new patch seems to be based on some older version of the >>>>patch, it >>>>uses wrong option name, sysdb_update_ssh_host still has the >>>>confdb >>>>argument, etc. >>>> >>>Fixed, and fixed the documentation into the xml file, removed the >>>references to the .pot file. >>I don't see any of the requested fixes in the patch. >> >>See attachment for fixed patch. I also did a few additional >>tweaks to >>make >>the patch work when SSSD is updated from a previous version and >>when >>building without SSH bits. >Awesome thank you, I guess some of us need to review the patch now >you're also an author. While Jan was so kind to give me some hints on how to test patch properly, we found out a little bug. So Jan is going to prepare an updated patch.
I'll do the review then.
I can't reproduce the issue, maybe something else was wrong in your setup?
For the record, I installed IPA in a VM and then ran these commands on it:
$ sss_ssh_knownhostsproxy --debug 10 $HOSTNAME [no errors] $ cat /var/lib/sss/pubconf/known_hosts [not empty] # systemctl stop ipa $ sss_ssh_knownhostsproxy --debug 10 $HOSTNAME [no errors] $ cat /var/lib/sss/pubconf/known_hosts [not empty] # sss_cache -H $ sss_ssh_knownhostsproxy --debug 10 $HOSTNAME [no errors] $ cat /var/lib/sss/pubconf/known_hosts [empty] # systemctl start ipa $ sss_ssh_knownhostsproxy --debug 10 $HOSTNAME [no errors] $ cat /var/lib/sss/pubconf/known_hosts [not empty]
Pavel had an empty /var/lib/sss/pubconf/known_hosts in the last step, I have it non-empty as expected.
(sorry for the delay, I was on leave)
This test ran OK this time, so I think you are right.
I have some comments to the patch itself.
I'm not personally fan of big patches like this one. Could you at least separate the man page changes to separate patch - so we can ask some native speaker to review it.
Also new options in sss_cache -h and -H are not documented in man pages, could you document them?
Please see some minor comments inline:
Fixed everything, except:
- ret = sysdb_set_entry_attr(domain->sysdb, dn, attrs, mod_op);
We should check return value here, right?
We want to return this value directly, there's no need to check it (other sysdb_set_*_attr functions do this as well).
Updated patch attached.
Thanks,
<varlistentry condition="with_ssh">
<term>
<option>-h</option>,<option>--ssh-host</option>
- <replaceable>autofs-map</replaceable>
^^^^^
copy&paste mistake?
</term>
<listitem>
<para>
Invalidate SSH public keys of a specific host.
</para>
</listitem>
</varlistentry>
Could you also add ticket number (#2358) to the commit message? Thanks!
Honza, we have a template in git repo ".git-commit-template"
For details see commit 3d9edb4c510028def2df41aa7b0ce705b197e6fc
LS
Dne 2.9.2014 v 16:37 Lukas Slebodnik napsal(a):
On (02/09/14 16:21), Pavel Reichl wrote:
On 09/01/2014 02:27 PM, Pavel Reichl wrote:
On 08/29/2014 10:41 AM, Jan Cholasta wrote:
Dne 28.8.2014 v 18:11 Pavel Reichl napsal(a):
On 08/25/2014 02:05 PM, Jan Cholasta wrote:
Dne 4.8.2014 v 19:34 Pavel Reichl napsal(a): > > On 07/21/2014 02:08 PM, Jakub Hrozek wrote: >> On Mon, Jul 21, 2014 at 01:55:20PM +0200, Jan Cholasta wrote: >>> On 18.7.2014 06:41, William wrote: >>>>>> Ignore that last patch, I messed up and didn't include a >>>>>> .h file. >>>>>> Here >>>>>> is the fixed patch. >>>>> This new patch seems to be based on some older version of the >>>>> patch, it >>>>> uses wrong option name, sysdb_update_ssh_host still has the >>>>> confdb >>>>> argument, etc. >>>>> >>>> Fixed, and fixed the documentation into the xml file, removed the >>>> references to the .pot file. >>> I don't see any of the requested fixes in the patch. >>> >>> See attachment for fixed patch. I also did a few additional >>> tweaks to >>> make >>> the patch work when SSSD is updated from a previous version and >>> when >>> building without SSH bits. >> Awesome thank you, I guess some of us need to review the patch now >> you're also an author. > While Jan was so kind to give me some hints on how to test patch > properly, we found out a little bug. So Jan is going to prepare an > updated patch. > > I'll do the review then.
I can't reproduce the issue, maybe something else was wrong in your setup?
For the record, I installed IPA in a VM and then ran these commands on it:
$ sss_ssh_knownhostsproxy --debug 10 $HOSTNAME [no errors] $ cat /var/lib/sss/pubconf/known_hosts [not empty] # systemctl stop ipa $ sss_ssh_knownhostsproxy --debug 10 $HOSTNAME [no errors] $ cat /var/lib/sss/pubconf/known_hosts [not empty] # sss_cache -H $ sss_ssh_knownhostsproxy --debug 10 $HOSTNAME [no errors] $ cat /var/lib/sss/pubconf/known_hosts [empty] # systemctl start ipa $ sss_ssh_knownhostsproxy --debug 10 $HOSTNAME [no errors] $ cat /var/lib/sss/pubconf/known_hosts [not empty]
Pavel had an empty /var/lib/sss/pubconf/known_hosts in the last step, I have it non-empty as expected.
(sorry for the delay, I was on leave)
This test ran OK this time, so I think you are right.
I have some comments to the patch itself.
I'm not personally fan of big patches like this one. Could you at least separate the man page changes to separate patch - so we can ask some native speaker to review it.
Also new options in sss_cache -h and -H are not documented in man pages, could you document them?
Please see some minor comments inline:
Fixed everything, except:
- ret = sysdb_set_entry_attr(domain->sysdb, dn, attrs, mod_op);
We should check return value here, right?
We want to return this value directly, there's no need to check it (other sysdb_set_*_attr functions do this as well).
Updated patch attached.
Thanks,
<varlistentry condition="with_ssh">
<term>
<option>-h</option>,<option>--ssh-host</option>
- <replaceable>autofs-map</replaceable>
^^^^^
copy&paste mistake?
Fixed.
</term>
<listitem>
<para>
Invalidate SSH public keys of a specific host.
</para>
</listitem>
</varlistentry>
Could you also add ticket number (#2358) to the commit message? Thanks!
I wasn't aware there is a ticket for this.
Honza, we have a template in git repo ".git-commit-template"
For details see commit 3d9edb4c510028def2df41aa7b0ce705b197e6fc
OK, thanks.
LS
Updated patch attached.
On 09/03/2014 02:40 PM, Jan Cholasta wrote:
Dne 2.9.2014 v 16:37 Lukas Slebodnik napsal(a):
On (02/09/14 16:21), Pavel Reichl wrote:
On 09/01/2014 02:27 PM, Pavel Reichl wrote:
On 08/29/2014 10:41 AM, Jan Cholasta wrote:
Dne 28.8.2014 v 18:11 Pavel Reichl napsal(a):
On 08/25/2014 02:05 PM, Jan Cholasta wrote: > Dne 4.8.2014 v 19:34 Pavel Reichl napsal(a): >> >> On 07/21/2014 02:08 PM, Jakub Hrozek wrote: >>> On Mon, Jul 21, 2014 at 01:55:20PM +0200, Jan Cholasta wrote: >>>> On 18.7.2014 06:41, William wrote: >>>>>>> Ignore that last patch, I messed up and didn't include a >>>>>>> .h file. >>>>>>> Here >>>>>>> is the fixed patch. >>>>>> This new patch seems to be based on some older version of the >>>>>> patch, it >>>>>> uses wrong option name, sysdb_update_ssh_host still has the >>>>>> confdb >>>>>> argument, etc. >>>>>> >>>>> Fixed, and fixed the documentation into the xml file, >>>>> removed the >>>>> references to the .pot file. >>>> I don't see any of the requested fixes in the patch. >>>> >>>> See attachment for fixed patch. I also did a few additional >>>> tweaks to >>>> make >>>> the patch work when SSSD is updated from a previous version and >>>> when >>>> building without SSH bits. >>> Awesome thank you, I guess some of us need to review the patch >>> now >>> you're also an author. >> While Jan was so kind to give me some hints on how to test patch >> properly, we found out a little bug. So Jan is going to prepare an >> updated patch. >> >> I'll do the review then. > > I can't reproduce the issue, maybe something else was wrong in your > setup? > > For the record, I installed IPA in a VM and then ran these > commands on > it: > > $ sss_ssh_knownhostsproxy --debug 10 $HOSTNAME > [no errors] > $ cat /var/lib/sss/pubconf/known_hosts > [not empty] > # systemctl stop ipa > $ sss_ssh_knownhostsproxy --debug 10 $HOSTNAME > [no errors] > $ cat /var/lib/sss/pubconf/known_hosts > [not empty] > # sss_cache -H > $ sss_ssh_knownhostsproxy --debug 10 $HOSTNAME > [no errors] > $ cat /var/lib/sss/pubconf/known_hosts > [empty] > # systemctl start ipa > $ sss_ssh_knownhostsproxy --debug 10 $HOSTNAME > [no errors] > $ cat /var/lib/sss/pubconf/known_hosts > [not empty] > > Pavel had an empty /var/lib/sss/pubconf/known_hosts in the last > step, > I have it non-empty as expected. > > (sorry for the delay, I was on leave) > This test ran OK this time, so I think you are right.
I have some comments to the patch itself.
I'm not personally fan of big patches like this one. Could you at least separate the man page changes to separate patch - so we can ask some native speaker to review it.
Also new options in sss_cache -h and -H are not documented in man pages, could you document them?
Please see some minor comments inline:
Fixed everything, except:
> + ret = sysdb_set_entry_attr(domain->sysdb, dn, attrs, mod_op);
We should check return value here, right?
We want to return this value directly, there's no need to check it (other sysdb_set_*_attr functions do this as well).
Updated patch attached.
Thanks,
<varlistentry condition="with_ssh">
<term>
<option>-h</option>,<option>--ssh-host</option>
- <replaceable>autofs-map</replaceable>
^^^^^
copy&paste mistake?
Fixed.
</term>
<listitem>
<para>
Invalidate SSH public keys of a specific
host.
</para>
</listitem>
</varlistentry>
Could you also add ticket number (#2358) to the commit message? Thanks!
I wasn't aware there is a ticket for this.
Neither did I till yesterday's evening.
Honza, we have a template in git repo ".git-commit-template"
For details see commit 3d9edb4c510028def2df41aa7b0ce705b197e6fc
OK, thanks.
LS
Updated patch attached.
Thanks, ACK
sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
sssd-devel@lists.fedorahosted.org