Drop OpenAPI from Fulcio #262

mattmoor · 2021-12-02T17:18:34Z

Signed-off-by: Matt Moore mattmoor@chainguard.dev

Release Note

Fulcio no longer uses OpenAPI.
The generated client in ./pkg/client has been replaced with an artisan hand-crafted client in ./pkg/api.NewClient

mattmoor · 2021-12-02T17:39:45Z

This needs fairly careful testing and scrutiny before it goes in. This drops a number of things on the floor, some of which we clearly need a replacement for including:

pkg/client: Used by cosign, so kinda important. We probably don't need swagger for this, but we need something.
Discovery docs: I think the API used to serve this, but now it doesn't. We should confirm if this is a problem, and probably add test coverage if we want it.

I like the whole "dropping 3500 LoC outside of vendor/" aspect of this change, though. 🤣

Related to: sigstore/fulcio#262 Signed-off-by: Matt Moore <mattmoor@chainguard.dev>

mattmoor · 2021-12-02T23:44:09Z

Alright, I think that this is relatively complete, assuming we're cool with ditching the discovery stuff. I staged the update to cosign to consume the new client library here: sigstore/cosign#1126

I'm not sure what else might be relying on the existing fulcio client logic, so any feedback would be appreciated.

mattmoor · 2021-12-02T23:47:04Z

pkg/config/config.go

