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

Delegate to targets/releases per extended MVP #80

Draft
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

trishankatdatadog
Copy link
Member

@trishankatdatadog trishankatdatadog commented Jun 9, 2020

Signed-off-by: Trishank Karthik Kuppusamy [email protected]

  • Reuse targets/releases key across bundles/images/repos
  • Investigate how to name in-toto files
  • Delegate bundles and links to targets/releases
  • Sign bundle in targets/releases
  • Verify bundle from targets/releases
  • Get the GUN, no need for tag
  • Sign layout and pubkeys in targets
  • Sign links in targets/releases
  • Verify layout and pubkeys from targets
  • Verify links from targets/releases
  • Optional: check for duplicate links?
  • Optional: pushing/pulling in-toto metadata as OCI artifacts

Signed-off-by: Trishank Karthik Kuppusamy <[email protected]>
@trishankatdatadog trishankatdatadog force-pushed the trishankatdatadog/delegate-targets branch from ed58458 to 2313823 Compare June 10, 2020 03:13
Signed-off-by: Trishank Karthik Kuppusamy <[email protected]>
Signed-off-by: Trishank Karthik Kuppusamy <[email protected]>
@trishankatdatadog
Copy link
Member Author

Paging @radu-matei to take a look and help 🙂

@radu-matei
Copy link
Member

radu-matei commented Jun 29, 2020

We have two failing tests for now, which I am currently investigating:

FAIL	github.com/cnabio/signy/pkg/docker	13.762s
--- FAIL: TestVerify (0.00s)
    os_test.go:15: 
        	Error Trace:	os_test.go:15
        	Error:      	Received unexpected error:
        	            	invalid metadata found: invalid scheme for key '': should be 'rsassa-pss-sha256', got: ''
        	Test:       	TestVerify
--- FAIL: TestValidate (0.00s)
    os_test.go:25: 
        	Error Trace:	os_test.go:25
        	Error:      	Received unexpected error:
        	            	cannot load layout from file file ../../testdata/intoto/root.layout: open ../../testdata/intoto/root.layout: no such file or directory
        	Test:       	TestValidate
panic: runtime error: invalid memory address or nil pointer dereference [recovered]
	panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0xdaa04a]

I assume this is happening because in-toto tools are not installed by default in GitHub Actions workers.

@trishankatdatadog
Copy link
Member Author

We have two failing tests for now, which I am currently investigating:

FAIL	github.com/cnabio/signy/pkg/docker	13.762s
--- FAIL: TestVerify (0.00s)
    os_test.go:15: 
        	Error Trace:	os_test.go:15
        	Error:      	Received unexpected error:
        	            	invalid metadata found: invalid scheme for key '': should be 'rsassa-pss-sha256', got: ''
        	Test:       	TestVerify
--- FAIL: TestValidate (0.00s)
    os_test.go:25: 
        	Error Trace:	os_test.go:25
        	Error:      	Received unexpected error:
        	            	cannot load layout from file file ../../testdata/intoto/root.layout: open ../../testdata/intoto/root.layout: no such file or directory
        	Test:       	TestValidate
panic: runtime error: invalid memory address or nil pointer dereference [recovered]
	panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0xdaa04a]

I assume this is happening because in-toto tools are not installed by default in GitHub Actions workers.

I think the second error is because the file was renamed and moved to the minimal root layout example

@radu-matei
Copy link
Member

Ah, you're right.
Changing the directory to the correct one:

--- FAIL: TestVerify (0.00s)
    os_test.go:15: 
        	Error Trace:	os_test.go:15
        	Error:      	Received unexpected error:
        	            	failed verification: No signature found for key '556caebdc0877eed53d419b60eddb1e57fa773e4e31d70698b588f3e9cc48b35'
        	Test:       	TestVerify

This commit updates the minimal in-toto root layout, removes an
unused signature, as well as updates the directory structure, putting
the links at the same level as the layout and public key. (the directory
structure should be updated in the future, but for now, getting the
tests to pass should suffice.)

Finally, it updates `docker_test.go` and `os_test.go` accordingly.

Signed-off-by: Radu M <[email protected]>
@radu-matei
Copy link
Member

radu-matei commented Jun 29, 2020

Thanks for the help, @adityasaky and @trishankatdatadog!
Now both verification tests are passing.

pkg/intoto/metadata.go Outdated Show resolved Hide resolved
}

// WriteMetadataFiles writes the content of a metadata object into files in a directory
// TODO
Copy link
Member

Choose a reason for hiding this comment

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

Is this supposed to be used?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, this is one of the features I need help with: collecting the root layout, its pubkeys, all the links, and writing them to this directory for verification...

Copy link
Contributor

Choose a reason for hiding this comment

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

Let me know if I can help with this. If nothing else, I can listen and help work through what we want this function to do.

@@ -11,14 +11,15 @@ import (
"path/filepath"
Copy link
Member

Choose a reason for hiding this comment

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

Re - the comment at the top of this file.
The comment was relevant when the file contained transport-related code, which was indeed adapted from the Notary repo.

That is not the case anymore, so we should update the comments accordingly.

Copy link
Member Author

Choose a reason for hiding this comment

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

It might be true elsewhere, so how to best resolve this overall?

pkg/tuf/common_test.go Outdated Show resolved Hide resolved
pkg/tuf/sign.go Outdated Show resolved Hide resolved
)

// GetTargetAndSHA returns the target with roles and the SHA256 of the target file
Copy link
Member

Choose a reason for hiding this comment

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

