Tuf: Add input validation to simple metadata api

Created on 10 Sep 2020  Â·  8Comments  Â·  Source: theupdateframework/tuf

Coordinate with validation guidelines https://github.com/theupdateframework/tuf/issues/1130

Description of issue or feature request:
Some suggestions:

  • Avoid schema, see 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

Most helpful comment

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'

All 8 comments

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

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

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 https://github.com/theupdateframework/tuf/pull/1223#issuecomment-737188686). 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.

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'

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

I wonder if sane defaults will always be possible, e.g. when thinking of the newly added MetadataInfo or TargetInfo in https://github.com/theupdateframework/tuf/pull/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.

In https://github.com/theupdateframework/tuf/pull/1223/commits/7cfd100eeb37a7878741722a1b766b5313022287#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.

Was this page helpful?
0 / 5 - 0 ratings