fix: Add the token's rootcert public key to the list of known keys #4471

josegomezr · 2024-09-24T14:17:48Z

Borrowed two functions from docker/libtrust to generate the key ID from the certificate bundle in the same fashion as v2.8.3.

Addresses: #4470

milosgajdos

Thanks for the PR! FWIW, we actually want to avoid any of the libtrust cruft seeping back to this codebase, unless the code references some standard/specification.

I need to have a proper look at this. Trusted keys should be read from jwks', keyed by their KeyID assuming your jwks files have been correctly generated I'm a bit puzzled why this breaks 🤔

josegomezr · 2024-09-24T14:52:14Z

Trusted keys should be read from jwks', keyed by their KeyID assuming your jwks files have been correctly generated I'm a bit puzzled why this breaks 🤔

That part works, in our setup we did not have the jwks key defined (as with 2.8.3) and that caused the registry not trust any key, even the certificate one.

Thanks for the PR! FWIW, we actually want to avoid any of the libtrust cruft seeping back to this codebase, unless the code references some standard/specification.

I've seen that the ruby implementation of JWT uses just the plain SHA256 hexdigest as kid by default. But also an RFC mentioned, RFC7638.

josegomezr · 2024-09-24T15:29:51Z

c5c0e46f is an to add unit tests to the changes. I felt it was reaching a bit too much but gave it a try nevertheless. Let me know if it's too bad and I'll drop that commit.

Addenda: forgot the sign-off.

josegomezr · 2024-09-24T17:14:04Z

Took the liberty to implement the JWT hashing mechanism that RFC7638 defines to use it as KeyID instead of the old docker/libtrust impl.

It should satisfy:

the code references some standard/specification.

or at least get closer 😅

milosgajdos · 2024-09-25T08:42:11Z

that caused the registry not trust any key, even the certificate one.

this is what puzzles me...as I said there was a regression that should have been fixed by #4415 which should have made the cert validation kick in before the KeyID check kicks in 🤔 this is what I'm trying to understand - why is this still broken

josegomezr · 2024-09-25T09:01:10Z

Maybe is worth noticing that our token server does not include the x5c header in the JWT Token

milosgajdos · 2024-09-25T09:10:13Z

Maybe is worth noticing that our token server does not include the x5c header in the JWT Token

Hah! Yeah, that'll do it...

josegomezr · 2024-09-25T09:17:51Z

Hah! Yeah, that'll do it...

😅 I knew I missed something

milosgajdos · 2024-09-25T09:37:33Z

That should also probably be added to the docs though 🤔

josegomezr · 2024-09-25T10:42:17Z

Should I write something up in this PR? Or a new one?

milosgajdos · 2024-09-25T11:44:32Z

I think it can go into this PR, but mind you I haven't had time to look at this PR yet

josegomezr · 2024-09-25T11:59:51Z

Ok, let's do two PR's to keep it lightly then. I'll wait for your feedback on this one :)

milosgajdos

I like this approach and I think it could work in distribution.

Couple of things:

some stylistic changes needed as per the comments
we need to update the docs mentioning how the JWK thumbprint is calculated

But I think if we iron this out, this would be good to be merged in.

Thoughts @Jamstah @thaJeztah ?

registry/auth/token/accesscontroller.go

registry/auth/token/accesscontroller_test.go

registry/auth/token/util.go

Jamstah

Overall, doesn't degrade anything, and seems to be a sensibly defined spec for making a kid from a JWK.

I'm still not 100% on the need - if the token server is providing JWTs with a kid, then it should also be providing a JWKS url that can be used to lookup the keys. My read of the RFC is that its defining a sensible and secure approach for people writing token servers to generate kid values.

I'm happy with the implementation (other than the comments already raised by Milos!). Requesting changes as I would like to have the documentation alongside the code change.

registry/auth/token/accesscontroller.go

registry/auth/token/accesscontroller_test.go

josegomezr · 2024-09-30T17:16:09Z

if the token server is providing JWTs with a kid, then it should also be providing a JWKS url that can be used to lookup the keys

Does distribution supports this discovery mechanism? I could swear only the JSON file path is allowed 🤔

milosgajdos · 2024-09-30T17:26:23Z

Does distribution supports this discovery mechanism? I could swear only the JSON file path is allowed 🤔

Correct, it doesn't. It would be nice though, because we could technically then implement key rotation, alas...

josegomezr · 2024-09-30T17:39:13Z

With regards to documenting:

in docs/content/spec/auth/token.md there are no pointers to docs/content/spec/auth/jwt.md maybe is worth linking them from item 4 on the list in token.md

The authorization service returns an opaque Bearer token representing the client's authorized access.

What do y'all think?

And in jwt.md I'm thinking on reflecting the x5c header in Getting a Bearer Token > Headers and within Verifying the token list

Stating that the "Signing Key verification" happens first and it goes a little something like:

