Skip to content
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

New metadata API: Validate dictionary keys as well as values #1356

Closed
4 tasks
MVrachev opened this issue Apr 20, 2021 · 2 comments · Fixed by #1668
Closed
4 tasks

New metadata API: Validate dictionary keys as well as values #1356

MVrachev opened this issue Apr 20, 2021 · 2 comments · Fixed by #1668
Assignees
Labels
backlog Issues to address with priority for current development goals
Milestone

Comments

@MVrachev
Copy link
Collaborator

MVrachev commented Apr 20, 2021

Description of issue or feature request:
By resolving issue #1139 we are going to add classes for all complex fields inside the top-level metadata classes e.g. meta/ targets (in Timestamp, Snapshot, Targets), delegations (in Targets), keys/roles (in not yet existent Delegation).

The problem is, that many of those complex fields are represented by a dictionary and we are adding classes for the values of
the dictionary in order to easily validate them later, but we should make sure to validate the dictionary keys as well.

Current behavior:
We are not validating dictionary keys yet.

Expected behavior:
We should make sure to validate:

@MVrachev MVrachev added the backlog Issues to address with priority for current development goals label Jul 7, 2021
@sechkova sechkova added this to the Sprint 9 milestone Sep 29, 2021
@jku jku modified the milestones: Sprint 9, Sprint 10 Oct 13, 2021
@MVrachev
Copy link
Collaborator Author

  • KEYID in Root and Delegations

It's validated that keyid is a string inside the Key class during initialization.

During serialization, it's not validated. Maybe there is no sense to add that as Key is an internal helper class and we don't expect the user to modify the Key instances.

Maybe we can validate that the given keyid actually points to the keyval given during the initialization of a Key instance?
It's problematic to validate that keyid points to exactly one keyval.

Validate that role is exactly one of the allowed in the specification: https://theupdateframework.github.io/specification/latest/#root-role

This should happen during instance creation when calling from_dict probably as we are already traversing it there.

In timestamp, we have generally fixed that problem by removing meta and replacing it with snapshot_meta.
For Snapshot, I think we should at least check that METAPATH is a non-empty string.

I think we should at least validate that it's a non-empty string. as defined by the spec.

Maybe we do want to validate that TARGETPATH is not one of the top-level metadata files or roles? That sounds like a reasonable decision to prevent accidental mistakes.

@jku what's your opinion? I ask you as you reviewed other of my prs about validation.

@jku
Copy link
Member

jku commented Oct 26, 2021

With keyids I think there's very little we can or should do.

root rolenames are now validated in 1516. The additional things we maybe could still do are

  • mark roles as a Mapping (to indicate that users should not add or remove values from the dictionary during the lifetime of the Root object).
  • accept roles=None in the constructor (and create the default set in that case). This is not strictly validation but lowers the need for validation since then no-one ever needs to manually create the roles dict.

I think we should at least check that METAPATH is a non-empty string.

I think we should at least validate that it [TARGETPATH]'s a non-empty string

I think these make sense, yes

Maybe we do want to validate that TARGETPATH is not one of the top-level metadata files or roles?

that does not make sense for TARGETPATH but if you meant checking that METAPATH is not in ["root.json", "snapshot.json", "targets.json", "timestamp.json"]... that might make sense as a sanity check.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backlog Issues to address with priority for current development goals
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants