Replace docker/libtrust with go-jose/go-jose #4096

milosgajdos · 2023-10-08T22:16:49Z

This PR replaces all the docker/libtrust JWT machinery with go-jose/go-jose module. Some of the code has been adopted from the libtrust and adjusted for some of the use cases covered by the token authorization code, especially in the unit tests.

NOTE: docker/libtrust module has been archived for several years now.

Closes #1820

thaJeztah · 2023-10-08T22:31:31Z

So from the discussion on slack; did we actually still need it at all? /cc @dmcgowan

thaJeztah · 2023-10-08T22:32:50Z

Oh! Should've looked; it somehow found it's way into auth as well, not just for signing v1 images

milosgajdos · 2023-10-08T22:39:26Z

It is indeed. This PR is in a draft because its a ghetto code that needs cleaning up.

Basically you can sign your token with different types of keys etc and we need to make sure they're legit and that the token hasn't been tampered with.

docs/spec/auth/jwt.md

go.mod

registry/auth/token/certificates.go

registry/auth/token/crypto.go

registry/auth/token/token_test.go

milosgajdos · 2023-10-09T19:41:42Z

Ok, I've cleaned up my last night's horrible code and I think we can proceed to getting this reviewed and iterated on.

PTAL @thaJeztah @dmcgowan @corhere @Jamstah

Jamstah

I still think we need to separate the test code more cleanly.

I'd like to remove the libtrust kid format if we can. I know we're no longer depending on the package, but consumers would either need to depend on the libtrust package to make the kid, or also have their own code. It doesn't seem to be a standards thing.

registry/auth/token/certificates.go

registry/auth/token/crypto.go

docs/spec/auth/jwt.md

registry/auth/token/crypto.go

Jamstah · 2023-10-10T10:44:46Z

FYI, the export regulation disclaimer worried me too, so I raised this: go-jose/go-jose#61

milosgajdos · 2023-10-10T18:17:40Z

I'd like to remove the libtrust kid format if we can. I know we're no longer depending on the package, but consumers would either need to depend on the libtrust package to make the kid, or also have their own code. It doesn't seem to be a standards thing.

What are your thoughts @dmcgowan? I kinda agree but we need some replacement for that if we do rip that out.

milosgajdos · 2023-10-10T18:44:02Z

I kinda wonder if we could replace the KeyID with a fingerprint similar to the SSH one: https://pkg.go.dev/golang.org/x/crypto/ssh#FingerprintSHA256

Jamstah · 2023-10-10T19:20:13Z

I'd like to remove the libtrust kid format if we can. I know we're no longer depending on the package, but consumers would either need to depend on the libtrust package to make the kid, or also have their own code. It doesn't seem to be a standards thing.

What are your thoughts @dmcgowan? I kinda agree but we need some replacement for that if we do rip that out.

Do we need a replacement?

The kid is really for picking a key from a JWKS that is provided by an issuer. I don't think that inventing one from the key material itself is valuable unless someone is coding an issuer implementation specifically for us that also matches (like what libtrust did and expected).

I guess one option would be to ask the user to specify the kid for each public cert. Another option would be to accept a JWKS url and pick up the kid/cert pairs from that. A final slightly less secure option would be to ignore the kid and test all the roots.

milosgajdos · 2023-10-10T19:26:31Z

Do we need a replacement?

Well accesscontroller maintains a map of trusted publick keys

distribution/registry/auth/token/accesscontroller.go

Line 135 in f7b3869

trustedKeys map[string]libtrust.PublicKey

Those are optionally supplied by config

distribution/registry/auth/token/accesscontroller.go

Line 184 in f7b3869

fp, err := os.Open(config.rootCertBundle)

Jamstah · 2023-10-10T19:30:28Z

The key of the map is what we're talking about though, its the libtrust format. Its only valuable if the token issuer uses the same mapping for the keys in its JWKS. Considering that no one should be using libtrust any more, that's a big ask.

We really need the user to specify kid/root pairs so they can match them to their issuer. Even better would be to accept a JWKS and use that instead.

milosgajdos · 2023-10-10T19:32:54Z

The key of the map is what we're talking about though

Yes very much so - too many overloaded terms!

Jamstah · 2023-10-10T19:34:20Z

I think this might be the right option:

If the user specifies roots, then we only verify tokens that are x5c.
If the user specifies a file that contains a JWKS, then we give it to go-jose and let it do the work.

So, auth.rootCertBundle and auth.jwks in the configuration.

milosgajdos · 2023-10-10T22:31:15Z

I think this might be the right option:
* If the user specifies roots, then we only verify tokens that are x5c.

* If the user specifies a file that contains a JWKS, then we give it to go-jose and let it do the work.
So, auth.rootCertBundle and auth.jwks in the configuration.

Correct me if I'm wrong but there should be an option to specify both, right 🤔 It should be your choice what tokens your registry trusts. I may be overthinking this though...

