Switch to calling gRPC-based fulcio v2 APIs

[bobcallaway]

Waiting for fulcio gRPC to be enabled in prod

@nsmith5

[nsmith5]

Hmm it looks like you might be hitting similar code-gen issues I ran into. You'll need to run hack/update-codegen.sh I think and make want this patch for that file: https://github.com/sigstore/cosign/pull/1760/files#diff-149dfe7bb29d1191dceae3a52915e750e64b7f87257a5fb309c29d3056e2a95dL61-L65

Even after updating the codegen for cosigned I hit some CI issues... no sure what is up there

[nsmith5]

If we're close to getting GRPC running in prod I'll close my PR in favour of yours as well

[codecov-commenter]

Codecov Report

Merging #1762 (8911066) into main (40fa54c) will decrease coverage by 0.44%. The diff coverage is 55.23%.

@@            Coverage Diff             @@
##             main    #1762      +/-   ##
==========================================
- Coverage   30.12%   29.67%   -0.45%     
==========================================
  Files         136      136              
  Lines        8459     8515      +56     
==========================================
- Hits         2548     2527      -21     
- Misses       5582     5677      +95     
+ Partials      329      311      -18     

Impacted Files	Coverage Δ
cmd/cosign/cli/attest.go	0.00% <0.00%> (ø)
cmd/cosign/cli/options/fulcio.go	0.00% <0.00%> (ø)
cmd/cosign/cli/policy_init.go	1.35% <0.00%> (-0.01%)	:arrow_down:
cmd/cosign/cli/sign.go	0.00% <0.00%> (ø)
cmd/cosign/cli/signblob.go	0.00% <0.00%> (ø)
cmd/cosign/cli/sign/sign.go	15.84% <20.00%> (ø)
cmd/cosign/cli/fulcio/fulcio.go	30.67% <59.42%> (+12.41%)	:arrow_up:
pkg/oci/static/signature.go	50.00% <66.66%> (ø)
internal/pkg/cosign/fulcio/signer.go	62.50% <100.00%> (ø)
pkg/oci/mutate/options.go	89.18% <100.00%> (ø)
... and 7 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[nsmith5]

@bobcallaway is it worth resurrecting my other PR just to unblock bumping the Fulcio library here? Or are we pretty close to being able to switch over to GRPC with this work?

[bobcallaway]

i'm almost done with a rebase on this; where do we stand on getting the v0.4.0 fulcio code into prod?

[dlorenc]

i'm almost done with a rebase on this; where do we stand on getting the v0.4.0 fulcio code into prod?

Should be next week. I just need to cut over the IPs in DNS.

[dlorenc]

cc @k4leung4

[haydentherapper]

We're discussing a rollout plan - Will ping this PR once Fulcio's been updated

[haydentherapper]

@bobcallaway .4 is out! This should be good to go

[bobcallaway]

yes, that was the intent.

On Thu, Jun 2, 2022 at 11:51 AM Hayden B @.***> wrote:

@.**** commented on this pull request.

In cmd/cosign/cli/fulcio/fulcio.go https://github.com/sigstore/cosign/pull/1762#discussion_r888114729:

fulcioServer, err := url.Parse(fulcioURL)

if err != nil { return nil, err }

• fClient := api.NewClient(fulcioServer, api.WithUserAgent(clioptions.UserAgent()))
• return fClient, nil

