Do not require signing if no there are no targets changes #1104
Conversation
| } | ||
|
|
||
| // Removing targets from a role that exists but without metadata succeeds. | ||
| func TestRemoveTargetsNonexistentMetadata(t *testing.T) { |
There was a problem hiding this comment.
Note - this test didn't actually seem to do what it claimed - the body was a duplicate of TestRemoveTargetsRoleDoesntExist. Since the previous test was supposed to test adding existing and non-existing targets anyway, I just removed this test and updated the previous one.
b765789 to
62c836f
Compare
… signing keys Signed-off-by: Ying Li <ying.li@docker.com>
2ebe68a to
96a9e1a
Compare
riyazdf
left a comment
There was a problem hiding this comment.
code LGTM
Only thing is I'm wondering if this will this be an issue for re-signing the contents of an almost expired targets file?
tuf/tuf_test.go
Outdated
|
|
||
| // now remove the target - it should fail | ||
| err = repo.RemoveTargets("targets/test", "f") | ||
| // remove a nonexistant target - it should not fail |
|
@riyazdf I only changed |
…igning keys Signed-off-by: Ying Li <ying.li@docker.com>
96a9e1a to
187d076
Compare
|
@cyli - makes sense, I agree with that. This could be an extension of |
| if f.Custom == nil && o.Custom == nil { | ||
| return true | ||
| } | ||
| fBytes, err := f.Custom.MarshalJSON() |
There was a problem hiding this comment.
Custom is of type *json.RawMessage which is just a re-typing of []byte. I'm wondering if it makes sense to unmarshal into a map[string]interface{} and remarshal canonically to determine actual equivalence vs face value byte for byte equality.
There was a problem hiding this comment.
I guess on the other hand, there's no guarantee it's JSON inside the Custom field so maybe that's not worth doing.
| if o.Length != f.Length || len(f.Hashes) != len(f.Hashes) { | ||
| return false | ||
| } | ||
| if f.Custom == nil && o.Custom != nil || f.Custom != nil && o.Custom == nil { |
There was a problem hiding this comment.
This and the custom checks further down could probably all just be replaced with if *f.Custom != *o.Custom { return false }
There was a problem hiding this comment.
If f.Custom is nil, won't we get a panic trying to get *f.Custom? Also, if neither were nil, I think that still wouldn't work because the equality operator doesn't work on byte slices. :| For instance: https://play.golang.org/p/jY4yrw7lrL
There was a problem hiding this comment.
ugh, yeah. I was thinking of the RawMessage being a []byte but we essentially have double indirection here. I remember we specifically put that in because of the whole "addressability" thing cause the RawMessage to get double base64 encoded, if that's not necessary any more (I feel like I remember something addressing RawMessage and this problem being merged in Go 1.7, possible 1.6) we could also make Custom a non-pointer RawMessage.
endophage
left a comment
There was a problem hiding this comment.
LGTM. We'll work out the RawMessage pointer or not in a separate PR at another time.
AddTargetandRemoveTargetcurrently checks if the user has valid signing keys regardless of whether the target actually needs to be added or removed, because both require signing regardless of whether the target needs to be added or removed.Updates both functions so that if the target being added does not actually change the role, or if the target being removed doesn't actually change the role, signing keys are not required and the roles are not necessarily marked as dirty.