Skip to content

(WIP) ASN.1: Initial commit of asn1 metadata definitions #779

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

Closed
wants to merge 2 commits into from

Conversation

awwad
Copy link
Contributor

@awwad awwad commented Aug 30, 2018

Currently, the TUF reference implementation uses only a JSON-compatible metadata format defined in the TUF specification.

It's my intention to add ASN.1 metadata support to the TUF reference implementation (decoding, encoding, signing over, verifying signatures over). If necessary, this can be made into an optional feature or even an external package. Either way, the reference implementation would profit from having an easy means of using ASN.1 metadata.

In this PR, for conversation purposes, is a draft of the metadata definitions (in ASN.1 and in a pyasn1-compatible module).

both in abstract ASN.1 definition language and a format compatible
with pyasn1.

These will be used for conversion of TUF metadata between the
JSON-compatible internal TUF metadata format and ASN.1/DER.

Signed-off-by: Sebastien Awwad <[email protected]>
@awwad
Copy link
Contributor Author

awwad commented Sep 4, 2018

@alyptik Any thoughts on these? (Choices for types, structure)

Copy link

@alyptik alyptik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks sane to me; the small issues I did find were pointed out with comments (and may not even be issues).

e.g. use: DefaultedNamedType('terminating', univ.Boolean(0))
instead of: DefaultedNamedType('terminating', univ.Boolean().subtype(value=0))

- Move superclass value overrides into bodies of classes
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very good modification, ACK

# For thresholds, versions, timestamps, etc., we want only non-negative
# integers, so we're stuck setting a maximum (or not placing any constraints
# at all in the ASN.1 metadata definition itself, would would also be fine).
MAX = 2**32-1
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this inclusive or exclusive?

Copy link
Contributor Author

@awwad awwad Sep 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's inclusive, which is a little odd, but that's why this is -1. That is: ValueRangeConstraint(), where this is used below, is inclusive. http://snmplabs.com/pyasn1/pyasn1/type/constraint/valuerange.html

class HashOfSnapshot(univ.Sequence):
componentType = NamedTypes(
NamedType('filename', FileName()),
NamedType('num-hashes', Length()),
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason Length() is reused for num-keys, num-keyids, and num-hashes? To clarify, is it possible to have a more descriptive name?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess we could call it SequenceLength. Do you think that's better? Or do you think it's clearer if I just exclude it and use an integer primitive directly in each of these places?



A note for implementers of TUF-specification-conformant systems:
There are constraints and optimizations that you can bake into your ASN.1
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was this meant to be "These are..."? If so, a better wording might be "The following are..."

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, it's intended to be "There are", i.e. "There exist". I'll reword it to make it clearer.



Questions:
- Would it be better to use fewer empty wrapped classes? e.g. is it better to
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm personally not a fan of abstraction which isn't used for some form of data hiding. My vote would be towards yes, less empty classes.


- I've tried to minimize the differences between this ASN.1 definition set and
the TUF spec (which uses JSON). Consider each:
- No dictionary structures (not clear it's possible?); instead, former
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a perfectly fine way to store dictionaries inside lists, ACK

Once TAP 5 is accepted and implemented,	root role metadata will
allow for an optional URL list element for each top-level role.
See TAP 5 for more details.

The placeholders here are commented-out.

Signed-off-by: Sebastien Awwad <[email protected]>
@joshuagl
Copy link
Member

We plan to include this functionality in a future release of tuf (or securesystemslib) once we have completed refactoring efforts.

@joshuagl joshuagl closed this Sep 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants