add file system environment and other clarifiacations #2

SantiagoTorres · 2017-09-18T21:47:08Z

fixes minor issues with the code style in the algorithm descriptions
Adds a section for the environment opaque field.
Adds more thorough clarification on how verification is done on section 5
Adds mention of ecdsa as a signature algorithm.

It also sneaks some typo fixes, etc.

JustinCappos · 2017-09-27T21:24:59Z

in-toto-spec.md

@@ -760,6 +769,12 @@ match any materials.
  other words, this path must appear as a material and a product within this
 step and their hashes must not match.

+The artifact rules contained in the `"expected_materials"` and
+`"expected_products"` fields operate in a similar fashion as a firewal. This


typo 'firewal'

Thanks for the heads up. I ran a spell checker on the patch to correct this and other typos.

reza-curtmola

Left a bunch of comments for this PR.

I tried to open an issue for specific lines of the spec (that are not included in the PR), based on these steps, but it didn't seem to work:
https://help.github.com/articles/opening-an-issue-from-code/

Maybe it's because the text in the spec wasn't code?

Anyways, here is an issue that is not directly part of what the lines you changed:

"The current reference implementation of in-toto defines two signing methods"

Right below this, there are 3 signing methods defined, so two should be three?

reza-curtmola · 2017-10-02T13:35:59Z

in-toto-spec.md

@@ -760,6 +769,12 @@ match any materials.
  other words, this path must appear as a material and a product within this
 step and their hashes must not match.

+The artifact rules contained in the `"expected_materials"` and
+`"expected_products"` fields operate in a similar fashion as a firewall. This


change "as a firewall" to "as firewall rules do".

reza-curtmola · 2017-10-02T13:36:44Z

in-toto-spec.md

+The artifact rules contained in the `"expected_materials"` and
+`"expected_products"` fields operate in a similar fashion as a firewall. This
+means that the first rule that matches a specific artifact in the link metadata
+will be used to match that artifact. Likewise, there is an implicit `ALLOW *`


shouldn't this be "implicit DENY *"?

We opted for an implicit ALLOW based on the feedback on people (it's mostly a bet for usability), although I understand the security concern.

I don't feel strongly that we should use one over the other between ALLOW * or DENY *.
It's ok to choose one over the other if you have a good reason.

However, if you choose ALLOW *, we should not say "Likewise, ...", because this implies that firewalls also work like that. Instead, I suggest to say "In addition, there is an implicit ALLOW * at the end of such fields."

I changed the wording for this

reza-curtmola · 2017-10-02T13:53:49Z

in-toto-spec.md

+    * **variables**: a list of environment variables set in the host system.
+    * **filesystem**: a list of filepath/hash values of the relevant files in
+      the filesystem. Another alternative could be to store an MTREE of the
+relevant directories. A third alternative would be to use the hashes of the


there are some formatting issues in the resulting in-toto-spec.md file.

reza-curtmola · 2017-10-02T13:56:31Z

in-toto-spec.md

-`build.BOBS_KEYID.link`, and the corresponding pieces of link metadata will be
-stored on a folder named `build.BOBS_KEYID`.
+`build.BOBS-KEYID-PREFIX.link`, and the corresponding pieces of link metadata
+will be stored on a folder named `build.BOBS-KEYID-PREFIX`.


better to say "stored in a folder" rather than "stored on a folder". Same applies to the previous paragraph.

Now that I think about it, probably the better name is directory

reza-curtmola · 2017-10-02T14:02:02Z

in-toto-spec.md

-   layout, one or more pieces of link or layout metadata is loaded.
-1. If the file loaded metadata is a link metadata file, a data structure
+   layout, one or more pieces of link or layout metadata is loaded. 
+  1. If the file loaded metadata is a link metadata file, a data structure


better to say "If the loaded metadata file is a ..."

reza-curtmola · 2017-10-02T14:02:39Z

in-toto-spec.md

-   functionaries made a sublayout while others didn’t), then verification
-should fail. The reason as to why this happens is to avoid ambiguities between
-both versions of the supply chain.
+  1. If the file is a layout file instead, the algorithm will recurse into that


To be consistent with the previous item in this list, we should say:
"If the loaded metadata file is a layout file instead, ..."

Agreed. WIll do

reza-curtmola · 2017-10-02T14:14:08Z

in-toto-spec.md

