diff --git a/ssh/keys_test.go b/ssh/keys_test.go index a1165ec..e1412bb 100644 --- a/ssh/keys_test.go +++ b/ssh/keys_test.go @@ -272,18 +272,18 @@ func TestParseEncryptedPrivateKeysWithPassphrase(t *testing.T) { } func TestParseEncryptedPrivateKeysWithUnsupportedCiphers(t *testing.T) { - for _, tt := range testdata.UnsupportedCipherData { - t.Run(tt.Name, func(t *testing.T){ - _, err := ParsePrivateKeyWithPassphrase(tt.PEMBytes, []byte(tt.EncryptionKey)) - if err == nil { - t.Fatalf("expected 'unknown cipher' error for %q, got nil", tt.Name) - // If this cipher is now supported, remove it from testdata.UnsupportedCipherData - } - if !strings.Contains(err.Error(), "unknown cipher") { - t.Errorf("wanted 'unknown cipher' error, got %v", err.Error()) - } - }) - } + for _, tt := range testdata.UnsupportedCipherData { + t.Run(tt.Name, func(t *testing.T) { + _, err := ParsePrivateKeyWithPassphrase(tt.PEMBytes, []byte(tt.EncryptionKey)) + if err == nil { + t.Fatalf("expected 'unknown cipher' error for %q, got nil", tt.Name) + // If this cipher is now supported, remove it from testdata.UnsupportedCipherData + } + if !strings.Contains(err.Error(), "unknown cipher") { + t.Errorf("wanted 'unknown cipher' error, got %v", err.Error()) + } + }) + } } func TestParseEncryptedPrivateKeysWithIncorrectPassphrase(t *testing.T) { diff --git a/ssh/server.go b/ssh/server.go index 064dcba..7f7ecf9 100644 --- a/ssh/server.go +++ b/ssh/server.go @@ -242,8 +242,10 @@ func (c *pubKeyCache) add(candidate cachedPubKey) { type ServerConn struct { Conn - // If the succeeding authentication callback returned a - // non-nil Permissions pointer, it is stored here. + // If the succeeding authentication callback returned a non-nil Permissions + // pointer, it is stored here. These are the permissions from the final, + // successful authentication method. Permissions returned by callbacks that + // return PartialSuccessError are not preserved and must be nil. Permissions *Permissions } @@ -824,6 +826,13 @@ userAuthLoop: var failureMsg userAuthFailureMsg if partialSuccess, ok := authErr.(*PartialSuccessError); ok { + // Permissions are not preserved between authentication steps. To + // avoid confusion about the final state of the connection, we + // disallow returning non-nil Permissions combined with + // PartialSuccessError. + if perms != nil { + return nil, errors.New("ssh: permissions must be nil when returning PartialSuccessError") + } // After a partial success error we don't allow changing the user // name and execute the NoClientAuthCallback. partialSuccessReturned = true diff --git a/ssh/server_test.go b/ssh/server_test.go index e186193..9b514ad 100644 --- a/ssh/server_test.go +++ b/ssh/server_test.go @@ -808,6 +808,60 @@ func TestVerifiedPublicKeyCallbackOnly(t *testing.T) { <-done } +func TestPartialSuccessWithNonNilPerms(t *testing.T) { + c1, c2, err := netPipe() + if err != nil { + t.Fatalf("netPipe: %v", err) + } + defer c1.Close() + defer c2.Close() + + serverConf := &ServerConfig{ + PublicKeyCallback: func(conn ConnMetadata, key PublicKey) (*Permissions, error) { + if bytes.Equal(key.Marshal(), testPublicKeys["rsa"].Marshal()) { + return nil, nil + } + return nil, errors.New("invalid credentials") + }, + VerifiedPublicKeyCallback: func(conn ConnMetadata, key PublicKey, permissions *Permissions, signatureAlgorithm string) (*Permissions, error) { + if bytes.Equal(key.Marshal(), testPublicKeys["rsa"].Marshal()) { + // Intentionally return non-nil Permissions along with a + // PartialSuccessError. Since permissions are reset between + // authentication steps, this constitutes invalid library usage + // and the server is expected to reject the connection. + return &Permissions{Extensions: map[string]string{"permit-port-forwarding": ""}}, &PartialSuccessError{ + Next: ServerAuthCallbacks{ + PasswordCallback: func(conn ConnMetadata, password []byte) (*Permissions, error) { + if string(password) == clientPassword { + return nil, nil + } + return nil, nil + }, + }, + } + } + return nil, errors.New("invalid credentials") + }, + } + serverConf.AddHostKey(testSigners["rsa"]) + + clientConf := ClientConfig{ + User: "user", + Auth: []AuthMethod{ + PublicKeys(testSigners["rsa"]), + Password(clientPassword), + }, + HostKeyCallback: InsecureIgnoreHostKey(), + } + + go NewServerConn(c1, serverConf) + + _, _, _, err = NewClientConn(c2, "", &clientConf) + if err == nil { + t.Fatal("authentication succeeded unexpectedly; server should have rejected non-nil Permissions combined with PartialSuccessError") + } +} + type markerConn struct { closed uint32 used uint32