Repository navigation
Add input validation to simple metadata api #1140
Description
Activity
- added a commit that references this issue
on Sep 10, 2020 - added a commit that references this issue
on Sep 17, 2020 This looks like a nice pattern. Each class defines appropriate validation methods, which are called by the Mixin's
validate()method at the end of the object's construction.Any thoughts on whether a decorator might be a more idiomatic approach than a Mixin? We could wrap classes with a decorator which performs validation after init, before returning the instantiated object.
Reacted by Trishank Karthik KuppusamyAnother idea is to copy validation techniques Django/Rails use...
Nice idea. I took a brief look those look to be quite complicated and not equivalent to what we're trying to do here Django Model validation.
Reacted by Trishank Karthik KuppusamyGood thinking, @trishankatdatadog. But I agree with @joshuagl that we might not need something as powerful (Django's validation is really tailored towards web forms and/or ORM, similar is true for
WTForms, which I also briefly considered).I think the in-toto approach is not so bad. It's also featured in this quite interesting blog post about different instance attribute validation techniques, at least the "individual validation functions" aspect (not the neat self-inspecting mixin part).
The blog also mentions two promising validation libraries,
marshmallowandpydantic, which both are actually de/serialization libraries with validation features. Given that we want to minimize dependencies (see #1165), I'm leaning towards rolling our own validation, which doesn't even have to be that generic (see secure-systems-lab/securesystemslib#183).The blog also mentions a Python built-in feature, i.e.
Descriptors, that as per official docs seems well-suited for attribute validators. Although it looks interesting, I'm unsure if it gives us the flexibility of e.g. initializing empty objects, assigning values, and only then calling validate, which might be a desirable usage pattern (see snippet 2 in #1223 (comment)). Same reservation goes for decorators, which the blog also mentions in conjunction with descriptors. But I can check if there is a solution that involves decorators and/or descriptors that allows for said flexibility.What I like about both the decorator and descriptor approach(es) is that they make the constraints on the attributes more visible (in the head of the class definition) than vanilla validation methods.
Reacted by Trishank Karthik Kuppusamy, Joshua Lock and Marina MooreThe blog also mentions a Python built-in feature, i.e.
Descriptors, that as per official docs seems well-suited for attribute validators. Although it looks interesting, I'm unsure if it gives us the flexibility of e.g. initializing empty objects, assigning values, and only then calling validate, which might be a desirable usage pattern (see snippet 2 in #1223 (comment)).Descriptors look nice, and I'm all for avoid additional dependencies. I believe the pattern of: initialising empty objects, assigning values, and then validating should still work – so long as our empty objects have sane defaults. Based on 5mins experimentation in the Python interpreter:
>>> class MetadataType: ... def __get__(self, obj, objtype=None): ... return self.value ... def __set__(self, obj, value): ... if value not in ['root', 'timestamp', 'snapshot', 'targets']: ... raise ValueError('Invalid _type field') ... self.value = value ... >>> class Metadata: ... type = MetadataType() ... def __init__(self, type): ... self.type = type ... >>> class Targets(Metadata): ... def __init__(self): ... super().__init__('targets') ... >>> md = Metadata('badger') Traceback (most recent call last): File "<stdin>", line 1, in <module> File "<stdin>", line 4, in __init__ File "<stdin>", line 6, in __set__ ValueError: Invalid _type field >>> md = Metadata('root') >>> md.type = 'badger' Traceback (most recent call last): File "<stdin>", line 1, in <module> File "<stdin>", line 6, in __set__ ValueError: Invalid _type field >>> t = Targets() >>> t.type 'targets'Reacted by Trishank Karthik Kuppusamy and Marina MooreReacted by Lukas Pühringer and Martin VrachevThe
__slots__mechanism, described in the Descriptor HowTo, could be something worth including in our new classes also. hat-tip @sechkovaI wonder if sane defaults will always be possible, e.g. when thinking of the newly added
MetadataInfoorTargetInfoin #1223.But maybe the usage pattern that requires initialization of empty objects is suboptimal. I must say that it would be quite nice to always have certainty about the validity of tuf objects.
- added 2 commits that reference this issue
on Dec 10, 2020 - added a commit that references this issue
on Jan 5, 2021 - added 2 commits that reference this issue
on Jan 18, 2021 In 7cfd100#r541018751 we discussed whether
JsonDictis the right name for our genericDict[str, Any]type, and agreed to handle that type's name with this issue instead.I think we can lose this issue as we discussed that each of the cases has to be discussed separately.
We don't need a common strategy to do this.We had discussions validation for all cases separately:
- Signed attributes: Metadata Attribute research: version #1418, New metadata API: spec_version attribute validation #1430, Metadata Attribute research: expires #1420
- Key attributes: Metadata Attribute research: KEY #1438
- Role attributes: 0c3131b, Metadata Attribute research: Threshold #1439
- Root: Metadata API: validate root role names #1630
- MetaFile: api/metadata input validation: length and hashes #1451
- DelegatedRole: 76d4633, aa480b1, Metadata API: prevent Delegation role names to be one of top level metadata roles #1558, Metadata API: Delegation role names validation #1527 (still open)
- TargetFile: api/metadata input validation: length and hashes #1451
- Targets: we didn't find need to validate it
- Additional: New metadata API: Validate dictionary keys as well as values #1356,
Agreed, thanks Martin. Closing because we have implemented validation on a case-by-case basis.
Coordinate with validation guidelines #1130
Description of issue or feature request:
Some suggestions:
from_json_file/to_json_filemethod, but with option todisable check as there might be a justified reason to read or write WIP
metadata to json.
work in progress. E.g. it might be convenient to create empty metadata
and assign attributes later on.
ValidationMixin(see themixinand it's usage for details).Current behavior:
No input validation
Expected behavior:
Add input validation