-
Notifications
You must be signed in to change notification settings - Fork 11
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
base: main
Are you sure you want to change the base?
Conversation
04028e5
to
ed58458
Compare
Signed-off-by: Trishank Karthik Kuppusamy <trishank.kuppusamy@datadoghq.com>
ed58458
to
2313823
Compare
Signed-off-by: Trishank Karthik Kuppusamy <trishank.kuppusamy@datadoghq.com>
Signed-off-by: Trishank Karthik Kuppusamy <trishank.kuppusamy@datadoghq.com>
Paging @radu-matei to take a look and help 🙂 |
We have two failing tests for now, which I am currently investigating:
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 |
Ah, you're right.
|
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 <root@radu.sh>
Thanks for the help, @adityasaky and @trishankatdatadog! |
} | ||
|
||
// WriteMetadataFiles writes the content of a metadata object into files in a directory | ||
// TODO |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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" |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
) | ||
|
||
// GetTargetAndSHA returns the target with roles and the SHA256 of the target file |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 <trishank.kuppusamy@datadoghq.com>
f786da0
to
0a87d1b
Compare
} | ||
|
||
return canonicaljson.RawMessage(raw), nil | ||
return bytes, nil |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this 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) |
There was a problem hiding this comment.
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"` |
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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.
pkg/tuf/delegations.go
Outdated
// the public keys used to verify the delegatee | ||
publicKeys := []data.PublicKey{publicKey} | ||
// the target paths entrusted to the delegatee | ||
paths := make([]string, 2) |
There was a problem hiding this comment.
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
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 { |
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
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}) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
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... |
Signed-off-by: Trishank Karthik Kuppusamy trishank.kuppusamy@datadoghq.com