Nearexpirywarnings for re-signing roles on read or write #786
Conversation
avaid96
force-pushed
the
nearexpirywarnings
branch
2 times, most recently
from
2af0156 to
2d9883a
Compare
avaid96
force-pushed
the
nearexpirywarnings
branch
from
2d9883a to
490b9c7
Compare
| //get every role and its respective signed common and call nearExpiry on it | ||
| //Root check | ||
| //Reset levels to display warnings through logrus | ||
| logrus.SetLevel(logrus.WarnLevel) |
There was a problem hiding this comment.
we shouldn't actually change the log level here, so we can remove this line
There was a problem hiding this comment.
Sounds good, editing the commit
avaid96
force-pushed
the
nearexpirywarnings
branch
2 times, most recently
from
6e82bce to
111b01d
Compare
| //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 key") |
There was a problem hiding this comment.
I think the log warning message should indicate that we need to re-sign the role metadata, not the key itself (same for targets and snapshot below)
There was a problem hiding this comment.
coming with the other changes
avaid96
force-pushed
the
nearexpirywarnings
branch
from
111b01d to
36492e5
Compare
| if nearExpiry(r.Snapshot.Signed.SignedCommon) { | ||
| logrus.Warn("snapshot is nearing expiry, you should re-sign the role metadata") | ||
| } | ||
| //Timestamp is not checked since the user doesn't need to worry about it, we deal with it |
There was a problem hiding this comment.
can we clarify the we deal with it part by saying something like ...doesn't need to worry about it, notary signer will re-sign with the timestamp key?
There was a problem hiding this comment.
will do with other changes (if necessary)
|
thank you for adding this in, LGTM after comments! |
|
jenkins, test this please |
| 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") | ||
| } |
There was a problem hiding this comment.
Could we also set the timestamp expiry date to nearexpdate, and assert that b does not contain any information about the timestamp?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Right - so this would just test that we don't warn about the timestamp even though it is near expiry.
There was a problem hiding this comment.
Will edit this as per your recommendation. Thanks for the suggestion! +1
|
LGTM pending comments - thanks so much for adding the warnings! |
Signed-off-by: avaid96 <avaid1996@gmail.com>
avaid96
force-pushed
the
nearexpirywarnings
branch
from
36492e5 to
2bee3f7
Compare
|
jenkins, test this please |
|
LGTM! |
Warnings are now output on read and write actions. Should indicate which role will be expiring soon and will need to be re-signed.
Also deleted an unused function in testutils- AddTarget
closes #733