Repository: distribution/distribution
Pull request #4471 (2024-09-24 14:17:48)

fix: Add the token's rootcert public key to the list of known keys
- Borrowed two functions from docker/libtrust to generate the key ID from the certificate bundle in the same fashion as v2.8.3.

Addresses: distribution/distribution#4470

josegomezr: > 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](https://datatracker.ietf.org/doc/html/rfc7638).

josegomezr: [b887712](https://github.com/distribution/distribution/pull/4471/commits/b8877128a806e36af3423aac7ef9213f4bd2f2b6) 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.
josegomezr: Took the liberty to implement the JWT hashing mechanism that [RFC7638](https://datatracker.ietf.org/doc/html/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 :sweat_smile: 
milosgajdos: > 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 https://github.com/distribution/distribution/pull/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: Maybe is worth noticing that our token server does not include the `x5c` header in the JWT Token
milosgajdos: > 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: > Hah! Yeah, that'll do it...

:sweat_smile: I knew I missed something
josegomezr: Should I write something up in this PR? Or a new one?
milosgajdos: I think it can go into this PR, but mind you I haven't had time to look at this PR yet
josegomezr: Ok, let's do two PR's to keep it lightly then. I'll wait for your feedback on this one :)
josegomezr: > 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 :thinking: 
milosgajdos: > 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: 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`

> 4. 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:

1. Ensure that the certificate chain provided is valid. If it fails (due to the `x5c` header missing):
  1. Ensure that the provided JWK in the JWT is valid.
  2. Ensure that the provided  KeyID is known (as per the configured `jwks.json` in `auth.token.jwks` config)
  3. Fail with Invalid Token

Maybe a mermaid flow diagram could illustrate better the logic?
milosgajdos: > 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: 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](https://datatracker.ietf.org/doc/html/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 :sweat_smile: 
milosgajdos: > 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: 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.
```
josegomezr: 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? :thinking: 
milosgajdos: I would put it into the configuration page and cross-reference it in the token auth 🤷‍♂️
milosgajdos: @josegomezr can you please squash and sign your commit?

Patch:
diff --git a/docs/content/about/configuration.md b/docs/content/about/configuration.md
index 88c4e858..0411fc79 100644
--- a/docs/content/about/configuration.md
+++ b/docs/content/about/configuration.md
@@ -666,6 +666,11 @@ Default `signingalgorithms`:
 - PS384
 - PS512
 
+Additional notes on `rootcertbundle`:
+
+- The public key of this certificate will be automatically added to the list of known keys.
+- The public key will be identified by it's [RFC7638 Thumbprint](https://datatracker.ietf.org/doc/html/rfc7638).
+
 For more information about Token based authentication configuration, see the
 [specification](../spec/auth/token.md).
 
diff --git a/docs/content/spec/auth/jwt.md b/docs/content/spec/auth/jwt.md
index 80a790f3..ef1a8598 100644
--- a/docs/content/spec/auth/jwt.md
+++ b/docs/content/spec/auth/jwt.md
@@ -12,7 +12,7 @@ Web Token schema that `distribution/distribution` has adopted to implement the
 client-opaque Bearer token issued by an authentication service and
 understood by the registry.
 
-This document borrows heavily from the [JSON Web Token Draft Spec](https://tools.ietf.org/html/draft-ietf-oauth-json-web-token-32)
+This document borrows heavily from the [JSON Web Token Spec: RFC7519](https://datatracker.ietf.org/doc/html/rfc7519)
 
 ## Getting a Bearer Token
 
@@ -63,14 +63,19 @@ Token has 3 main parts:
 
 1.  Headers
 
-    The header of a JSON Web Token is a standard JOSE header. The "typ" field
-    will be "JWT" and it will also contain the "alg" which identifies the
-    signing algorithm used to produce the signature. It also must have a "kid"
-    field, representing the ID of the key which was used to sign the token.
+    The header of a JSON Web Token is a standard JOSE header compliant with
+    [Section 5 of RFC7519](https://datatracker.ietf.org/doc/html/rfc7515#section-5).
 
-    It specifies that this object is going to be a JSON Web token signed using
-    the key with the given ID using the Elliptic Curve signature algorithm
-    using a SHA256 hash.
+    It **must** have:
+
+    * `alg` **(Algorithm)**: Identifies the signing algorithm used to produce the signature.
+    * `typ` **(Type)**: Must be equal to `JWT` as recommended by [Section 5.1 RFC7519](https://datatracker.ietf.org/doc/html/rfc7519#section-5.1)
+
+    It should have at least one of:
+
+    * `kid` **(KeyID)**: Represents the ID of the key which was used to sign the token.
+    * `jwk` **(JWK)**: Represents the public key used to sign the token, compliant with [RFC7517](https://datatracker.ietf.org/doc/html/rfc7517)
+    * `x5c` **(X.509 Certificate Chain)**: Represents the chain of certificates used to sign the token.
 
 2.  Claim Set
 
@@ -226,7 +231,7 @@ Token has 3 main parts:
 
     This is then used as the payload to a the `ES256` signature algorithm
     specified in the JOSE header and specified fully in [Section 3.4 of the JSON Web Algorithms (JWA)
-    draft specification](https://tools.ietf.org/html/draft-ietf-jose-json-web-algorithms-38#section-3.4)
+    specification](https://datatracker.ietf.org/doc/html/rfc7518)
 
     This example signature will use the following ECDSA key for the server:
 
@@ -281,6 +286,12 @@ This is also described in [Section 2.1 of RFC 6750: The OAuth 2.0 Authorization
 The registry must now verify the token presented by the user by inspecting the
 claim set within. The registry will:
 
+- Ensure that the certificate chain provided (in the `x5c` header) is valid.
+  - If it fails (eg. `x5c` header not present), then the registry will either:
+    - If provided, verify the provided JWK (in the `jwt` header) in the JWT is
+      known or trusted.
+    - If provided, verify that the provided KeyID (in the `kid` header) is a
+      known (as per configured in `auth.token.jwks` config).
 - Ensure that the issuer (`iss` claim) is an authority it trusts.
 - Ensure that the registry identifies as the audience (`aud` claim).
 - Check that the current time is between the `nbf` and `exp` claim times.
diff --git a/docs/content/spec/auth/token.md b/docs/content/spec/auth/token.md
index a878b328..f3b11734 100644
--- a/docs/content/spec/auth/token.md
+++ b/docs/content/spec/auth/token.md
@@ -16,7 +16,8 @@ This document outlines the v2 Distribution registry authentication scheme:
 3. The registry client makes a request to the authorization service for a
    Bearer token.
 4. The authorization service returns an opaque Bearer token representing the
-   client's authorized access.
+   client's authorized access. The token must comply with the structure
+   described in the [Token Authentication Implementation page](./jwt.md).
 5. The client retries the original request with the Bearer token embedded in
    the request's Authorization header.
 6. The Registry authorizes the client by validating the Bearer token and the
diff --git a/registry/auth/token/accesscontroller.go b/registry/auth/token/accesscontroller.go
index 3d6b9c2c..e8698953 100644
--- a/registry/auth/token/accesscontroller.go
+++ b/registry/auth/token/accesscontroller.go
@@ -238,6 +238,11 @@ func checkOptions(options map[string]interface{}) (tokenAccessOptions, error) {
 	return opts, nil
 }
 
+var (
+	rootCertFetcher func(string) ([]*x509.Certificate, error) = getRootCerts
+	jwkFetcher      func(string) (*jose.JSONWebKeySet, error) = getJwks
+)
+
 func getRootCerts(path string) ([]*x509.Certificate, error) {
 	fp, err := os.Open(path)
 	if err != nil {
@@ -316,14 +321,14 @@ func newAccessController(options map[string]interface{}) (auth.AccessController,
 	)
 
 	if config.rootCertBundle != "" {
-		rootCerts, err = getRootCerts(config.rootCertBundle)
+		rootCerts, err = rootCertFetcher(config.rootCertBundle)
 		if err != nil {
 			return nil, err
 		}
 	}
 
 	if config.jwks != "" {
-		jwks, err = getJwks(config.jwks)
+		jwks, err = jwkFetcher(config.jwks)
 		if err != nil {
 			return nil, err
 		}
@@ -334,12 +339,15 @@ func newAccessController(options map[string]interface{}) (auth.AccessController,
 		return nil, errors.New("token auth requires at least one token signing key")
 	}
 
+	trustedKeys := make(map[string]crypto.PublicKey)
 	rootPool := x509.NewCertPool()
 	for _, rootCert := range rootCerts {
 		rootPool.AddCert(rootCert)
+		if key := GetRFC7638Thumbprint(rootCert.PublicKey); key != "" {
+			trustedKeys[key] = rootCert.PublicKey
+		}
 	}
 
-	trustedKeys := make(map[string]crypto.PublicKey)
 	if jwks != nil {
 		for _, key := range jwks.Keys {
 			trustedKeys[key.KeyID] = key.Public()
diff --git a/registry/auth/token/util.go b/registry/auth/token/util.go
index 52e9bae2..29b8811f 100644
--- a/registry/auth/token/util.go
+++ b/registry/auth/token/util.go
@@ -1,5 +1,15 @@
 package token
 
+import (
+	"crypto"
+	"crypto/ecdsa"
+	"crypto/rsa"
+	"crypto/sha256"
+	"encoding/base64"
+	"fmt"
+	"math/big"
+)
+
 // actionSet is a special type of stringSet.
 type actionSet struct {
 	stringSet
@@ -36,3 +46,41 @@ func containsAny(ss []string, q []string) bool {
 
 	return false
 }
+
+// NOTE: RFC7638 does not prescribe which hashing function to use, but suggests
+// sha256 as a sane default as of time of writing
+func hashAndEncode(payload string) string {
+	shasum := sha256.Sum256([]byte(payload))
+	return base64.RawURLEncoding.EncodeToString(shasum[:])
+}
+
+// RFC7638 states in section 3 sub 1 that the keys in the JSON object payload
+// are required to be ordered lexicographical order. Golang does not guarantee
+// order of keys[0]
+// [0]: https://groups.google.com/g/golang-dev/c/zBQwhm3VfvU
+//
+// The payloads are small enough to create the JSON strings manually
+func GetRFC7638Thumbprint(publickey crypto.PublicKey) string {
+	var payload string
+
+	switch pubkey := publickey.(type) {
+	case *rsa.PublicKey:
+		e_big := big.NewInt(int64(pubkey.E)).Bytes()
+
+		e := base64.RawURLEncoding.EncodeToString(e_big)
+		n := base64.RawURLEncoding.EncodeToString(pubkey.N.Bytes())
+
+		payload = fmt.Sprintf(`{"e":"%s","kty":"RSA","n":"%s"}`, e, n)
+	case *ecdsa.PublicKey:
+		params := pubkey.Params()
+		crv := params.Name
+		x := base64.RawURLEncoding.EncodeToString(params.Gx.Bytes())
+		y := base64.RawURLEncoding.EncodeToString(params.Gy.Bytes())
+
+		payload = fmt.Sprintf(`{"crv":"%s","kty":"EC","x":"%s","y":"%s"}`, crv, x, y)
+	default:
+		return ""
+	}
+
+	return hashAndEncode(payload)
+}


Test patch:
diff --git a/registry/auth/token/accesscontroller_test.go b/registry/auth/token/accesscontroller_test.go
index fde20311..ba3d0a37 100644
--- a/registry/auth/token/accesscontroller_test.go
+++ b/registry/auth/token/accesscontroller_test.go
@@ -1,9 +1,16 @@
 package token
 
 import (
+	"testing"
+
+	"crypto/rand"
+	"crypto/rsa"
+	"crypto/x509"
+
 	"net/http"
 	"net/http/httptest"
-	"testing"
+
+	"github.com/go-jose/go-jose/v4"
 )
 
 func TestBuildAutoRedirectURL(t *testing.T) {
@@ -87,3 +94,86 @@ func TestCheckOptions(t *testing.T) {
 		t.Fatal("autoredirectpath should be /auth/token")
 	}
 }
+
+func mockGetRootCerts(path string) ([]*x509.Certificate, error) {
+	caPrivKey, err := rsa.GenerateKey(rand.Reader, 1024) // not to slow down the test that much
+	if err != nil {
+		return nil, err
+	}
+
+	ca := &x509.Certificate{
+		PublicKey: &caPrivKey.PublicKey,
+	}
+
+	return []*x509.Certificate{ca}, nil
+}
+
+func mockGetJwks(path string) (*jose.JSONWebKeySet, error) {
+	return &jose.JSONWebKeySet{
+		Keys: []jose.JSONWebKey{
+			{
+				KeyID: "sample-key-id",
+			},
+		},
+	}, nil
+}
+
+func TestRootCertIncludedInTrustedKeys(t *testing.T) {
+	old := rootCertFetcher
+	rootCertFetcher = mockGetRootCerts
+	defer func() { rootCertFetcher = old }()
+
+	realm := "https://auth.example.com/token/"
+	issuer := "test-issuer.example.com"
+	service := "test-service.example.com"
+
+	options := map[string]interface{}{
+		"realm":            realm,
+		"issuer":           issuer,
+		"service":          service,
+		"rootcertbundle":   "something-to-trigger-our-mock",
+		"autoredirect":     true,
+		"autoredirectpath": "/auth",
+	}
+
+	ac, err := newAccessController(options)
+	if err != nil {
+		t.Fatal(err)
+	}
+	// newAccessController return type is an interface built from
+	// accessController struct. The type check can be safely ignored.
+	ac2, _ := ac.(*accessController)
+	if got := len(ac2.trustedKeys); got != 1 {
+		t.Fatalf("Unexpected number of trusted keys, expected 1 got: %d", got)
+	}
+}
+
+func TestJWKSIncludedInTrustedKeys(t *testing.T) {
+	old := jwkFetcher
+	jwkFetcher = mockGetJwks
+	defer func() { jwkFetcher = old }()
+
+	realm := "https://auth.example.com/token/"
+	issuer := "test-issuer.example.com"
+	service := "test-service.example.com"
+
+	options := map[string]interface{}{
+		"realm":            realm,
+		"issuer":           issuer,
+		"service":          service,
+		"jwks":             "something-to-trigger-our-mock",
+		"autoredirect":     true,
+		"autoredirectpath": "/auth",
+	}
+
+	ac, err := newAccessController(options)
+	if err != nil {
+		t.Fatal(err)
+	}
+	// newAccessController return type is an interface built from
+	// accessController struct. The type check can be safely ignored.
+	ac2, _ := ac.(*accessController)
+	if got := len(ac2.trustedKeys); got != 1 {
+		t.Fatalf("Unexpected number of trusted keys, expected 1 got: %d", got)
+	}
+}