Add input validation to simple metadata api

[lukpueh]

Coordinate with validation guidelines #1130

Description of issue or feature request:
Some suggestions:

• Avoid schema, see Revise schema and formats facility secure-systems-lab/securesystemslib#183 (just don't use it)
• Make use of type hints for simple type validation
• Perform additional non-metadata parameter validation at user boundary
• Provide methods to validate JSON representation at user boundary, i.e. fail on bad json metadata in from_json_file/to_json_file method, but with option to
  disable check as there might be a justified reason to read or write WIP
  metadata to json.
• Be lenient on bad/invalid metadata objects in memory, they might be
  work in progress. E.g. it might be convenient to create empty metadata
  and assign attributes later on.
• Consider using in-toto style ValidationMixin (see the mixin and it's usage for details).

Current behavior:
No input validation

Expected behavior:
Add input validation

[joshuagl]

• Consider using in-toto style ValidationMixin (see the mixin and it's usage for details).

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.

• https://aboutsimon.com/blog/2018/04/04/Python3-Type-Checking-And-Data-Validation-With-Type-Hints.html
• https://realpython.com/primer-on-python-decorators

[trishankatdatadog]

Another idea is to copy validation techniques Django/Rails use...

[joshuagl]

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.

[lukpueh]

Good 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, marshmallow and pydantic, 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.

[joshuagl]

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)).

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'

[joshuagl]

The __slots__ mechanism, described in the Descriptor HowTo, could be something worth including in our new classes also. hat-tip @sechkova

[lukpueh]

I wonder if sane defaults will always be possible, e.g. when thinking of the newly added MetadataInfo or TargetInfo in #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.

[joshuagl]

In 7cfd100#r541018751 we discussed whether JsonDict is the right name for our generic Dict[str, Any] type, and agreed to handle that type's name with this issue instead.

[MVrachev]

In a discussion with @jku, we noticed that our tests for the metadata classes don't test instantiating metadata objects without their required fields.
We should add tests for that.

[MVrachev]

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,

[MVrachev]

@lukpueh and/or @joshuagl do you agree with me?

[joshuagl]

Agreed, thanks Martin. Closing because we have implemented validation on a case-by-case basis.