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

Nearexpirywarnings for re-signing roles on read or write #786

Merged
merged 1 commit into from
Jun 15, 2016
Merged

Nearexpirywarnings for re-signing roles on read or write #786

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
5 changes: 4 additions & 1 deletion client/client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -612,7 +612,7 @@ func (r *NotaryRepository) publish(cl changelist.Changelist) error {
// check if our root file is nearing expiry or dirty. Resign if it is. If
// root is not dirty but we are publishing for the first time, then just
// publish the existing root we have.
if nearExpiry(r.tufRepo.Root) || r.tufRepo.Root.Dirty {
if nearExpiry(r.tufRepo.Root.Signed.SignedCommon) || r.tufRepo.Root.Dirty {
rootJSON, err := serializeCanonicalRole(r.tufRepo, data.CanonicalRootRole)
if err != nil {
return err
Expand Down Expand Up @@ -781,7 +781,10 @@ func (r *NotaryRepository) Update(forWrite bool) error {
}
return err
}
// we can be assured if we are at this stage that the repo we built is good
// no need to test the following function call for an error as it will always be fine should the repo be good- it is!
r.tufRepo = repo
warnRolesNearExpiry(repo)
return nil
}

Expand Down
24 changes: 22 additions & 2 deletions client/helpers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -190,9 +190,29 @@ func applyRootRoleChange(repo *tuf.Repo, c changelist.Change) error {
return nil
}

func nearExpiry(r *data.SignedRoot) bool {
func nearExpiry(r data.SignedCommon) bool {
plus6mo := time.Now().AddDate(0, 6, 0)
return r.Signed.Expires.Before(plus6mo)
return r.Expires.Before(plus6mo)
}

func warnRolesNearExpiry(r *tuf.Repo) {
//get every role and its respective signed common and call nearExpiry on it
//Root check
if nearExpiry(r.Root.Signed.SignedCommon) {
logrus.Warn("root is nearing expiry, you should re-sign the role metadata")
}
//Targets and delegations check
for role, signedTOrD := range r.Targets {
//signedTOrD is of type *data.SignedTargets
if nearExpiry(signedTOrD.Signed.SignedCommon) {
logrus.Warn(role, " metadata is nearing expiry, you should re-sign the role metadata")
}
}
//Snapshot check
if nearExpiry(r.Snapshot.Signed.SignedCommon) {
logrus.Warn("snapshot is nearing expiry, you should re-sign the role metadata")
}
//do not need to worry about Timestamp, notary signer will re-sign with the timestamp key
}

// Fetches a public key from a remote store, given a gun and role
Expand Down
51 changes: 51 additions & 0 deletions client/helpers_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,13 @@
package client

import (
"bytes"
"crypto/sha256"
"encoding/json"
"testing"
"time"

log "github.com/Sirupsen/logrus"
"github.com/docker/notary/client/changelist"
"github.com/docker/notary/tuf/data"
"github.com/docker/notary/tuf/testutils"
Expand Down Expand Up @@ -968,3 +971,51 @@ func TestChangeTargetMetaFailsIfPrefixError(t *testing.T) {
require.Empty(t, repo.Targets[data.CanonicalTargetsRole].Signed.Targets)
require.Empty(t, repo.Targets["targets/level1"].Signed.Targets)
}

func TestAllNearExpiry(t *testing.T) {
repo, _, err := testutils.EmptyRepo("docker.com/notary")
require.NoError(t, err)
nearexpdate := time.Now().AddDate(0, 1, 0)
repo.Root.Signed.SignedCommon.Expires = nearexpdate
repo.Snapshot.Signed.SignedCommon.Expires = nearexpdate
repo.Targets["targets"].Signed.Expires = nearexpdate
_, err1 := repo.InitTargets("targets/exp")
require.NoError(t, err1)
repo.Targets["targets/exp"].Signed.Expires = nearexpdate
//Reset levels to display warnings through logrus
orgLevel := log.GetLevel()
log.SetLevel(log.WarnLevel)
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: it'd be nice if this test (and the next one) reverted the log level upon finishing. Can we do something like:

originalLogLevel := log.GetLevel()
// change to log warnings through logrus for testing purposes
log.SetLevel(log.WarnLevel)
// clean up log level upon test completion
defer log.SetLevel(originalLogLevel)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sounds good- coming with the other changes

defer log.SetLevel(orgLevel)
b := bytes.NewBuffer(nil)
log.SetOutput(b)
warnRolesNearExpiry(repo)
require.Contains(t, b.String(), "targets metadata is nearing expiry, you should re-sign the role metadata", "targets should show near expiry")
require.Contains(t, b.String(), "targets/exp metadata is nearing expiry, you should re-sign the role metadata", "targets/exp should show near expiry")
require.Contains(t, b.String(), "root is nearing expiry, you should re-sign the role metadata", "Root should show near expiry")
require.Contains(t, b.String(), "snapshot is nearing expiry, you should re-sign the role metadata", "Snapshot should show near expiry")
require.NotContains(t, b.String(), "timestamp", "there should be no logrus warnings pertaining to timestamp")
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we also set the timestamp expiry date to nearexpdate, and assert that b does not contain any information about the timestamp?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Timestamp isn't being warned about since @riyazdf mentioned that Notary deals with that role and the user needs to worry about it. Hence there is no warning output related to it. Also, the role will always throw a warning since defaults are lower than the 6 month threshold. Let me know what you think, I'm okay to do it either ways but what Riyaz mentioned seemed to make sense

Copy link
Contributor

Choose a reason for hiding this comment

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

Right - so this would just test that we don't warn about the timestamp even though it is near expiry.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will edit this as per your recommendation. Thanks for the suggestion! +1


func TestAllNotNearExpiry(t *testing.T) {
repo, _, err := testutils.EmptyRepo("docker.com/notary")
require.NoError(t, err)
notnearexpdate := time.Now().AddDate(0, 10, 0)
repo.Root.Signed.SignedCommon.Expires = notnearexpdate
repo.Snapshot.Signed.SignedCommon.Expires = notnearexpdate
repo.Targets["targets"].Signed.Expires = notnearexpdate
_, err1 := repo.InitTargets("targets/noexp")
require.NoError(t, err1)
repo.Targets["targets/noexp"].Signed.Expires = notnearexpdate
//Reset levels to display warnings through logrus
orgLevel := log.GetLevel()
log.SetLevel(log.WarnLevel)
defer log.SetLevel(orgLevel)
a := bytes.NewBuffer(nil)
log.SetOutput(a)
warnRolesNearExpiry(repo)
require.NotContains(t, a.String(), "targets metadata is nearing expiry, you should re-sign the role metadata", "targets should not show near expiry")
require.NotContains(t, a.String(), "targets/noexp metadata is nearing expiry, you should re-sign the role metadata", "targets/noexp should not show near expiry")
require.NotContains(t, a.String(), "root is nearing expiry, you should re-sign the role metadata", "Root should not show near expiry")
require.NotContains(t, a.String(), "snapshot is nearing expiry, you should re-sign the role metadata", "Snapshot should not show near expiry")
require.NotContains(t, a.String(), "timestamp", "there should be no logrus warnings pertaining to timestamp")
}
19 changes: 0 additions & 19 deletions tuf/testutils/repo.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,6 @@ import (
"github.com/docker/notary/passphrase"
"github.com/docker/notary/trustmanager"
"github.com/docker/notary/tuf/data"
"github.com/docker/notary/tuf/utils"
fuzz "github.com/google/gofuzz"
"github.com/stretchr/testify/require"

tuf "github.com/docker/notary/tuf"
Expand Down Expand Up @@ -142,23 +140,6 @@ func CopyRepoMetadata(from map[string][]byte) map[string][]byte {
return copied
}

// AddTarget generates a fake target and adds it to a repo.
func AddTarget(role string, r *tuf.Repo) (name string, meta data.FileMeta, content []byte, err error) {
randness := fuzz.Continue{}
content = RandomByteSlice(1024)
name = randness.RandString()
t := data.FileMeta{
Length: int64(len(content)),
Hashes: data.Hashes{
"sha256": utils.DoHash("sha256", content),
"sha512": utils.DoHash("sha512", content),
},
}
files := data.Files{name: t}
_, err = r.AddTargets(role, files)
return
}

// RandomByteSlice generates some random data to be used for testing only
func RandomByteSlice(maxSize int) []byte {
r := rand.New(rand.NewSource(time.Now().UnixNano()))
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 removing the unused function!

Expand Down