Improve log messages and add more tests

The messages should look less confusing now, as the phrasing for
expected errors has been changed.

dovecot/keyproxy.go

@@ -151,12 +151,18 @@ func (s *KeyLookupProxy) lookupUserdb(ctx context.Context, username string) (int
 }
 
 func (s *KeyLookupProxy) lookupPassdb(ctx context.Context, username, password string) (interface{}, bool, error) {
-	// If the password is a SSO token, try to fetch the
-	// unencrypted key from the keystore daemon.
+	// The password might be a SSO token, so first of all we try
+	// to fetch the unencrypted key from the keystore daemon.
 	priv, err := s.keystore.Get(ctx, s.config.Shard, username, password)
-	if err != nil {
+	switch {
+	case err == client.ErrNoKeys:
+		log.Printf("no encryption keys for %s in keystore", username)
+	case isErr403(err):
+		log.Printf("no encryption keys for %s in keystore (no SSO token)", username)
+	case err != nil:
+		// This is an unexpected error.
 		log.Printf("keystore lookup for %s failed: %v", username, err)
-	} else {
+	default:
 		log.Printf("passdb lookup for %s (from keystore)", username)
 		return newPassDBResponse(s.b64encode(priv)), true, nil
 	}
@@ -168,7 +174,7 @@ func (s *KeyLookupProxy) lookupPassdb(ctx context.Context, username, password st
 		return nil, false, err
 	}
 	if len(encKeys) == 0 {
-		log.Printf("failed passdb lookup for %s (no keys)", username)
+		log.Printf("no encryption keys for %s in database", username)
 		return nil, false, nil
 	}
 	key, err := userenckey.Decrypt(encKeys, []byte(password))
@@ -191,3 +197,10 @@ func (s *KeyLookupProxy) b64encode(b []byte) string {
 	}
 	return string(b)
 }
+
+func isErr403(err error) bool {
+	if rerr, ok := err.(*clientutil.RemoteError); ok && rerr.StatusCode() == 403 {
+		return true
+	}
+	return false
+}

server/keystore_test.go

@@ -22,7 +22,7 @@ type testContext struct {
 	signer     sso.Signer
 }
 
-func newTestContext(t testing.TB) *testContext {
+func newTestContext(t testing.TB) (*testContext, *KeyStore, func()) {
 	dir, err := ioutil.TempDir("", "")
 	if err != nil {
 		t.Fatal(err)
@@ -42,9 +42,26 @@ func newTestContext(t testing.TB) *testContext {
 		t.Fatal("sso.NewSigner():", err)
 	}
 
-	ioutil.WriteFile(ctx.pubkeyPath, pub, 0644)
+	ioutil.WriteFile(ctx.pubkeyPath, pub, 0644) // nolint
 
-	return ctx
+	db := &testDB{
+		keys: map[string][][]byte{
+			"testuser": [][]byte{encPrivKey},
+		},
+	}
+
+	keystore, err := newKeyStoreWithBackend(&Config{
+		SSOPublicKeyFile: ctx.pubkeyPath,
+		SSOService:       "keystore/",
+		SSODomain:        "domain",
+	}, db)
+	if err != nil {
+		t.Fatal("newKeyStoreWithBackend():", err)
+	}
+
+	return ctx, keystore, func() {
+		ctx.Close()
+	}
 }
 
 func (c *testContext) Close() {
@@ -88,30 +105,17 @@ func init() {
 }
 
 func TestKeystore_OpenAndGet(t *testing.T) {
-	c := newTestContext(t)
-	defer c.Close()
-
-	db := &testDB{
-		keys: map[string][][]byte{
-			"testuser": [][]byte{encPrivKey},
-		},
-	}
-
-	keystore, err := newKeyStoreWithBackend(&Config{
-		SSOPublicKeyFile: c.pubkeyPath,
-		SSOService:       "keystore/",
-		SSODomain:        "domain",
-	}, db)
-	if err != nil {
-		t.Fatal(err)
-	}
+	c, keystore, cleanup := newTestContext(t)
+	defer cleanup()
 
 	// Decrypt the private key with the right password.
-	err = keystore.Open(context.Background(), "testuser", string(pw), 60)
+	err := keystore.Open(context.Background(), "testuser", string(pw), 60)
 	if err != nil {
 		t.Fatal("keystore.Open():", err)
 	}
 
+	// Call expire() now to make sure we don't wipe data that is
+	// not expired yet.
 	keystore.expire(time.Now())
 
 	// Sign a valid SSO ticket and use it to obtain the private
@@ -126,29 +130,58 @@ func TestKeystore_OpenAndGet(t *testing.T) {
 	if !bytes.Equal(result, expectedPEM) {
 		t.Fatalf("keystore.Get() returned bad key: got %v, expected %v", result, expectedPEM)
 	}
+
+	// Call Close() and forget the key.
+	keystore.Close("testuser")
+	if _, err := keystore.Get("testuser", ssoTicket); err == nil {
+		t.Fatal("keystore.Get() returned no error after Close()")
+	}
 }
 
-func TestKeystore_Expire(t *testing.T) {
-	c := newTestContext(t)
-	defer c.Close()
+func TestKeystore_OpenAndGet_NoKeys(t *testing.T) {
+	c, keystore, cleanup := newTestContext(t)
+	defer cleanup()
 
-	db := &testDB{
-		keys: map[string][][]byte{
-			"testuser": [][]byte{encPrivKey},
-		},
+	// Check the return value of Open() when the user has no keys.
+	username := "no-keys-user"
+	err := keystore.Open(context.Background(), username, string(pw), 60)
+	if err != errNoKeys {
+		t.Fatalf("keystore.Open() returned unexpected err=%v", err)
 	}
 
-	keystore, err := newKeyStoreWithBackend(&Config{
-		SSOPublicKeyFile: c.pubkeyPath,
-		SSOService:       "keystore/",
-		SSODomain:        "domain",
-	}, db)
-	if err != nil {
-		t.Fatal(err)
+	// Sign a valid SSO ticket and use it to obtain the private
+	// key we just stored.
+	ssoTicket := c.sign(username, "keystore/", "domain")
+	_, err = keystore.Get(username, ssoTicket)
+	if err != errNoKeys {
+		t.Fatalf("keystore.Get() returned unexpected err=%v", err)
 	}
+}
+
+func TestKeystore_Get_Unauthorized(t *testing.T) {
+	c, keystore, cleanup := newTestContext(t)
+	defer cleanup()
+
+	// Valid SSO ticket, but for the wrong user.
+	ssoTicket := c.sign("wrong-user", "keystore/", "domain")
+	_, err := keystore.Get("testuser", ssoTicket)
+	if err != errBadUser {
+		t.Fatalf("keystore.Get(wrong-user) returned unexpected err=%v", err)
+	}
+
+	// Invalid SSO ticket.
+	_, err = keystore.Get("testuser", "not-a-valid-sso-ticket")
+	if err != errUnauthorized {
+		t.Fatalf("keystore.Get(invalid-sso) returned unexpected err=%v", err)
+	}
+}
+
+func TestKeystore_Expire(t *testing.T) {
+	c, keystore, cleanup := newTestContext(t)
+	defer cleanup()
 
 	// Decrypt the private key with the right password.
-	err = keystore.Open(context.Background(), "testuser", string(pw), 60)
+	err := keystore.Open(context.Background(), "testuser", string(pw), 60)
 	if err != nil {
 		t.Fatal("keystore.Open():", err)
 	}

server/server_test.go

+package server
+
+import (
+	"bytes"
+	"context"
+	"net/http/httptest"
+	"testing"
+
+	"git.autistici.org/ai3/go-common/clientutil"
+	"git.autistici.org/id/keystore/client"
+)
+
+type serverTestContext struct {
+	*testContext
+	keystore *KeyStore
+	srv      *httptest.Server
+}
+
+func newServerTestContext(t testing.TB) (*serverTestContext, func()) {
+	ctx, keystore, cleanup := newTestContext(t)
+	srv := httptest.NewServer(NewServer(keystore))
+
+	return &serverTestContext{
+			testContext: ctx,
+			keystore:    keystore,
+			srv:         srv,
+		}, func() {
+			srv.Close()
+			cleanup()
+		}
+}
+
+func (c *serverTestContext) client(t testing.TB) client.Client {
+	kc, err := client.New(&clientutil.BackendConfig{URL: c.srv.URL})
+	if err != nil {
+		t.Fatalf("client.New(): %v", err)
+	}
+	return kc
+}
+
+func TestServer_OpenAndGet(t *testing.T) {
+	c, cleanup := newServerTestContext(t)
+	defer cleanup()
+	kc := c.client(t)
+
+	// Decrypt the private key with the right password.
+	err := kc.Open(context.Background(), "", "testuser", string(pw), 60)
+	if err != nil {
+		t.Fatal("keystore.Open():", err)
+	}
+
+	// Sign a valid SSO ticket and use it to obtain the private
+	// key we just stored.
+	ssoTicket := c.sign("testuser", "keystore/", "domain")
+	result, err := kc.Get(context.Background(), "", "testuser", ssoTicket)
+	if err != nil {
+		t.Fatal("keystore.Get():", err)
+	}
+
+	expectedPEM, _ := privKey.PEM()
+	if !bytes.Equal(result, expectedPEM) {
+		t.Fatalf("keystore.Get() returned bad key: got %v, expected %v", result, expectedPEM)
+	}
+
+	// Call Close and forget the key.
+	if err := kc.Close(context.Background(), "", "testuser"); err != nil {
+		t.Fatalf("keystore.Close(): %v", err)
+	}
+	if _, err := kc.Get(context.Background(), "", "testuser", ssoTicket); err != client.ErrNoKeys {
+		t.Fatalf("keystore.Get() after Close() returned err=%v", err)
+	}
+}
+
+func TestServer_OpenAndGet_NoKeys(t *testing.T) {
+	c, cleanup := newServerTestContext(t)
+	defer cleanup()
+	kc := c.client(t)
+
+	// Check the return value of Open() when the user has no keys.
+	username := "no-keys-user"
+	err := kc.Open(context.Background(), "", username, string(pw), 60)
+	if err != nil {
+		t.Fatalf("keystore.Open(%s): unexpected err=%v", username, err)
+	}
+
+	// Sign a valid SSO ticket and use it to obtain the private
+	// key we just stored.
+	ssoTicket := c.sign(username, "keystore/", "domain")
+	_, err = kc.Get(context.Background(), "", username, ssoTicket)
+	if err != client.ErrNoKeys {
+		t.Fatalf("keystore.Get(%s): unexpected err=%v", username, err)
+	}
+}
+
+func TestServer_OpenAndGet_Unauthorized(t *testing.T) {
+	c, cleanup := newServerTestContext(t)
+	defer cleanup()
+	kc := c.client(t)
+
+	// Valid SSO ticket, but for the wrong user.
+	ssoTicket := c.sign("wrong-user", "keystore/", "domain")
+	_, err := kc.Get(context.Background(), "", "testuser", ssoTicket)
+	if !isErr403(err) {
+		t.Fatalf("keystore.Get(wrong-user) returned unexpected err=%v", err)
+	}
+
+	// Invalid SSO ticket.
+	_, err = kc.Get(context.Background(), "", "testuser", "not-a-valid-sso-ticket")
+	if !isErr403(err) {
+		t.Fatalf("keystore.Get(invalid-sso) returned unexpected err=%v", err)
+	}
+}
+
+func isErr403(err error) bool {
+	if rerr, ok := err.(*clientutil.RemoteError); ok && rerr.StatusCode() == 403 {
+		return true
+	}
+	return false
+}