openssh unix dev - Feb 2018 - Attempts to connect to Axway SFTP server result in publickey auth loopin

On Fri, Feb 23, 2018 at 05:01:00PM +1100, Darren Tucker
wrote:
> You could try this patch which defers resetting the "tried" flag
on the
> pubkeys until the list of authentication methods changes.  I don't have
> a server with this behaviour so I'm not sure if it helps (and I'm
not
> sure it's the right thing to do anyway).
I think this is a better way to handle it: keep track of the signatures
sent and mark the successful one to not be used again.  This seems to
behave as expected against a server hacked up to behave more or less as
you describe.

diff --git a/sshconnect2.c b/sshconnect2.c
index 8138e46..3f475d9 100644
--- a/sshconnect2.c
+++ b/sshconnect2.c
@@ -246,6 +246,7 @@ typedef struct cauthmethod Authmethod;
 typedef struct identity Identity;
 typedef struct idlist Idlist;
 
+#define IDENTITY_SUCCESSFUL 0x1000
 struct identity {
 	TAILQ_ENTRY(identity) next;
 	int	agent_fd;		/* >=0 if agent supports key */
@@ -268,6 +269,7 @@ struct cauthctxt {
 	int attempt;
 	/* pubkey */
 	struct idlist keys;
+	struct identity *sent_signed_id;
 	int agent_fd;
 	/* hostbased */
 	Sensitive *sensitive;
@@ -562,6 +564,11 @@ input_userauth_failure(int type, u_int32_t seq, struct ssh
*ssh)
 
 	if (partial != 0) {
 		verbose("Authenticated with partial success.");
+		if (authctxt->sent_signed_id != NULL) {
+			debug3("Marking key %s as successful",
+			    authctxt->sent_signed_id->filename);
+			authctxt->sent_signed_id->tried = IDENTITY_SUCCESSFUL;
+		}
 		/* reset state */
 		pubkey_reset(authctxt);
 	}
@@ -1168,6 +1175,7 @@ sign_and_send_pubkey(Authctxt *authctxt, Identity *id)
 	packet_put_raw(buffer_ptr(&b), buffer_len(&b));
 	buffer_free(&b);
 	packet_send();
+	authctxt->sent_signed_id = id;
 
 	return 1;
 }
@@ -1422,6 +1430,7 @@ pubkey_cleanup(Authctxt *authctxt)
 		free(id->filename);
 		free(id);
 	}
+	authctxt->sent_signed_id = NULL;
 }
 
 static void
@@ -1430,7 +1439,10 @@ pubkey_reset(Authctxt *authctxt)
 	Identity *id;
 
 	TAILQ_FOREACH(id, &authctxt->keys, next)
-		id->tried = 0;
+		if (id->tried != IDENTITY_SUCCESSFUL)
+			id->tried = 0;
+	authctxt->sent_signed_id = NULL;
+
 }
 
 static int

-- 
Darren Tucker (dtucker at dtucker.net)
GPG key 11EAA6FA / A86E 3E07 5B19 5880 E860  37F4 9357 ECEF 11EA A6FA (new)
    Good judgement comes with experience. Unfortunately, the experience
usually comes from bad judgement.

> +	struct identity *sent_signed_id;
What happens if the server is configured to allow two different SSH 
keys? Wouldn't that then cycle between these two?

On 23 February 2018 at 23:07, Philipp Marek <philipp at marek.priv.at>
wrote:
>
>> +       struct identity *sent_signed_id;
>
> What happens if the server is configured to allow two different SSH keys?
> Wouldn't that then cycle between these two?
I don't think so.  I think once both have succeeded neither will be
sent and it'll drop through to "we did not send a packet, disable
method" and either move to the next method or fail if there are no
more.

sent_signed_id is used to track the key that was just used to sign the
challenge from the server.  The state of the keys is stored in a list
of Identity structures.

When the reply comes back after setting sent_signed_id there's 3 cases:
 - failure: no change in behaviour.  (maybe it should null out
sent_signed_id, although it should be set again before ever being
read).
 - partial success: we mark id->tried with IDENTITY_SUCCESSFUL.  The
non-zero value stops it from being sent again.
 - complete success: ssh_userauth2 immediately cleans up the identities.

-- 
Darren Tucker (dtucker at dtucker.net)
GPG key 11EAA6FA / A86E 3E07 5B19 5880 E860  37F4 9357 ECEF 11EA A6FA (new)
    Good judgement comes with experience. Unfortunately, the experience
usually comes from bad judgement.