Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP: Client rotate remote key #553

Closed
wants to merge 8 commits into from
Closed

WIP: Client rotate remote key #553

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
cc72108
Add a server endpoint to force rotate remote-managed keys.
cyli Feb 1, 2016
e4bb888
Update the httpstore to be able to rotate keys.
cyli Feb 1, 2016
f236451
Update the client to attempt to use the remote rotation endpoint when…
cyli Feb 1, 2016
cc2ee80
Update the TUF remote store's RotateKey function to expect a CryptoSe…
cyli Feb 8, 2016
413e1cd
Update the client to load the root in order to remotely rotate the key.
cyli Feb 8, 2016
e9182f7
Refactor getting signing keys for a role into a function on tuf.Repo.
cyli Feb 9, 2016
5ea5208
Better server error logging.
cyli Feb 9, 2016
4f39c93
Addressing minor review comments:
cyli Feb 10, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 3 additions & 8 deletions client/backwards_compatibility_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ import (
"testing"
"time"

"github.com/docker/notary/client/changelist"
"github.com/docker/notary/passphrase"
"github.com/docker/notary/tuf/data"
"github.com/docker/notary/tuf/store"
Expand Down Expand Up @@ -107,13 +106,9 @@ func Test0Dot1RepoFormat(t *testing.T) {
// one and try to create a new one from scratch, which will be the wrong version
require.NoError(t, repo.fileStore.RemoveMeta(data.CanonicalTimestampRole))

// rotate the timestamp key, since the server doesn't have that one
timestampPubKey, err := getRemoteKey(ts.URL, gun, data.CanonicalTimestampRole, http.DefaultTransport)
require.NoError(t, err)
require.NoError(
t, repo.rootFileKeyChange(data.CanonicalTimestampRole, changelist.ActionCreate, timestampPubKey))

require.NoError(t, repo.Publish())
// We need to rotate the timestamp key, since the server doesn't have the one
// specified by the metadata.
require.NoError(t, repo.RotateKey(data.CanonicalTimestampRole, true))

targets, err = repo.ListTargets()
require.NoError(t, err)
Expand Down
74 changes: 62 additions & 12 deletions client/client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,18 @@ type ErrInvalidRemoteRole struct {

func (err ErrInvalidRemoteRole) Error() string {
return fmt.Sprintf(
"notary does not support the server managing the %s key", err.Role)
"notary does not permit the server managing the %s key", err.Role)
}

// ErrInvalidLocalRole is returned when the client wants to manage
// an unsupported key type
type ErrInvalidLocalRole struct {
Role string
}

func (err ErrInvalidLocalRole) Error() string {
return fmt.Sprintf(
"notary does not permit the client managing the %s key", err.Role)
}

// ErrRepositoryNotExist is returned when an action is taken on a remote
Expand Down Expand Up @@ -511,11 +522,10 @@ func (r *NotaryRepository) Publish() error {
initialPublish = true
} else {
// We could not update, so we cannot publish.
logrus.Error("Could not publish Repository: ", err.Error())
logrus.Error("Could not publish Repository since we could not update: ", err.Error())
return err
}
}

cl, err := r.GetChangelist()
if err != nil {
return err
Expand Down Expand Up @@ -608,7 +618,7 @@ func (r *NotaryRepository) Publish() error {
// to load metadata for all roles. Since server snapshots are supported,
// if the snapshot metadata fails to load, that's ok.
// This can also be unified with some cache reading tools from tuf/client.
// This assumes that bootstrapRepo is only used by Publish()
// This assumes that bootstrapRepo is only used by Publish() or RotateKey()
func (r *NotaryRepository) bootstrapRepo() error {
kdb := keys.NewDB()
tufRepo := tuf.NewRepo(kdb, r.CryptoService)
Expand Down Expand Up @@ -829,28 +839,68 @@ func (r *NotaryRepository) validateRoot(rootJSON []byte) (*data.SignedRoot, erro
// creates and adds one new key or delegates managing the key to the server.
// These changes are staged in a changelist until publish is called.
func (r *NotaryRepository) RotateKey(role string, serverManagesKey bool) error {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method could probably use some Debug output.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added one Debug, and otherwise just tried to return better errors. Does it look reasonable atm?

if role == data.CanonicalRootRole || role == data.CanonicalTimestampRole {
switch {
case role == data.CanonicalRootRole:
return fmt.Errorf(
"notary does not currently support rotating the %s key", role)
}
if serverManagesKey && role == data.CanonicalTargetsRole {
"notary does not currently permit rotating the %s key", role)

// We currently support locally managing targets keys, remotely managing
// timestamp keys, and locally or remotely managing snapshot keys.
case serverManagesKey && role == data.CanonicalTargetsRole:
return ErrInvalidRemoteRole{Role: data.CanonicalTargetsRole}
case !serverManagesKey && role == data.CanonicalTimestampRole:
return ErrInvalidLocalRole{Role: data.CanonicalTimestampRole}
}

var (
pubKey data.PublicKey
err error
pubKey data.PublicKey
errFmtString string
err error
)
// Key rotation is an offline operation unless we are rotating a remote key, in which case we
// need to ensure the rotation can actually happen: the user needs to have the root key, and
// we want to publish right away so that the remote key doesn't expire
if serverManagesKey {
pubKey, err = getRemoteKey(r.baseURL, r.gun, role, r.roundTrip)
if err = r.bootstrapRepo(); err != nil {
if _, ok := err.(store.ErrMetaNotFound); ok {
err = ErrRepoNotInitialized{}
}
logrus.Debug("Repository should be valid and initialized before rotating a remote key:", err)
return err
}

// get root keys - since we bootstrapped the repo, a valid root should always specify
// the root role
rootKeys, err := r.tufRepo.SigningKeysForRole(data.CanonicalRootRole)
if err != nil {
logrus.Debug("The root is invalid, because it does not contain any signing keys")
return err
}

errFmtString = "unable to rotate remote key: %s"
remote, err := getRemoteStore(r.baseURL, r.gun, r.roundTrip)
if err == nil {
pubKey, err = remote.RotateKey(role, r.CryptoService, rootKeys...)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the signing functionality currently contained in remote.RotateKey should be happening here and the stores should continue to receive and return on []byte

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm of the opposite opinion :D

} else if _, ok := err.(signed.ErrNoKeys); ok {
err = fmt.Errorf("root signing key unavailable so unable to rotate key")
}
} else {
pubKey, err = r.CryptoService.Create(role, data.ECDSAKey)
errFmtString = "unable to generate key: %s"
}

if err != nil {
return fmt.Errorf(errFmtString, err)
}

if err = r.rootFileKeyChange(role, changelist.ActionCreate, pubKey); err != nil {
return err
}

return r.rootFileKeyChange(role, changelist.ActionCreate, pubKey)
if serverManagesKey {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment to the effect: Key rotation is an offline operation unless we are rotating a remote key, in which case we need to ensure the rotation actually happens

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

return r.Publish()
}
return nil
}

func (r *NotaryRepository) rootFileKeyChange(role, action string, key data.PublicKey) error {
Expand Down
105 changes: 78 additions & 27 deletions client/client_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import (
"github.com/Sirupsen/logrus"
ctxu "github.com/docker/distribution/context"
"github.com/docker/go/canonical/json"
"github.com/gorilla/mux"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for updating this 👍

"github.com/stretchr/testify/assert"
"golang.org/x/net/context"

Expand Down Expand Up @@ -93,13 +94,13 @@ func (p *passRoleRecorder) assertAsked(t *testing.T, expected []string, args ...
var passphraseRetriever = passphrase.ConstantRetriever(password)

func simpleTestServer(t *testing.T, roles ...string) (
*httptest.Server, *http.ServeMux, map[string]data.PrivateKey) {
*httptest.Server, *mux.Router, map[string]data.PrivateKey) {

if len(roles) == 0 {
roles = []string{data.CanonicalTimestampRole, data.CanonicalSnapshotRole}
}
keys := make(map[string]data.PrivateKey)
mux := http.NewServeMux()
m := mux.NewRouter()

for _, role := range roles {
key, err := trustmanager.GenerateECDSAKey(rand.Reader)
Expand All @@ -112,15 +113,15 @@ func simpleTestServer(t *testing.T, roles ...string) (
keyJSON := string(jsonBytes)

// TUF will request /v2/docker.com/notary/_trust/tuf/<role>.key
mux.HandleFunc(
fmt.Sprintf("/v2/docker.com/notary/_trust/tuf/%s.key", role),
m.Methods("GET").Path(
fmt.Sprintf("/v2/docker.com/notary/_trust/tuf/%s.key", role)).HandlerFunc(
func(w http.ResponseWriter, r *http.Request) {
fmt.Fprint(w, keyJSON)
})
}

ts := httptest.NewServer(mux)
return ts, mux, keys
ts := httptest.NewServer(m)
return ts, m, keys
}

func fullTestServer(t *testing.T) *httptest.Server {
Expand Down Expand Up @@ -247,7 +248,7 @@ func TestInitRepositoryManagedRolesIncludingRoot(t *testing.T) {
assert.IsType(t, ErrInvalidRemoteRole{}, err)
// Just testing the error message here in this one case
assert.Equal(t, err.Error(),
"notary does not support the server managing the root key")
"notary does not permit the server managing the root key")
// no key creation happened
rec.assertCreated(t, nil)
}
Expand Down Expand Up @@ -1019,7 +1020,7 @@ func testListEmptyTargets(t *testing.T, rootType string) {

// reads data from the repository in order to fake data being served via
// the ServeMux.
func fakeServerData(t *testing.T, repo *NotaryRepository, mux *http.ServeMux,
func fakeServerData(t *testing.T, repo *NotaryRepository, m *mux.Router,
keys map[string]data.PrivateKey) {

timestampKey, ok := keys[data.CanonicalTimestampRole]
Expand Down Expand Up @@ -1085,54 +1086,54 @@ func fakeServerData(t *testing.T, repo *NotaryRepository, mux *http.ServeMux,
cksmBytes = sha256.Sum256(level2JSON)
level2Checksum := hex.EncodeToString(cksmBytes[:])

mux.HandleFunc("/v2/docker.com/notary/_trust/tuf/root.json",
m.HandleFunc("/v2/docker.com/notary/_trust/tuf/root.json",
func(w http.ResponseWriter, r *http.Request) {
assert.NoError(t, err)
fmt.Fprint(w, string(rootFileBytes))
})
mux.HandleFunc("/v2/docker.com/notary/_trust/tuf/root."+rootChecksum+".json",
m.HandleFunc("/v2/docker.com/notary/_trust/tuf/root."+rootChecksum+".json",
func(w http.ResponseWriter, r *http.Request) {
assert.NoError(t, err)
fmt.Fprint(w, string(rootFileBytes))
})

mux.HandleFunc("/v2/docker.com/notary/_trust/tuf/timestamp.json",
m.HandleFunc("/v2/docker.com/notary/_trust/tuf/timestamp.json",
func(w http.ResponseWriter, r *http.Request) {
fmt.Fprint(w, string(timestampJSON))
})

mux.HandleFunc("/v2/docker.com/notary/_trust/tuf/snapshot.json",
m.HandleFunc("/v2/docker.com/notary/_trust/tuf/snapshot.json",
func(w http.ResponseWriter, r *http.Request) {
fmt.Fprint(w, string(snapshotJSON))
})
mux.HandleFunc("/v2/docker.com/notary/_trust/tuf/snapshot."+snapshotChecksum+".json",
m.HandleFunc("/v2/docker.com/notary/_trust/tuf/snapshot."+snapshotChecksum+".json",
func(w http.ResponseWriter, r *http.Request) {
fmt.Fprint(w, string(snapshotJSON))
})

mux.HandleFunc("/v2/docker.com/notary/_trust/tuf/targets.json",
m.HandleFunc("/v2/docker.com/notary/_trust/tuf/targets.json",
func(w http.ResponseWriter, r *http.Request) {
fmt.Fprint(w, string(targetsJSON))
})
mux.HandleFunc("/v2/docker.com/notary/_trust/tuf/targets."+targetsChecksum+".json",
m.HandleFunc("/v2/docker.com/notary/_trust/tuf/targets."+targetsChecksum+".json",
func(w http.ResponseWriter, r *http.Request) {
fmt.Fprint(w, string(targetsJSON))
})

mux.HandleFunc("/v2/docker.com/notary/_trust/tuf/targets/level1.json",
m.HandleFunc("/v2/docker.com/notary/_trust/tuf/targets/level1.json",
func(w http.ResponseWriter, r *http.Request) {
fmt.Fprint(w, string(level1JSON))
})
mux.HandleFunc("/v2/docker.com/notary/_trust/tuf/targets/level1."+level1Checksum+".json",
m.HandleFunc("/v2/docker.com/notary/_trust/tuf/targets/level1."+level1Checksum+".json",
func(w http.ResponseWriter, r *http.Request) {
fmt.Fprint(w, string(level1JSON))
})

mux.HandleFunc("/v2/docker.com/notary/_trust/tuf/targets/level2.json",
m.HandleFunc("/v2/docker.com/notary/_trust/tuf/targets/level2.json",
func(w http.ResponseWriter, r *http.Request) {
fmt.Fprint(w, string(level2JSON))
})
mux.HandleFunc("/v2/docker.com/notary/_trust/tuf/targets/level2."+level2Checksum+".json",
m.HandleFunc("/v2/docker.com/notary/_trust/tuf/targets/level2."+level2Checksum+".json",
func(w http.ResponseWriter, r *http.Request) {
fmt.Fprint(w, string(level2JSON))
})
Expand All @@ -1146,7 +1147,7 @@ func (k targetSorter) Swap(i, j int) { k[i], k[j] = k[j], k[i] }
func (k targetSorter) Less(i, j int) bool { return k[i].Name < k[j].Name }

func testListTarget(t *testing.T, rootType string) {
ts, mux, keys := simpleTestServer(t)
ts, m, keys := simpleTestServer(t)
defer ts.Close()

repo, _ := initializeRepo(t, rootType, "docker.com/notary", ts.URL, false)
Expand All @@ -1170,7 +1171,7 @@ func testListTarget(t *testing.T, rootType string) {
err = applyChangelist(repo.tufRepo, cl)
assert.NoError(t, err, "could not apply changelist")

fakeServerData(t, repo, mux, keys)
fakeServerData(t, repo, m, keys)

targets, err := repo.ListTargets(data.CanonicalTargetsRole)
assert.NoError(t, err)
Expand Down Expand Up @@ -1202,7 +1203,7 @@ func testListTarget(t *testing.T, rootType string) {
}

func testListTargetWithDelegates(t *testing.T, rootType string) {
ts, mux, keys := simpleTestServer(t)
ts, m, keys := simpleTestServer(t)
defer ts.Close()

repo, _ := initializeRepo(t, rootType, "docker.com/notary", ts.URL, false)
Expand Down Expand Up @@ -1253,7 +1254,7 @@ func testListTargetWithDelegates(t *testing.T, rootType string) {
_, ok = repo.tufRepo.Targets["targets/level2"].Signed.Targets["level2"]
assert.True(t, ok)

fakeServerData(t, repo, mux, keys)
fakeServerData(t, repo, m, keys)

// test default listing
targets, err := repo.ListTargets()
Expand Down Expand Up @@ -2392,14 +2393,16 @@ func TestRotateKeyInvalidRole(t *testing.T) {
repo, _ := initializeRepo(t, data.ECDSAKey, "docker.com/notary", ts.URL, false)
defer os.RemoveAll(repo.baseDir)

// the equivalent of: (root, true), (root, false), (timestamp, true),
// (timestamp, false), (targets, true)
// the equivalent of: (root, true), (root, false), (timestamp, false), (targets, true)
for _, role := range data.BaseRoles {
if role == data.CanonicalSnapshotRole {
if role == data.CanonicalSnapshotRole { // remote or local can manage snapshot
continue
}
for _, serverManagesKey := range []bool{true, false} {
if role == data.CanonicalTargetsRole && !serverManagesKey {
if role == data.CanonicalTargetsRole && !serverManagesKey { // only local can manage targets
continue
}
if role == data.CanonicalTimestampRole && serverManagesKey { // only remote can manage timestamp
continue
}
err := repo.RotateKey(role, serverManagesKey)
Expand Down Expand Up @@ -2567,6 +2570,54 @@ func testRotateKeySuccess(t *testing.T, serverManagesSnapshotInit bool,
}
}

// If remotely rotating key fails for a non-rate-limit reason, fail the rotation
// entirely
func TestRemoteRotationNonRateLimitError(t *testing.T) {
ts, _, _ := simpleTestServer(t)
defer ts.Close()

repo, _ := initializeRepo(t, data.ECDSAKey, "docker.com/notary", ts.URL, true)
defer os.RemoveAll(repo.baseDir)

// simpleTestServer has no rotate key endpoint, so this should fail
err := repo.RotateKey(data.CanonicalTimestampRole, true)
assert.Error(t, err)
assert.IsType(t, store.ErrMetaNotFound{}, err)
}

// The rotator is not the owner of the repository, they cannot rotate the remote
// key
func TestRemoteRotationNoRootKey(t *testing.T) {
ts := fullTestServer(t)
defer ts.Close()

repo, _ := initializeRepo(t, data.ECDSAKey, "docker.com/notary", ts.URL, true)
defer os.RemoveAll(repo.baseDir)
assert.NoError(t, repo.Publish())

newRepo, _ := newRepoToTestRepo(t, repo, true)
defer os.RemoveAll(newRepo.baseDir)
_, err := newRepo.ListTargets()
assert.NoError(t, err)

err = newRepo.RotateKey(data.CanonicalSnapshotRole, true)
assert.Error(t, err)
assert.IsType(t, signed.ErrNoKeys{}, err)
}

// The repo hasn't been initialized, so we can't rotate
func TestRemoteRotationNonexistentRepo(t *testing.T) {
ts, _, _ := simpleTestServer(t)
defer ts.Close()

repo := newBlankRepo(t, ts.URL)
defer os.RemoveAll(repo.baseDir)

err := repo.RotateKey(data.CanonicalTimestampRole, true)
assert.Error(t, err)
assert.IsType(t, ErrRepoNotInitialized{}, err)
}

// If there is no local cache, notary operations return the remote error code
func TestRemoteServerUnavailableNoLocalCache(t *testing.T) {
tempBaseDir, err := ioutil.TempDir("/tmp", "notary-test-")
Expand Down
Loading