From b80790a061f35c411d4b79c552f92ff77df58642 Mon Sep 17 00:00:00 2001 From: Bobby DeSimone Date: Fri, 31 Jan 2020 20:24:52 -0800 Subject: [PATCH] cache: add option validations (#468) Signed-off-by: Bobby DeSimone --- cache/cache.go | 27 +++++++++-- cache/cache_test.go | 46 ++++++++++++++++++ cache/grpc_test.go | 112 ++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 180 insertions(+), 5 deletions(-) create mode 100644 cache/cache_test.go create mode 100644 cache/grpc_test.go diff --git a/cache/cache.go b/cache/cache.go index fceaebf6e..dee4a14c4 100644 --- a/cache/cache.go +++ b/cache/cache.go @@ -10,11 +10,13 @@ import ( stdlog "log" "github.com/pomerium/pomerium/config" + "github.com/pomerium/pomerium/internal/cryptutil" "github.com/pomerium/pomerium/internal/kv" "github.com/pomerium/pomerium/internal/kv/autocache" "github.com/pomerium/pomerium/internal/kv/bolt" "github.com/pomerium/pomerium/internal/kv/redis" "github.com/pomerium/pomerium/internal/log" + "github.com/pomerium/pomerium/internal/urlutil" ) // Cache represents the cache service. The cache service is a simple interface @@ -25,7 +27,11 @@ type Cache struct { // New creates a new cache service. func New(opts config.Options) (*Cache, error) { - cache, err := NewCacheStore(opts.CacheStore, &opts) + if err := validate(opts); err != nil { + return nil, fmt.Errorf("cache: bad option: %w", err) + } + + cache, err := newCacheStore(opts.CacheStore, &opts) if err != nil { return nil, err } @@ -34,20 +40,31 @@ func New(opts config.Options) (*Cache, error) { }, nil } -// NewCacheStore creates a new cache store by name and given a set of +// validate checks that proper configuration settings are set to create +// a cache instance +func validate(o config.Options) error { + if _, err := cryptutil.NewAEADCipherFromBase64(o.SharedKey); err != nil { + return fmt.Errorf("invalid 'SHARED_SECRET': %w", err) + } + if err := urlutil.ValidateURL(o.CacheURL); err != nil { + return fmt.Errorf("invalid 'CACHE_SERVICE_URL': %w", err) + } + return nil +} + +// newCacheStore creates a new cache store by name and given a set of // configuration options. -func NewCacheStore(name string, o *config.Options) (s kv.Store, err error) { +func newCacheStore(name string, o *config.Options) (s kv.Store, err error) { switch name { case bolt.Name: s, err = bolt.New(&bolt.Options{Path: o.CacheStorePath}) case redis.Name: - // todo(bdd): make path configurable in config.Options s, err = redis.New(&redis.Options{ Addr: o.CacheStoreAddr, Password: o.CacheStorePassword, }) case autocache.Name: - acLog := log.Logger.With().Str("service", "autocache").Logger() + acLog := log.Logger.With().Str("service", autocache.Name).Logger() s, err = autocache.New(&autocache.Options{ SharedKey: o.SharedKey, Log: stdlog.New(acLog, "", 0), diff --git a/cache/cache_test.go b/cache/cache_test.go new file mode 100644 index 000000000..a2e5da537 --- /dev/null +++ b/cache/cache_test.go @@ -0,0 +1,46 @@ +package cache + +import ( + "io/ioutil" + "log" + "net/url" + "os" + "testing" + + "github.com/pomerium/pomerium/config" + "github.com/pomerium/pomerium/internal/cryptutil" +) + +func TestNew(t *testing.T) { + dir, err := ioutil.TempDir("", "example") + if err != nil { + log.Fatal(err) + } + defer os.RemoveAll(dir) + + tests := []struct { + name string + opts config.Options + wantErr bool + }{ + {"good - autocache", config.Options{CacheStore: "autocache", SharedKey: cryptutil.NewBase64Key(), CacheURL: &url.URL{Scheme: "http", Host: "example"}}, false}, + {"redis failed", config.Options{CacheStore: "redis", SharedKey: cryptutil.NewBase64Key(), CacheURL: &url.URL{Scheme: "http", Host: "example"}}, true}, + {"bad cache store name", config.Options{CacheStore: "pringles-can", SharedKey: cryptutil.NewBase64Key(), CacheURL: &url.URL{Scheme: "http", Host: "example"}}, true}, + {"bad shared secret", config.Options{CacheStorePath: dir + "/bolt.db", CacheStore: "bolt", SharedKey: string([]byte(cryptutil.NewBase64Key())[:31]), CacheURL: &url.URL{Scheme: "http", Host: "example"}}, true}, + {"bad cache url", config.Options{SharedKey: cryptutil.NewBase64Key(), CacheURL: &url.URL{}}, true}, + {"no store set", config.Options{SharedKey: cryptutil.NewBase64Key(), CacheURL: &url.URL{Scheme: "http", Host: "example"}}, true}, + {"good - bolt", config.Options{CacheStorePath: dir + "/bolt.db", CacheStore: "bolt", SharedKey: cryptutil.NewBase64Key(), CacheURL: &url.URL{Scheme: "http", Host: "example"}}, false}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + s, err := New(tt.opts) + if (err != nil) != tt.wantErr { + t.Errorf("New() error = %v, wantErr %v", err, tt.wantErr) + return + } + if err == nil { + s.Close() + } + }) + } +} diff --git a/cache/grpc_test.go b/cache/grpc_test.go new file mode 100644 index 000000000..7b969578f --- /dev/null +++ b/cache/grpc_test.go @@ -0,0 +1,112 @@ +//go:generate protoc -I ../internal/grpc/cache/ --go_out=plugins=grpc:../internal/grpc/cache/ ../internal/grpc/cache/cache.proto + +package cache + +import ( + "context" + "io/ioutil" + "log" + "net/url" + "os" + "testing" + + "github.com/google/go-cmp/cmp" + "github.com/pomerium/pomerium/config" + "github.com/pomerium/pomerium/internal/cryptutil" + "github.com/pomerium/pomerium/internal/grpc/cache" +) + +func TestCache_Get_and_Set(t *testing.T) { + hugeKey := cryptutil.NewRandomStringN(10 << 20) + dir, err := ioutil.TempDir("", "example") + if err != nil { + log.Fatal(err) + } + c, err := New(config.Options{ + CacheStorePath: dir + "/bolt.db", CacheStore: "bolt", + SharedKey: cryptutil.NewBase64Key(), + CacheURL: &url.URL{Scheme: "http", Host: "example"}}) + if err != nil { + t.Fatal(err) + } + defer os.RemoveAll(dir) + defer c.Close() + tests := []struct { + name string + ctx context.Context + SetRequest *cache.SetRequest + SetReply *cache.SetReply + + GetRequest *cache.GetRequest + GetReply *cache.GetReply + wantSetError bool + wantGetError bool + }{ + {"good", + context.TODO(), + &cache.SetRequest{Key: "key", Value: []byte("hello")}, + &cache.SetReply{}, + &cache.GetRequest{Key: "key"}, + &cache.GetReply{ + Exists: true, + Value: []byte("hello"), + XXX_NoUnkeyedLiteral: struct{}{}, + XXX_unrecognized: nil, + XXX_sizecache: 0, + }, + false, + false, + }, + {"miss", + context.TODO(), + &cache.SetRequest{Key: "key", Value: []byte("hello")}, + &cache.SetReply{}, + &cache.GetRequest{Key: "no-such-key"}, + &cache.GetReply{ + Exists: false, + Value: nil, + XXX_NoUnkeyedLiteral: struct{}{}, + XXX_unrecognized: nil, + XXX_sizecache: 0, + }, + false, + false, + }, + {"key too large", + context.TODO(), + &cache.SetRequest{Key: hugeKey, Value: []byte("hello")}, + nil, + &cache.GetRequest{Key: hugeKey}, + &cache.GetReply{ + Exists: false, + Value: nil, + XXX_NoUnkeyedLiteral: struct{}{}, + XXX_unrecognized: nil, + XXX_sizecache: 0, + }, + true, + false, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + + setGot, err := c.Set(tt.ctx, tt.SetRequest) + if (err != nil) != tt.wantSetError { + t.Errorf("Cache.Set() error = %v, wantSetError %v", err, tt.wantSetError) + return + } + if diff := cmp.Diff(setGot, tt.SetReply); diff != "" { + t.Errorf("Cache.Set() = %v", diff) + } + getGot, err := c.Get(tt.ctx, tt.GetRequest) + if (err != nil) != tt.wantGetError { + t.Errorf("Cache.Get() error = %v, wantGetError %v", err, tt.wantGetError) + return + } + if diff := cmp.Diff(getGot, tt.GetReply); diff != "" { + t.Errorf("Cache.Get() = %v", diff) + } + }) + } +}