Hi,
this patch set changes the way the known_hosts file is updated so that only entries for hosts that were requested recently (in the last 5 minutes) are written to the file. There is no need to keep older entries in the file, as host keys are needed only before the connection to the host is established (which usually takes just a few seconds).
The individual patches are:
[PATCH 1/3] DB: Add function for deleting values from sysdb_attrs
[PATCH 2/3] SSH: Refactor sysdb code
[PATCH 3/3] SSH: Expire hosts in known_hosts
Honza
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On Wed 19 Sep 2012 06:09:59 AM EDT, Jan Cholasta wrote:
Hi,
this patch set changes the way the known_hosts file is updated so that only entries for hosts that were requested recently (in the last 5 minutes) are written to the file. There is no need to keep older entries in the file, as host keys are needed only before the connection to the host is established (which usually takes just a few seconds).
I don't think we want to be deleting entries from the sysdb for any reason other than that they were deleted on the LDAP server. We'll be behaving poorly if we get into a situation where LDAP is unreachable but we can still reach the SSH server.
The individual patches are:
[PATCH 1/3] DB: Add function for deleting values from sysdb_attrs
[PATCH 2/3] SSH: Refactor sysdb code
[PATCH 3/3] SSH: Expire hosts in known_hosts
Honza
N�n�r����)em�h�yhiם�w^��
Hi,
Dne 19.9.2012 20:01, Stephen Gallagher napsal(a):
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On Wed 19 Sep 2012 06:09:59 AM EDT, Jan Cholasta wrote:
Hi,
this patch set changes the way the known_hosts file is updated so that only entries for hosts that were requested recently (in the last 5 minutes) are written to the file. There is no need to keep older entries in the file, as host keys are needed only before the connection to the host is established (which usually takes just a few seconds).
I don't think we want to be deleting entries from the sysdb for any reason other than that they were deleted on the LDAP server. We'll be behaving poorly if we get into a situation where LDAP is unreachable but we can still reach the SSH server.
I'm not exactly sure how did you came to the conslusion that anything is being deleted from sysdb, but rest assured that it is not.
The individual patches are:
[PATCH 1/3] DB: Add function for deleting values from sysdb_attrs
[PATCH 2/3] SSH: Refactor sysdb code
[PATCH 3/3] SSH: Expire hosts in known_hosts
Honza
Honza
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
- -----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On Wed 19 Sep 2012 06:09:59 AM EDT, Jan Cholasta wrote:
Hi,
this patch set changes the way the known_hosts file is updated so that only entries for hosts that were requested recently (in the last 5 minutes) are written to the file. There is no need to keep older entries in the file, as host keys are needed only before the connection to the host is established (which usually takes just a few seconds).
I don't think we want to be deleting entries from the sysdb for any reason other than that they were deleted on the LDAP server. We'll be behaving poorly if we get into a situation where LDAP is unreachable but we can still reach the SSH server.
The individual patches are:
[PATCH 1/3] DB: Add function for deleting values from sysdb_attrs
[PATCH 2/3] SSH: Refactor sysdb code
[PATCH 3/3] SSH: Expire hosts in known_hosts
Honza
N�n�r����)em�h�yhiם�w^��
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
- -----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1
On Wed 19 Sep 2012 06:09:59 AM EDT, Jan Cholasta wrote:
Hi,
this patch set changes the way the known_hosts file is updated so that only entries for hosts that were requested recently (in the last 5 minutes) are written to the file. There is no need to keep older entries in the file, as host keys are needed only before the connection to the host is established (which usually takes just a few seconds).
I don't think we want to be deleting entries from the sysdb for any reason other than that they were deleted on the LDAP server. We'll be behaving poorly if we get into a situation where LDAP is unreachable but we can still reach the SSH server.
The individual patches are:
[PATCH 1/3] DB: Add function for deleting values from sysdb_attrs
[PATCH 2/3] SSH: Refactor sysdb code
[PATCH 3/3] SSH: Expire hosts in known_hosts
Honza
N�n�r����)em�h�yhiם�w^��
On 09/19/2012 12:09 PM, Jan Cholasta wrote:
Hi,
this patch set changes the way the known_hosts file is updated so that only entries for hosts that were requested recently (in the last 5 minutes) are written to the file. There is no need to keep older entries in the file, as host keys are needed only before the connection to the host is established (which usually takes just a few seconds).
The individual patches are:
[PATCH 1/3] DB: Add function for deleting values from sysdb_attrs
Nack. Can you make new copy of attrs without the deleted attributes instead of touching the original attrs?
[PATCH 2/3] SSH: Refactor sysdb code
Nack.
sysdb_update_ssh_host(), sysdb_store_ssh_host(): Is there any advantage to allow the input attrs parameter to be NULL? Currently there is no code flow how this could happened (except sysdb test) so I would simply prohibit it and make the code simpler.
sysdb_update_ssh_host(): What is the purpose of deleting OC and NAME attributes and then adding them again?
[PATCH 3/3] SSH: Expire hosts in known_hosts
Ack.
Dne 24.9.2012 15:03, Pavel Březina napsal(a):
On 09/19/2012 12:09 PM, Jan Cholasta wrote:
Hi,
this patch set changes the way the known_hosts file is updated so that only entries for hosts that were requested recently (in the last 5 minutes) are written to the file. There is no need to keep older entries in the file, as host keys are needed only before the connection to the host is established (which usually takes just a few seconds).
The individual patches are:
[PATCH 1/3] DB: Add function for deleting values from sysdb_attrs
Nack. Can you make new copy of attrs without the deleted attributes instead of touching the original attrs?
Sure.
[PATCH 2/3] SSH: Refactor sysdb code
Nack.
sysdb_update_ssh_host(), sysdb_store_ssh_host(): Is there any advantage to allow the input attrs parameter to be NULL? Currently there is no code flow how this could happened (except sysdb test) so I would simply prohibit it and make the code simpler.
I believe that was a copy & paste from some other sysdb function, you are probably right that it can be safely removed.
sysdb_update_ssh_host(): What is the purpose of deleting OC and NAME attributes and then adding them again?
ldb complains when the same value is present more than once in a single attribute (such as name) and refuses to store the entry.
[PATCH 3/3] SSH: Expire hosts in known_hosts
Ack.
Honza
Hi,
Dne 24.9.2012 15:56, Jan Cholasta napsal(a):
Dne 24.9.2012 15:03, Pavel Březina napsal(a):
On 09/19/2012 12:09 PM, Jan Cholasta wrote:
Hi,
this patch set changes the way the known_hosts file is updated so that only entries for hosts that were requested recently (in the last 5 minutes) are written to the file. There is no need to keep older entries in the file, as host keys are needed only before the connection to the host is established (which usually takes just a few seconds).
The individual patches are:
[PATCH 1/3] DB: Add function for deleting values from sysdb_attrs
Nack. Can you make new copy of attrs without the deleted attributes instead of touching the original attrs?
Sure.
It turns out I don't need this function anymore, so this patch is removed.
[PATCH 2/3] SSH: Refactor sysdb code
Nack.
sysdb_update_ssh_host(), sysdb_store_ssh_host(): Is there any advantage to allow the input attrs parameter to be NULL? Currently there is no code flow how this could happened (except sysdb test) so I would simply prohibit it and make the code simpler.
I believe that was a copy & paste from some other sysdb function, you are probably right that it can be safely removed.
Done.
sysdb_update_ssh_host(): What is the purpose of deleting OC and NAME attributes and then adding them again?
ldb complains when the same value is present more than once in a single attribute (such as name) and refuses to store the entry.
I have taken a slightly different approach here: instead of deleting unwanted attributed in sysdb functions, I copy only host keys from the LDAP result in hostid provider (the rest of attributes is not interesting from SSH perspective anyway).
[PATCH 3/3] SSH: Expire hosts in known_hosts
Ack.
Added a new configuration option ssh_known_hosts_timeout, which can be used to specify the timeout of known_hosts entries in seconds. Also added a sysdb upgrade routine so that this patch works for existing installs as well.
Honza
Honza
On 10/01/2012 10:23 AM, Jan Cholasta wrote:
Hi,
Dne 24.9.2012 15:56, Jan Cholasta napsal(a):
Dne 24.9.2012 15:03, Pavel Březina napsal(a):
On 09/19/2012 12:09 PM, Jan Cholasta wrote:
Hi,
this patch set changes the way the known_hosts file is updated so that only entries for hosts that were requested recently (in the last 5 minutes) are written to the file. There is no need to keep older entries in the file, as host keys are needed only before the connection to the host is established (which usually takes just a few seconds).
The individual patches are:
[PATCH 1/3] DB: Add function for deleting values from sysdb_attrs
Nack. Can you make new copy of attrs without the deleted attributes instead of touching the original attrs?
Sure.
It turns out I don't need this function anymore, so this patch is removed.
[PATCH 2/3] SSH: Refactor sysdb code
Nack.
sysdb_update_ssh_host(), sysdb_store_ssh_host(): Is there any advantage to allow the input attrs parameter to be NULL? Currently there is no code flow how this could happened (except sysdb test) so I would simply prohibit it and make the code simpler.
I believe that was a copy & paste from some other sysdb function, you are probably right that it can be safely removed.
Done.
sysdb_update_ssh_host(): What is the purpose of deleting OC and NAME attributes and then adding them again?
ldb complains when the same value is present more than once in a single attribute (such as name) and refuses to store the entry.
I have taken a slightly different approach here: instead of deleting unwanted attributed in sysdb functions, I copy only host keys from the LDAP result in hostid provider (the rest of attributes is not interesting from SSH perspective anyway).
[PATCH 3/3] SSH: Expire hosts in known_hosts
Ack.
Added a new configuration option ssh_known_hosts_timeout, which can be used to specify the timeout of known_hosts entries in seconds. Also added a sysdb upgrade routine so that this patch works for existing installs as well.
Nack (nitpick). sysdb_ssh.c:125 - you have "if (alias)" inside "if (alias)". Can you put a comment there that would say that the value may change during execution? Either that or you can use a boolean variable that would indicate present of the alias in attrs. Just make sure that it is clear from the first sight...
Otherwise ack. Good job!
Hi,
On 1.10.2012 19:54, Pavel Březina wrote:
On 10/01/2012 10:23 AM, Jan Cholasta wrote:
Hi,
Dne 24.9.2012 15:56, Jan Cholasta napsal(a):
Dne 24.9.2012 15:03, Pavel Březina napsal(a):
On 09/19/2012 12:09 PM, Jan Cholasta wrote:
Hi,
this patch set changes the way the known_hosts file is updated so that only entries for hosts that were requested recently (in the last 5 minutes) are written to the file. There is no need to keep older entries in the file, as host keys are needed only before the connection to the host is established (which usually takes just a few seconds).
The individual patches are:
[PATCH 1/3] DB: Add function for deleting values from sysdb_attrs
Nack. Can you make new copy of attrs without the deleted attributes instead of touching the original attrs?
Sure.
It turns out I don't need this function anymore, so this patch is removed.
[PATCH 2/3] SSH: Refactor sysdb code
Nack.
sysdb_update_ssh_host(), sysdb_store_ssh_host(): Is there any advantage to allow the input attrs parameter to be NULL? Currently there is no code flow how this could happened (except sysdb test) so I would simply prohibit it and make the code simpler.
I believe that was a copy & paste from some other sysdb function, you are probably right that it can be safely removed.
Done.
sysdb_update_ssh_host(): What is the purpose of deleting OC and NAME attributes and then adding them again?
ldb complains when the same value is present more than once in a single attribute (such as name) and refuses to store the entry.
I have taken a slightly different approach here: instead of deleting unwanted attributed in sysdb functions, I copy only host keys from the LDAP result in hostid provider (the rest of attributes is not interesting from SSH perspective anyway).
[PATCH 3/3] SSH: Expire hosts in known_hosts
Ack.
Added a new configuration option ssh_known_hosts_timeout, which can be used to specify the timeout of known_hosts entries in seconds. Also added a sysdb upgrade routine so that this patch works for existing installs as well.
Nack (nitpick). sysdb_ssh.c:125 - you have "if (alias)" inside "if (alias)". Can you put a comment there that would say that the value may change during execution? Either that or you can use a boolean variable that would indicate present of the alias in attrs. Just make sure that it is clear from the first sight...
Sure.
Otherwise ack. Good job!
Thanks.
Updated & rebased patches attached.
Honza
On 10/02/2012 12:30 PM, Jan Cholasta wrote:
Hi,
On 1.10.2012 19:54, Pavel Březina wrote:
On 10/01/2012 10:23 AM, Jan Cholasta wrote:
Hi,
Dne 24.9.2012 15:56, Jan Cholasta napsal(a):
Dne 24.9.2012 15:03, Pavel Březina napsal(a):
On 09/19/2012 12:09 PM, Jan Cholasta wrote:
Hi,
this patch set changes the way the known_hosts file is updated so that only entries for hosts that were requested recently (in the last 5 minutes) are written to the file. There is no need to keep older entries in the file, as host keys are needed only before the connection to the host is established (which usually takes just a few seconds).
The individual patches are:
[PATCH 1/3] DB: Add function for deleting values from sysdb_attrs
Nack. Can you make new copy of attrs without the deleted attributes instead of touching the original attrs?
Sure.
It turns out I don't need this function anymore, so this patch is removed.
[PATCH 2/3] SSH: Refactor sysdb code
Nack.
sysdb_update_ssh_host(), sysdb_store_ssh_host(): Is there any advantage to allow the input attrs parameter to be NULL? Currently there is no code flow how this could happened (except sysdb test) so I would simply prohibit it and make the code simpler.
I believe that was a copy & paste from some other sysdb function, you are probably right that it can be safely removed.
Done.
sysdb_update_ssh_host(): What is the purpose of deleting OC and NAME attributes and then adding them again?
ldb complains when the same value is present more than once in a single attribute (such as name) and refuses to store the entry.
I have taken a slightly different approach here: instead of deleting unwanted attributed in sysdb functions, I copy only host keys from the LDAP result in hostid provider (the rest of attributes is not interesting from SSH perspective anyway).
[PATCH 3/3] SSH: Expire hosts in known_hosts
Ack.
Added a new configuration option ssh_known_hosts_timeout, which can be used to specify the timeout of known_hosts entries in seconds. Also added a sysdb upgrade routine so that this patch works for existing installs as well.
Nack (nitpick). sysdb_ssh.c:125 - you have "if (alias)" inside "if (alias)". Can you put a comment there that would say that the value may change during execution? Either that or you can use a boolean variable that would indicate present of the alias in attrs. Just make sure that it is clear from the first sight...
Sure.
Otherwise ack. Good job!
Thanks.
Updated & rebased patches attached.
Honza
Ack. Good job.
sssd-devel@lists.fedorahosted.org
-
Jakub Hrozek -
Jan Cholasta -
Pavel Březina -
Stephen Gallagher