-      }
+    },
+    "environment": {
+      "variables": [""],


why variables has an empty list-like description [""], whereas filesystem and workdir have empty non-list-like description "" ?
(Same comment applies in other places in this document)

reza-curtmola · 2017-10-02T14:14:33Z

in-toto-spec.md

@@ -1158,13 +1245,18 @@ metadata:
      "stdin": "",
      "stdout": "foo.py",
      "return-value": "0"
+    },
+    "environment": {
+      "variables": [""],


why variables has an empty list-like description [""], whereas filesystem and workdir have empty non-list-like description "" ?

In the case of variables I know that it's going to be a list of them, on the other case, given that it's an opaque field, it could contain just a string (with e.g., the hash of the filesystem tree). I also think it makes sense to have the CWD just as a string.

Do you think I should work on clarifying this on the spec or was this more of a general question?

I think that the general description of the environment information (Section 4.4.1) already provides clarity that "variables" is a list, "filesystem" is a list or an item, and "workdir" is just one string (one path).

(BTW, in Sec. 4.4.1, change "Variables" to "variables" -- uppercase V to lowercase v).

However, the examples in Sections 5.3.1, 5.3.2, 5.3.3 contain specific instantiations of the above general rule. As such, in these examples the "variables" field is just an empty list.
It looks strange to denote 3 fields which are empty using different notation (even if one field is a list in this case).

But, if you think this is fine, I don't feel strongly about this.

Ah, I see. I'll check the examples and make sure they are compliant with the specification.

reza-curtmola · 2017-10-02T14:15:21Z

in-toto-spec.md

    }
  },
  "signatures" : [
    { "keyid" : "<BOBS_KEYID>",
      "method" : "ed25519",
      "sig" :
-          "ae3aee92ea33a8f461f736a698e082e12c300dfe5022a06c7a6c2a6a93a9f5771eb2e5ce0c93dd580bebc2080d10894623cfd6eaedf1d5aa394df84890d7ace3"
+          "ae3aee92ea33a8f461f736a698e082e12c300dfe5022a06c7a6c2a6a"


What's the reason you removed part of the signature?
(In Alice's link above you left the signature as it was)

I mostly did this for readability. I don't know if having a 30-char length hex string adds more information than a shorter one. On the examples outside of the spec I ellipsized it, would this help here?

reza-curtmola · 2017-10-02T14:20:19Z

in-toto-spec.md

 1. The root.layout file exists and is signed with a trusted key (in this case,
   Alice's).
 1. The code that Alice wrote was tested by two people, in this case, Caroline
   and Alfred.
-1. Every step in the layout has a corresponding [name].link metadata file
+1. Every step in the layout has a corresponding [name].[keyid-prefix].link metadata file


Should put [name].[keyid-prefix].link between quotes to obtain the grayed text effect.
(Same for later in this paragraph)

Right. Will do.

SantiagoTorres · 2017-10-03T16:51:52Z

@reza-curtmola I did a round of fixes. I'm left with these two issues:

Should we change the implicit ALLOW to a deny?
Should we change the string for filesystem and cwd to a list too (or clarify why this is)

I wanted to have your feedback on this. Thanks!

reza-curtmola · 2017-10-05T04:30:59Z

I provided feedback for each individual issue above.

BTW, have you seen my initial comment about this:
"The current reference implementation of in-toto defines two signing methods"
However, right below this, there are 3 signing methods defined.
So, should we change the sentence to:
"The current reference implementation of in-toto defines three signing methods" ?

reza-curtmola · 2017-10-06T17:07:00Z

Since the spec document has become quite long, it would be useful to have in the beginning a table of contents to help the reader quickly assess its contents and be able to reach faster certain sections in the spec.

SantiagoTorres · 2017-10-06T17:45:22Z

Since the spec document has become quite long, it would be useful to have in the beginning a table of contents to help the reader quickly assess its contents and be able to reach faster certain sections in the spec.

edit: ticket is #3

SantiagoTorres · 2017-10-06T17:51:24Z

"The current reference implementation of in-toto defines three signing methods" ?

I changed this now.

SantiagoTorres · 2017-10-06T17:52:18Z

I think all the issues are addressed and this is ready to merge unless you have any other comments.

reza-curtmola · 2017-10-06T18:05:42Z

Good to go from my pov

lukpueh · 2017-10-06T20:39:34Z

in-toto-spec.md

+`"expected_products"` fields operate in a similar fashion as firewall rules do.
+This means that the first rule that matches a specific artifact in the link
+metadata will be used to match that artifact. In addition, there is an implicit
+`ALLOW *` at the end of such fields.


Since when do we have an implicit ALLOW *?
The pseudo code below as well as the implementation require all artifacts to be consumed by a rule.

I'll update the pseudocode, and I was thinking to start documenting a rationale for this on the wiki. Can you make a ticket for this please?

There you go... #4

lukpueh · 2017-10-06T20:48:11Z

in-toto-spec.md

-clashes between step names on layouts and sublayouts, as the creator of a
-layout may not be aware of the names used by the creators of sublayouts.
-
+Link metadata that pertains to a sublayout must be placed in a file system


In the rest of the document you always use "filesystem"...

lukpueh · 2017-10-06T20:53:12Z

in-toto-spec.md

-layout may not be aware of the names used by the creators of sublayouts.
-
+Link metadata that pertains to a sublayout must be placed in a file system
+directory for such layout. The name of the directory will be have the same


lukpueh · 2017-10-06T20:53:46Z

in-toto-spec.md

@@ -457,24 +457,25 @@ how it is laid out within link and layout metadata.
 ### 4.1 Metaformat

 To provide descriptive examples, we will adopt "canonical JSON," as described


"canonical JSON," --> "canonical JSON",

I personally agree with this, but this is an american thing afaik.

Intriguing... Disregard my comment, then!

lukpueh · 2017-10-06T20:54:09Z

in-toto-spec.md

@@ -516,7 +518,7 @@ The 'rsa' format is:
 where PUBLIC and PRIVATE are in PEM format and are strings.  All RSA keys must
 be at least 2048 bits.

-The 'ed25519' format is:
+The elliptic-curve variants ('ed25519' and ecdsa) format are:


ecdsa --> 'ecdsa'

lukpueh · 2017-10-06T20:59:25Z

in-toto-spec.md

+The following algorithm contains an in-depth description of the verification
+procedure.
+
+1. Toto inspects the final product to find a root.layout file that describes


Toto --> in-toto

lukpueh · 2017-10-06T21:13:46Z

There are still a couple of inconsistencies regarding the use of backticks "`". IMHO that's not a blocker, but it should be taken care of at some point. Worth a ticket?

SantiagoTorres · 2017-10-06T21:14:20Z

Worth a ticket, I guess. I'll address the others right away

lukpueh

Please fix remaining tiny comment and we are good to merge.

lukpueh · 2017-10-09T15:20:06Z

in-toto-spec.md

+
+```json
+{ "environment": {
+    "Variables": [


Variables is lower case everywhere else.

SantiagoTorres · 2017-10-10T17:22:31Z

@lukpueh pushed 👍

SantiagoTorres added 2 commits September 18, 2017 17:41

ENH: in-toto-spec.md: adds filesystem env support

1d9921f

It also sneaks some typo fixes, etc.

FIX: in-toto-spec.md: fix json decode errors

597b94f

SantiagoTorres requested review from lukpueh, JustinCappos and reza-curtmola September 18, 2017 21:47

JustinCappos reviewed Sep 27, 2017

View reviewed changes

MAINT: in-toto-spec: fix minor typos

28eb580

reza-curtmola reviewed Oct 2, 2017

View reviewed changes

SantiagoTorres added 2 commits October 3, 2017 12:43

ENH: Address reza's comments

a43a5e8

ENH: Grammar fixes (folder -> directory)

c86284c

FIX: minor rewording issues.

263d072

lukpueh requested changes Oct 6, 2017

View reviewed changes

SantiagoTorres added 2 commits October 6, 2017 17:42

FIX: in-toto-spec: address lukas's comments

81a6c3f

FIX: in-toto-spec: add implicit ALLOW to pseudocode

db5cdbb

lukpueh mentioned this pull request Oct 9, 2017

Document "artifact rule" rationale #4

Open

lukpueh approved these changes Oct 9, 2017

View reviewed changes

in-toto-spec.md Outdated

```json

{ "environment": {

"Variables": [

Copy link

Member

lukpueh Oct 9, 2017

Choose a reason for hiding this comment

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

Variables is lower case everywhere else.

FIX: in-toto-spec: s/Variables/variables/ keyword

69c29b4

lukpueh merged commit ddc8f97 into master Oct 11, 2017

SantiagoTorres deleted the filesystem-env branch October 11, 2017 15:19

lukpueh mentioned this pull request Oct 11, 2017

Adopt replace implicit DISALLOW with ALLOW in-toto/in-toto#131

Closed

		@@ -457,24 +457,25 @@ how it is laid out within link and layout metadata.
		### 4.1 Metaformat

		To provide descriptive examples, we will adopt "canonical JSON," as described

add file system environment and other clarifiacations #2

add file system environment and other clarifiacations #2

Conversation

SantiagoTorres commented Sep 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reza-curtmola 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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SantiagoTorres commented Oct 3, 2017 • edited Loading

reza-curtmola commented Oct 5, 2017

reza-curtmola commented Oct 6, 2017

SantiagoTorres commented Oct 6, 2017 • edited Loading

SantiagoTorres commented Oct 6, 2017

SantiagoTorres commented Oct 6, 2017

reza-curtmola commented Oct 6, 2017

lukpueh Oct 6, 2017 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukpueh commented Oct 6, 2017

SantiagoTorres commented Oct 6, 2017

lukpueh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SantiagoTorres commented Oct 10, 2017

SantiagoTorres commented Oct 3, 2017 •

edited

Loading

SantiagoTorres commented Oct 6, 2017 •

edited

Loading

lukpueh Oct 6, 2017 •

edited

Loading