-		cache, err := lru.New2Q(100 /* size */)
-		if err != nil {
+		config := DefaultConfig
+		if err := config.prepare(); err != nil {


This is a bug at HEAD. We don't set up the OIDCIssuers (just the lru).

mattmoor · 2021-12-02T23:47:53Z

pkg/challenges/challenges.go

+	if u.Scheme != "spiffe" {
+		return false
+	}
+	if u.Hostname() == host {


I changed this because writing my unit test (api_test.go) this doesn't properly handle port numbers at HEAD.

pkg/ca/x509ca/common.go

mattmoor · 2021-12-04T00:11:11Z

TODO: incorporate: #264

bobcallaway

overall, seems like this is a functionally equivalent swap on a per-request basis so if your goal was to remove thousands of lines of generated code, I think you've achieved it :)

we'd also need to remove the dep on go-swagger from hack/tools/ even though it isn't called.

bobcallaway · 2021-12-06T12:20:44Z

pkg/api/client.go

+}
+
+// NewClient creates a new Fulcio API client talking to the provided URL.
+func NewClient(url *url.URL) Client {


could be an additional patch, but would be nice to be able to override the client here (e.g. to set a specific timeout value)

Let's follow up with that, I opened: #266

bobcallaway · 2021-12-06T12:21:37Z

pkg/api/client.go

+	CertPEM  []byte
+	ChainPEM []byte


worth parsing them into x509.Certificate and []x509.Certificate for increased usability?

Lemme dig into how this is used on the cosign side more, since this came from there originally. Generally SGTM, but I just want to avoid a nightmare picking up this change.

Looks like these are passed around a fair amount. First to Signer here:

https://github.com/sigstore/cosign/blob/a47a835480c45d6db7a9751d7cb8bf87772799fc/cmd/cosign/cli/fulcio/fulcio.go#L185-L187

Next to SignerVerifier here:

https://github.com/sigstore/cosign/blob/7e5ff000f6af25793631eb590964b9a861d672dc/cmd/cosign/cli/sign/sign.go#L413-L415

Chasing the usages of the SignerVerifier's .Cert it seems like the Rekor uploads want things like this, but only the policy code (to extract email/issuer) wants the structured form.

I'm mildly inclined to "Nack" on this, since it's a bit of plumbing and (given current usage) I'm not sure it helps usability a ton. Maybe we can add an accessor to get the x509.Certificate later. I can change if you feel strongly though, just LMK.

I'm OK with a nack here.

bobcallaway · 2021-12-06T12:25:17Z

openapi.yaml

@@ -1,116 +0,0 @@
-#


do we need to delete this? I get the desire to remove go-swagger and the code-gen, but we've had several requests for API documentation.

I can bring it back, but we'd need to find a way to ensure it stays in sync with the code. Any ideas?

cmd/app/serve.go

We serve a single handler, so OpenAPI creates a ton of overhead for our use case. This change switches to a relatively simple http.Handler based API, and a hand-rolled API client. This also contains unit testing of the client/server using a trivial OIDC implementation and ephemeralca. Signed-off-by: Matt Moore <mattmoor@chainguard.dev>

Signed-off-by: Matt Moore <mattmoor@chainguard.dev>

bobcallaway

LGTM, I dont see any other functional issues. Just a couple minor things I saw but I think this is fine.

cmd/app/serve.go

pkg/api/ca.go

bobcallaway · 2021-12-07T17:42:00Z

pkg/api/client.go

+	CertPEM  []byte
+	ChainPEM []byte


I'm OK with a nack here.

bobcallaway · 2021-12-07T17:53:53Z

pkg/config/config.go

@@ -132,6 +132,24 @@ func (fc *FulcioConfig) GetVerifier(issuerURL string) (*oidc.IDTokenVerifier, bo
 	return verifier, true
 }

+func (fc *FulcioConfig) prepare() error {
+	fc.verifiers = make(map[string]*oidc.IDTokenVerifier, len(fc.OIDCIssuers))
+	for _, iss := range fc.OIDCIssuers {


do we need to ensure we have at least 1 issuer in the config? not sure if that was actually enforced before but it popped into my mind reviewing this.

It is possible now that they only configure meta issuers. I think our KinD e2e tests have a leg that does that now.

Signed-off-by: Matt Moore <mattmoor@chainguard.dev>

mattmoor · 2021-12-07T18:16:45Z

Fixed the two suggestions, thanks!

mattmoor · 2021-12-07T18:17:09Z

I'll start dusting off my changes to import this into sigstore/cosign

Related to: sigstore/fulcio#262 Signed-off-by: Matt Moore <mattmoor@chainguard.dev>

…st for it. I wrapped it in a job, since otherwise it will be trickier to get access to the metrics from outside the kind cluster. Fix the dangling pointer from sigstore#262 and fix a tiny typo, though I kind of do like exposing a singingCert that might come handy in karaoke? Signed-off-by: Ville Aikas <vaikas@chainguard.dev>

#267) * Wrap the server with the Prometheus so we get metrics + add an e2e test for it. I wrapped it in a job, since otherwise it will be trickier to get access to the metrics from outside the kind cluster. Fix the dangling pointer from #262 and fix a tiny typo, though I kind of do like exposing a singingCert that might come handy in karaoke? Signed-off-by: Ville Aikas <vaikas@chainguard.dev> * oops :) Signed-off-by: Ville Aikas <vaikas@chainguard.dev> * Expose the prometheus port. Signed-off-by: Ville Aikas <vaikas@chainguard.dev> * Just dumping the metrics to see if they are there or not. Signed-off-by: Ville Aikas <vaikas@chainguard.dev> * Working theory is that because there's 3 replicas, we get super duper lucky and hit the one that we didn't sign, so testing that. Signed-off-by: Ville Aikas <vaikas@chainguard.dev> * Switch to single replica so we can scrape easier. Remove debug cruft. Signed-off-by: Ville Aikas <vaikas@chainguard.dev> * Modify deployment in place instead of modifying the deployment.yaml. Signed-off-by: Ville Aikas <vaikas@chainguard.dev>

mattmoor force-pushed the drop-openapi branch 4 times, most recently from 9ebf622 to a03c19c Compare December 2, 2021 17:29

mattmoor added a commit to mattmoor/cosign that referenced this pull request Dec 2, 2021

[WIP][DNM] Experiment with new fulcio client code.

d6892c3

Related to: sigstore/fulcio#262 Signed-off-by: Matt Moore <mattmoor@chainguard.dev>

mattmoor mentioned this pull request Dec 2, 2021

Pull in the new Fulcio client sigstore/cosign#1126

Merged

mattmoor force-pushed the drop-openapi branch 3 times, most recently from 2725ca6 to d0b4a85 Compare December 2, 2021 23:06

mattmoor added a commit to mattmoor/cosign that referenced this pull request Dec 2, 2021

[WIP][DNM] Experiment with new fulcio client code.

0a8a3e5

Related to: sigstore/fulcio#262 Signed-off-by: Matt Moore <mattmoor@chainguard.dev>

mattmoor commented Dec 2, 2021

View reviewed changes

pkg/ca/x509ca/common.go Outdated Show resolved Hide resolved

mattmoor force-pushed the drop-openapi branch from d0b4a85 to 385929d Compare December 2, 2021 23:50

mattmoor changed the title ~~[WIP] Experiment with dropping OpenAPI~~ [WIP] Drop OpenAPI from Fulcio Dec 2, 2021

mattmoor requested a review from bobcallaway December 3, 2021 00:04

mattmoor force-pushed the drop-openapi branch from 385929d to 71ca2cb Compare December 3, 2021 20:59

bobcallaway reviewed Dec 6, 2021

View reviewed changes

mattmoor mentioned this pull request Dec 6, 2021

ClientOption for specifying request timeout #266

Closed

mattmoor force-pushed the drop-openapi branch from 71ca2cb to 5b36f68 Compare December 6, 2021 22:10

mattmoor changed the title ~~[WIP] Drop OpenAPI from Fulcio~~ Drop OpenAPI from Fulcio Dec 6, 2021

Add back host/port flags

aa723db

Signed-off-by: Matt Moore <mattmoor@chainguard.dev>

mattmoor force-pushed the drop-openapi branch from fd06228 to aa723db Compare December 6, 2021 22:41

mattmoor added 2 commits December 6, 2021 14:42

Drop go-swagger vendor

7db3154

Signed-off-by: Matt Moore <mattmoor@chainguard.dev>

Move flags to rootCmd to match others

9c69502

Signed-off-by: Matt Moore <mattmoor@chainguard.dev>

bobcallaway reviewed Dec 7, 2021

View reviewed changes

Check ErrServerClosed and fix status code for media types

ca5e2e4

Signed-off-by: Matt Moore <mattmoor@chainguard.dev>

mattmoor added a commit to mattmoor/cosign that referenced this pull request Dec 7, 2021

[WIP][DNM] Experiment with new fulcio client code.

09d9bda

Related to: sigstore/fulcio#262 Signed-off-by: Matt Moore <mattmoor@chainguard.dev>

mattmoor added a commit to mattmoor/cosign that referenced this pull request Dec 7, 2021

[WIP][DNM] Experiment with new fulcio client code.

62a5237

Related to: sigstore/fulcio#262 Signed-off-by: Matt Moore <mattmoor@chainguard.dev>

dlorenc approved these changes Dec 7, 2021

View reviewed changes

bobcallaway approved these changes Dec 7, 2021

View reviewed changes

mattmoor merged commit f4746cc into sigstore:main Dec 7, 2021

mattmoor deleted the drop-openapi branch December 7, 2021 18:44

mattmoor added a commit to mattmoor/cosign that referenced this pull request Dec 7, 2021

Pull in the new Fulcio client code.

56d6eda

Related to: sigstore/fulcio#262 Signed-off-by: Matt Moore <mattmoor@chainguard.dev>

mattmoor added a commit to sigstore/cosign that referenced this pull request Dec 7, 2021

Pull in the new Fulcio client code. (#1126)

9394f85

Related to: sigstore/fulcio#262 Signed-off-by: Matt Moore <mattmoor@chainguard.dev>

vaikas mentioned this pull request Dec 8, 2021

Wrap the server with the Prometheus so we get metrics + add an e2e te… #267

Merged

di mentioned this pull request Mar 25, 2022

Add a simple Rekor client sigstore/sigstore-python#12

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Drop OpenAPI from Fulcio #262

Drop OpenAPI from Fulcio #262

mattmoor commented Dec 2, 2021 •

edited

mattmoor commented Dec 2, 2021

mattmoor commented Dec 2, 2021

mattmoor Dec 2, 2021

mattmoor Dec 2, 2021

mattmoor commented Dec 4, 2021

bobcallaway left a comment

bobcallaway Dec 6, 2021

mattmoor Dec 6, 2021

bobcallaway Dec 6, 2021

mattmoor Dec 6, 2021

mattmoor Dec 6, 2021

bobcallaway Dec 7, 2021

bobcallaway Dec 6, 2021

mattmoor Dec 6, 2021

bobcallaway left a comment

bobcallaway Dec 7, 2021

bobcallaway Dec 7, 2021

mattmoor Dec 7, 2021

mattmoor commented Dec 7, 2021

mattmoor commented Dec 7, 2021

Drop OpenAPI from Fulcio #262

Drop OpenAPI from Fulcio #262

Conversation

mattmoor commented Dec 2, 2021 • edited

Release Note

mattmoor commented Dec 2, 2021

mattmoor commented Dec 2, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattmoor commented Dec 4, 2021

bobcallaway left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bobcallaway left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattmoor commented Dec 7, 2021

mattmoor commented Dec 7, 2021

mattmoor commented Dec 2, 2021 •

edited