Switch to calling gRPC-based fulcio v2 APIs #1762

bobcallaway · 2022-04-14T20:55:03Z

Waiting for fulcio gRPC to be enabled in prod

Signed-off-by: Bob Callaway <[email protected]>

nsmith5 · 2022-04-14T21:16:51Z

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 · 2022-04-14T21:17:32Z

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

Signed-off-by: Bob Callaway <[email protected]>

pkg/cosign/kubernetes/webhook/validator.go

cmd/cosign/cli/fulcio/fulcio.go

Signed-off-by: Bob Callaway <[email protected]>

codecov-commenter · 2022-04-15T12:53:24Z

Codecov Report

Merging #1762 (2cdefb5) into main (d067d40) will increase coverage by 0.14%.
The diff coverage is 54.20%.

@@            Coverage Diff             @@
##             main    #1762      +/-   ##
==========================================
+ Coverage   30.48%   30.63%   +0.14%     
==========================================
  Files         136      136              
  Lines        8384     8442      +58     
==========================================
+ Hits         2556     2586      +30     
- Misses       5494     5520      +26     
- Partials      334      336       +2

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%)`	⬇️
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.30% <57.74%> (+12.04%)`	⬆️
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 2 more

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

Signed-off-by: Bob Callaway <[email protected]>

nsmith5 · 2022-05-13T15:49:37Z

@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 · 2022-05-20T10:16:30Z

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

dlorenc · 2022-05-20T11:56:11Z

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 · 2022-05-20T11:56:17Z

cc @k4leung4

haydentherapper · 2022-05-24T20:30:18Z

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

haydentherapper · 2022-06-02T15:49:44Z

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

cmd/cosign/cli/fulcio/fulcio.go

bobcallaway · 2022-06-02T15:55:37Z

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 <#1762 (comment)>: > 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 <#1762 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAVWTJOCE7YSQBDBSPS7ZVDVNDJ7DANCNFSM5TO3UCEA> . You are receiving this because you were mentioned.Message ID: ***@***.***>

cmd/cosign/cli/fulcio/fulcio.go

haydentherapper · 2022-06-02T16:58:30Z

cmd/cosign/cli/fulcio/fulcio.go

