-
Notifications
You must be signed in to change notification settings - Fork 165
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
Add TUF type #383
Add TUF type #383
Conversation
Looking forward to hacking on this this week! |
Co-authored-by: Santiago Torres <santiagotorres@purdue.edu> Co-authored-by: Trishank Karthik Kuppusamy <trishank.kuppusamy@datadoghq.com> Co-authored-by: Marina Moore <mnm678@gmail.com> Signed-off-by: Asra Ali <asraa@google.com>
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.
The code LGTM, but I'll let it stick around for @bobcallaway to have a look.
One nit: could you add some docs here: https://github.com/sigstore/rekor/blob/main/types.md
Signed-off-by: Asra Ali <asraa@google.com>
Done! |
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.
Great work @asraa! Sometime in the future it'd be great if we could generate the test data so that it doesn't clog up the repo, but otherwise LGTM
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.
overall looking good, a few questions and nits
Signed-off-by: Asra Ali <asraa@google.com>
Comments addressed! had a minor snafu around decoding strfmt.DateTime -- all good now |
Nice! I'll let @bobcallaway take another pass. |
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.
Some initial comments. Great job, Asra!
"threshold": 3 | ||
} | ||
}, | ||
"spec_version": "1.0", |
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.
Hmm, this should fail, no? Don't we expect the spec version to be 1.0.0?
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.
Two things: just fixed the TODO that actually grabs the spec version.
Second: go-tuf doesn't actually do major.minor.patch.... https://github.com/theupdateframework/go-tuf/blob/aee6270feb5596036edde4b6d7564fa17db811cb/client/testdata/go-tuf/consistent-snapshot-true/0/repository/root.json#L84
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.
To close this out I made this a general string since go-tuf doesn't write in that format anyway.
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.
To close this out I made this a general string since go-tuf doesn't write in that format anyway.
"version": 1 | ||
} | ||
}, | ||
"spec_version": "1.0", |
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.
Same: should not work bcos should be 1.0.0, no?
IntegratedTime: 2021-08-13T19:17:33Z | ||
UUID: 6ed8fa5e9f0aa31b6cdfd2cc6877692f5afba52edd7ff5774eebfb22228e8847 | ||
Body: { | ||
"TufObj": { |
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.
Hmm, no version
property here, is that intended?
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'm unsure why this is, I think on get it doesn't write the version, but when I unmarshal the actual content of the stored entry I can view it.
asraa@asraa-glaptop:~/git/rekor$ curl -fs0 http://localhost:3000/api/v1/log/entries/68163ebd7d7ba41ece736e2910915944a85985c61892f947841729476f81b951 > logEntry.json
asraa@asraa-glaptop:~/git/rekor$ jq '."68163ebd7d7ba41ece736e2910915944a85985c61892f947841729476f81b951".body' logEntry.json | tr -d \" | base64 -d > body.json
asraa@asraa-glaptop:~/git/rekor$ cat body.json | jq
{
"apiVersion": "0.0.1",
"spec": {
"metadata": {
"content": "..."
},
"root": {
"content": "..."
},
"spec_version": "1.0"
},
"kind": "tuf"
}
pkg/types/tuf/v0.0.1/entry.go
Outdated
result = append(result, strings.ToLower(hex.EncodeToString(rootHash[:]))) | ||
} | ||
|
||
// TODO: Index individual key IDs? |
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.
Aha, I was looking for this :)
Yes, I think it would be super useful to be able to query Rekor for TUF metadata based on keyids. Could we pretty please add this? Happy to help with this effort also.
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!! Let's do this in a follow up. Should be pretty easy to do by adding a method for KeyIDs in the PublicKey type for TUF
Signed-off-by: Asra Ali <asraa@google.com>
OK TUF metadata files are now stored as JSON. Unmarshalling the content is just the whole JSON of the metadata file rather than the inline content + the fields we cared about (type, expires, etc)
If it's okay I'd like to address the size limits #414 in a separate PR. It's a little complicated and I want to make it general to each type (so that there are configurable limits on the bytes per type if you send content with a file or URL) |
SGTM |
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'm happy merging this PR now, and think we can address any improvements/changes in future PRs. Great work, Asra! 💯 💯
Here we go! |
Makes #265 up to date. It is functional and tested now!
Some notes:
@SantiagoTorres @trishankatdatadog @mnm678