nit for the future: when moving bits of code around, it's pretty difficult to understand whether this was just moved, or something else also changed.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, sorry about that, I felt guilty about it, too...

Signed-off-by: Trishank Karthik Kuppusamy <[email protected]>
}

return canonicaljson.RawMessage(raw), nil
return bytes, nil
Copy link
Member

@radu-matei radu-matei Jul 29, 2020

Choose a reason for hiding this comment

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

So this function returns ioutil.ReadFile, right?
It might be easier to just use it directly.

Copy link
Member Author

Choose a reason for hiding this comment

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

💯 yes, please go for it, sorry missed it after refactoring it

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed, Radu's simplification seems like a good change.

Copy link
Contributor

@carolynvs carolynvs left a comment

Choose a reason for hiding this comment

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

Reviewing just the first 1/4 of the files. I'm still working my way through this as I learn, sorry!

if err != nil {
return "", err
}
repo, err := registry.ParseRepositoryInfo(r)
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand what this second function call does for you? It seems that you can return r.Name directly and the test still passes.

Key []byte `json:"key"`
Layout []byte `json:"layout"`
Links map[string][]byte `json:"links"`
Data []byte `json:"data"`
Copy link
Contributor

Choose a reason for hiding this comment

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

This struct is a bit confusing to understand what it actually represents. Seems like it sometimes populates Data with the contents of either a layout, link or key but you can't tell which is stored based on the struct fields. Then in some cases either public key or link is populated as well.

I am wondering if it really makes sense to reuse in all of these cases? I am brand new to this so I'm really just trying to understand, and am not saying that it needs to be changed (other than the documentation perhaps to clarify).

if err != nil {
return err
return nil, fmt.Errorf("cannot encode custom metadata into canonical json: %v", err)
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: this error message will get bubbled up into CNAB tooling eventually, so it would help to say "in-toto custom metadata" so that it's more clear where things went wrong. Otherwise custom metadata means something completely different in cnab (the custom section in the bundle.json).

for _, fileinfo := range fileinfos {
filename := fileinfo.Name()
if !strings.Contains(filename, ".link") {
return nil, fmt.Errorf("%s does not have a .link suffix", filename)
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we have to assume that every file in the directory must be a link? I could see someone wanting to have another file, such as a readme that may provide additional documentation or info about the files in the directory. Does it make sense to instead skip files that aren't links?

}

// WriteMetadataFiles writes the content of a metadata object into files in a directory
// TODO
Copy link
Contributor

Choose a reason for hiding this comment

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

Let me know if I can help with this. If nothing else, I can listen and help work through what we want this function to do.

// the public keys used to verify the delegatee
publicKeys := []data.PublicKey{publicKey}
// the target paths entrusted to the delegatee
paths := make([]string, 2)
Copy link
Contributor

Choose a reason for hiding this comment

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

This creates an array with 2 indexes allocated, and then you append two more, resulting in an array of length 4. I assume you meant this instead

Suggested change
paths := make([]string, 2)
paths := make([]string, 0, 2)


// Delegate all paths ("*") to targets/releases.
// https://github.com/theupdateframework/notary/blob/f255ae779066dc28ae4aee196061e58bb38a2b49/cmd/notary/delegations.go
func delegateToReleases(repo client.Repository, publicKey data.PublicKey) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this function have a unit test?

switch len(keyList) {
case 0:
log.Debugf("No %s key available, need to make one", releasesRoleName)
return cryptoService.Create(releasesRoleName, r.GetGUN(), data.ECDSAKey)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this creating a new key if one wasn't found? Is it persisted somewhere? Some more comments about what is happening here, and if it should ever be okay to happen outside of tests would be really helpful.

Copy link
Contributor

Choose a reason for hiding this comment

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

general suggestion: When I call functions that I don't control, I like to wrap the error using github.com/pkg/errors.Wrap(err, "message with more context") so that when the error is bubbled up, you can tell where the code failed. Usually error messages from stdlib and other libraries don't have enough info to tell what the task was that failed, what file was being used, etc. For example, you could get an error like "invalid repository path" or something like that and have no way to trace it back to this line, to signy vs the calling CNAB tool, etc.

case 1:
log.Debug("Nothing to do, only one targets key available")
return nil
case 2:
// First, we publish current changes to repository in order to list roles.
// FIXME: Find a find better way to list roles w/o publishing changes first.
Copy link
Contributor

Choose a reason for hiding this comment

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

You may want to create an issue to track this, so it's not lost. Seems like publishing first is pretty unideal. Have you asked Notary if they would be willing to accept a PR to make this easier?

err = r.RotateKey(data.CanonicalTargetsRole, false, []string{thatTargetsKeyID})
log.Debugf("That targets keyID: %s", thatKeyID)
log.Debugf("Before rotating targets key from %s to %s", thisKeyID, thatKeyID)
err = r.RotateKey(data.CanonicalTargetsRole, false, []string{thatKeyID})
Copy link
Contributor

Choose a reason for hiding this comment

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

So this case statement and line here are assuming that the current key (thiskeyid) was created automagially and really thatKeyId is the one to reuse? It's not clear if there are other situations where you can have two target keys or if it's only caused by notary's automagic.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also if this function is unit or integration testable, this seems complicated enough to warrant it.

Base automatically changed from master to main February 5, 2021 18:18
@trishankatdatadog
Copy link
Member Author

Thanks for all your great comments, @carolynvs! So sorry I haven't gotten around to this yet, but been swamped with life and work. I can't promise I'll get to it this week, but please keep prodding me every two weeks as you see fit...

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.

4 participants