Ensure that the certificate chain provided is valid. If it fails (due to the x5c header missing):
Ensure that the provided JWK in the JWT is valid.
Ensure that the provided KeyID is known (as per the configured jwks.json in auth.token.jwks config)
Fail with Invalid Token

Maybe a mermaid flow diagram could illustrate better the logic?

milosgajdos · 2024-09-30T17:42:45Z

in docs/content/spec/auth/token.md there are no pointers to docs/content/spec/auth/jwt.md maybe is worth linking them from item 4 on the list in token.md

I think that'd make sense, yeah.

And in jwt.md I'm thinking on reflecting the x5c header in Getting a Bearer Token > Headers and within Verifying the token list

Stating that the "Signing Key verification" happens first and it goes a little something like:

Ensure that the certificate chain provided is valid. If it fails (due to the x5c header missing):

Ensure that the provided JWK in the JWT is valid.

Ensure that the provided KeyID is known (as per the configured jwks.json in auth.token.jwks config)

Fail with Invalid Token

Maybe a mermaid flow diagram could illustrate better the logic?

This sounds good. Not sure I'd take it as far as using Mermaid, but if you do I think you'd have to add some deps into package.json in docs and make sure it renders properly.

josegomezr · 2024-09-30T17:47:13Z

but if you do I think you'd have to add some deps into package.json in docs and make sure it renders properly.

How about ASCII art? (Anything just not to deal with package.json "fun")

milosgajdos · 2024-09-30T17:53:25Z

How about ASCII art? (Anything just not to deal with package.json "fun")

Yeah, ASCII diagram would work. I think we have some already scattered around the docs.

josegomezr · 2024-09-30T19:16:34Z

Small question, in jwt.md this sentence does not match the implementation in go-jose/go-jose@v4

The "typ" field will be "JWT"

typ is exposed in the jose.Header struct under the ExtraHeaders map but not used in distribution anywhere.

Additionally, RFC7515 Section 4.1.9 defines typ
although it does not mention "JWT" as a value.

It does mention typ = JOSE to signal that there's a JWS contained. Would it be worth to align this value to the RFC?

OOOOR, on the other hand, maybe is mean to be cty = JWT instead?

https://github.com/go-jose/go-jose/blob/v4.0.4/jwt/jwt.go#L189

p.s.: looks like I didn't need ASCII art after all 😅

milosgajdos · 2024-09-30T19:41:11Z

It does mention typ = JOSE to signal that there's a JWS contained. Would it be worth to align this value to the RFC?

I think so, that's an interesting catch

OOOOR, on the other hand, maybe is mean to be cty = JWT instead?

https://github.com/go-jose/go-jose/blob/v4.0.4/jwt/jwt.go#L189

Not sure about that...cty seems to be suggesting content type whilst typ is a media type (which usually starts with application/... so something like application/jwt see https://www.iana.org/assignments/media-types/media-types.xhtml)

josegomezr · 2024-09-30T19:57:26Z

aha! found the reference

https://datatracker.ietf.org/doc/html/rfc7519#section-5.1

5.1.  "typ" (Type) Header Parameter

   The "typ" (type) Header Parameter defined by [JWS] and [JWE] is used
   by JWT applications to declare the media type [IANA.MediaTypes] of
   this complete JWT.  This is intended for use by the JWT application
   when values that are not JWTs could also be present in an application
   data structure that can contain a JWT object; the application can use
   this value to disambiguate among the different kinds of objects that
   might be present.  It will typically not be used by applications when
   it is already known that the object is a JWT.  This parameter is
   ignored by JWT implementations; any processing of this parameter is
   performed by the JWT application.  If present, it is RECOMMENDED that
   its value be "JWT" to indicate that this object is a JWT.  While
   media type names are not case sensitive, it is RECOMMENDED that "JWT" # <<<--- ding ding ding ding
   always be spelled using uppercase characters for compatibility with
   legacy implementations.  Use of this Header Parameter is OPTIONAL.

docs/content/spec/auth/jwt.md

josegomezr · 2024-09-30T20:10:12Z

There's one piece I don't know well where to put, the Certificate Thumbprint as the Default KeyID of the Certificate configured in auth.token.rootcertbundle.

There are no references of that key in the JWT/Token docs, but it is referenced in docs/content/about/configuration.md.

Any suggestions where that piece would be better written? 🤔

milosgajdos · 2024-09-30T20:21:19Z

I would put it into the configuration page and cross-reference it in the token auth 🤷‍♂️

docs/content/spec/auth/jwt.md

Jamstah

LGTM.

milosgajdos · 2024-10-02T09:33:23Z

@josegomezr can you please squash and sign your commit?

- Add Unit tests for `token.newAccessController` + Implemented swappable implementations for `token.getRootCerts` and `getJwks` to unit test their behavior over the accessController struct. - Use RFC7638 [0] mechanics to compute the KeyID of the rootcertbundle provided in the token auth config. - Extends token authentication docs: + Extend `jwt.md` write up on JWT headers & JWT Validation + Updated old reference to a draft that's now RFC7515. + Extended the JWT validation steps with the JWT Header validation. + Reference `jwt.md` in `token.md` [0]: https://datatracker.ietf.org/doc/html/rfc7638#autoid-13 Signed-off-by: Jose D. Gomez R <jose.gomez@suse.com>

josegomezr · 2024-10-02T18:22:22Z

A pleasure to contribute :) until the next ~~bug~~ feature 🐞 🚀

