From 5396cdc8992a3fa9a121aa6160c0b03bf00d05a0 Mon Sep 17 00:00:00 2001 From: Kyle Isom Date: Fri, 5 Aug 2016 09:09:41 -0700 Subject: [PATCH] Address @jkroll-cf's feedback on keycache interface. + persistLabels moved from cryptor to persist package global. + Restore now explicitly checks for the case where there aren't enough shares to return `ErrRestoreDelegations`. + The users responsible for restoring the cache are now logged. --- cryptor/cryptor.go | 24 +++++++++++++++--------- cryptor/cryptor_test.go | 5 +++++ msp/msp.go | 6 +++++- persist/persist.go | 12 ++++++++++-- 4 files changed, 35 insertions(+), 12 deletions(-) diff --git a/cryptor/cryptor.go b/cryptor/cryptor.go index fd7a126..c509764 100644 --- a/cryptor/cryptor.go +++ b/cryptor/cryptor.go @@ -12,8 +12,10 @@ import ( "crypto/sha1" "encoding/json" "errors" + "log" "sort" "strconv" + "strings" "github.com/cloudflare/redoctober/config" "github.com/cloudflare/redoctober/keycache" @@ -705,8 +707,6 @@ func (c *Cryptor) DelegateStatus(name string, labels, admins []string) (adminsDe return c.cache.DelegateStatus(name, labels, admins) } -var persistLabels = []string{"restore"} - // store serialises the key cache, encrypts it, and writes it to disk. func (c *Cryptor) store() error { // If the store isn't currently active, we shouldn't attempt @@ -726,7 +726,7 @@ func (c *Cryptor) store() error { Predicate: c.persist.Policy(), } - cache, err = c.Encrypt(cache, persistLabels, access) + cache, err = c.Encrypt(cache, persist.Labels, access) if err != nil { return err } @@ -747,19 +747,25 @@ func (c *Cryptor) Restore(name, password string, uses int, slot, durationString return errors.New("Missing user on disk") } - err := c.persist.Delegate(record, name, password, c.persist.Users(), persistLabels, uses, slot, durationString) + err := c.persist.Delegate(record, name, password, c.persist.Users(), persist.Labels, uses, slot, durationString) if err != nil { return err } - // A failure to decrypt isn't an error, it just means there - // aren't enough delegations yet; the sentinal value - // ErrRestoreDelegations is returned to indicate this. - cache, _, _, _, err := c.decrypt(c.persist.Cache(), c.persist.Blob(), name) + // A failure to decrypt isn't a restore error, it (most often) + // just means there aren't enough delegations yet; the + // sentinal value ErrRestoreDelegations is returned to + // indicate this. However, the error + cache, _, names, _, err := c.decrypt(c.persist.Cache(), c.persist.Blob(), name) if err != nil { - return ErrRestoreDelegations + if err == msp.ErrNotEnoughShares { + return ErrRestoreDelegations + } + return err } + log.Printf("cryptor.restore success: names=%s", strings.Join(names, ",")) + var uk map[string]keycache.ActiveUser err = json.Unmarshal(cache, &uk) if err != nil { diff --git a/cryptor/cryptor_test.go b/cryptor/cryptor_test.go index 85ddc80..183d1b8 100644 --- a/cryptor/cryptor_test.go +++ b/cryptor/cryptor_test.go @@ -293,6 +293,11 @@ func TestRestore(t *testing.T) { persist.Inactive, status.State) } + err = c.Restore("Carl", "weakpassword", 0, "", "0h") + if err != ErrRestoreDelegations { + t.Fatal(err) + } + err = c.Restore("Bob", "weakpassword", 2, "", "1h") if err != nil { t.Fatal(err) diff --git a/msp/msp.go b/msp/msp.go index 65bdd43..75a7782 100644 --- a/msp/msp.go +++ b/msp/msp.go @@ -211,6 +211,10 @@ func (m MSP) DistributeShares(sec []byte, db UserDatabase) (map[string][][]byte, return out, nil } +// ErrNotEnoughShares is returned if there aren't enough shares to +// decrypt the secret. +var ErrNotEnoughShares = errors.New("Not enough shares to recover.") + // RecoverSecret takes a user database storing secret shares as input and returns the original secret. func (m MSP) RecoverSecret(db UserDatabase) ([]byte, error) { cache := make(map[string][][]byte, 0) // Caches un-used shares for a user. @@ -225,7 +229,7 @@ func (m MSP) recoverSecret(db UserDatabase, cache map[string][][]byte) ([]byte, ok, names, locs, _ := m.DerivePath(db) if !ok { - return nil, errors.New("Not enough shares to recover.") + return nil, ErrNotEnoughShares } for _, name := range names { diff --git a/persist/persist.go b/persist/persist.go index 07384ae..8eaf5f8 100644 --- a/persist/persist.go +++ b/persist/persist.go @@ -13,9 +13,12 @@ import ( var defaultStore Store = &File{} +// Labels are the labels that the keycache should be encrypted with. +var Labels = []string{"restore"} + const ( - // NeverPersist indicates that the persistence store will - // never persist active delegations. + // Disabled indicates that the persistence store will never + // persist active delegations. Disabled = "disabled" // Inactive indicates that the persistence store requires @@ -52,6 +55,7 @@ type Store interface { Cache() *keycache.Cache } +// FileMechanism indicates that the persistence mechanism is a file. const FileMechanism = "file" type mechanism func(*config.Delegations) (Store, error) @@ -61,6 +65,8 @@ var stores = map[string]mechanism{ FileMechanism: newFile, } +// New attempts to create a new persistence store from the +// configuration. func New(config *config.Delegations) (Store, error) { if config == nil { return nil, errors.New("persist: nil configuration") @@ -78,4 +84,6 @@ func New(config *config.Delegations) (Store, error) { return constructor(config) } +// ErrInvalidConfig is returned when the configuration is invalid for +// the type of persistence store in use. var ErrInvalidConfig = errors.New("persist: invalid configuration")