From cc6cad765083a8de5e2899d52206ad06be332b6c Mon Sep 17 00:00:00 2001 From: David Juhasz Date: Thu, 4 Jan 2024 09:21:05 -0800 Subject: [PATCH 1/2] Don't retry SSH connection when auth fails Authentication failures can not be resolved by retrying the connection with the same credentials. --- internal/am/upload_transfer.go | 10 +++++- internal/am/upload_transfer_test.go | 48 ++++++++++++++++++++++------- internal/sftp/client.go | 16 ++++++++++ internal/sftp/goclient.go | 2 +- internal/sftp/goclient_test.go | 29 +++++++++++------ internal/sftp/ssh.go | 23 ++++++++------ 6 files changed, 97 insertions(+), 31 deletions(-) diff --git a/internal/am/upload_transfer.go b/internal/am/upload_transfer.go index a0077a602..5597aed64 100644 --- a/internal/am/upload_transfer.go +++ b/internal/am/upload_transfer.go @@ -7,6 +7,7 @@ import ( "path/filepath" "github.com/go-logr/logr" + "go.artefactual.dev/tools/temporal" "github.com/artefactual-sdps/enduro/internal/sftp" ) @@ -46,7 +47,14 @@ func (a *UploadTransferActivity) Execute(ctx context.Context, params *UploadTran filename := filepath.Base(params.SourcePath) bytes, path, err := a.client.Upload(ctx, src, filename) if err != nil { - return nil, fmt.Errorf("%s: %v", UploadTransferActivityName, err) + e := fmt.Errorf("%s: %v", UploadTransferActivityName, err) + + switch err.(type) { + case *sftp.AuthError: + return nil, temporal.NewNonRetryableError(e) + default: + return nil, e + } } return &UploadTransferActivityResult{ diff --git a/internal/am/upload_transfer_test.go b/internal/am/upload_transfer_test.go index a781ec24d..d50cb8512 100644 --- a/internal/am/upload_transfer_test.go +++ b/internal/am/upload_transfer_test.go @@ -8,6 +8,7 @@ import ( "github.com/go-logr/logr" "go.artefactual.dev/tools/mockutil" + "go.artefactual.dev/tools/temporal" temporalsdk_activity "go.temporal.io/sdk/activity" temporalsdk_testsuite "go.temporal.io/sdk/testsuite" "go.uber.org/mock/gomock" @@ -15,6 +16,7 @@ import ( tfs "gotest.tools/v3/fs" "github.com/artefactual-sdps/enduro/internal/am" + "github.com/artefactual-sdps/enduro/internal/sftp" sftp_fake "github.com/artefactual-sdps/enduro/internal/sftp/fake" ) @@ -25,11 +27,12 @@ func TestUploadTransferActivity(t *testing.T) { ) type test struct { - name string - params am.UploadTransferActivityParams - want am.UploadTransferActivityResult - recorder func(*sftp_fake.MockClientMockRecorder) - errMsg string + name string + params am.UploadTransferActivityParams + recorder func(*sftp_fake.MockClientMockRecorder) + want am.UploadTransferActivityResult + wantErr string + wantNonRetryErr bool } for _, tt := range []test{ { @@ -56,10 +59,10 @@ func TestUploadTransferActivity(t *testing.T) { params: am.UploadTransferActivityParams{ SourcePath: td.Join("missing"), }, - errMsg: fmt.Sprintf("UploadTransferActivity: open %s: no such file or directory", td.Join("missing")), + wantErr: fmt.Sprintf("activity error (type: UploadTransferActivity, scheduledEventID: 0, startedEventID: 0, identity: ): UploadTransferActivity: open %s: no such file or directory", td.Join("missing")), }, { - name: "Errors when upload fails", + name: "Retryable error when SSH connection fails", params: am.UploadTransferActivityParams{ SourcePath: td.Join(filename), }, @@ -72,10 +75,32 @@ func TestUploadTransferActivity(t *testing.T) { ).Return( 0, "", - errors.New("SSH: failed to connect: dial tcp 127.0.0.1:2200: connect: connection refused"), + errors.New("ssh: failed to connect: dial tcp 127.0.0.1:2200: connect: connection refused"), ) }, - errMsg: "UploadTransferActivity: SSH: failed to connect: dial tcp 127.0.0.1:2200: connect: connection refused", + wantErr: "activity error (type: UploadTransferActivity, scheduledEventID: 0, startedEventID: 0, identity: ): UploadTransferActivity: ssh: failed to connect: dial tcp 127.0.0.1:2200: connect: connection refused", + }, + { + name: "Non-retryable error when authentication fails", + params: am.UploadTransferActivityParams{ + SourcePath: td.Join(filename), + }, + recorder: func(m *sftp_fake.MockClientMockRecorder) { + var t *os.File + m.Upload( + mockutil.Context(), + gomock.AssignableToTypeOf(t), + filename, + ).Return( + 0, + "", + sftp.NewAuthError( + errors.New("ssh: handshake failed: ssh: unable to authenticate, attempted methods [none publickey], no supported methods remain"), + ), + ) + }, + wantErr: "activity error (type: UploadTransferActivity, scheduledEventID: 0, startedEventID: 0, identity: ): UploadTransferActivity: ssh: handshake failed: ssh: unable to authenticate, attempted methods [none publickey], no supported methods remain", + wantNonRetryErr: true, }, } { tt := tt @@ -98,8 +123,9 @@ func TestUploadTransferActivity(t *testing.T) { ) fut, err := env.ExecuteActivity(am.UploadTransferActivityName, tt.params) - if tt.errMsg != "" { - assert.ErrorContains(t, err, tt.errMsg) + if tt.wantErr != "" { + assert.Error(t, err, tt.wantErr) + assert.Assert(t, temporal.NonRetryableError(err) == tt.wantNonRetryErr) return } diff --git a/internal/sftp/client.go b/internal/sftp/client.go index 95c7de75e..a74d5aa03 100644 --- a/internal/sftp/client.go +++ b/internal/sftp/client.go @@ -5,6 +5,22 @@ import ( "io" ) +type AuthError struct { + err error +} + +func (e *AuthError) Error() string { + return e.err.Error() +} + +func (e *AuthError) Unwrap() error { + return e.err +} + +func NewAuthError(e error) *AuthError { + return &AuthError{err: e} +} + // A Client manages the transmission of data over SFTP. // // Implementations of the Client interface handle the connection details, diff --git a/internal/sftp/goclient.go b/internal/sftp/goclient.go index 84cf043e4..0a7d35b89 100644 --- a/internal/sftp/goclient.go +++ b/internal/sftp/goclient.go @@ -101,7 +101,7 @@ func (c *GoClient) dial(ctx context.Context) (*connection, error) { conn.ssh, err = sshConnect(ctx, c.logger, c.cfg) if err != nil { - return nil, fmt.Errorf("SSH: %v", err) + return nil, err } conn.sftp, err = sftp.NewClient(conn.ssh) diff --git a/internal/sftp/goclient_test.go b/internal/sftp/goclient_test.go index 6dfc9fcc7..f6f0bcd3b 100644 --- a/internal/sftp/goclient_test.go +++ b/internal/sftp/goclient_test.go @@ -4,11 +4,13 @@ import ( "bufio" "bytes" "context" + "errors" "fmt" "io" "log" "net" "os" + "reflect" "strings" "testing" "time" @@ -174,7 +176,7 @@ func TestUpload(t *testing.T) { cfg sftp.Config params params want results - wantErr string + wantErr error } for _, tc := range []test{ { @@ -231,7 +233,9 @@ func TestUpload(t *testing.T) { src: strings.NewReader("Testing 1-2-3"), dest: "test.txt", }, - wantErr: "SSH: parse private key with passphrase: x509: decryption password incorrect", + wantErr: sftp.NewAuthError( + errors.New("ssh: parse private key with passphrase: x509: decryption password incorrect"), + ), }, { name: "Errors when the SFTP server isn't there", @@ -247,8 +251,8 @@ func TestUpload(t *testing.T) { src: strings.NewReader("Testing 1-2-3"), dest: "test.txt", }, - wantErr: fmt.Sprintf( - "SSH: connect: dial tcp %s:%s: connect: connection refused", + wantErr: fmt.Errorf( + "ssh: connect: dial tcp %s:%s: connect: connection refused", badHost, badPort, ), }, @@ -262,7 +266,9 @@ func TestUpload(t *testing.T) { Path: "./testdata/clientkeys/test_unk_ed25519", }, }, - wantErr: "SSH: connect: ssh: handshake failed: ssh: unable to authenticate, attempted methods [none publickey], no supported methods remain", + wantErr: sftp.NewAuthError( + errors.New("ssh: handshake failed: ssh: unable to authenticate, attempted methods [none publickey], no supported methods remain"), + ), }, { name: "Errors when the host key is not in known_hosts", @@ -274,7 +280,9 @@ func TestUpload(t *testing.T) { Path: "./testdata/clientkeys/test_ed25519", }, }, - wantErr: "SSH: connect: ssh: handshake failed: knownhosts: key is unknown", + wantErr: sftp.NewAuthError( + errors.New("ssh: handshake failed: knownhosts: key is unknown"), + ), }, { name: "Errors when the known_hosts file doesn't exist", @@ -286,7 +294,9 @@ func TestUpload(t *testing.T) { Path: "./testdata/clientkeys/test_ed25519", }, }, - wantErr: "SSH: parse known_hosts: open testdata/missing: no such file or directory", + wantErr: sftp.NewAuthError( + errors.New("ssh: parse known_hosts: open testdata/missing: no such file or directory"), + ), }, } { tc := tc @@ -300,8 +310,9 @@ func TestUpload(t *testing.T) { sftpc := sftp.NewGoClient(logr.Discard(), tc.cfg) bytes, remotePath, err := sftpc.Upload(context.Background(), tc.params.src, tc.params.dest) - if tc.wantErr != "" { - assert.Error(t, err, tc.wantErr) + if tc.wantErr != nil { + assert.Error(t, err, tc.wantErr.Error()) + assert.Assert(t, reflect.TypeOf(err) == reflect.TypeOf(tc.wantErr)) return } diff --git a/internal/sftp/ssh.go b/internal/sftp/ssh.go index c75144448..98f8b7363 100644 --- a/internal/sftp/ssh.go +++ b/internal/sftp/ssh.go @@ -6,6 +6,7 @@ import ( "net" "os" "path/filepath" + "strings" "time" "github.com/go-logr/logr" @@ -17,12 +18,12 @@ import ( // returns a client connection. // // Only private key authentication is currently supported, with or without a -// passphrase. +// passphrase.SSH: %v", func sshConnect(ctx context.Context, logger logr.Logger, cfg Config) (*ssh.Client, error) { // Load private key for authentication. keyBytes, err := os.ReadFile(filepath.Clean(cfg.PrivateKey.Path)) // #nosec G304 -- File data is validated below if err != nil { - return nil, fmt.Errorf("read private key: %v", err) + return nil, NewAuthError(fmt.Errorf("ssh: read private key: %v", err)) } // Create a signer from the private key, with or without a passphrase. @@ -30,19 +31,19 @@ func sshConnect(ctx context.Context, logger logr.Logger, cfg Config) (*ssh.Clien if cfg.PrivateKey.Passphrase != "" { signer, err = ssh.ParsePrivateKeyWithPassphrase(keyBytes, []byte(cfg.PrivateKey.Passphrase)) if err != nil { - return nil, fmt.Errorf("parse private key with passphrase: %v", err) + return nil, NewAuthError(fmt.Errorf("ssh: parse private key with passphrase: %v", err)) } } else { signer, err = ssh.ParsePrivateKey(keyBytes) if err != nil { - return nil, fmt.Errorf("parse private key: %v", err) + return nil, NewAuthError(fmt.Errorf("ssh: parse private key: %v", err)) } } // Check that the host key is in the client's known_hosts file. hostcallback, err := knownhosts.New(filepath.Clean(cfg.KnownHostsFile)) if err != nil { - return nil, fmt.Errorf("parse known_hosts: %v", err) + return nil, NewAuthError(fmt.Errorf("ssh: parse known_hosts: %v", err)) } // Configure the SSH client. @@ -61,14 +62,18 @@ func sshConnect(ctx context.Context, logger logr.Logger, cfg Config) (*ssh.Clien dialer := &net.Dialer{} conn, err := dialer.DialContext(ctx, "tcp", address) if err != nil { - logger.V(2).Info("SSH dial failed", "address", address, "user", cfg.User) - return nil, fmt.Errorf("connect: %v", err) + logger.V(2).Info("ssh: dial failed", "address", address, "user", cfg.User) + return nil, fmt.Errorf("ssh: connect: %v", err) } sshConn, chans, reqs, err := ssh.NewClientConn(conn, address, sshConfig) if err != nil { - logger.V(2).Info("SSH dial failed", "address", address, "user", cfg.User) - return nil, fmt.Errorf("connect: %v", err) + if strings.Contains(err.Error(), "ssh: unable to authenticate") || strings.Contains(err.Error(), "knownhosts: key is unknown") { + logger.V(2).Info("ssh: authentication failed", "address", address, "user", cfg.User) + return nil, NewAuthError(err) + } + logger.V(2).Info("ssh: failed to connect", "address", address, "user", cfg.User) + return nil, err } return ssh.NewClient(sshConn, chans, reqs), nil From fdb128b9d181913ad8600cf9dc328b202c3f30c7 Mon Sep 17 00:00:00 2001 From: David Juhasz Date: Wed, 10 Jan 2024 12:06:15 -0800 Subject: [PATCH 2/2] Address code review feedback --- internal/am/upload_transfer_test.go | 8 ++++---- internal/sftp/client.go | 13 +++++-------- internal/sftp/goclient_test.go | 25 ++++++++++++------------- 3 files changed, 21 insertions(+), 25 deletions(-) diff --git a/internal/am/upload_transfer_test.go b/internal/am/upload_transfer_test.go index d50cb8512..125fe967c 100644 --- a/internal/am/upload_transfer_test.go +++ b/internal/am/upload_transfer_test.go @@ -94,12 +94,12 @@ func TestUploadTransferActivity(t *testing.T) { ).Return( 0, "", - sftp.NewAuthError( - errors.New("ssh: handshake failed: ssh: unable to authenticate, attempted methods [none publickey], no supported methods remain"), - ), + &sftp.AuthError{ + Message: "ssh: handshake failed: ssh: unable to authenticate, attempted methods [none publickey], no supported methods remain", + }, ) }, - wantErr: "activity error (type: UploadTransferActivity, scheduledEventID: 0, startedEventID: 0, identity: ): UploadTransferActivity: ssh: handshake failed: ssh: unable to authenticate, attempted methods [none publickey], no supported methods remain", + wantErr: "activity error (type: UploadTransferActivity, scheduledEventID: 0, startedEventID: 0, identity: ): UploadTransferActivity: auth: ssh: handshake failed: ssh: unable to authenticate, attempted methods [none publickey], no supported methods remain", wantNonRetryErr: true, }, } { diff --git a/internal/sftp/client.go b/internal/sftp/client.go index a74d5aa03..159700d41 100644 --- a/internal/sftp/client.go +++ b/internal/sftp/client.go @@ -2,23 +2,20 @@ package sftp import ( "context" + "fmt" "io" ) type AuthError struct { - err error + Message string } func (e *AuthError) Error() string { - return e.err.Error() + return fmt.Sprintf("auth: %s", e.Message) } -func (e *AuthError) Unwrap() error { - return e.err -} - -func NewAuthError(e error) *AuthError { - return &AuthError{err: e} +func NewAuthError(e error) error { + return &AuthError{Message: e.Error()} } // A Client manages the transmission of data over SFTP. diff --git a/internal/sftp/goclient_test.go b/internal/sftp/goclient_test.go index f6f0bcd3b..1f559ba9c 100644 --- a/internal/sftp/goclient_test.go +++ b/internal/sftp/goclient_test.go @@ -4,7 +4,6 @@ import ( "bufio" "bytes" "context" - "errors" "fmt" "io" "log" @@ -233,9 +232,9 @@ func TestUpload(t *testing.T) { src: strings.NewReader("Testing 1-2-3"), dest: "test.txt", }, - wantErr: sftp.NewAuthError( - errors.New("ssh: parse private key with passphrase: x509: decryption password incorrect"), - ), + wantErr: &sftp.AuthError{ + Message: "ssh: parse private key with passphrase: x509: decryption password incorrect", + }, }, { name: "Errors when the SFTP server isn't there", @@ -266,9 +265,9 @@ func TestUpload(t *testing.T) { Path: "./testdata/clientkeys/test_unk_ed25519", }, }, - wantErr: sftp.NewAuthError( - errors.New("ssh: handshake failed: ssh: unable to authenticate, attempted methods [none publickey], no supported methods remain"), - ), + wantErr: &sftp.AuthError{ + Message: "ssh: handshake failed: ssh: unable to authenticate, attempted methods [none publickey], no supported methods remain", + }, }, { name: "Errors when the host key is not in known_hosts", @@ -280,9 +279,9 @@ func TestUpload(t *testing.T) { Path: "./testdata/clientkeys/test_ed25519", }, }, - wantErr: sftp.NewAuthError( - errors.New("ssh: handshake failed: knownhosts: key is unknown"), - ), + wantErr: &sftp.AuthError{ + Message: "ssh: handshake failed: knownhosts: key is unknown", + }, }, { name: "Errors when the known_hosts file doesn't exist", @@ -294,9 +293,9 @@ func TestUpload(t *testing.T) { Path: "./testdata/clientkeys/test_ed25519", }, }, - wantErr: sftp.NewAuthError( - errors.New("ssh: parse known_hosts: open testdata/missing: no such file or directory"), - ), + wantErr: &sftp.AuthError{ + Message: "ssh: parse known_hosts: open testdata/missing: no such file or directory", + }, }, } { tc := tc