github-actions bot added the area/auth label Sep 24, 2024

milosgajdos reviewed Sep 24, 2024

View reviewed changes

josegomezr force-pushed the fix_rootcert_keyid_token branch 3 times, most recently from af62d0b to 147347f Compare September 24, 2024 17:11

milosgajdos reviewed Sep 30, 2024

View reviewed changes

Jamstah requested changes Sep 30, 2024

View reviewed changes

milosgajdos reviewed Sep 30, 2024

View reviewed changes

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

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

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

josegomezr force-pushed the fix_rootcert_keyid_token branch from 0254957 to 6a58b70 Compare September 30, 2024 17:50

github-actions bot added the area/docs label Sep 30, 2024

josegomezr commented Sep 30, 2024

View reviewed changes

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

milosgajdos reviewed Oct 2, 2024

View reviewed changes

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

josegomezr force-pushed the fix_rootcert_keyid_token branch from 9d66c6e to b41534d Compare October 2, 2024 09:03

milosgajdos requested a review from Jamstah October 2, 2024 09:13

Jamstah approved these changes Oct 2, 2024

View reviewed changes

josegomezr force-pushed the fix_rootcert_keyid_token branch from b41534d to b53946d Compare October 2, 2024 09:58

milosgajdos approved these changes Oct 2, 2024

View reviewed changes

milosgajdos merged commit 2c7d93a into distribution:main Oct 2, 2024
16 checks passed

josegomezr mentioned this pull request Oct 24, 2024

Token Auth requires JWKS although docs says is optional #4470

Open

brandond mentioned this pull request Dec 3, 2024

Load libtrust KeyIDs from token certificate bundle #4521

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Add the token's rootcert public key to the list of known keys #4471

fix: Add the token's rootcert public key to the list of known keys #4471

josegomezr commented Sep 24, 2024

milosgajdos left a comment

josegomezr commented Sep 24, 2024

josegomezr commented Sep 24, 2024 •

edited

Loading

josegomezr commented Sep 24, 2024

milosgajdos commented Sep 25, 2024

josegomezr commented Sep 25, 2024

milosgajdos commented Sep 25, 2024

josegomezr commented Sep 25, 2024

milosgajdos commented Sep 25, 2024

josegomezr commented Sep 25, 2024

milosgajdos commented Sep 25, 2024

josegomezr commented Sep 25, 2024

milosgajdos left a comment

Jamstah left a comment •

edited

Loading

josegomezr commented Sep 30, 2024

milosgajdos commented Sep 30, 2024

josegomezr commented Sep 30, 2024

milosgajdos commented Sep 30, 2024

josegomezr commented Sep 30, 2024

milosgajdos commented Sep 30, 2024

josegomezr commented Sep 30, 2024

milosgajdos commented Sep 30, 2024

josegomezr commented Sep 30, 2024

josegomezr commented Sep 30, 2024

milosgajdos commented Sep 30, 2024

Jamstah left a comment

milosgajdos commented Oct 2, 2024

josegomezr commented Oct 2, 2024

fix: Add the token's rootcert public key to the list of known keys #4471

fix: Add the token's rootcert public key to the list of known keys #4471

Conversation

josegomezr commented Sep 24, 2024

milosgajdos left a comment

Choose a reason for hiding this comment

josegomezr commented Sep 24, 2024

josegomezr commented Sep 24, 2024 • edited Loading

josegomezr commented Sep 24, 2024

milosgajdos commented Sep 25, 2024

josegomezr commented Sep 25, 2024

milosgajdos commented Sep 25, 2024

josegomezr commented Sep 25, 2024

milosgajdos commented Sep 25, 2024

josegomezr commented Sep 25, 2024

milosgajdos commented Sep 25, 2024

josegomezr commented Sep 25, 2024

milosgajdos left a comment

Choose a reason for hiding this comment

Jamstah left a comment • edited Loading

Choose a reason for hiding this comment

josegomezr commented Sep 30, 2024

milosgajdos commented Sep 30, 2024

josegomezr commented Sep 30, 2024

milosgajdos commented Sep 30, 2024

josegomezr commented Sep 30, 2024

milosgajdos commented Sep 30, 2024

josegomezr commented Sep 30, 2024

milosgajdos commented Sep 30, 2024

josegomezr commented Sep 30, 2024

josegomezr commented Sep 30, 2024

milosgajdos commented Sep 30, 2024

Jamstah left a comment

Choose a reason for hiding this comment

milosgajdos commented Oct 2, 2024

josegomezr commented Oct 2, 2024

josegomezr commented Sep 24, 2024 •

edited

Loading

Jamstah left a comment •

edited

Loading