• host := fulcioServer.Host
• switch fulcioServer.Scheme {
• case "https":

• opts = append(opts, grpc.WithTransportCredentials(credentials.NewTLS(&tls.Config{MinVersion: tls.VersionTLS12})))
  

• if fulcioServer.Port() == "" {
  

• 	host = fmt.Sprintf("%s:443", host)
  

• }
  

• default:

Why do we have the option for an insecure mode? For local testing?

— Reply to this email directly, view it on GitHub https://github.com/sigstore/cosign/pull/1762#pullrequestreview-993777164, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAVWTJOCE7YSQBDBSPS7ZVDVNDJ7DANCNFSM5TO3UCEA . You are receiving this because you were mentioned.Message ID: @.***>

[github-actions[bot]]

This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 10 days.

[github-actions[bot]]

This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 10 days.

[bobcallaway]

looks like scaffolding only exposes the HTTP port for fulcio, not the grpc port which is the root issue with this test failure.

@haydentherapper

[haydentherapper]

@vaikas fyi, https://github.com/sigstore/cosign/pull/1762#issuecomment-1258831932

[vaikas]

zomg, I totes missed this and I'm so sorry. I'm curious why we have two separate ports for HTTP and GRPC and not just use Duplex there, but I reckon that's an issue that should be brought up in Fulcio land.

[bobcallaway]

zomg, I totes missed this and I'm so sorry. I'm curious why we have two separate ports for HTTP and GRPC and not just use Duplex there, but I reckon that's an issue that should be brought up in Fulcio land.

mostly because gRPC support was added later and the majority of the duplex implementations I've seen simply just use HTTP/2 as a signal to route to gRPC vs HTTP interfaces, but that could/would break some clients.

[bobcallaway]

https://github.com/sigstore/scaffolding/pull/463 aims to fix the test harness issue blocking this

[vaikas]

zomg, I totes missed this and I'm so sorry. I'm curious why we have two separate ports for HTTP and GRPC and not just use Duplex there, but I reckon that's an issue that should be brought up in Fulcio land.

mostly because gRPC support was added later and the majority of the duplex implementations I've seen simply just use HTTP/2 as a signal to route to gRPC vs HTTP interfaces, but that could/would break some clients.

I think we should be fine here because at least our implementation here (and: https://github.com/chainguard-dev/go-grpc-kit

Switches based off of the grpc headers. https://github.com/chainguard-dev/go-grpc-kit/blob/main/pkg/duplex/server.go#L29

So, I'd be surprised if that would break HTTP clients, since I wouldn't be expecting them to be setting grpc headers.

We can add a bunch of testing for these, but I reckon it would simplify bunch of things to be able to utilize duplex. I'll try to spin up some testing for this.

[bobcallaway]

if you're checking for Content-Type that seems reasonable - I think the code examples I had seen in the past (not the chainguard-dev one) were only switching on HTTP 1 v 2 which didn't seem client-safe.

We did ship Fulcio v1 with the --grpc-port cmd line arg so if we retain that ability while also having the duplex support I think we could accommodate both.

[vaikas]

if you're checking for Content-Type that seems reasonable - I think the code examples I had seen in the past (not the chainguard-dev one) were only switching on HTTP 1 v 2 which didn't seem client-safe.

We did ship Fulcio v1 with the --grpc-port cmd line arg so if we retain that ability while also having the duplex support I think we could accommodate both.

Just to make sure I understand, you're saying that we should retain that flag on the server side and hence be able to serve on two ports?

[bobcallaway]

if you're checking for Content-Type that seems reasonable - I think the code examples I had seen in the past (not the chainguard-dev one) were only switching on HTTP 1 v 2 which didn't seem client-safe. We did ship Fulcio v1 with the --grpc-port cmd line arg so if we retain that ability while also having the duplex support I think we could accommodate both.

Just to make sure I understand, you're saying that we should retain that flag on the server side and hence be able to serve on two ports?

We could deprecate --grpc-port as a command line arg and find another way to express the intent when launching the server, but I dont think we should just remove the ability to run on separate ports given that Fulcio is v1.0.0 now.

[vaikas]

Yeah, I just wanted to make sure I understood, ack!

On Wed, Nov 30, 2022 at 8:25 AM Bob Callaway @.***> wrote:

if you're checking for Content-Type that seems reasonable - I think the code examples I had seen in the past (not the chainguard-dev one) were only switching on HTTP 1 v 2 which didn't seem client-safe. We did ship Fulcio v1 with the --grpc-port cmd line arg so if we retain that ability while also having the duplex support I think we could accommodate both.

Just to make sure I understand, you're saying that we should retain that flag on the server side and hence be able to serve on two ports?

We could deprecate --grpc-port as a command line arg and find another way to express the intent when launching the server, but I dont think we should just remove the ability to run on separate ports given that Fulcio is v1.0.0 now.

— Reply to this email directly, view it on GitHub https://github.com/sigstore/cosign/pull/1762#issuecomment-1332424824, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACWB45FDZ6M4C2B54LS6KUDWK55XXANCNFSM5TO3UCEA . You are receiving this because you were mentioned.Message ID: @.***>

[haydentherapper]

@bobcallaway What is the status of the PR? Looks like most tests are passing