From 1d9ec77a06b1322b18c84303baf85cce2064aadc Mon Sep 17 00:00:00 2001 From: Grant Linville Date: Tue, 11 Mar 2025 16:38:30 -0400 Subject: [PATCH 1/3] enhance: add method for recreating all credentials Signed-off-by: Grant Linville --- pkg/credentials/factory.go | 8 ++--- pkg/credentials/noop.go | 4 +++ pkg/credentials/overrides.go | 4 +++ pkg/credentials/store.go | 67 +++++++++++++++++++++++++++++++----- pkg/sdkserver/credentials.go | 17 +++++++++ pkg/sdkserver/routes.go | 1 + 6 files changed, 89 insertions(+), 12 deletions(-) diff --git a/pkg/credentials/factory.go b/pkg/credentials/factory.go index 42295fc8..60f1c838 100644 --- a/pkg/credentials/factory.go +++ b/pkg/credentials/factory.go @@ -72,8 +72,8 @@ func (s *StoreFactory) NewStore(credCtxs []string) (CredentialStore, error) { return nil, err } if s.file { - return withOverride{ - target: Store{ + return &withOverride{ + target: &Store{ credCtxs: credCtxs, cfg: s.cfg, }, @@ -81,8 +81,8 @@ func (s *StoreFactory) NewStore(credCtxs []string) (CredentialStore, error) { credContext: credCtxs, }, nil } - return withOverride{ - target: Store{ + return &withOverride{ + target: &Store{ credCtxs: credCtxs, cfg: s.cfg, program: s.program, diff --git a/pkg/credentials/noop.go b/pkg/credentials/noop.go index 414f8a12..540d80aa 100644 --- a/pkg/credentials/noop.go +++ b/pkg/credentials/noop.go @@ -25,3 +25,7 @@ func (s NoopStore) Remove(context.Context, string) error { func (s NoopStore) List(context.Context) ([]Credential, error) { return nil, nil } + +func (s NoopStore) RecreateAll(context.Context) error { + return nil +} diff --git a/pkg/credentials/overrides.go b/pkg/credentials/overrides.go index 0911cac5..747909d7 100644 --- a/pkg/credentials/overrides.go +++ b/pkg/credentials/overrides.go @@ -147,3 +147,7 @@ func (w withOverride) List(ctx context.Context) ([]Credential, error) { return creds, nil } + +func (w withOverride) RecreateAll(ctx context.Context) error { + return w.target.RecreateAll(ctx) +} diff --git a/pkg/credentials/store.go b/pkg/credentials/store.go index be4be183..76054abe 100644 --- a/pkg/credentials/store.go +++ b/pkg/credentials/store.go @@ -5,6 +5,7 @@ import ( "fmt" "regexp" "slices" + "sync" "github.com/docker/cli/cli/config/credentials" "github.com/docker/cli/cli/config/types" @@ -24,15 +25,20 @@ type CredentialStore interface { Refresh(ctx context.Context, cred Credential) error Remove(ctx context.Context, toolName string) error List(ctx context.Context) ([]Credential, error) + RecreateAll(ctx context.Context) error } type Store struct { - credCtxs []string - cfg *config.CLIConfig - program client.ProgramFunc + credCtxs []string + cfg *config.CLIConfig + program client.ProgramFunc + recreateAllLock sync.RWMutex } -func (s Store) Get(_ context.Context, toolName string) (*Credential, bool, error) { +func (s *Store) Get(_ context.Context, toolName string) (*Credential, bool, error) { + s.recreateAllLock.RLock() + defer s.recreateAllLock.RUnlock() + if len(s.credCtxs) > 0 && s.credCtxs[0] == AllCredentialContexts { return nil, false, fmt.Errorf("cannot get a credential with context %q", AllCredentialContexts) } @@ -80,7 +86,10 @@ func (s Store) Get(_ context.Context, toolName string) (*Credential, bool, error // Add adds a new credential to the credential store. // Any context set on the credential object will be overwritten with the first context of the credential store. -func (s Store) Add(_ context.Context, cred Credential) error { +func (s *Store) Add(_ context.Context, cred Credential) error { + s.recreateAllLock.RLock() + defer s.recreateAllLock.RUnlock() + first := first(s.credCtxs) if first == AllCredentialContexts { return fmt.Errorf("cannot add a credential with context %q", AllCredentialContexts) @@ -99,7 +108,10 @@ func (s Store) Add(_ context.Context, cred Credential) error { } // Refresh updates an existing credential in the credential store. -func (s Store) Refresh(_ context.Context, cred Credential) error { +func (s *Store) Refresh(_ context.Context, cred Credential) error { + s.recreateAllLock.RLock() + defer s.recreateAllLock.RUnlock() + if !slices.Contains(s.credCtxs, cred.Context) { return fmt.Errorf("context %q not in list of valid contexts for this credential store", cred.Context) } @@ -115,7 +127,10 @@ func (s Store) Refresh(_ context.Context, cred Credential) error { return store.Store(auth) } -func (s Store) Remove(_ context.Context, toolName string) error { +func (s *Store) Remove(_ context.Context, toolName string) error { + s.recreateAllLock.RLock() + defer s.recreateAllLock.RUnlock() + first := first(s.credCtxs) if len(s.credCtxs) > 1 || first == AllCredentialContexts { return fmt.Errorf("error: credential deletion is not supported when multiple credential contexts are provided") @@ -129,7 +144,10 @@ func (s Store) Remove(_ context.Context, toolName string) error { return store.Erase(toolNameWithCtx(toolName, first)) } -func (s Store) List(_ context.Context) ([]Credential, error) { +func (s *Store) List(_ context.Context) ([]Credential, error) { + s.recreateAllLock.RLock() + defer s.recreateAllLock.RUnlock() + store, err := s.getStore() if err != nil { return nil, err @@ -199,6 +217,39 @@ func (s Store) List(_ context.Context) ([]Credential, error) { return maps.Values(credsByName), nil } +func (s *Store) RecreateAll(_ context.Context) error { + s.recreateAllLock.Lock() + defer s.recreateAllLock.Unlock() + + store, err := s.getStore() + if err != nil { + return err + } + + all, err := store.GetAll() + if err != nil { + return err + } + + // Loop through and recreate each individual credential. + for serverAddress := range all { + authConfig, err := store.Get(serverAddress) + if err != nil { + return err + } + + if err := store.Erase(serverAddress); err != nil { + return err + } + + if err := store.Store(authConfig); err != nil { + return err + } + } + + return nil +} + func (s *Store) getStore() (credentials.Store, error) { if s.program != nil { return &toolCredentialStore{ diff --git a/pkg/sdkserver/credentials.go b/pkg/sdkserver/credentials.go index 2b527b2b..adf86bc7 100644 --- a/pkg/sdkserver/credentials.go +++ b/pkg/sdkserver/credentials.go @@ -20,6 +20,23 @@ func (s *server) initializeCredentialStore(_ context.Context, credCtxs []string) return store, nil } +func (s *server) recreateAllCredentials(w http.ResponseWriter, r *http.Request) { + logger := gcontext.GetLogger(r.Context()) + + store, err := s.initializeCredentialStore(r.Context(), []string{credentials.AllCredentialContexts}) + if err != nil { + writeError(logger, w, http.StatusInternalServerError, err) + return + } + + if err := store.RecreateAll(r.Context()); err != nil { + writeError(logger, w, http.StatusInternalServerError, fmt.Errorf("failed to recreate all credentials: %w", err)) + return + } + + writeResponse(logger, w, map[string]any{"stdout": "All credentials recreated successfully"}) +} + func (s *server) listCredentials(w http.ResponseWriter, r *http.Request) { logger := gcontext.GetLogger(r.Context()) req := new(credentialsRequest) diff --git a/pkg/sdkserver/routes.go b/pkg/sdkserver/routes.go index d520e97a..b1bd4c3b 100644 --- a/pkg/sdkserver/routes.go +++ b/pkg/sdkserver/routes.go @@ -70,6 +70,7 @@ func (s *server) addRoutes(mux *http.ServeMux) { mux.HandleFunc("POST /credentials/create", s.createCredential) mux.HandleFunc("POST /credentials/reveal", s.revealCredential) mux.HandleFunc("POST /credentials/delete", s.deleteCredential) + mux.HandleFunc("POST /credentials/recreate-all", s.recreateAllCredentials) mux.HandleFunc("POST /datasets", s.listDatasets) mux.HandleFunc("POST /datasets/list-elements", s.listDatasetElements) From 51a38d79e7326f49fbdc7f6cfe5dd88dda8524bf Mon Sep 17 00:00:00 2001 From: Grant Linville Date: Tue, 11 Mar 2025 18:30:25 -0400 Subject: [PATCH 2/3] improve locking strategy Signed-off-by: Grant Linville --- pkg/credentials/store.go | 21 ++++++++++++++++++--- 1 file changed, 18 insertions(+), 3 deletions(-) diff --git a/pkg/credentials/store.go b/pkg/credentials/store.go index 76054abe..b73c3d81 100644 --- a/pkg/credentials/store.go +++ b/pkg/credentials/store.go @@ -218,33 +218,48 @@ func (s *Store) List(_ context.Context) ([]Credential, error) { } func (s *Store) RecreateAll(_ context.Context) error { - s.recreateAllLock.Lock() - defer s.recreateAllLock.Unlock() - store, err := s.getStore() if err != nil { return err } + // We repeatedly lock and unlock the mutex in this function to give other threads a chance to talk to the credential store. + // It can take several minutes to recreate the credentials if there are hundreds of them, and we don't want to + // block all other threads while we do that. + // New credentials might be created after our GetAll, but they will be created with the current encryption configuration, + // so it's okay that they are skipped by this function. + + s.recreateAllLock.Lock() all, err := store.GetAll() + s.recreateAllLock.Unlock() if err != nil { return err } // Loop through and recreate each individual credential. for serverAddress := range all { + s.recreateAllLock.Lock() authConfig, err := store.Get(serverAddress) if err != nil { + s.recreateAllLock.Unlock() + + if IsCredentialsNotFoundError(err) { + // This can happen if the credential was deleted between the GetAll and the Get by another thread. + continue + } return err } if err := store.Erase(serverAddress); err != nil { + s.recreateAllLock.Unlock() return err } if err := store.Store(authConfig); err != nil { + s.recreateAllLock.Unlock() return err } + s.recreateAllLock.Unlock() } return nil From dad6e3af8cc0e8dfa832efee1525f4dff979e7ff Mon Sep 17 00:00:00 2001 From: Grant Linville Date: Thu, 13 Mar 2025 14:40:27 -0400 Subject: [PATCH 3/3] PR feedback Signed-off-by: Grant Linville --- pkg/credentials/store.go | 43 ++++++++++++++++++++-------------------- 1 file changed, 22 insertions(+), 21 deletions(-) diff --git a/pkg/credentials/store.go b/pkg/credentials/store.go index b73c3d81..def6ff89 100644 --- a/pkg/credentials/store.go +++ b/pkg/credentials/store.go @@ -223,12 +223,8 @@ func (s *Store) RecreateAll(_ context.Context) error { return err } - // We repeatedly lock and unlock the mutex in this function to give other threads a chance to talk to the credential store. - // It can take several minutes to recreate the credentials if there are hundreds of them, and we don't want to - // block all other threads while we do that. // New credentials might be created after our GetAll, but they will be created with the current encryption configuration, // so it's okay that they are skipped by this function. - s.recreateAllLock.Lock() all, err := store.GetAll() s.recreateAllLock.Unlock() @@ -238,28 +234,33 @@ func (s *Store) RecreateAll(_ context.Context) error { // Loop through and recreate each individual credential. for serverAddress := range all { - s.recreateAllLock.Lock() - authConfig, err := store.Get(serverAddress) - if err != nil { - s.recreateAllLock.Unlock() - - if IsCredentialsNotFoundError(err) { - // This can happen if the credential was deleted between the GetAll and the Get by another thread. - continue - } + if err := s.recreateCredential(store, serverAddress); err != nil { return err } + } - if err := store.Erase(serverAddress); err != nil { - s.recreateAllLock.Unlock() - return err - } + return nil +} - if err := store.Store(authConfig); err != nil { - s.recreateAllLock.Unlock() - return err +func (s *Store) recreateCredential(store credentials.Store, serverAddress string) error { + s.recreateAllLock.Lock() + defer s.recreateAllLock.Unlock() + + authConfig, err := store.Get(serverAddress) + if err != nil { + if IsCredentialsNotFoundError(err) { + // This can happen if the credential was deleted between the GetAll and the Get by another thread. + return nil } - s.recreateAllLock.Unlock() + return err + } + + if err := store.Erase(serverAddress); err != nil { + return err + } + + if err := store.Store(authConfig); err != nil { + return err } return nil