From c2df344bce3068cfe340988a5937c421eee52803 Mon Sep 17 00:00:00 2001 From: Ying Li Date: Thu, 29 Sep 2016 16:56:13 -0700 Subject: [PATCH 1/2] Update client to not fail updates if operating offline Signed-off-by: Ying Li --- client/client_update_test.go | 39 +++++++++++ client/tufclient.go | 2 +- storage/httpstore.go | 36 +++++----- storage/httpstore_test.go | 123 +++++++++++++++++++++++++++++++---- 4 files changed, 168 insertions(+), 32 deletions(-) diff --git a/client/client_update_test.go b/client/client_update_test.go index 06acd12dc..06c9c2a5c 100644 --- a/client/client_update_test.go +++ b/client/client_update_test.go @@ -172,6 +172,45 @@ func TestUpdateSucceedsEvenIfCannotWriteExistingRepo(t *testing.T) { } } +// If there is no local cache, update will error if it can't connect to the server. Otherwise +// it uses the local cache. +func TestUpdateInOfflineMode(t *testing.T) { + if testing.Short() { + t.Skip("skipping test in short mode") + } + + // invalid URL, no cache - errors + invalidURLRepo := newBlankRepo(t, "https://nothisdoesnotexist.com") + defer os.RemoveAll(invalidURLRepo.baseDir) + err := invalidURLRepo.Update(false) + require.Error(t, err) + require.IsType(t, store.NetworkError{}, err) + + // offline client: no cache - errors + tempBaseDir, err := ioutil.TempDir("", "notary-test-") + require.NoError(t, err, "failed to create a temporary directory: %s", err) + defer os.RemoveAll(tempBaseDir) + + offlineRepo, err := NewNotaryRepository(tempBaseDir, "docker.com/notary", "https://nope", + nil, passphrase.ConstantRetriever("pass"), trustpinning.TrustPinConfig{}) + require.NoError(t, err) + err = offlineRepo.Update(false) + require.Error(t, err) + require.IsType(t, store.ErrOffline{}, err) + + // set existing metadata on the repo + serverMeta, _, err := testutils.NewRepoMetadata("docker.com/notary", metadataDelegations...) + require.NoError(t, err) + for name, metaBytes := range serverMeta { + require.NoError(t, invalidURLRepo.fileStore.Set(name, metaBytes)) + require.NoError(t, offlineRepo.fileStore.Set(name, metaBytes)) + } + + // both of these can read from cache and load repo + require.NoError(t, invalidURLRepo.Update(false)) + require.NoError(t, offlineRepo.Update(false)) +} + type swizzleFunc func(*testutils.MetadataSwizzler, string) error type swizzleExpectations struct { desc string diff --git a/client/tufclient.go b/client/tufclient.go index 409b019ae..a6abdac7c 100644 --- a/client/tufclient.go +++ b/client/tufclient.go @@ -116,7 +116,7 @@ func (c *TUFClient) downloadTimestamp() error { switch remoteErr.(type) { case nil: return nil - case store.ErrMetaNotFound, store.ErrServerUnavailable: + case store.ErrMetaNotFound, store.ErrServerUnavailable, store.ErrOffline, store.NetworkError: break default: return remoteErr diff --git a/storage/httpstore.go b/storage/httpstore.go index abc7ffd4c..994f94912 100644 --- a/storage/httpstore.go +++ b/storage/httpstore.go @@ -33,6 +33,15 @@ type ErrServerUnavailable struct { code int } +// NetworkError represents any kind of network error when attempting to make a request +type NetworkError struct { + Wrapped error +} + +func (n NetworkError) Error() string { + return n.Wrapped.Error() +} + func (err ErrServerUnavailable) Error() string { if err.code == 401 { return fmt.Sprintf("you are not authorized to perform this operation: server returned 401.") @@ -152,7 +161,7 @@ func (s HTTPStore) GetSized(name string, size int64) ([]byte, error) { } resp, err := s.roundTrip.RoundTrip(req) if err != nil { - return nil, err + return nil, NetworkError{Wrapped: err} } defer resp.Body.Close() if err := translateStatusToError(resp, name); err != nil { @@ -174,22 +183,9 @@ func (s HTTPStore) GetSized(name string, size int64) ([]byte, error) { return body, nil } -// Set uploads a piece of TUF metadata to the server +// Set sends a single piece of metadata to the TUF server func (s HTTPStore) Set(name string, blob []byte) error { - url, err := s.buildMetaURL("") - if err != nil { - return err - } - req, err := http.NewRequest("POST", url.String(), bytes.NewReader(blob)) - if err != nil { - return err - } - resp, err := s.roundTrip.RoundTrip(req) - if err != nil { - return err - } - defer resp.Body.Close() - return translateStatusToError(resp, "POST "+name) + return s.SetMulti(map[string][]byte{name: blob}) } // Remove always fails, because we should never be able to delete metadata @@ -239,7 +235,7 @@ func (s HTTPStore) SetMulti(metas map[string][]byte) error { } resp, err := s.roundTrip.RoundTrip(req) if err != nil { - return err + return NetworkError{Wrapped: err} } defer resp.Body.Close() // if this 404's something is pretty wrong @@ -258,7 +254,7 @@ func (s HTTPStore) RemoveAll() error { } resp, err := s.roundTrip.RoundTrip(req) if err != nil { - return err + return NetworkError{Wrapped: err} } defer resp.Body.Close() return translateStatusToError(resp, "DELETE metadata for GUN endpoint") @@ -299,7 +295,7 @@ func (s HTTPStore) GetKey(role string) ([]byte, error) { } resp, err := s.roundTrip.RoundTrip(req) if err != nil { - return nil, err + return nil, NetworkError{Wrapped: err} } defer resp.Body.Close() if err := translateStatusToError(resp, role+" key"); err != nil { @@ -324,7 +320,7 @@ func (s HTTPStore) RotateKey(role string) ([]byte, error) { } resp, err := s.roundTrip.RoundTrip(req) if err != nil { - return nil, err + return nil, NetworkError{Wrapped: err} } defer resp.Body.Close() if err := translateStatusToError(resp, role+" key"); err != nil { diff --git a/storage/httpstore_test.go b/storage/httpstore_test.go index 09bceb6b0..3f7f407d6 100644 --- a/storage/httpstore_test.go +++ b/storage/httpstore_test.go @@ -26,6 +26,12 @@ func (rt *TestRoundTripper) RoundTrip(req *http.Request) (*http.Response, error) return http.DefaultClient.Do(req) } +type failRoundTripper struct{} + +func (ft failRoundTripper) RoundTrip(req *http.Request) (*http.Response, error) { + return nil, fmt.Errorf("FAIL") +} + func TestHTTPStoreGetSized(t *testing.T) { handler := func(w http.ResponseWriter, r *http.Request) { w.Write([]byte(testRoot)) @@ -46,6 +52,19 @@ func TestHTTPStoreGetSized(t *testing.T) { p := &data.Signed{} err = json.Unmarshal(j, p) require.NoError(t, err) + + // if there is a network error, it gets translated to NetworkError + store, err = NewHTTPStore( + server.URL, + "metadata", + "txt", + "key", + failRoundTripper{}, + ) + require.NoError(t, err) + _, err = store.GetSized("root", 4801) + require.IsType(t, NetworkError{}, err) + require.Equal(t, "FAIL", err.Error()) } // Test that passing -1 to httpstore's GetSized will return all content @@ -71,16 +90,18 @@ func TestHTTPStoreGetAllMeta(t *testing.T) { require.NoError(t, err) } -func TestSetMultiMeta(t *testing.T) { +func TestSetSingleAndSetMultiMeta(t *testing.T) { metas := map[string][]byte{ "root": []byte("root data"), "targets": []byte("targets data"), } + var updates map[string][]byte + handler := func(w http.ResponseWriter, r *http.Request) { reader, err := r.MultipartReader() require.NoError(t, err) - updates := make(map[string][]byte) + updates = make(map[string][]byte) for { part, err := reader.NextPart() if err == io.EOF { @@ -90,21 +111,44 @@ func TestSetMultiMeta(t *testing.T) { updates[role], err = ioutil.ReadAll(part) require.NoError(t, err) } - rd, rok := updates["root"] - require.True(t, rok) - require.Equal(t, rd, metas["root"]) - - td, tok := updates["targets"] - require.True(t, tok) - require.Equal(t, td, metas["targets"]) - } server := httptest.NewServer(http.HandlerFunc(handler)) defer server.Close() store, err := NewHTTPStore(server.URL, "metadata", "json", "key", http.DefaultTransport) require.NoError(t, err) - store.SetMulti(metas) + require.NoError(t, store.SetMulti(metas)) + require.Len(t, updates, 2) + rd, rok := updates["root"] + require.True(t, rok) + require.Equal(t, rd, metas["root"]) + td, tok := updates["targets"] + require.True(t, tok) + require.Equal(t, td, metas["targets"]) + + require.NoError(t, store.Set("root", metas["root"])) + require.Len(t, updates, 1) + rd, rok = updates["root"] + require.True(t, rok) + require.Equal(t, rd, metas["root"]) + + // if there is a network error, it gets translated to NetworkError + store, err = NewHTTPStore( + server.URL, + "metadata", + "txt", + "key", + failRoundTripper{}, + ) + require.NoError(t, err) + + err = store.SetMulti(metas) + require.IsType(t, NetworkError{}, err) + require.Equal(t, "FAIL", err.Error()) + + err = store.Set("root", metas["root"]) + require.IsType(t, NetworkError{}, err) + require.Equal(t, "FAIL", err.Error()) } func testErrorCode(t *testing.T, errorCode int, errType error) { @@ -204,10 +248,25 @@ func TestHTTPStoreRemoveAll(t *testing.T) { err = store.RemoveAll() require.NoError(t, err) + + // if there is a network error, it gets translated to NetworkError + store, err = NewHTTPStore( + server.URL, + "metadata", + "txt", + "key", + failRoundTripper{}, + ) + require.NoError(t, err) + err = store.RemoveAll() + require.IsType(t, NetworkError{}, err) + require.Equal(t, "FAIL", err.Error()) } func TestHTTPStoreRotateKey(t *testing.T) { handler := func(w http.ResponseWriter, r *http.Request) { + require.Equal(t, "POST", r.Method) + require.Equal(t, "/metadata/snapshot.key", r.URL.Path) w.Write([]byte(testRootKey)) } server := httptest.NewServer(http.HandlerFunc(handler)) @@ -218,6 +277,48 @@ func TestHTTPStoreRotateKey(t *testing.T) { pubKeyBytes, err := store.RotateKey(data.CanonicalSnapshotRole) require.NoError(t, err) require.Equal(t, pubKeyBytes, []byte(testRootKey)) + + // if there is a network error, it gets translated to NetworkError + store, err = NewHTTPStore( + server.URL, + "metadata", + "txt", + "key", + failRoundTripper{}, + ) + require.NoError(t, err) + _, err = store.RotateKey(data.CanonicalSnapshotRole) + require.IsType(t, NetworkError{}, err) + require.Equal(t, "FAIL", err.Error()) +} + +func TestHTTPStoreGetKey(t *testing.T) { + handler := func(w http.ResponseWriter, r *http.Request) { + require.Equal(t, "GET", r.Method) + require.Equal(t, "/metadata/snapshot.key", r.URL.Path) + w.Write([]byte(testRootKey)) + } + server := httptest.NewServer(http.HandlerFunc(handler)) + defer server.Close() + store, err := NewHTTPStore(server.URL, "metadata", "json", "key", http.DefaultTransport) + require.NoError(t, err) + + pubKeyBytes, err := store.GetKey(data.CanonicalSnapshotRole) + require.NoError(t, err) + require.Equal(t, pubKeyBytes, []byte(testRootKey)) + + // if there is a network error, it gets translated to NetworkError + store, err = NewHTTPStore( + server.URL, + "metadata", + "txt", + "key", + failRoundTripper{}, + ) + require.NoError(t, err) + _, err = store.GetKey(data.CanonicalSnapshotRole) + require.IsType(t, NetworkError{}, err) + require.Equal(t, "FAIL", err.Error()) } func TestHTTPOffline(t *testing.T) { From 95593e5a99556f90c73f8db89f78e25fa4421eb3 Mon Sep 17 00:00:00 2001 From: Ying Li Date: Thu, 29 Sep 2016 17:04:31 -0700 Subject: [PATCH 2/2] Update changelog Signed-off-by: Ying Li --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 7b68d9696..da439fd55 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,7 @@ + Output message to CLI when repo changes have been successfully published [#974](https://github.com/docker/notary/pull/974) + Improved error messages for client authentication errors and for the witness command [#972](https://github.com/docker/notary/pull/972) + Support for finding keys that are anywhere in the notary directory's "private" directory, not just under "private/root_keys" or "private/tuf_keys" [#981](https://github.com/docker/notary/pull/981) ++ Previously, on any error updating, the client would fall back on the cache. Now we only do so if there is a network error or if the server is unavailable or missing the TUF data. Invalid TUF data will cause the update to fail - for example if there was an invalid root rotation. [#982](https://github.com/docker/notary/pull/982) ## [v0.4.0](https://github.com/docker/notary/releases/tag/v0.4.0) 9/21/2016 + Server-managed key rotations [#889](https://github.com/docker/notary/pull/889)