-
Notifications
You must be signed in to change notification settings - Fork 164
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
initial gRPC API & message definition #771
Conversation
Signed-off-by: Bob Callaway <[email protected]>
Signed-off-by: Bob Callaway <[email protected]>
Signed-off-by: Bob Callaway <[email protected]>
If you're targeting a feature branch, feel free to just merge away and we can do a larger review after. Feel free to leave them open though if you do want reviews before the merge. |
I will plan to do that for lots of the functional changes, but I would like some feedback on the external APIs if possible. |
/* | ||
* The universally unique identifier for the entry within the log shard | ||
*/ | ||
string uuid = 1; |
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: consider renaming uuid to entryID to indicate that we want the universal identifier
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.
if we're assuming that the v2 API will only be run with rekor server >=0.6, then we can probably rename all mentions of uuid to entryID?
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, I thought about that as well as normalizing "tree" vs "shard" and trying to make it consistent.
pkg/types/alpine/alpine.proto
Outdated
/* | ||
* Key / value pairs from .PKGINFO file inside alpine package | ||
*/ | ||
map<string, string> pkginfo = 3; |
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 let users store arbitrary data?
* | ||
* TimestampRequest uses an identical input and output format; | ||
* we create both for potential forward compatibility of separating and changing them; | ||
* this also makes the assignment more explicit |
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 a little confused by this - So ProposedTImestampRequest is the request, and TimestampRequest is an internal type?
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 is a typo, good catch
pkg/types/rpm/rpm.proto
Outdated
/* | ||
* Key / value pairs from RPM headers | ||
*/ | ||
map<string, string> headers = 3; |
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 question, does this allow arbitrary 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.
same as alpine, the code currently extracts and stores cryptographically-verified metadata from the package headers.
/* | ||
* The public key | ||
*/ | ||
bytes content = 1; |
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 can contain either a public key, certificate or chain, correct? Should we rename this?
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.
well, it could contain a PGP public key, an X509 certificate (or certificate chain), a TUF metadata root, etc. We could use a oneOf with specific types here if you think its better
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'd be in support of switching to a oneOf. I think it makes it more explicit and less prone to 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.
FYI this linked issue #809 -- would support switching to a oneof
|
||
message Signature { | ||
/* | ||
* The crypto algorithm used to generate the signature |
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: This sounds like the signature algorithm, i.e rsa-pss, ecdsa, whereas it's actually the format to hold the signature. Consider rephrasing
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, this is a carry forward from the current implementation... open to alternative suggestions for sure
MINISIGN = 2; | ||
X509 = 3; | ||
SSH = 4; | ||
PKCS7 = 5; |
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 only work with the JAR type, or any type?
rekor_common.proto
Outdated
PublicKey public_key = 3; | ||
} | ||
|
||
enum PKIAlgorithm { |
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: Comments for each enum
rekor_timestamp.proto
Outdated
/* | ||
* The set of PEM-encoded certificate chains for the timestamp service; | ||
* the chain will start with any intermediate certificates (if present), | ||
* and finishwith the root certificate. |
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.
finish with
/* | ||
* The raw HTTP body is bound to this field; this is an RFC3161 compliant blob. | ||
*/ | ||
google.api.HttpBody http_body = 1 [(google.api.field_behavior) = REQUIRED]; |
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.
Can it be a byte array, or does it have to be an HTTP body?
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.
well http_body
includes a byte array, but in order for rekor to remain an RFC3161 compliant TSA, I think we need to do it this way (could be wrong though, haven't coded it up yet)
This is still WIP, but commiting this iteration. Signed-off-by: Bob Callaway <[email protected]>
*/ | ||
message ProposedIntoto { | ||
/* | ||
* DSSE envelope encompassing the entire Intoto statement |
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.
what about reusing the DSSE proto?
https://github.com/secure-systems-lab/dsse/blob/master/envelope.proto
or too much overhead on clients needing to pull that library?
* Intoto represents the structure of the entry persisted in the transparency log for | ||
* the Intoto type. | ||
*/ | ||
message Intoto { |
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.
My biggest issue with this is that you can't take an Intoto
entry and "verify" it the way that Rekor will verify it.
Can we add the repeated Signatures signature
in here too? So that clients can verify the entry signature using the Signatures signing the payload_hash?
/* | ||
* The public key | ||
*/ | ||
bytes content = 1; |
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.
FYI this linked issue #809 -- would support switching to a oneof
Note: this is targeted at a feature branch, not main. I intend to continue to refine the gRPC work in a branch until we get it ready to merge over into main since it will touch most of Rekor in some way or another.
This starts work on #484; when complete, it also aims to simplify the code generation for
rekor-server
, custom type definition and easier multi-language client code generation. This PR just shares a draft of the external service & message definitions for feedback, and starts to plumb in the golang client & server side code generation.TODO (likely incomplete but wanted to start a list):
v1
API support through legacy gRPC interface (similar to Fulcio)types
interfaces (opportunity for leveraging 1.18 generic support?)openapi.yaml
file?Signed-off-by: Bob Callaway [email protected]