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.
cyli
force-pushed
the
do-not-sign-if-no-targets-change
branch
from
b765789 to
62c836f
Compare
… signing keys Signed-off-by: Ying Li <ying.li@docker.com>
cyli
force-pushed
the
do-not-sign-if-no-targets-change
branch
2 times, most recently
from
2ebe68a to
96a9e1a
Compare
tuf/tuf_test.go
Outdated
| @@ -917,8 +952,12 @@ func TestRemoveTargetsNoSigningKeys(t *testing.T) { | |||
| // now delete the signing key (all keys) | |||
| repo.cryptoService = signed.NewEd25519() | |||
|
|
|||
| // 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>
cyli
force-pushed
the
do-not-sign-if-no-targets-change
branch
from
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.
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.