From d7a75c5b5d416753ee021cce0739dac9447ae4c6 Mon Sep 17 00:00:00 2001 From: Eric Chiang Date: Thu, 27 Oct 2016 15:55:23 -0700 Subject: [PATCH] storage/kubernetes: allow arbitrary client IDs Use a hash algorithm to match client IDs to Kubernetes object names. Because cryptographic hash algorithms produce sums larger than a Kubernetes name can fit, a non-cryptographic hash is used instead. Hash collisions are checked and result in errors. --- storage/kubernetes/client.go | 24 +++++++++++ storage/kubernetes/client_test.go | 29 ++++++++++++- storage/kubernetes/storage.go | 71 ++++++++++++++++++++++++------- storage/kubernetes/types.go | 26 ++++------- 4 files changed, 115 insertions(+), 35 deletions(-) diff --git a/storage/kubernetes/client.go b/storage/kubernetes/client.go index 04ee9cae..dfb1fe35 100644 --- a/storage/kubernetes/client.go +++ b/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 diff --git a/storage/kubernetes/client_test.go b/storage/kubernetes/client_test.go index 92ae204a..01b4eb7a 100644 --- a/storage/kubernetes/client_test.go +++ b/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) diff --git a/storage/kubernetes/storage.go b/storage/kubernetes/storage.go index bd48b3db..05e088d4 100644 --- a/storage/kubernetes/storage.go +++ b/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 - if err := cli.get(resourceClient, id, &c); err != nil { + c, err := cli.getClient(id) + 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 - if err := cli.get(resourcePassword, emailToID(email), &p); err != nil { + p, err := cli.getPassword(email) + 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 - if err := cli.get(resourceClient, id, &c); err != nil { - return err - } - updated, err := updater(toStorageClient(c)) + c, err := cli.getClient(id) 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) - var p Password - if err := cli.get(resourcePassword, id, &p); err != nil { + p, err := cli.getPassword(email) + if 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 { diff --git a/storage/kubernetes/types.go b/storage/kubernetes/types.go index 8c8f9d6e..900a8187 100644 --- a/storage/kubernetes/types.go +++ b/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,