-
Notifications
You must be signed in to change notification settings - Fork 138
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
Drop OpenAPI from Fulcio #262
Conversation
9ebf622
to
a03c19c
Compare
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:
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>
2725ca6
to
d0b4a85
Compare
Related to: sigstore/fulcio#262 Signed-off-by: Matt Moore <mattmoor@chainguard.dev>
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. |
cache, err := lru.New2Q(100 /* size */) | ||
if err != nil { | ||
config := DefaultConfig | ||
if err := config.prepare(); err != 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.
This is a bug at HEAD. We don't set up the OIDCIssuers (just the lru).
if u.Scheme != "spiffe" { | ||
return false | ||
} | ||
if u.Hostname() == host { |
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 changed this because writing my unit test (api_test.go
) this doesn't properly handle port numbers at HEAD.
d0b4a85
to
385929d
Compare
385929d
to
71ca2cb
Compare
TODO: incorporate: #264 |
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.
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.
pkg/api/client.go
Outdated
} | ||
|
||
// NewClient creates a new Fulcio API client talking to the provided URL. | ||
func NewClient(url *url.URL) Client { |
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.
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)
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.
Let's follow up with that, I opened: #266
CertPEM []byte | ||
ChainPEM []byte |
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.
worth parsing them into x509.Certificate and []x509.Certificate
for increased usability?
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.
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.
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 like these are passed around a fair amount. First to Signer
here:
Next to SignerVerifier
here:
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.
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 OK with a nack here.
@@ -1,116 +0,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.
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.
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 can bring it back, but we'd need to find a way to ensure it stays in sync with the code. Any ideas?
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>
71ca2cb
to
5b36f68
Compare
Signed-off-by: Matt Moore <mattmoor@chainguard.dev>
fd06228
to
aa723db
Compare
Signed-off-by: Matt Moore <mattmoor@chainguard.dev>
Signed-off-by: Matt Moore <mattmoor@chainguard.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.
LGTM, I dont see any other functional issues. Just a couple minor things I saw but I think this is fine.
CertPEM []byte | ||
ChainPEM []byte |
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 OK with a nack here.
@@ -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 { |
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.
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.
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 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>
Fixed the two suggestions, thanks! |
I'll start dusting off my changes to import this into |
Related to: sigstore/fulcio#262 Signed-off-by: Matt Moore <mattmoor@chainguard.dev>
Related to: sigstore/fulcio#262 Signed-off-by: Matt Moore <mattmoor@chainguard.dev>
Related to: sigstore/fulcio#262 Signed-off-by: Matt Moore <mattmoor@chainguard.dev>
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>
Signed-off-by: Matt Moore mattmoor@chainguard.dev
Release Note