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?
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
[PATCH 2/2] SSH: Add more debugging messages
Both should be applied on both master and sssd_1_8.
Honza
--
Jan Cholasta