Metadata API: Add Key attributes types validation #1449
Conversation
In our discussion with Jussi we come to the conclusion that we want to verify that all Key attributes contain values in the expected types, but at the same time, we don't want to focus on validating the semantics behind them. The reason is that having a Key instance with invalid attributes is possible and supported by the spec. That's why we have a "threshold" for the roles meaning we can have up to a certain number of invalid Keys until we satisfy the required threshold. Also, for deeper semantic validation it's better to be done in securesystemslib which does the actual work with keys. For context see: theupdateframework#1438 Signed-off-by: Martin Vrachev <[email protected]>
|
Should I assume from this PR that we want to implement the validation checks of the various metadata classes in the constructor and not in the deserialization methods ( |
From my understanding, yes. The reason is that this way no matter how we are creating Key objects - directly through |
There was a problem hiding this comment.
This seems like the approach we have to take: explicit type checking is ewww, but we don't want to / cannot try to understand the semantics of our input at serialization time so it probably makes sense.
The repetition of if-raise could easily be avoided (as the unique error messages are not that important here) so the whole thing would be just 3 lines but I'm fine with this as well, your choice. Left one simplification suggestion though
tuf/api/metadata.py
Outdated
| public_val = keyval.get("public") | ||
| if not public_val or not isinstance(public_val, str): |
There was a problem hiding this comment.
We're relying on KeyErrors from index access elsewhere in serialization (and this code also relies on TypeError if keyval is not a dict). I think letting index access fail with keyerror is simpler and works just as well:
if not isinstance(keyval["public"], str):
|
I experimented with a shorter variation. |
MVrachev
force-pushed
the
key-validation
branch
2 times, most recently
from
9fe15ff to
9630a10
Compare
tuf/api/metadata.py
Outdated
| raise ValueError("keyval doesn't follow the specification format!") | ||
| val = keyval["public"] | ||
| if not all(isinstance(at, str) for at in [keyid, keytype, scheme, val]): | ||
| raise ValueError("Attributes don't follow the type annotations!") |
There was a problem hiding this comment.
LGTM, I agree getting rid of the repetition outweighs the slightly more complex code.
The message is really not terribly important but maybe something like "Unexpected Key attributes" might be better than referring to annotations?
approved in any case.
There was a problem hiding this comment.
Updated with Unexpected Key attributes types! message.
Clarify that we don't semantically validate "Key" instances during initialization and that this is a responsibility of securesystemslib. Signed-off-by: Martin Vrachev <[email protected]>
In pr theupdateframework#1449 we introduced Key validation and there decided to raise "ValueError" if one of keyid, scheme, keyval or keytype is not a string. That seems like a mistake given that in the python docs it's written: "Passing arguments of the wrong type (e.g. passing a list when an int is expected) should result in a TypeError, but passing arguments with the wrong value (e.g. a number outside expected boundaries) should result in a ValueError." Signed-off-by: Martin Vrachev <[email protected]>
In pr theupdateframework#1449 we introduced Key validation and there decided to raise "ValueError" if one of keyid, scheme, keyval or keytype is not a string. That seems like a mistake given that in the python docs it's written: "Passing arguments of the wrong type (e.g. passing a list when an int is expected) should result in a TypeError, but passing arguments with the wrong value (e.g. a number outside expected boundaries) should result in a ValueError." Signed-off-by: Martin Vrachev <[email protected]>
In pr theupdateframework#1449 we introduced Key validation and there decided to raise "ValueError" if one of keyid, scheme, keyval or keytype is not a string. That seems like a mistake given that in the python docs it's written: "Passing arguments of the wrong type (e.g. passing a list when an int is expected) should result in a TypeError, but passing arguments with the wrong value (e.g. a number outside expected boundaries) should result in a ValueError." Signed-off-by: Martin Vrachev <[email protected]>
In pr theupdateframework#1449 we introduced Key validation and there decided to raise "ValueError" if one of keyid, scheme, keyval or keytype is not a string. That seems like a mistake given that in the python docs it's written: "Passing arguments of the wrong type (e.g. passing a list when an int is expected) should result in a TypeError, but passing arguments with the wrong value (e.g. a number outside expected boundaries) should result in a ValueError." Signed-off-by: Martin Vrachev <[email protected]>
Fixes #1438
Description of the changes being introduced by the pull request:
In our discussion with @jku , we come to the conclusion that we want
to verify that all Key attributes contain values in the expected types,
but at the same time, we don't want to focus on validating the semantics
behind them.
The reason is that having a Key instance with invalid attributes is
possible and supported by the spec.
That's why we have a "threshold" for the roles meaning we can have up to
a certain number of invalid Keys until we satisfy
the required threshold.
Also, for deeper semantic validation it's better to be done in
securesystemslib which does the actual work with keys.
Signed-off-by: Martin Vrachev [email protected]
Please verify and check that the pull request fulfills the following
requirements: