From 8e06e8ddd9629eb88639aba897641bff8031f1d3 Mon Sep 17 00:00:00 2001 From: Peter Moody Date: Sat, 10 Sep 2016 11:59:01 -0700 Subject: [PATCH] x/crypto/ssh/agent: honor constraints on keys in the keyring. If a key is added to an agent keyring with constraints, honor them. This will remove keys when they've been on the keyring for LifetimeSecs seconds or longer and will ask the user to confirm a signing operation if ConfirmBeforeUse is set. Change-Id: I633713c5f78b13a628a5d752f11b306b6e16a2ef Reviewed-on: https://go-review.googlesource.com/28956 Reviewed-by: Han-Wen Nienhuys Run-TryBot: Han-Wen Nienhuys TryBot-Result: Gobot Gobot --- ssh/agent/client_test.go | 24 +++++++++++++++--- ssh/agent/keyring.go | 53 +++++++++++++++++++++++++++++++-------- ssh/agent/keyring_test.go | 4 +-- 3 files changed, 63 insertions(+), 18 deletions(-) diff --git a/ssh/agent/client_test.go b/ssh/agent/client_test.go index 230351fd..e33d4713 100644 --- a/ssh/agent/client_test.go +++ b/ssh/agent/client_test.go @@ -86,6 +86,11 @@ func testAgent(t *testing.T, key interface{}, cert *ssh.Certificate, lifetimeSec testAgentInterface(t, agent, key, cert, lifetimeSecs) } +func testKeyring(t *testing.T, key interface{}, cert *ssh.Certificate, lifetimeSecs uint32) { + a := NewKeyring() + testAgentInterface(t, a, key, cert, lifetimeSecs) +} + func testAgentInterface(t *testing.T, agent Agent, key interface{}, cert *ssh.Certificate, lifetimeSecs uint32) { signer, err := ssh.NewSignerFromKey(key) if err != nil { @@ -137,11 +142,25 @@ func testAgentInterface(t *testing.T, agent Agent, key interface{}, cert *ssh.Ce if err := pubKey.Verify(data, sig); err != nil { t.Fatalf("Verify(%s): %v", pubKey.Type(), err) } + + // If the key has a lifetime, is it removed when it should be? + if lifetimeSecs > 0 { + time.Sleep(time.Second*time.Duration(lifetimeSecs) + 100*time.Millisecond) + keys, err := agent.List() + if err != nil { + t.Fatalf("List: %v", err) + } + if len(keys) > 0 { + t.Fatalf("key not expired") + } + } + } func TestAgent(t *testing.T) { for _, keyType := range []string{"rsa", "dsa", "ecdsa", "ed25519"} { testAgent(t, testPrivateKeys[keyType], nil, 0) + testKeyring(t, testPrivateKeys[keyType], nil, 1) } } @@ -154,10 +173,7 @@ func TestCert(t *testing.T) { cert.SignCert(rand.Reader, testSigners["ecdsa"]) testAgent(t, testPrivateKeys["rsa"], cert, 0) -} - -func TestConstraints(t *testing.T) { - testAgent(t, testPrivateKeys["rsa"], nil, 3600 /* lifetime in seconds */) + testKeyring(t, testPrivateKeys["rsa"], cert, 1) } // netPipe is analogous to net.Pipe, but it uses a real net.Conn, and diff --git a/ssh/agent/keyring.go b/ssh/agent/keyring.go index 12ffa82b..a6ba06ab 100644 --- a/ssh/agent/keyring.go +++ b/ssh/agent/keyring.go @@ -11,6 +11,7 @@ import ( "errors" "fmt" "sync" + "time" "golang.org/x/crypto/ssh" ) @@ -18,6 +19,7 @@ import ( type privKey struct { signer ssh.Signer comment string + expire *time.Time } type keyring struct { @@ -48,15 +50,9 @@ func (r *keyring) RemoveAll() error { return nil } -// Remove removes all identities with the given public key. -func (r *keyring) Remove(key ssh.PublicKey) error { - r.mu.Lock() - defer r.mu.Unlock() - if r.locked { - return errLocked - } - - want := key.Marshal() +// removeLocked does the actual key removal. The caller must already be holding the +// keyring mutex. +func (r *keyring) removeLocked(want []byte) error { found := false for i := 0; i < len(r.keys); { if bytes.Equal(r.keys[i].signer.PublicKey().Marshal(), want) { @@ -75,7 +71,18 @@ func (r *keyring) Remove(key ssh.PublicKey) error { return nil } -// Lock locks the agent. Sign and Remove will fail, and List will empty an empty list. +// Remove removes all identities with the given public key. +func (r *keyring) Remove(key ssh.PublicKey) error { + r.mu.Lock() + defer r.mu.Unlock() + if r.locked { + return errLocked + } + + return r.removeLocked(key.Marshal()) +} + +// Lock locks the agent. Sign and Remove will fail, and List will return an empty list. func (r *keyring) Lock(passphrase []byte) error { r.mu.Lock() defer r.mu.Unlock() @@ -104,6 +111,17 @@ func (r *keyring) Unlock(passphrase []byte) error { return nil } +// expireKeysLocked removes expired keys from the keyring. If a key was added +// with a lifetimesecs contraint and seconds >= lifetimesecs seconds have +// ellapsed, it is removed. The caller *must* be holding the keyring mutex. +func (r *keyring) expireKeysLocked() { + for _, k := range r.keys { + if k.expire != nil && time.Now().After(*k.expire) { + r.removeLocked(k.signer.PublicKey().Marshal()) + } + } +} + // List returns the identities known to the agent. func (r *keyring) List() ([]*Key, error) { r.mu.Lock() @@ -113,6 +131,7 @@ func (r *keyring) List() ([]*Key, error) { return nil, nil } + r.expireKeysLocked() var ids []*Key for _, k := range r.keys { pub := k.signer.PublicKey() @@ -146,7 +165,17 @@ func (r *keyring) Add(key AddedKey) error { } } - r.keys = append(r.keys, privKey{signer, key.Comment}) + p := privKey{ + signer: signer, + comment: key.Comment, + } + + if key.LifetimeSecs > 0 { + t := time.Now().Add(time.Duration(key.LifetimeSecs) * time.Second) + p.expire = &t + } + + r.keys = append(r.keys, p) return nil } @@ -159,6 +188,7 @@ func (r *keyring) Sign(key ssh.PublicKey, data []byte) (*ssh.Signature, error) { return nil, errLocked } + r.expireKeysLocked() wanted := key.Marshal() for _, k := range r.keys { if bytes.Equal(k.signer.PublicKey().Marshal(), wanted) { @@ -176,6 +206,7 @@ func (r *keyring) Signers() ([]ssh.Signer, error) { return nil, errLocked } + r.expireKeysLocked() s := make([]ssh.Signer, 0, len(r.keys)) for _, k := range r.keys { s = append(s, k.signer) diff --git a/ssh/agent/keyring_test.go b/ssh/agent/keyring_test.go index 7f059057..e5d50e7e 100644 --- a/ssh/agent/keyring_test.go +++ b/ssh/agent/keyring_test.go @@ -4,9 +4,7 @@ package agent -import ( - "testing" -) +import "testing" func addTestKey(t *testing.T, a Agent, keyName string) { err := a.Add(AddedKey{