Merge pull request #642 from ericchiang/k8s-client-id

storage/kubernetes: allow arbitrary client IDs

storage/kubernetes/client.go

@@ -4,10 +4,13 @@ import (
 	"bytes"
 	"crypto/tls"
 	"crypto/x509"
+	"encoding/base32"
 	"encoding/base64"
 	"encoding/json"
 	"errors"
 	"fmt"
+	"hash"
+	"hash/fnv"
 	"io"
 	"io/ioutil"
 	"net"
@@ -31,6 +34,14 @@ type client struct {
 	baseURL   string
 	namespace string
 
+	// Hash function to map IDs (which could span a large range) to Kubernetes names.
+	// While this is not currently upgradable, it could be in the future.
+	//
+	// The default hash is a non-cryptographic hash, because cryptographic hashes
+	// always produce sums too long to fit into a Kubernetes name. Because of this,
+	// gets, updates, and deletes are _always_ checked for collisions.
+	hash func() hash.Hash
+
 	// API version of the oidc resources. For example "oidc.coreos.com". This is
 	// currently not configurable, but could be in the future.
 	apiVersion string
@@ -40,6 +51,18 @@ type client struct {
 	cancel context.CancelFunc
 }
 
+// idToName maps an arbitrary ID, such as an email or client ID to a Kubernetes object name.
+func (c *client) idToName(s string) string {
+	return idToName(s, c.hash)
+}
+
+// Kubernetes names must match the regexp '[a-z0-9]([-a-z0-9]*[a-z0-9])?'.
+var encoding = base32.NewEncoding("abcdefghijklmnopqrstuvwxyz234567")
+
+func idToName(s string, h func() hash.Hash) string {
+	return strings.TrimRight(encoding.EncodeToString(h().Sum([]byte(s))), "=")
+}
+
 func (c *client) urlFor(apiVersion, namespace, resource, name string) string {
 	basePath := "apis/"
 	if apiVersion == "v1" {
@@ -277,6 +300,7 @@ func newClient(cluster k8sapi.Cluster, user k8sapi.AuthInfo, namespace string) (
 	return &client{
 		client:     &http.Client{Transport: t},
 		baseURL:    cluster.Server,
+		hash:       func() hash.Hash { return fnv.New64() },
 		namespace:  namespace,
 		apiVersion: "oidc.coreos.com/v1",
 	}, nil

storage/kubernetes/client_test.go

@@ -1,6 +1,33 @@
 package kubernetes
 
-import "testing"
+import (
+	"hash"
+	"hash/fnv"
+	"sync"
+	"testing"
+)
+
+// This test does not have an explicit error condition but is used
+// with the race detector to detect the safety of idToName.
+func TestIDToName(t *testing.T) {
+	n := 100
+	var wg sync.WaitGroup
+	wg.Add(n)
+	c := make(chan struct{})
+
+	h := func() hash.Hash { return fnv.New64() }
+
+	for i := 0; i < n; i++ {
+		go func() {
+			<-c
+			name := idToName("foo", h)
+			_ = name
+			wg.Done()
+		}()
+	}
+	close(c)
+	wg.Wait()
+}
 
 func TestNamespaceFromServiceAccountJWT(t *testing.T) {
 	namespace, err := namespaceFromServiceAccountJWT(serviceAccountToken)

storage/kubernetes/storage.go

@@ -5,6 +5,7 @@ import (
 	"fmt"
 	"log"
 	"net/http"
+	"strings"
 	"time"
 
 	"golang.org/x/net/context"
@@ -187,21 +188,47 @@ func (cli *client) GetAuthCode(id string) (storage.AuthCode, error) {
 }
 
 func (cli *client) GetClient(id string) (storage.Client, error) {
-	var c Client
+	c, err := cli.getClient(id)
-	if err := cli.get(resourceClient, id, &c); err != nil {
+	if err != nil {
 		return storage.Client{}, err
 	}
 	return toStorageClient(c), nil
 }
 
+func (cli *client) getClient(id string) (Client, error) {
+	var c Client
+	name := cli.idToName(id)
+	if err := cli.get(resourceClient, name, &c); err != nil {
+		return Client{}, err
+	}
+	if c.ID != id {
+		return Client{}, fmt.Errorf("get client: ID %q mapped to client with ID %q", id, c.ID)
+	}
+	return c, nil
+}
+
 func (cli *client) GetPassword(email string) (storage.Password, error) {
-	var p Password
+	p, err := cli.getPassword(email)
-	if err := cli.get(resourcePassword, emailToID(email), &p); err != nil {
+	if err != nil {
 		return storage.Password{}, err
 	}
 	return toStoragePassword(p), nil
 }
 
+func (cli *client) getPassword(email string) (Password, error) {
+	// TODO(ericchiang): Figure out whose job it is to lowercase emails.
+	email = strings.ToLower(email)
+	var p Password
+	name := cli.idToName(email)
+	if err := cli.get(resourcePassword, name, &p); err != nil {
+		return Password{}, err
+	}
+	if email != p.Email {
+		return Password{}, fmt.Errorf("get email: email %q mapped to password with email %q", email, p.Email)
+	}
+	return p, nil
+}
+
 func (cli *client) GetKeys() (storage.Keys, error) {
 	var keys Keys
 	if err := cli.get(resourceKeys, keysName, &keys); err != nil {
@@ -242,7 +269,12 @@ func (cli *client) DeleteAuthCode(code string) error {
 }
 
 func (cli *client) DeleteClient(id string) error {
-	return cli.delete(resourceClient, id)
+	// Check for hash collition.
+	c, err := cli.getClient(id)
+	if err != nil {
+		return err
+	}
+	return cli.delete(resourceClient, c.ObjectMeta.Name)
 }
 
 func (cli *client) DeleteRefresh(id string) error {
@@ -250,28 +282,34 @@ func (cli *client) DeleteRefresh(id string) error {
 }
 
 func (cli *client) DeletePassword(email string) error {
-	return cli.delete(resourcePassword, emailToID(email))
+	// Check for hash collition.
+	p, err := cli.getPassword(email)
+	if err != nil {
+		return err
+	}
+	return cli.delete(resourcePassword, p.ObjectMeta.Name)
 }
 
 func (cli *client) UpdateClient(id string, updater func(old storage.Client) (storage.Client, error)) error {
-	var c Client
+	c, err := cli.getClient(id)
-	if err := cli.get(resourceClient, id, &c); err != nil {
-		return err
-	}
-	updated, err := updater(toStorageClient(c))
 	if err != nil {
 		return err
 	}
 
+	updated, err := updater(toStorageClient(c))
+	if err != nil {
+		return err
+	}
+	updated.ID = c.ID
+
 	newClient := cli.fromStorageClient(updated)
 	newClient.ObjectMeta = c.ObjectMeta
-	return cli.put(resourceClient, id, newClient)
+	return cli.put(resourceClient, c.ObjectMeta.Name, newClient)
 }
 
 func (cli *client) UpdatePassword(email string, updater func(old storage.Password) (storage.Password, error)) error {
-	id := emailToID(email)
+	p, err := cli.getPassword(email)
-	var p Password
+	if err != nil {
-	if err := cli.get(resourcePassword, id, &p); err != nil {
 		return err
 	}
 
@@ -279,10 +317,11 @@ func (cli *client) UpdatePassword(email string, updater func(old storage.Passwor
 	if err != nil {
 		return err
 	}
+	updated.Email = p.Email
 
 	newPassword := cli.fromStoragePassword(updated)
 	newPassword.ObjectMeta = p.ObjectMeta
-	return cli.put(resourcePassword, id, newPassword)
+	return cli.put(resourcePassword, p.ObjectMeta.Name, newPassword)
 }
 
 func (cli *client) UpdateKeys(updater func(old storage.Keys) (storage.Keys, error)) error {

storage/kubernetes/types.go

@@ -1,7 +1,6 @@
 package kubernetes
 
 import (
-	"encoding/base32"
 	"strings"
 	"time"
 
@@ -75,13 +74,14 @@ const keysName = "openid-connect-keys"
 
 // Client is a mirrored struct from storage with JSON struct tags and
 // Kubernetes type metadata.
-//
-// TODO(ericchiang): Kubernetes has an extremely restricted set of characters it can use for IDs.
-// Consider base32ing client IDs.
 type Client struct {
+	// Name is a hash of the ID.
 	k8sapi.TypeMeta   `json:",inline"`
 	k8sapi.ObjectMeta `json:"metadata,omitempty"`
 
+	// ID is immutable, since it's a primary key and should not be changed.
+	ID string `json:"id,omitempty"`
+
 	Secret       string   `json:"secret,omitempty"`
 	RedirectURIs []string `json:"redirectURIs,omitempty"`
 	TrustedPeers []string `json:"trustedPeers,omitempty"`
@@ -106,9 +106,10 @@ func (cli *client) fromStorageClient(c storage.Client) Client {
 			APIVersion: cli.apiVersion,
 		},
 		ObjectMeta: k8sapi.ObjectMeta{
-			Name:      c.ID,
+			Name:      cli.idToName(c.ID),
 			Namespace: cli.namespace,
 		},
+		ID:           c.ID,
 		Secret:       c.Secret,
 		RedirectURIs: c.RedirectURIs,
 		TrustedPeers: c.TrustedPeers,
@@ -120,7 +121,7 @@ func (cli *client) fromStorageClient(c storage.Client) Client {
 
 func toStorageClient(c Client) storage.Client {
 	return storage.Client{
-		ID:           c.ObjectMeta.Name,
+		ID:           c.ID,
 		Secret:       c.Secret,
 		RedirectURIs: c.RedirectURIs,
 		TrustedPeers: c.TrustedPeers,
@@ -258,17 +259,6 @@ type Password struct {
 	UserID   string `json:"userID,omitempty"`
 }
 
-// Kubernetes only allows lower case letters for names.
-//
-// NOTE(ericchiang): This is currently copied from the storage package's NewID()
-// method. Once we refactor those into the storage, just use that instead.
-var encoding = base32.NewEncoding("abcdefghijklmnopqrstuvwxyz234567")
-
-// Map an arbitrary email to a valid Kuberntes name.
-func emailToID(email string) string {
-	return strings.TrimRight(encoding.EncodeToString([]byte(strings.ToLower(email))), "=")
-}
-
 func (cli *client) fromStoragePassword(p storage.Password) Password {
 	email := strings.ToLower(p.Email)
 	return Password{
@@ -277,7 +267,7 @@ func (cli *client) fromStoragePassword(p storage.Password) Password {
 			APIVersion: cli.apiVersion,
 		},
 		ObjectMeta: k8sapi.ObjectMeta{
-			Name:      emailToID(email),
+			Name:      cli.idToName(email),
 			Namespace: cli.namespace,
 		},
 		Email:    email,