From 9f8357ada726235533061e35a97b44abd5ec60cd Mon Sep 17 00:00:00 2001
From: Arnav Singh <me@arnavion.dev>
Date: Fri, 17 Mar 2023 04:44:19 -0700
Subject: [PATCH] sftp: fix using key_use_agent and key_file together needing
 private key file

When using ssh-agent to hold multiple keys, it is common practice to configure
openssh to use a specific key by setting the corresponding public key as
the `IdentityFile`. This change makes a similar behavior possible in rclone
by having it parse the `key_file` config as the public key when
`key_use_agent` is `true`.

rclone already attempted this behavior before this change, but it assumed that
`key_file` is the private key and that the public key is specified in
`${key_file}.pub`. So for parity with the openssh behavior, this change makes
rclone first attempt to read the public key from `${key_file}.pub` as before
(for the sake of backward compatibility), then fall back to reading it from
`key_file`.

Fixes #6791
---
 backend/sftp/sftp.go | 29 ++++++++++++++++++++++++++---
 1 file changed, 26 insertions(+), 3 deletions(-)

diff --git a/backend/sftp/sftp.go b/backend/sftp/sftp.go
index c998180c7..86763ce21 100644
--- a/backend/sftp/sftp.go
+++ b/backend/sftp/sftp.go
@@ -10,6 +10,7 @@ import (
 	"errors"
 	"fmt"
 	"io"
+	iofs "io/fs"
 	"os"
 	"path"
 	"regexp"
@@ -782,10 +783,32 @@ func NewFs(ctx context.Context, name, root string, m configmap.Mapper) (fs.Fs, e
 			return nil, fmt.Errorf("couldn't read ssh agent signers: %w", err)
 		}
 		if keyFile != "" {
+			// If `opt.KeyUseAgent` is false, then it's expected that `opt.KeyFile` contains the private key
+			// and `${opt.KeyFile}.pub` contains the public key.
+			//
+			// If `opt.KeyUseAgent` is true, then it's expected that `opt.KeyFile` contains the public key.
+			// This is how it works with openssh; the `IdentityFile` in openssh config points to the public key.
+			// It's not necessary to specify the public key explicitly when using ssh-agent, since openssh and rclone
+			// will try all the keys they find in the ssh-agent until they find one that works. But just like
+			// `IdentityFile` is used in openssh config to limit the search to one specific key, so does
+			// `opt.KeyFile` in rclone config limit the search to that specific key.
+			//
+			// However, previous versions of rclone would always expect to find the public key in
+			// `${opt.KeyFile}.pub` even if `opt.KeyUseAgent` was true. So for the sake of backward compatibility
+			// we still first attempt to read the public key from `${opt.KeyFile}.pub`. But if it fails with
+			// an `fs.ErrNotExist` then we also try to read the public key from `opt.KeyFile`.
 			pubBytes, err := os.ReadFile(keyFile + ".pub")
 			if err != nil {
-				return nil, fmt.Errorf("failed to read public key file: %w", err)
+				if errors.Is(err, iofs.ErrNotExist) && opt.KeyUseAgent {
+					pubBytes, err = os.ReadFile(keyFile)
+					if err != nil {
+						return nil, fmt.Errorf("failed to read public key file: %w", err)
+					}
+				} else {
+					return nil, fmt.Errorf("failed to read public key file: %w", err)
+				}
 			}
+
 			pub, _, _, _, err := ssh.ParseAuthorizedKey(pubBytes)
 			if err != nil {
 				return nil, fmt.Errorf("failed to parse public key file: %w", err)
@@ -807,8 +830,8 @@ func NewFs(ctx context.Context, name, root string, m configmap.Mapper) (fs.Fs, e
 		}
 	}
 
-	// Load key file if specified
-	if keyFile != "" || opt.KeyPem != "" {
+	// Load key file as a private key, if specified. This is only needed when not using an ssh agent.
+	if (keyFile != "" && !opt.KeyUseAgent) || opt.KeyPem != "" {
 		var key []byte
 		if opt.KeyPem == "" {
 			key, err = os.ReadFile(keyFile)