Jamstah · 2023-10-11T00:22:09Z

Yes, I don't see why not. If there are both roots and a JWKS configured, use whichever is appropriate for the token.

Or, if there is an x5c in the token, we could verify that with any roots found in the JWKS (which is like our behaviour now). Then we would only have the JWKS option in the config.

If there is a kid in the token, we use it to pick something from the JWKS.

registry/auth/token/crypto.go

milosgajdos · 2023-10-17T19:44:24Z

Alright, so I got a first step in the direction we agreed on - unless I misunderstood 🙃

PTAL @Jamstah Mind you, it's a bit of a ghetto code that will need cleaning up, but would like your take on this 😬

Also, if @corhere has some spare cycles, that'd be dope (I'm still expecting him to open that context cleanup PR he keeps promising me 😏)

(yes we will still need to update docs with the jwks option, but I postponed it until we've agreed on the code 😄 )

corhere · 2023-10-17T20:12:39Z

(I'm still expecting him to open that context cleanup PR he keeps promising me 😏)

Hey now, I only promised to review the context cleanup PR you were going to open @milosgajdos

milosgajdos · 2023-10-17T20:16:00Z

Hey now, I only promised to review the context cleanup PR you were going to open @milosgajdos

Saw right through my innocuous gaslighting 🙃

Jamstah · 2023-10-17T21:23:33Z

Should we be looking at something like this?

https://pkg.go.dev/github.com/s12v/go-jwks

milosgajdos · 2023-10-17T21:28:37Z

Should we be looking at something like this?

https://pkg.go.dev/github.com/s12v/go-jwks

You mean for rotation? Yeah, seems reasonable. Let's rip libtrust out first though 😄

Jamstah

We can rip out a load of the libtrust crypto from the test files too.

We don't have to, but it reduces the complexity.

Jamstah@f485172

registry/auth/token/accesscontroller.go

milosgajdos · 2023-10-18T06:51:49Z

We can rip out a load of the libtrust crypto from the test files too.

We don't have to, but it reduces the complexity.

Jamstah@f485172

I was going to look into ripping that crypto_test.go file out in the followup. There is more cleanup to be done beyond that, I feel 😅 Nice! I'll [shamelessly] adopt these changes 😄

thaJeztah · 2023-10-18T12:34:07Z

I'll try to find some time to review, but with a big disclaimer that this area is not really my expertise. I did ping @jcarter3 and @technicallyjosh on our Slack, as they are more familiar with this, and they were definitely in favor of removing libtrust (but don't know to what extent they reviewed the change in-depth).

milosgajdos · 2023-10-18T12:45:04Z

@thaJeztah we need @dmcgowan here primarily. He was the OG creator so he likely understands the ins and outs of this code.

dmcgowan · 2023-10-18T16:07:32Z

Agreed that removing the KID definition here makes sense to match the spec, since it is "unspecified". It should be up to the token service and jwk creator to ensure it matches using whatever format or criteria they determine to be the most secure.

The key sets from an endpoint seem interesting as well as a future change and align nicely with the distribution architecture of horizontally scaled registry instances using a token service which owns the token private key and could potentially serve that public key on an endpoint. This would make key rotation much simpler. Going along with that, supporting multiple signing keys could aid in that rotation.

milosgajdos · 2023-10-19T10:43:48Z

@Jamstah anything we're missing here that's preventing you from ✅ ? Other than the [inevitable] squash 😄

thaJeztah · 2023-10-19T10:47:24Z

Squash (or cleanup of some of the commits at least) would be nice 😅

milosgajdos · 2023-10-19T11:02:44Z

Squash (or cleanup of some of the commits at least) would be nice 😅

Yes, just wanna make sure we're done updating before I squash and merge. Otherwise all the discussion in this terrible GH Review UI will be lost in the darkness of our memories

Jamstah · 2023-10-19T11:54:31Z

Sorry, had forgotten I had a request changes open!

Lgtm.

docker/libtrust repository has been archived for several years now. This commit replaces all the libtrust JWT machinery with go-jose/go-jose module. Some of the code has been adopted from libtrust and adjusted for some of the use cases covered by the token authorization flow especially in the tests. Signed-off-by: Milos Gajdos <milosthegajdos@gmail.com>

for the sake of backward compatibility Fix distribution#4096 Fix distribution#4299 Signed-off-by: Youfu Zhang <zhangyoufu@gmail.com>

https://build.opensuse.org/request/show/1289281 by user dirkmueller + anag_factory - update to 3.0.0 (jsc#PED-11728): * This is the first v3 stable release since `v2.8.3` which is a culmination of years of hard work of the container community and registry maintainers! * If you are upgrading from `v2.x` and have never used any of the release candidates, please familiarise yourselves with the `v2.x` deprecations properly. * oss and swift storage drivers are no longer supported * `docker/libtrust` has been replaced with `go-jose/go-jose` in distribution/distribution#4096 * `client` is no longer supported as a standalone package in distribution/distribution#4126 * the default configuration path has changed to `/etc/distribution/config.yml` * `ManifestBuilder` interface in 3886 * `manif

Jamstah reviewed Oct 8, 2023

View reviewed changes

docs/spec/auth/jwt.md Outdated Show resolved Hide resolved

Jamstah reviewed Oct 8, 2023

View reviewed changes

go.mod Show resolved Hide resolved

registry/auth/token/certificates.go Outdated Show resolved Hide resolved

registry/auth/token/crypto.go Outdated Show resolved Hide resolved

registry/auth/token/token_test.go Show resolved Hide resolved

milosgajdos marked this pull request as ready for review October 9, 2023 19:39

milosgajdos requested review from Jamstah, dmcgowan, corhere, thaJeztah and squizzi October 9, 2023 19:41

Jamstah requested changes Oct 10, 2023

View reviewed changes

milosgajdos added the area/auth label Oct 16, 2023

milosgajdos force-pushed the libtrust-wipeout branch from 8fc7d61 to f34d182 Compare October 17, 2023 10:44

corhere reviewed Oct 17, 2023

View reviewed changes

registry/auth/token/crypto.go Outdated Show resolved Hide resolved

registry/auth/token/crypto.go Outdated Show resolved Hide resolved

milosgajdos requested review from Jamstah and corhere October 17, 2023 19:44

Jamstah reviewed Oct 18, 2023

View reviewed changes

registry/auth/token/accesscontroller.go Outdated Show resolved Hide resolved

milosgajdos requested a review from Jamstah October 18, 2023 07:22

milosgajdos requested a review from wy65701436 October 18, 2023 12:47

dmcgowan approved these changes Oct 18, 2023

View reviewed changes

jcarter3 approved these changes Oct 18, 2023

View reviewed changes

Jamstah approved these changes Oct 19, 2023

View reviewed changes

milosgajdos force-pushed the libtrust-wipeout branch from bbba63f to fe21f43 Compare October 19, 2023 14:36

milosgajdos merged commit dfd191e into distribution:main Oct 19, 2023

milosgajdos deleted the libtrust-wipeout branch October 19, 2023 14:52

azr mentioned this pull request Dec 19, 2023

Release cadence #4199

Closed

milosgajdos mentioned this pull request Dec 20, 2023

don't check kid, check jwt signature against all keys #3625

Closed

This was referenced Mar 12, 2024

INFO[0101] failed to verify token: token signed by untrusted key with ID: #4299

Closed

docker-registry has changed its JWT implementation, no longer supports libtrust key IDs cesanta/docker_auth#386

Closed

markuskirch mentioned this pull request May 8, 2024

JWKS section to docs #4345

Closed

milosgajdos mentioned this pull request Jul 18, 2024

Valid JWT yields "token signed by untrusted key with ID" #2875

Closed

zhangyoufu mentioned this pull request Jul 22, 2024

feat: trust rootcertbundle public keys with libtrust keyid format #4417

Closed

Replace docker/libtrust with go-jose/go-jose #4096

Replace docker/libtrust with go-jose/go-jose #4096

Uh oh!

Conversation

milosgajdos commented Oct 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thaJeztah commented Oct 8, 2023

Uh oh!

thaJeztah commented Oct 8, 2023

Uh oh!

milosgajdos commented Oct 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

milosgajdos commented Oct 9, 2023

Uh oh!

Jamstah left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Jamstah commented Oct 10, 2023

Uh oh!

milosgajdos commented Oct 10, 2023

Uh oh!

milosgajdos commented Oct 10, 2023

Uh oh!

Jamstah commented Oct 10, 2023

Uh oh!

milosgajdos commented Oct 10, 2023

Uh oh!

Jamstah commented Oct 10, 2023

Uh oh!

milosgajdos commented Oct 10, 2023

Uh oh!

Jamstah commented Oct 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

milosgajdos commented Oct 10, 2023

Uh oh!

Jamstah commented Oct 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

milosgajdos commented Oct 17, 2023

Uh oh!

corhere commented Oct 17, 2023

Uh oh!

milosgajdos commented Oct 17, 2023

Uh oh!

Jamstah commented Oct 17, 2023

Uh oh!

milosgajdos commented Oct 17, 2023

Uh oh!

Jamstah left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

milosgajdos commented Oct 18, 2023

Uh oh!

thaJeztah commented Oct 18, 2023

Uh oh!

milosgajdos commented Oct 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dmcgowan commented Oct 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

milosgajdos commented Oct 8, 2023 •

edited

Loading

milosgajdos commented Oct 8, 2023 •

edited

Loading

Jamstah commented Oct 10, 2023 •

edited

Loading

Jamstah commented Oct 11, 2023 •

edited

Loading

Jamstah left a comment •

edited

Loading

milosgajdos commented Oct 18, 2023 •

edited

Loading

dmcgowan commented Oct 18, 2023 •

edited

Loading