On Mon, Feb 27, 2012 at 11:07:33AM +0100, Jan Cholasta wrote:
On 27.2.2012 10:46, Jan Cholasta wrote:
>On 27.2.2012 02:39, Stephen Gallagher wrote:
>>On Sun, 2012-02-26 at 21:12 +0100, Jan Cholasta wrote:
>>>Hi,
>>>
>>>I have updated and rebased the patches on top of current master.
>>>
>>>On 24.2.2012 20:35, Stephen Gallagher wrote:
>>>>On Fri, 2012-02-24 at 19:10 +0100, Jan Cholasta wrote:
>>>>>Hi,
>>>>>
>>>>>this patchset contains these patches:
>>>>>
>>>>>[PATCH 1/3] SSH: Save SSH host name aliases
>>>>>
>>>>>This is needed in order to properly generate the known_hosts file
when
>>>>>user uses ssh with non-FQDN host name.
>>>>>
>>>>
>>>>I fixed up a few minor issues with this patch. The check for
>>>>in_transaction should not have been conditional on ret (in the current
>>>>code it was fine, but defensively it only needs to check for
>>>>in_transaction).
>>>>
>>>>It's against our coding policy to mix pointers to structs and plain
>>>>structs in the same variable declaration, so I split
>>>>+ struct addrinfo ai_hint;
>>>>+ struct addrinfo *ai = NULL;
>>>>for clarity.
>>>>
>>>>You forgot to check for memory allocation error in
>>>>sss_ssh_cmd_get_host_pubkeys() when creating the new primary name. I
>>>>added it.
>>>>
>>>>And I moved the SSS_SSH_KNOWN_HOSTS_PATH and
>>>>SSS_SSH_KNOWN_HOSTS_TEMP_TMPL define to patch 0003 where it belongs.
>>>
>>>Thanks.
>>>
>>>>
>>>>Functionally, this is fine, so ack with my changes (attached).
>>>
>>>I have done this additional little change in ipa_hostid.c:
>>>
>>>@@ -227,8 +231,7 @@ hosts_get_connect_done(struct tevent_req *subreq)
>>>subreq = ipa_host_info_send(state, state->ev, state->sysdb,
>>>sdap_id_op_handle(state->op),
>>>state->ctx->sdap_id_ctx->opts,
>>>state->name,
>>>- state->ctx->ipa_opts->host_map,
>>>- state->ctx->ipa_opts->hostgroup_map,
>>>+ state->ctx->ipa_opts->host_map, NULL,
>>>state->ctx->host_search_bases);
>>>if (!subreq) {
>>>tevent_req_error(req, ENOMEM);
>>>
>>>>
>>>>>
>>>>>[PATCH 2/3] SSH: Move responder and client common code to util
>>>>>
>>>>>Don't manage user-specific known_hosts file in
>>>>>sss_ssh_knownhostsproxy.
>>>>>
>>>>>Continue connecting to SSH server even when SSSD is not running in
>>>>>sss_ssh_knownhostsproxy and don't drop the connection when the
>>>>>sss_ssh_knownhostsproxy process receives a signal.
>>>>>
>>>>>https://fedorahosted.org/sssd/ticket/1179
>>>>>https://fedorahosted.org/sssd/ticket/1184
>>>>>
>>>>
>>>>I'm not comfortable enough in this code to review it. I'd prefer
to
>>>>have
>>>>Jakub's sign-off here.
>>>>
>>>>It would also be really nice if you could split this into multiple
>>>>logical patches, because it's really overwhelming.
>>>
>>>Done, I have split this patch into 3 patches:
>>>
>>>[PATCH 2/5] SSH: Refactor responder and client common code
>>
>>>[PATCH 3/5] UTIL: Add function for atomic I/O
>>>[PATCH 4/5] SSH: Continue connecting to SSH server even when SSSD is not
>>>running in sss_ssh_knownhostsproxy
>>>
>>>Additionally, don't drop the connection when the sss_ssh_knownhostsproxy
>>>process receives a signal.
>>>
>>>https://fedorahosted.org/sssd/ticket/1179
>>>https://fedorahosted.org/sssd/ticket/1184
>>>
>>>>
>>>>>
>>>>>[PATCH 3/3] SSH: Manage global known_hosts file in the responder
>>>>>
>>>>>https://fedorahosted.org/sssd/ticket/1193
>>>>>
>>>>>The known_hosts file is stored in /var/lib/sss/pubconf/known_hosts.
>>>>
>>>>
>>>>Nack
>>>>You need to wrap the write() in a loop to make sure it writes
>>>>completely. It also needs to loop on EINTR.
>>>
>>>Fixed.
>>>
>>>I have also moved the "don't manage user-specific known_hosts"
bit of
>>>the previous patch to this patch.
>>>
>>>Honza
>>>
>>
>>
>>
>>Patch 0001: Ack
>>
>>Patch 0002: Ack
>>
>>Patch 0003: Full ack, and please open a ticket to have us convert all of
>>our read and write loops to use this function.
>
>Done, see
https://fedorahosted.org/sssd/ticket/1209
>
>>
>>Patch 0004: Ack
>>
>>Patch 0005: Ack, with one caveat. Please submit a patch to correct the
>>manpage as to the proper option to set for GlobalKnownHosts. I want this
>>as a separate patch because we're in string freeze and we may decide to
>>only carry this change in master (and release-note the doc bug). In that
>>patch, please also add rsc/sss_client/ssh/* to po/POTFILES.in, since you
>>added translatable error message strings.
>
>There's one more thing: The --file option of sss_ssh_knownhostsproxy
>should be removed, because it is unused now that a global known_hosts is
>managed by SSSD. Can I remove it from the code in both master and
>sssd_1_8? Can I remove it from the documentation in both master and
>sssd_1_8?
Removing it from documentation is OK because I found out that we have a
Makefile.am bug that prevented us from getting any translations
whatsoever on manual pages added since the original translation work
(....).
Removing it from code should be also OK, at worst we get a translatable
string noone is going to use so we waste translator's work..but we would
waste it anyway when we remove the option during next release.
>
>>
>>
>>Pushed all five patches to master. Please provide a rebased set of
>>patches for sssd-1-8. This batch must currently be applied atop Jan's
>>ipa_host refactoring patch (which is master-only).
>
>Done, see attachment.
>
>
>I have also created 2 new patches:
>
>[PATCH 1/2] SSH: Don't abort known_hosts update when host search fails
>
Ack
>[PATCH 2/2] SSH: Add more debugging messages
Sent wrong version of the patch, see attachment for the correct one.
Maybe you should also add more DEBUG messages to src/util/sss_ssh.c
>
>Both should be applied on both master and sssd_1_8.
>
>
>Honza
>
--
Jan Cholasta