-
Notifications
You must be signed in to change notification settings - Fork 271
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.
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
Metadata API: Simplify unrecognized fields testing #1466
Conversation
Directly relevant to theupdateframework/specification#169 |
I think there's a difference between unrecognized and unauthorized fields. Unrecognized fields in the signature wrapper (which are also unauthorized) could be dangerous as they could add Therefore, I prefer the simplicity of allowing these unrecognized fields anywhere within the signed metadata to allow for extensions to the specification to be backwards compatible. |
We cannot allow unrecognized fields everywhere if we follow the spec.
and if a in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe the ADR change could be another PR? I'm not as invested in tweaking the ADR text but would like to see the tests land...
Anyway for ADR text:
- I wouldn't explain the reasons further than "don't store unrecognized fields when it is not allowed by specification: keys, roles, meta, hashes and targets are actual dictionaries (vs json objects that most structures in the format are) where 'unrecognised field' is not meaningful concept". This seems to cover it?
- there's a lot of added text that is IMO not necessary and that would require review: security claims (like "possibility to inject keys, compromise trust") do not seem useful when they refer to metadata that is signed and thus by definition able to add keys and compromise trust.
For the tests:
- I think this is a good way to do this (better than writing code anyway), I have no major comments
- Question: Have you reviewed the file format and made sure we are testing every possible dictionary?
- nitpick: Always adding a trailing comma after the last item in the test dataset would make the next PR easier to read
tests/test_metadata_serialization.py
Outdated
@@ -51,8 +51,11 @@ def wrapper(test_cls: "TestSerialization"): | |||
class TestSerialization(unittest.TestCase): | |||
|
|||
valid_keys: DataSet = { | |||
# Do we want to add unrecognized fields in the Key class? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# Do we want to add unrecognized fields in the Key class? |
this may be a valid question but maybe not useful in test code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will remove it, but wanted to ask raise that question.
Do we want to allow unrecognized fields in the Key class?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a desire to be able to store unrecognised fields in a keyval, see theupdateframework/specification#163 and theupdateframework/go-tuf#133
f90184f
to
d0a25bf
Compare
I updated this pr to include only the changes related to the tests. After the @joshuagl comment, I included a test that we do support unrecognized fields in
Yes, I reviewed the file formats yet again and it seems okay. The only places where we don't allow unrecognized fields is where I described above.
Fixed that. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the commented question is still in the test data?
anyway, looks fine, thanks
We have merged ADR 8 allowing for unrecognized fields and we have added tests for that which are too specific and not scalable. Now, I use table testing which we have used initially in theupdateframework#1416 to test unrecognized fields support in a cleaner and much more readable way. Signed-off-by: Martin Vrachev <mvrachev@vmware.com>
d0a25bf
to
f34cc7e
Compare
Removed the question, but still - it's okay to have unrecognized fields there, right? |
I don't see why not -- keyval is even designed for that, AFAICT (as the contents are not defined for keys in general). |
Fixes #1464
Description of the changes being introduced by the pull request:
We have merged ADR 8 allowing for unrecognized fields everywhere.
After a discussion with Jussi, we realized that there are a couple of
places where we don't want to allow
unrecognized fields
because theyare sensitive or there are limited acceptable values for them.
The places where we don't want to allow unrecognized fields are
keys
,roles
,meta
,hashes
ortargets
.If we allow unrecognized fields in them we won't follow the spec or
even open the door for possible security vulnerability.
Second, the test we added for
unrecognized fields
added tests for that which are too specific and not scalable.
Now, I use table testing which we have used initially in #1416
to test unrecognized fields support in a cleaner and much more readable
way.
Please verify and check that the pull request fulfills the following
requirements: