-
Notifications
You must be signed in to change notification settings - Fork 134
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 GRPC interface #472
add GRPC interface #472
Conversation
Codecov Report
@@ Coverage Diff @@
## main #472 +/- ##
===========================================
- Coverage 43.91% 33.65% -10.26%
===========================================
Files 16 18 +2
Lines 1273 1352 +79
===========================================
- Hits 559 455 -104
- Misses 633 834 +201
+ Partials 81 63 -18
Continue to review full report at Codecov.
|
Big +1, OpenAPI/swagger just don't add enough value for the complexity. |
@loosebazooka @mattmoor @dlorenc any chance I can get one of you to look at and/or recreate the failed test in kind outside of CI? I can't seem to reproduce it running locally (either with kind or docker-compose). The error from the
I read this as: I'm also able to query and parse the root cert for fulcio just fine:
|
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.
Generally +1, but I didn't review it super thoroughly.
The dance we have to do for the weird content types some of the APIs use is unfortunate, but hopefully we can switch the main consumers to the GRPC path.
fulcio.proto
Outdated
/** | ||
* Returns information about the current state of the transparency log | ||
*/ | ||
rpc GetSigningCert(CertificateRequest) returns (CertificateResponse){ | ||
option (google.api.http) = { | ||
post: "/api/v1/signingCert" | ||
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.
Can we document some of the custom response munging we have to do on the HTTP path? I could see folks naively trying to reimplement this proto API and having a bad time.
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 could resurrect the old openapi.yaml
doc and make sure it is up to date
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.
Oh, I was just talking about in the rpc
comment. Pretty sure you can generate swagger docs from these as well, although if I've done it it's been years...
Left a bunch of API-specific comments. The main consumer annoyance I predict is around consuming a certificate chain. I think we should try to resolve that here with a repeated field. |
Signed-off-by: Bob Callaway <[email protected]>
Signed-off-by: Bob Callaway <[email protected]>
Signed-off-by: Bob Callaway <[email protected]>
Signed-off-by: Bob Callaway <[email protected]>
Signed-off-by: Bob Callaway <[email protected]>
Signed-off-by: Bob Callaway <[email protected]>
Signed-off-by: Bob Callaway <[email protected]>
Signed-off-by: Bob Callaway <[email protected]>
Signed-off-by: Bob Callaway <[email protected]>
Signed-off-by: Bob Callaway <[email protected]>
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.
Super awesome work ❤️ Really excited for this to land and mega kudos for not breaking the v1 API in the process. Extra special high-fives to wiring up the OIDC token into the context and passing context through to the CTlog client!
Feedback is just nits and questions for my own learning mostly
} | ||
|
||
enum PublicKeyAlgorithm { | ||
PUBLIC_KEY_ALGORITHM_UNSPECIFIED = 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.
Question: Do we actually support anything outside ECDSA key-pairs right now? I've seen places around sigstore project where its sort of hard coded to ECDSA and wasn't sure if there is actually flexibility there
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.
We support RSA, ECDSA, and ED25519 keys. There may be a couple lingering places in Cosign that still hardcode ECDSA, but most code paths should use the helper function from Sigstore that builds the signer/verifier regardless of type.
FWIW, this algorithm is ignored currently, the functions Fulcio calls can parse the key regardless of 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.
Right right, is there even a need for this ever? What is the API even supposed to do if there is a mismatch? The alg type is always available in the public key anyways right?
Signed-off-by: Bob Callaway <[email protected]>
Signed-off-by: Bob Callaway <[email protected]>
Signed-off-by: Bob Callaway <[email protected]>
Signed-off-by: Bob Callaway <[email protected]>
Signed-off-by: Bob Callaway <[email protected]>
@@ -64,12 +66,31 @@ func newServeCmd() *cobra.Command { | |||
cmd.Flags().String("fileca-key", "", "Path to CA encrypted private key") | |||
cmd.Flags().String("fileca-key-passwd", "", "Password to decrypt CA private key") | |||
cmd.Flags().Bool("fileca-watch", true, "Watch filesystem for updates") | |||
cmd.Flags().String("host", "0.0.0.0", "The host on which to serve requests") | |||
cmd.Flags().String("port", "8080", "The port on which to serve requests") | |||
cmd.Flags().String("host", "0.0.0.0", "The host on which to serve requests for HTTP; --http-host is alias") |
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 handles routing to the correct host? Is Nginx/Kubernetes aware of if the request is a GRPC or HTTP request?
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 lines below, there is a separate listener on a different port for grpc requests VS http requests.
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 was wondering what handles routing, since both will be accessible from https://fulcio.sigstore.dev
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.
Nginx should be able to handle it
if legacyGRPCServer != nil { | ||
endpoint := fmt.Sprintf("unix:%v", legacyGRPCServer.grpcServerEndpoint) | ||
if err := legacy_gw.RegisterCAHandlerFromEndpoint(ctx, mux, endpoint, opts); err != nil { | ||
log.Logger.Fatal(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.
What determines if legacyGRPCServer
is set , does this bubble up from config?
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 doesn't bubble up from config (yet), but if we wanted to deprecate and turn that off we could just set it to 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.
gotcha, I was thinking there might be a flag/config leveraged that would decide on what runs (restAPI or gPRC (or even both)).
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, interesting thought. let me noodle on that for a bit.
Signed-off-by: Bob Callaway <[email protected]>
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.
❤️ Looks good to me!
I'm happy with this whenever you're all ready for the merge! |
Signed-off-by: Bob Callaway <[email protected]>
Signed-off-by: Bob Callaway <[email protected]>
Signed-off-by: Bob Callaway <[email protected]>
Merge when ready! |
Signed-off-by: Bob Callaway <[email protected]>
Signed-off-by: Bob Callaway <[email protected]>
This mirrors the change from sigstore/fulcio#472 Signed-off-by: Kenny Leung <[email protected]>
This mirrors the change from sigstore/fulcio#472 Signed-off-by: Kenny Leung <[email protected]>
This mirrors the change from sigstore/fulcio#472 Signed-off-by: Kenny Leung <[email protected]>
This mirrors the change from sigstore/fulcio#472 Signed-off-by: Kenny Leung <[email protected]>
This mirrors the change from sigstore/fulcio#472 Signed-off-by: Kenny Leung <[email protected]>
This mirrors the change from sigstore/fulcio#472 Signed-off-by: Kenny Leung <[email protected]>
This mirrors the change from sigstore/fulcio#472 Signed-off-by: Kenny Leung <[email protected]>
This mirrors the change from sigstore/fulcio#472 Signed-off-by: Kenny Leung <[email protected]>
This mirrors the change from sigstore/fulcio#472 Signed-off-by: Kenny Leung <[email protected]>
This mirrors the change from sigstore/fulcio#472 Signed-off-by: Kenny Leung <[email protected]>
Also renames Fulcio Client -> LegacyClient in response to sigstore/fulcio#472 (we can switch to the gRPC client in another PR).
Also renames Fulcio Client -> LegacyClient in response to sigstore/fulcio#472 (we can switch to the gRPC client in another PR). Signed-off-by: Billy Lynch <[email protected]>
Also renames Fulcio Client -> LegacyClient in response to sigstore/fulcio#472 (we can switch to the gRPC client in another PR). Signed-off-by: Billy Lynch <[email protected]>
This adds a GRPC interface to
fulcio
and moves the HTTP interface to be a wrapper around GRPC (via grpc-gateway) to ensure compatibility.The purpose of this is to provide a simpler, cleaner interface for client code generation across sigstore libraries/services and other consumers. I am also looking to move Rekor over to a similar model but wanted to start here since the API is simpler.
Signed-off-by: Bob Callaway [email protected]