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

Update example metadata. Fix issue #436. #437

Closed
wants to merge 2 commits into from

Conversation

vladimir-v-diaz
Copy link
Contributor

The metadata that is used by the unit and integration tests (available here) are the most up-to-date. This pull request moves the relevant metadata, key files, target files, etc. to the examples directory to address issue #436.

Please note that the Trident modifications have not yet been merged into the "develop" branch. An implementation of the map file is awaiting a review in PR #430, and support for multi-role delegations is available in Uptane, however, it is based on an old TAP/design that will need changing before being merged into TUF.

This pull request includes metadata that supports the specification and design goals of Mercury and Diplomat.

@vladimir-v-diaz
Copy link
Contributor Author

The example metadata listed in the specification should be up-to-date as well (excluding Trident modifications).
https://github.com/theupdateframework/tuf/blob/40aaf93f5a39f659cc312b66624f2cbff59b8582/docs/tuf-spec.txt#L658-L689

@coveralls
Copy link

Coverage Status

Coverage remained the same at 97.936% when pulling ed41796 on example_metadata into 40aaf93 on develop.

@vladimir-v-diaz
Copy link
Contributor Author

ping @awwad

@awwad
Copy link
Contributor

awwad commented Jul 12, 2017

re the linked metadata example in the spec: there's a capitalized role name and it includes root's hash in snapshot. The former should be changed either here or in #472 (after #472 is rebased). When are we changing the latter?

(Also, you made some of the ed25519 keys RSA keys. No particular reason, I assume?)

@vladimir-v-diaz
Copy link
Contributor Author

vladimir-v-diaz commented Jul 12, 2017 via email

"_type": "Snapshot",
"expires": "2030-01-01T00:00:00Z",
"_type": "Snapshot",
"expires": "2030-01-01T00:00:00Z",
Copy link
Contributor

@awwad awwad Jul 12, 2017

Choose a reason for hiding this comment

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

Trailing whitespace here makes two lines that did not change look like they have changed. There's more like this above and below, too. Was this hand-edited or written out by repository_tool? (Are the signatures valid?)

Copy link
Contributor Author

@vladimir-v-diaz vladimir-v-diaz Jul 12, 2017

Choose a reason for hiding this comment

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

I have it set up where my editor removes trailing whitespace. This might be the cause of the issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, geez. I looked at it backwards and processed it as whitespace added, not removed. Well, the other question stands, I suppose: are the signatures valid?

@awwad
Copy link
Contributor

awwad commented Jul 12, 2017

Will number these.
Call the two old questions/points 1 & 2:

  1. As mentioned, the metadata examples use capitalized role names, both in the spec and the examples directory. The former should be changed either here or in Use consistent naming of role names in metadata #472 (after Use consistent naming of role names in metadata #472 is rebased).

  2. Root's hash is included in snapshot. Where are we changing that?

Here are some more:

  1. In your PR description, you say that you're moving the examples from the test data folder to the examples folder, but it looks like you're adding new stuff to examples/ and leaving the test data in test_data. Is that intended?

  2. You made some of the ed25519 keys RSA keys. No particular reason, I assume? (just what you happened to generate? or is there a problem?)

  3. keyid_hash_algorithms was added to the public keys. I remember talking about expecting this to show up in public keys, but was this change intended here? Did we change code that generated keys, or is this an artifact of the switch to RSA keys? (In which case we should make this consistent across key types.)

  4. There's some trailing whitespace in some of the metadata files that makes me wonder if it's hand edited and makes several lines look like they've changed when they haven't. Are the signatures correct? Did whitespace somehow get added by repository_tool?

@vladimir-v-diaz
Copy link
Contributor Author

@awwad
Do you think we should have only one set of example metadata (available here)? The example metadata in METADATA.md is actually the metadata used in the unit and integration tests, so they are the most up to date. It also avoids the problem of having to edit two sets of example metadata when a change is made to the specification.

There's some trailing whitespace in some of the metadata files that makes me wonder if it's hand edited and makes several lines look like they've changed when they haven't. Are the signatures correct? Did whitespace somehow get added by repository_tool?

My editor likely stripped the trailing whitespace. The signatures might not be affected because metadata is always canonicalized. However, the hashes might...

@awwad
Copy link
Contributor

awwad commented Jul 25, 2017

There's sample or test metadata in several places:

  1. tuf spec (and perhaps other documentation)
  2. examples/ root directory
  3. tests/test_data directory

You could combine 2 and 3. Uptane has sample metadata in a root-level samples/ directory and is shifting to using that data for testing purposes (instead of data kept in tests/test_data), but this may prove to be too much clutter in the samples/ directory... so I don't really have a good answer for you. Yes, I think that having example data in too many places is bad. I would lean toward combining 2 and 3, but I can't say that with a lot of conviction.

@awwad
Copy link
Contributor

awwad commented Sep 29, 2017

Are you waiting for any further comments here?

@vladimir-v-diaz
Copy link
Contributor Author

vladimir-v-diaz commented Sep 29, 2017

No, I'm not waiting for further comments. I'll likely work on this soon to combine 2 and 3.

@coveralls
Copy link

coveralls commented Sep 29, 2017

Coverage Status

Coverage remained the same at 99.293% when pulling 868269a on example_metadata into e45084f on develop.

@vladimir-v-diaz
Copy link
Contributor Author

Items (2) and (3) here were combined in #496. Closing this pull request since it's been fixed in #496.

@vladimir-v-diaz vladimir-v-diaz deleted the example_metadata branch April 13, 2018 17:29
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