-	return fClient, nil
+	host := fulcioServer.Host
+	switch fulcioServer.Scheme {
+	case "https":


I've also seen dns:///foo.example.com:443 as a supported scheme for gRPC.

I also think we should allow scheme-less and match on port.

So either:

scheme=https

port=443
should use a TLS connection

otherwise, an insecure connection

haydentherapper · 2022-06-02T16:59:43Z

cmd/cosign/cli/fulcio/fulcioverifier/fulcioverifier.go

 	fs, err := fulcio.NewSigner(ctx, idToken, oidcIssuer, oidcClientID, oidcClientSecret, oidcRedirectURL, fClient)
 	if err != nil {
 		return nil, err
 	}

 	// verify the sct
-	if err := ctl.VerifySCT(ctx, fs.Cert, fs.Chain, fs.SCT); err != nil {
+	if err := ctl.VerifySCT(ctx, []byte(fs.Cert), []byte(strings.Join(fs.Chain, "\n")), fs.SCT); err != nil {


nit: Do you want to add a TODO here to change VerifySCT to take the list of certs rather than a single PEM-encoded chain? No need to change it now, but it would be better than converting back and forth going forward

haydentherapper · 2022-06-02T17:00:46Z

pkg/cosign/kubernetes/webhook/validator.go

-func getFulcioCert(u *apis.URL) (*x509.CertPool, error) {
-	fClient := api.NewClient(u.URL())
-	rootCertResponse, err := fClient.RootCert()
+func getFulcioCert(fulcioURL *apis.URL) (*x509.CertPool, error) {


Should we be providing this client in Fulcio to avoid duplication? I had hoped setting up a gRPC connection required very little boilerplate, but given that we need scheme/port detection, it might be good to centralize it.

fyi @wlynch, who would need the same in gitsign

github-actions · 2022-08-20T02:16:27Z

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 · 2022-09-20T02:17:18Z

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.

Signed-off-by: Bob Callaway <[email protected]>

vaikas · 2022-11-29T18:40:50Z

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 · 2022-11-29T20:37:14Z

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 · 2022-11-29T22:13:40Z

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 · 2022-11-30T16:25:21Z

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 · 2022-11-30T16:30:13Z

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 <#1762 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACWB45FDZ6M4C2B54LS6KUDWK55XXANCNFSM5TO3UCEA> . You are receiving this because you were mentioned.Message ID: ***@***.***>

haydentherapper · 2022-12-05T17:50:58Z

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

github-actions · 2023-01-05T02:01:26Z

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 · 2023-02-05T02:04:07Z

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 · 2023-03-09T02:06:34Z

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 · 2023-04-09T01:54:17Z

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 · 2023-04-19T01:55:42Z

This PR was closed because it has been stalled for 10 days with no activity.

github-actions · 2023-05-24T01:58:14Z

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 · 2023-06-30T02:04:48Z

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 · 2023-07-10T02:09:56Z

This PR was closed because it has been stalled for 10 days with no activity.

bobcallaway added 3 commits April 14, 2022 15:28

move to using gRPC interface of fulcio

0a5c2bc

Signed-off-by: Bob Callaway <[email protected]>

Merge remote-tracking branch 'upstream/main' into fulcio_grpc

f77123c

Signed-off-by: Bob Callaway <[email protected]>

add TLS support if https requested

849a2cb

Signed-off-by: Bob Callaway <[email protected]>

bobcallaway requested a review from haydentherapper April 14, 2022 20:55

ran hack/update-codegen.sh

00ce9be

Signed-off-by: Bob Callaway <[email protected]>

haydentherapper reviewed Apr 14, 2022

View reviewed changes

pkg/cosign/kubernetes/webhook/validator.go Outdated Show resolved Hide resolved

cmd/cosign/cli/fulcio/fulcio.go Outdated Show resolved Hide resolved

bobcallaway added 2 commits April 15, 2022 08:41

handle default port case

198e72e

Signed-off-by: Bob Callaway <[email protected]>

Merge remote-tracking branch 'upstream/main' into fulcio_grpc

abd9991

Signed-off-by: Bob Callaway <[email protected]>

nsmith5 mentioned this pull request Apr 18, 2022

Bump Fulcio client lib and use LegacyClient #1760

Closed

update to latest fulcio API

e06f099

Signed-off-by: Bob Callaway <[email protected]>

haydentherapper reviewed Jun 2, 2022

View reviewed changes

cmd/cosign/cli/fulcio/fulcio.go Outdated Show resolved Hide resolved

haydentherapper mentioned this pull request Jun 2, 2022

Add NewClientFromString func. sigstore/fulcio#634

Closed

haydentherapper requested changes Jun 2, 2022

View reviewed changes

github-actions bot added the no-pr-activity label Aug 20, 2022

haydentherapper removed the no-pr-activity label Aug 20, 2022

github-actions bot added the no-pr-activity label Sep 20, 2022

bobcallaway removed the no-pr-activity label Sep 21, 2022

Merge remote-tracking branch 'upstream/main' into fulcio_grpc

26809be

Signed-off-by: Bob Callaway <[email protected]>

vaikas mentioned this pull request Nov 30, 2022

[DNM] Test grpc with duplex. sigstore/scaffolding#489

Closed

github-actions bot added the no-pr-activity label Jan 5, 2023

haydentherapper removed the no-pr-activity label Jan 5, 2023

github-actions bot added the no-pr-activity label Feb 5, 2023

haydentherapper removed the no-pr-activity label Feb 6, 2023

github-actions bot added the no-pr-activity label Mar 9, 2023

haydentherapper removed the no-pr-activity label Mar 9, 2023

github-actions bot added the no-pr-activity label Apr 9, 2023

github-actions bot closed this Apr 19, 2023

bobcallaway removed the no-pr-activity label Apr 23, 2023

bobcallaway reopened this Apr 23, 2023

github-actions bot added the no-pr-activity label May 24, 2023

bobcallaway removed the no-pr-activity label May 30, 2023

github-actions bot added the no-pr-activity label Jun 30, 2023

github-actions bot closed this Jul 10, 2023

haydentherapper mentioned this pull request Feb 29, 2024

Switch to Fulcio v2 API #3569

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Switch to calling gRPC-based fulcio v2 APIs #1762

Switch to calling gRPC-based fulcio v2 APIs #1762

bobcallaway commented Apr 14, 2022 •

edited

Loading

nsmith5 commented Apr 14, 2022

nsmith5 commented Apr 14, 2022

codecov-commenter commented Apr 15, 2022 •

edited by codecov bot

Loading

nsmith5 commented May 13, 2022

bobcallaway commented May 20, 2022

dlorenc commented May 20, 2022

dlorenc commented May 20, 2022

haydentherapper commented May 24, 2022

haydentherapper commented Jun 2, 2022

bobcallaway commented Jun 2, 2022 via email

haydentherapper Jun 2, 2022

haydentherapper Jun 2, 2022

haydentherapper Jun 2, 2022

github-actions bot commented Aug 20, 2022

github-actions bot commented Sep 20, 2022

vaikas commented Nov 29, 2022

bobcallaway commented Nov 29, 2022

vaikas commented Nov 29, 2022

bobcallaway commented Nov 30, 2022

vaikas commented Nov 30, 2022 via email

haydentherapper commented Dec 5, 2022

github-actions bot commented Jan 5, 2023

github-actions bot commented Feb 5, 2023

github-actions bot commented Mar 9, 2023

github-actions bot commented Apr 9, 2023

github-actions bot commented Apr 19, 2023

github-actions bot commented May 24, 2023

github-actions bot commented Jun 30, 2023

github-actions bot commented Jul 10, 2023

Switch to calling gRPC-based fulcio v2 APIs #1762

Switch to calling gRPC-based fulcio v2 APIs #1762

Conversation

bobcallaway commented Apr 14, 2022 • edited Loading

nsmith5 commented Apr 14, 2022

nsmith5 commented Apr 14, 2022

codecov-commenter commented Apr 15, 2022 • edited by codecov bot Loading

Codecov Report

nsmith5 commented May 13, 2022

bobcallaway commented May 20, 2022

dlorenc commented May 20, 2022

dlorenc commented May 20, 2022

haydentherapper commented May 24, 2022

haydentherapper commented Jun 2, 2022

bobcallaway commented Jun 2, 2022 via email

haydentherapper Jun 2, 2022

Choose a reason for hiding this comment

haydentherapper Jun 2, 2022

Choose a reason for hiding this comment

haydentherapper Jun 2, 2022

Choose a reason for hiding this comment

github-actions bot commented Aug 20, 2022

github-actions bot commented Sep 20, 2022

vaikas commented Nov 29, 2022

bobcallaway commented Nov 29, 2022

vaikas commented Nov 29, 2022

bobcallaway commented Nov 30, 2022

vaikas commented Nov 30, 2022 via email

haydentherapper commented Dec 5, 2022

github-actions bot commented Jan 5, 2023

github-actions bot commented Feb 5, 2023

github-actions bot commented Mar 9, 2023

github-actions bot commented Apr 9, 2023

github-actions bot commented Apr 19, 2023

github-actions bot commented May 24, 2023

github-actions bot commented Jun 30, 2023

github-actions bot commented Jul 10, 2023

bobcallaway commented Apr 14, 2022 •

edited

Loading

codecov-commenter commented Apr 15, 2022 •

edited by codecov bot

Loading