TAP 7: Simplify and Augment: Wrapper Module, Fewer Return Values, Description of Data Provided #30

awwad · 2017-05-24T20:03:52Z

The primary changes in this PR to the way that TAP 7 works are:

Shift from a model involving command line instructions to a model involving a "Wrapper" module that is expected to implement three functions. Also provide a working example of a Wrapper for the TUF Reference Implementation.
Move from many return codes to just 3 options: Success 0, Failure 1, or (for debugging purposes) Unexpected Error 2
Describe the data that will be provided by the Tester to the (Wrapper and) Updater
Solidify terminology
Make clear that other implementations aren't expected to be in Python
TAP 4 support (optional)
Trim some config file options (root keys and thresholds) and add some others (TAP 4, delegations, mirrors)

Commit summaries can provide more details.

- References to compliance appeared in a few places. The document now consistently refers only to conformance. - For clarity, capitalized terms Wrapper and Updater are used.

Also bump the document version number from 1 to 2 and add a necessary heading 'Expected Output' to be linked to.

and make them less redundant

awwad · 2017-05-25T18:25:48Z

Okay, I think this is worth glancing over now, @JustinCappos, to see if the general scheme is satisfactory. (Also, @vladimir-v-diaz, LMK if it looks fishy compared to what we've discussed?)

Note that I haven't very substantially modified the Abstract, Motivation, or Rationale sections.

vladimir-v-diaz · 2017-05-25T18:48:23Z

tap7.md

+communicate with it. This will need to involve at least a few lines of Python.
+In order for the Tester to interact with the Updater implementation, a Wrapper
+around that implementation will need to support as an interface to the Tester
+the 3-5 functions listed below.


So far it sounds as if developers are required to create the Wrapper from scratch, but I think we'd like to provide a skeleton of the wrapper and have developers sort of fill in the blanks for the initialize_updater(), update_client(), etc. functionality.

True: we should provide a skeleton. It wouldn't be much larger than the specification text. At the same time, I wouldn't want to muddy the specification itself with a bunch of imports and extra detritus, so what do you think: where should we put the skeleton? .py file in a folder in the taps repository, linked to here?

Yup, I think the TAPs repository would be good place to store resources used by the TAPs. These resources can be images, python modules, example metadata, etc.

vladimir-v-diaz · 2017-05-25T19:19:35Z

tap7.md

+## Wrapper Specification
+
+The Wrapper must implement at least the first three functions specified
+[below](#wrapper_functions).


This link doesn't work on my end.

I used _ instead of - in the links in the document. Fixing.

vladimir-v-diaz · 2017-05-25T19:47:35Z

tap7.md

-(3) the Root file must be signed by 1 out of 2 keys (i.e., threshold of 1)
+- only Ed25519 keys are used and listed in metadata
+- only exactly 2 keys are supported for Root metadata, and only a threshold
+    of 1  (((Is this necessary?)))


The conformance tool can potentially test for a Root threshold that an implementation doesn't support. For instance, the tool attempts to test for multiple Root keys, but an implementation fixes the number of Root keys to 1.

Unless I misunderstand, I don't think that's helpful. (I'll poke you tomorrow and ask to make sure I do understand.) We shouldn't really worry about testing additional constraints above and beyond the TUF spec. (Implementers can test their own implementations for conformance with their additional constraints; we just need to provide a tool to help find out if implementations conform to the TUF spec)

To help settle the question of whether or not threshold and key count specification is necessary for Tester configuration: are you aware of any existing need for this, Vlad?

(@JustinCappos: wrt your question yesterday)

We talked about this briefly. We're not currently aware of any implementation that doesn't support thresholds > 1 or doesn't support multiple root keys¹.

We've decided to include a sentence along the lines of "Other configuration options may be added to handle other constraints, if implementers reach out with their needs - for example, if a particular implementation doesn't support key thresholds > 1, an option could be added to this configuration file."

^{1_{TBC, I mean multiple keys listed in root in metadata and treated as root keys simultaneously by the client. Being able to generate and process multiple signatures on a given piece of metadata is a required feature without which root key rotation breaks).}}

I don't know for certain, but Docker Content Trust might only allow one Root key per repository.
See https://docs.docker.com/engine/security/trust/trust_key_mng/

Notary's test case for the Root role appears to only consider a threshold of 1.

I can imagine other simpler adoptions that might only bother to support one Root key.

vladimir-v-diaz · 2017-05-25T19:50:45Z

tap7.md

+    - Run the case at least five different ways, where the offending metadata
+      is a different role: Timestamp, Snapshot, Target, Delegated Target
+      (((depth x > 1, x < 5?))), Root.
+- **Mirrors**


What if an implementation doesn't care about mirrors and doesn't bother to support it?

True. (Not supporting mirrors is OK per the spec, right?)

I'll fix this by noting in the Mirrors multiplier that if multiple mirrors are not supported, the tests will instead be with 1 good mirror or 1 bad mirror. I'll also add support for delegated roles to the sample config file.

vladimir-v-diaz · 2017-05-25T19:52:40Z

tap7.md

+that case set will need to be multiplied in the listed way.
+- **Per Role**
+    - Run the case at least five different ways, where the offending metadata
+      is a different role: Timestamp, Snapshot, Target, Delegated Target


What if an implementation doesn't use nor support delegated roles? I think Flynn's Go implementation falls into this case.

Not supporting delegated roles is OK per the spec, right?

I'll fix this by noting that delegated roles are optional in the Per Role multiplier, and adding support for delegated roles to the sample config file.

JustinCappos · 2017-05-26T16:30:51Z

We've decided to include a sentence along the lines of "Other configuration options may be added to handle other constraints, if implementers reach out with their needs - for example, if a particular implementation doesn't support key thresholds > 1, an option could be added to this configuration file."

Hold a moment. A TAP is supposed to say what an implementer needs to do to implement TUF correctly. It's strange to say something like 'we may add other options', unless you need them to future proof their implementation in some way.

vladimir-v-diaz · 2017-05-26T16:56:44Z

I think that the conformance tests will likely need updating as the specification evolves. They might also need updating in the future to include additional test cases.

How should we state that along with the conformance tests, additional restriction options may be supported in the future by the tool's configuration file?

Note that these options are for configuration settings that are not shared equally across all implementations, although they are still allowed by the specification. For example, the Go implementation might only support ECDSA keys, whereas another might support Ed25519 and RSA keys.

We are not yet certain if a restriction option for the number of Root keys is needed. Should we go ahead and mention this option in TAP 7, or state that it can be supported later if the need arises?

and add a few abbreviated docstrings to the example Wrapper

and also make it a bit easier to read, with the code itself in a main() function.

awwad · 2017-06-01T17:07:06Z

I think this is ready to be reviewed again, @JustinCappos and @vladimir-v-diaz.

awwad · 2017-06-01T17:08:28Z

Side question: I think I should probably remove the actual example Wrapper file, since the text of it is now in the TAP. I'll do that if it seems sensible to you (so that changes don't have to be copied around).

Also bump edited date and correct a link typo

also generalize a link so that it doesn't break when code on the TUF ref impl develop branch changes

and remove a few related blurbs of unclear text.

vladimir-v-diaz · 2017-06-01T19:55:25Z

The title should probably change, there is more involved in these changes than removal of the subprocess requirement.

vladimir-v-diaz

WIP

vladimir-v-diaz · 2017-06-01T20:14:37Z

tap7.md

+test the pre-TAP4 TUF Reference Implementation.
+
+
+## Test Specification


I was expecting a Tester Specification heading, to match the Wrapper Specification section.

External parties have to implement Wrappers with specific functions provided that behave in specific ways, whereas the Tester is something we'd provide, so it didn't seem to need the same level of attention. I thought that the key information would be what the tests look like.

There's also a sample of some Tester code linked to elsewhere, and a description of the high level behavior of the Tester in section Executing Conformance Testing.

Do you think we should specify more, and put it in its own section here?

I was only expecting that the title of the heading be Tester Specification, to follow the names used in the TAP up to that point.

If you don't think Tester Specification captures the spirit of that section, then consider making it clear in the title that it it goes over the tests used by the Tester.

I've reorganized most of the Specification portion of the document into these sections (commit 5a7c728), so that it'll hopefully be more intuitive:

Configuration File Specification

Tester Specification (incl. Test Battery and what used to be in the Executing the Tester section)

Wrapper Specification (incl. Example Wrapper)

I hope that makes more sense?

vladimir-v-diaz · 2017-06-01T20:16:54Z

tap7.md

@@ -1,29 +1,29 @@
 * TAP: 7
 * Title: Conformance testing
-* Version: 1
-* Last-Modified: 17-May-2017
+* Version: 3


Why was this changed to 3?

Perhaps this is more a question about the version number changes to this field: Is there a way for us to track or document the history of these version number changes?

I suppose the blame view is the best way to see this. https://github.com/awwad/taps/blame/1ef1c8066b78c2f31dcc538ede852ad6878d4098/tap7.md for example. Then you can click the little array-of-boxes (stack-of-papers?) button to see the previous blame.

In my case, I changed it to 2 when I switched to the Wrapper model, and then I changed it to 3 to capture the sum of the other changes.

The reader can go digging through the blame view, but that seems like a lot of work. Ideally, the version number changes should be documented in the TAP itself.

TAP 1: TAP purpose and guidelines states that the Post-History field should list the dates in which new versions of a TAP are posted to the mailing list. I'm assuming these versions of the TAP correspond to the Version field.

I guess the reader can go back and look at the documented changes on the mailing list, but that wouldn't work in our case since the changes are happening on GitHub.

I guess I was thinking of a much more granular version history. It looks like every TAP is currently listed at version 1 (or in the case of TAP 2 has no version). I'll make this version 1 again to fit that model. That makes sense, since GitHub has history for more feature-level changes.

Question while we're on the topic: from the description of Post-History, it's not clear to me which of these two Post-History should look like: Version X lists the posted date for version X in Post-History, or Version X lists the posted dates for all versions V<=X in Post-History.

Post-History: v1: 14-Aug-2001, v2: 01-Jan-2002, v3: 02-Feb-2003

?

Good question. I think a single date is expected according to the description, but the posted dates for all versions would be more useful.

vladimir-v-diaz · 2017-06-01T20:28:40Z

tap7.md

+to be the Updater used in production. It only needs to function as defined in
+this TAP for conformance testing, though it is expected that the behavior be
+the same at a high level -- for example, the validity of metadata should be
+determined the same way. ((TODO: This paragraph still seems wordy. Unnecessary?))


Do you think the content of the paragraph is unnecessary?

@JustinCappos requested that I include in the TAP. For one, it makes it clear that we are not requiring developers to change their updater (at least the one used in production), and a second reason is to prevent a production implementation from accidentally shipping with conformance testing features enabled, which have led to security issues in the past.

Justin can explain more.

I added the "though it is expected that the behavior be the same at a high level" bit to try to reduce the impression that you can have entirely different behavior in testing mode, but it feels wordy. I was hoping for something succinct that conveys that things may have to work slightly differently in test mode, and that that's okay.

Maybe:

If the behavior necessary to provide the Conformance Tester with what it needs to judge conformance is slightly different from the usual behavior of the Updater, that is OK. For example, if errors are usually ignored rather than producing any return value, that's something that the Wrapper module can adjust.

I went with this in commit 79ff09c:

The behavior necessary to provide the Conformance Tester with what it needs to judge conformance may be slightly different from the usual or production behavior of the Updater, resulting in a need for a testing mode, or logic in the Wrapper to interpret behavior. For example, if errors are usually ignored rather than producing any return value, that's something that may be adjusted by using test-mode-specific code in the Updater, or post-hoc by the Wrapper module. The validation behavior during testing should not vary significantly from that in production so that test results can represent real Updater performance.

That works for me!

- Put example wrapper in a subsection under Wrapper Specification - Put Tester execution instructions into Tester Specification section - Rename config file section to Configuration File Specification - Fix links - Fix a typo in setting tap4-support

vladimir-v-diaz

I find the reorganized TAP easier to read, maybe because it starts at a higher level (the conformance Tester) and works down to the more detailed Wrapper.

vladimir-v-diaz · 2017-06-02T19:21:17Z

tap7.md

+        ```
+        return value     outcome
+        -----------      ------
+        0                SUCCESS: target identified by target_filepath has been


A blank line after each outcome might help readability.

K, added in e42f27c

and also clarify a line about the configuration file (minor).

Move the notes on how the Tester will use the Wrapper functions to the Wrapper Specification instead, integrating and rewording. Move some basic text about what test cases are like to the Tester Specification section. Text on particular test cases is removed and will likely be added to another document in the future.

awwad · 2017-06-05T20:22:11Z

((comment removed and moved to new PR, because GitHub seems to have choked and excluded two PRs from this merge even though they had been on the branch for >12 minutes before the merge))

vladimir-v-diaz · 2017-06-05T20:35:06Z

Note: @JustinCappos requested that we merge this pull request.

awwad added 6 commits May 24, 2017 15:07

Add high-level description of new scheme (Python wrapper)

b2d17a5

Refine description of Wrapper

0a4dfb8

Minor: Remove trailing whitespace

9c00cbc

Make terminology more consistent: conformance, Updater, Wrapper

014690d

- References to compliance appeared in a few places. The document now consistently refers only to conformance. - For clarity, capitalized terms Wrapper and Updater are used.

Add to description of Wrapper's update_client func

f2865c1

Begin to remove references to command line args

1c0f530

Also bump the document version number from 1 to 2 and add a necessary heading 'Expected Output' to be linked to.

awwad self-assigned this May 24, 2017

awwad added 6 commits May 25, 2017 02:13

Elaborate on the Wrapper specification

b53a117

minor: tweaks to naming of functions and components, some style

286927a

Begin updates to example code, which is now an example Wrapper

8f521bb

Move from many return values to just two (plus unknown error)

400bb24

Begin to expand on the test battery listing

b4cab0f

Add specs for the Wrapper's optional metadata conversion funcs

9db400f

awwad force-pushed the remove_forced_use_of_subprocess branch from 84d30a6 to 9db400f Compare May 25, 2017 17:27

Fix remainder of command-line processing references

6266e64

awwad force-pushed the remove_forced_use_of_subprocess branch 2 times, most recently from 6d0718b to 2fac5a0 Compare May 25, 2017 17:54

Formatting adjustsments and minor wording improvements

ba5f6aa

awwad force-pushed the remove_forced_use_of_subprocess branch from 2fac5a0 to ba5f6aa Compare May 25, 2017 17:56

awwad added 3 commits May 25, 2017 14:21

More "optional" labels and clarification in update_client spec

cc9abb7

Fix config file explanation and execution instructions

66e79e2

and make them less redundant

Correct summary of steps for implementers

499e632

vladimir-v-diaz reviewed May 25, 2017

View reviewed changes

awwad added 2 commits May 25, 2017 17:02

minor: Correct link format to correctly use - instead of _

f17b694

Tester can work if delegated roles or mirrors not supported

9ecf849

awwad force-pushed the remove_forced_use_of_subprocess branch from 23ef5ff to 9ecf849 Compare May 25, 2017 21:06

awwad added 4 commits June 1, 2017 12:59

Add more section links, clarify text, html-comment out TODOs,

dac82aa

and add a few abbreviated docstrings to the example Wrapper

Mention the configuration file as one of the elements of TAP 7

cd5d6cd

Apply fixes&improvements made to example file to copied text in TAP

05f1847

Update sample Tester code for changes made to Wrapper functions

ac6a9d3

and also make it a bit easier to read, with the code itself in a main() function.

awwad changed the title ~~(WIP) TAP 7: Remove forced use of subprocess~~ TAP 7: Remove forced use of subprocess Jun 1, 2017

Move html-comment out of code quotes so it's hidden properly

7cf350c

Also bump edited date and correct a link typo

awwad force-pushed the remove_forced_use_of_subprocess branch from 908d775 to 7cf350c Compare June 1, 2017 18:38

awwad added 8 commits June 1, 2017 15:11

Add note about contents of targets directory

499aef7

Distinguish references to TUF & the reference implementation

7b63276

also generalize a link so that it doesn't break when code on the TUF ref impl develop branch changes

Clarify where the full list of test cases will be,

2ad80b4

and remove a few related blurbs of unclear text.

Generalize link to unit tests and tweak adjacent text formatting

b1a47d5

Move a metadata conversion paragraph a bit, to the correct section

d4ec539

Remove some unused lines of code from the Wrapper example

73cb372

Typos and consistent phrasing in config file comment example

831e078

Update Augmented Reference Implementation section

1ef1c80

awwad changed the title ~~TAP 7: Remove forced use of subprocess~~ TAP 7: Simplify and Augment: Wrapper Module, Fewer Return Values, Description of Data Provided Jun 1, 2017

vladimir-v-diaz reviewed Jun 1, 2017

View reviewed changes

awwad added 2 commits June 2, 2017 15:12

Kick version back to 1 to fit TAP versioning model

04fcf6e

vladimir-v-diaz reviewed Jun 2, 2017

View reviewed changes

awwad added 4 commits June 2, 2017 15:42

Clarify advice about test mode

79ff09c

and also clarify a line about the configuration file (minor).

Clarify reference to Wrapper functions in Test Battery section

cf8943f

minor: Space out the return value descriptions

e42f27c

vladimir-v-diaz merged commit 99be430 into theupdateframework:master Jun 5, 2017

		test the pre-TAP4 TUF Reference Implementation.


		## Test Specification

TAP 7: Simplify and Augment: Wrapper Module, Fewer Return Values, Description of Data Provided #30

TAP 7: Simplify and Augment: Wrapper Module, Fewer Return Values, Description of Data Provided #30

Conversation

awwad commented May 24, 2017 • edited

awwad commented May 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

awwad May 25, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

awwad May 26, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vladimir-v-diaz May 25, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JustinCappos commented May 26, 2017 via email

vladimir-v-diaz commented May 26, 2017

awwad commented Jun 1, 2017

awwad commented Jun 1, 2017 • edited

vladimir-v-diaz commented Jun 1, 2017

vladimir-v-diaz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

awwad Jun 2, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

awwad Jun 1, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

awwad Jun 2, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vladimir-v-diaz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

awwad commented Jun 5, 2017 • edited

vladimir-v-diaz commented Jun 5, 2017

awwad commented May 24, 2017 •

edited

awwad May 25, 2017 •

edited

awwad May 26, 2017 •

edited

vladimir-v-diaz May 25, 2017 •

edited

awwad commented Jun 1, 2017 •

edited

awwad Jun 2, 2017 •

edited

awwad Jun 1, 2017 •

edited

awwad Jun 2, 2017 •

edited

awwad commented Jun 5, 2017 •

edited