From f18420efc3b4f8e9f3d51f6bd2476e92c46260e9 Mon Sep 17 00:00:00 2001
From: Mark Severson <miquella@gmail.com>
Date: Tue, 15 Dec 2015 11:40:13 -0700
Subject: [PATCH] x/crypto/ssh/agent: Fix keyring removing the wrong key(s)

The Remove method for the keyring sliced the internal keys list
incorrectly when removing a key. This caused the wrong key to be removed
or sometimes multiple keys were removed. Additionally, if the key to be
removed was the last key, the method never returned.

Fixes golang/go#13628

Change-Id: I0facbcb8f8b65709222067ce37ea26e3fb5ba8e8
Reviewed-on: https://go-review.googlesource.com/17870
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
---
 ssh/agent/keyring.go      |  2 +-
 ssh/agent/keyring_test.go | 78 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 79 insertions(+), 1 deletion(-)
 create mode 100644 ssh/agent/keyring_test.go

diff --git a/ssh/agent/keyring.go b/ssh/agent/keyring.go
index 11b72df..12ffa82 100644
--- a/ssh/agent/keyring.go
+++ b/ssh/agent/keyring.go
@@ -62,7 +62,7 @@ func (r *keyring) Remove(key ssh.PublicKey) error {
 		if bytes.Equal(r.keys[i].signer.PublicKey().Marshal(), want) {
 			found = true
 			r.keys[i] = r.keys[len(r.keys)-1]
-			r.keys = r.keys[len(r.keys)-1:]
+			r.keys = r.keys[:len(r.keys)-1]
 			continue
 		} else {
 			i++
diff --git a/ssh/agent/keyring_test.go b/ssh/agent/keyring_test.go
new file mode 100644
index 0000000..7f05905
--- /dev/null
+++ b/ssh/agent/keyring_test.go
@@ -0,0 +1,78 @@
+// Copyright 2015 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+package agent
+
+import (
+	"testing"
+)
+
+func addTestKey(t *testing.T, a Agent, keyName string) {
+	err := a.Add(AddedKey{
+		PrivateKey: testPrivateKeys[keyName],
+		Comment:    keyName,
+	})
+	if err != nil {
+		t.Fatalf("failed to add key %q: %v", keyName, err)
+	}
+}
+
+func removeTestKey(t *testing.T, a Agent, keyName string) {
+	err := a.Remove(testPublicKeys[keyName])
+	if err != nil {
+		t.Fatalf("failed to remove key %q: %v", keyName, err)
+	}
+}
+
+func validateListedKeys(t *testing.T, a Agent, expectedKeys []string) {
+	listedKeys, err := a.List()
+	if err != nil {
+		t.Fatalf("failed to list keys: %v", err)
+		return
+	}
+	actualKeys := make(map[string]bool)
+	for _, key := range listedKeys {
+		actualKeys[key.Comment] = true
+	}
+
+	matchedKeys := make(map[string]bool)
+	for _, expectedKey := range expectedKeys {
+		if !actualKeys[expectedKey] {
+			t.Fatalf("expected key %q, but was not found", expectedKey)
+		} else {
+			matchedKeys[expectedKey] = true
+		}
+	}
+
+	for actualKey := range actualKeys {
+		if !matchedKeys[actualKey] {
+			t.Fatalf("key %q was found, but was not expected", actualKey)
+		}
+	}
+}
+
+func TestKeyringAddingAndRemoving(t *testing.T) {
+	keyNames := []string{"dsa", "ecdsa", "rsa", "user"}
+
+	// add all test private keys
+	k := NewKeyring()
+	for _, keyName := range keyNames {
+		addTestKey(t, k, keyName)
+	}
+	validateListedKeys(t, k, keyNames)
+
+	// remove a key in the middle
+	keyToRemove := keyNames[1]
+	keyNames = append(keyNames[:1], keyNames[2:]...)
+
+	removeTestKey(t, k, keyToRemove)
+	validateListedKeys(t, k, keyNames)
+
+	// remove all keys
+	err := k.RemoveAll()
+	if err != nil {
+		t.Fatalf("failed to remove all keys: %v", err)
+	}
+	validateListedKeys(t, k, []string{})
+}