[release/5.x] Cherry pick: Improve JWT auth error msg (#6435) (#6439)

Co-authored-by: Max <[email protected]>

CHANGELOG.md

@@ -9,6 +9,10 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
 
 [5.0.3]: https://github.com/microsoft/CCF/releases/tag/ccf-5.0.3
 
+### Changed
+
+- Improved JWT authentication error messages (#6427).
+
 ### Bug fix
 
 - In `GET gov/service/javascript-app`, `openApi` now correctly returns the schema set for the endpoint (#6430)

src/endpoints/authentication/jwt_auth.cpp

@@ -123,10 +123,8 @@ namespace ccf
 
     const auto token_opt =
       ::http::JwtVerifier::extract_token(headers, error_reason);
-
     if (!token_opt)
     {
-      error_reason = "Invalid JWT token";
       return nullptr;
     }
 
@@ -153,7 +151,7 @@ namespace ccf
       }
     }
 
-    if (!token_keys)
+    if (!token_keys || token_keys->empty())
     {
       error_reason =
         fmt::format("JWT signing key not found for kid {}", key_id);
@@ -165,6 +163,7 @@ namespace ccf
       auto verifier = verifiers->get_verifier(metadata.cert);
       if (!::http::JwtVerifier::validate_token_signature(token, verifier))
       {
+        error_reason = "Signature verification failed";
         continue;
       }
 
@@ -208,7 +207,6 @@ namespace ccf
       }
     }
 
-    error_reason = "Can't authenticate JWT token";
     return nullptr;
   }
 

tests/js-custom-authorization/custom_authorization.py

@@ -102,17 +102,19 @@ def set_issuer_with_a_key(primary, network, issuer, kid, constraint):
         network.consortium.set_jwt_issuer(primary, metadata_fp.name)
 
 
+def parse_error_message(r):
+    return r.body.json()["error"]["details"][0]["message"]
+
+
 def try_auth(primary, issuer, kid, iss, tid):
     with primary.client("user0") as c:
         LOG.info(f"Creating JWT with kid={kid} iss={iss} tenant={tid}")
-        r = c.get(
+        return c.get(
             "/app/jwt",
             headers=infra.jwt_issuer.make_bearer_header(
                 issuer.issue_jwt(kid, claims={"iss": iss, "tid": tid})
             ),
         )
-        assert r.status_code
-        return r.status_code
 
 
 @reqs.description("Test stack size limit")
@@ -135,7 +137,7 @@ def test_stack_size_limit(network, args):
                 depth *= 2
                 r = c.post("/app/recursive", body={"depth": depth})
                 if r.status_code == http.HTTPStatus.INTERNAL_SERVER_ERROR:
-                    message = r.body.json()["error"]["details"][0]["message"]
+                    message = parse_error_message(r)
                     assert message == "InternalError: stack overflow", message
                     LOG.info(
                         f"Stack overflow at depth={depth} with max_stack_bytes={max_stack_bytes}"
@@ -184,14 +186,14 @@ def test_heap_size_limit(network, args):
         with temporary_js_limits(network, primary, max_heap_bytes=3 * 1024 * 1024):
             r = c.post("/app/alloc", body={"size": safe_size})
             assert r.status_code == http.HTTPStatus.INTERNAL_SERVER_ERROR, r
-            message = r.body.json()["error"]["details"][0]["message"]
+            message = parse_error_message(r)
             assert message == "InternalError: out of memory", message
 
         r = c.post("/app/alloc", body={"size": safe_size})
         assert r.status_code == http.HTTPStatus.OK, r
 
         r = c.post("/app/alloc", body={"size": unsafe_size})
-        message = r.body.json()["error"]["details"][0]["message"]
+        message = parse_error_message(r)
         assert message == "InternalError: out of memory", message
 
         # Lower the cap until we likely run out of heap out of user code,
@@ -231,15 +233,15 @@ def test_execution_time_limit(network, args):
         with temporary_js_limits(network, primary, max_execution_time_ms=30):
             r = c.post("/app/sleep", body={"time": safe_time})
             assert r.status_code == http.HTTPStatus.INTERNAL_SERVER_ERROR, r
-            message = r.body.json()["error"]["details"][0]["message"]
+            message = parse_error_message(r)
             assert message == "InternalError: interrupted", message
 
         r = c.post("/app/sleep", body={"time": safe_time})
         assert r.status_code == http.HTTPStatus.OK, r
 
         r = c.post("/app/sleep", body={"time": unsafe_time})
         assert r.status_code == http.HTTPStatus.INTERNAL_SERVER_ERROR, r
-        message = r.body.json()["error"]["details"][0]["message"]
+        message = parse_error_message(r)
         assert message == "InternalError: interrupted", message
 
         # Lower the cap until we likely run out of heap out of user code,
@@ -310,7 +312,7 @@ def create_keypair(local_id, valid_from, validity_days):
     with primary.client(local_user_id) as c:
         r = c.get("/app/cert")
         assert r.status_code == HTTPStatus.UNAUTHORIZED, r
-        assert "Not After" in r.body.json()["error"]["details"][0]["message"], r
+        assert "Not After" in parse_error_message(r), r
 
     LOG.info("User with future cert cannot call user-authenticated endpoint")
     local_user_id = "in_the_future"
@@ -322,7 +324,7 @@ def create_keypair(local_id, valid_from, validity_days):
     with primary.client(local_user_id) as c:
         r = c.get("/app/cert")
         assert r.status_code == HTTPStatus.UNAUTHORIZED, r
-        assert "Not Before" in r.body.json()["error"]["details"][0]["message"], r
+        assert "Not Before" in parse_error_message(r), r
 
     LOG.info("No leeway added to cert time evaluation")
     local_user_id = "just_expired"
@@ -333,7 +335,7 @@ def create_keypair(local_id, valid_from, validity_days):
     with primary.client(local_user_id) as c:
         r = c.get("/app/cert")
         assert r.status_code == HTTPStatus.UNAUTHORIZED, r
-        assert "Not After" in r.body.json()["error"]["details"][0]["message"], r
+        assert "Not After" in parse_error_message(r), r
 
     return network
 
@@ -354,11 +356,13 @@ def test_jwt_auth(network, args):
     with primary.client("user0") as c:
         r = c.get("/app/jwt", headers=infra.jwt_issuer.make_bearer_header("garbage"))
         assert r.status_code == HTTPStatus.UNAUTHORIZED, r.status_code
+        assert "Malformed JWT" in parse_error_message(r), r
 
         jwt_mismatching_key_priv_pem, _ = infra.crypto.generate_rsa_keypair(2048)
         jwt = infra.crypto.create_jwt({}, jwt_mismatching_key_priv_pem, jwt_kid)
         r = c.get("/app/jwt", headers=infra.jwt_issuer.make_bearer_header(jwt))
         assert r.status_code == HTTPStatus.UNAUTHORIZED, r.status_code
+        assert "JWT payload is missing required field" in parse_error_message(r), r
 
         r = c.get(
             "/app/jwt",
@@ -374,6 +378,7 @@ def test_jwt_auth(network, args):
             ),
         )
         assert r.status_code == HTTPStatus.UNAUTHORIZED, r.status_code
+        assert "is before token's Not Before" in parse_error_message(r), r
 
         LOG.info("Calling JWT with too-early exp")
         r = c.get(
@@ -383,6 +388,7 @@ def test_jwt_auth(network, args):
             ),
         )
         assert r.status_code == HTTPStatus.UNAUTHORIZED, r.status_code
+        assert "is after token's Expiration Time" in parse_error_message(r), r
 
     network.consortium.remove_jwt_issuer(primary, issuer.name)
     return network
@@ -404,22 +410,22 @@ def test_jwt_auth_msft_single_tenant(network, args):
 
     set_issuer_with_a_key(primary, network, issuer, jwt_kid, ISSUER_TENANT)
 
-    assert (
-        try_auth(primary, issuer, jwt_kid, ISSUER_TENANT, "garbage_tenant")
-        == HTTPStatus.UNAUTHORIZED
-    )
-    assert (
-        try_auth(primary, issuer, jwt_kid, ISSUER_TENANT, "{tenantid}")
-        == HTTPStatus.UNAUTHORIZED
-    )
-    assert try_auth(primary, issuer, jwt_kid, ISSUER_TENANT, TENANT_ID) == HTTPStatus.OK
+    r = try_auth(primary, issuer, jwt_kid, ISSUER_TENANT, "garbage_tenant")
+    assert r.status_code == HTTPStatus.UNAUTHORIZED, r
+    assert "failed issuer constraint validation" in parse_error_message(r), r
+
+    r = try_auth(primary, issuer, jwt_kid, ISSUER_TENANT, "{tenantid}")
+    assert r.status_code == HTTPStatus.UNAUTHORIZED, r
+    assert "failed issuer constraint validation" in parse_error_message(r), r
+
+    r = try_auth(primary, issuer, jwt_kid, ISSUER_TENANT, TENANT_ID)
+    assert r.status_code == HTTPStatus.OK, r
 
     network.consortium.remove_jwt_issuer(primary, issuer.name)
 
-    assert (
-        try_auth(primary, issuer, jwt_kid, ISSUER_TENANT, TENANT_ID)
-        == HTTPStatus.UNAUTHORIZED
-    )
+    r = try_auth(primary, issuer, jwt_kid, ISSUER_TENANT, TENANT_ID)
+    assert r.status_code == HTTPStatus.UNAUTHORIZED, r
+    assert f"key not found for kid {jwt_kid}" in parse_error_message(r), r
 
     return network
 
@@ -469,36 +475,32 @@ def test_jwt_auth_msft_multitenancy(network, args):
         metadata_fp.flush()
         network.consortium.set_jwt_issuer(primary, metadata_fp.name)
 
-    assert (
-        try_auth(primary, issuer, jwt_kid_1, ISSUER_TENANT, TENANT_ID) == HTTPStatus.OK
-    )
-    assert (
-        try_auth(primary, issuer, jwt_kid_1, ISSUER_ANOTHER, ANOTHER_TENANT_ID)
-        == HTTPStatus.OK
-    )
-    assert (
-        try_auth(primary, issuer, jwt_kid_1, ISSUER_TENANT, ANOTHER_TENANT_ID)
-        == HTTPStatus.UNAUTHORIZED
-    )
+    r = try_auth(primary, issuer, jwt_kid_1, ISSUER_TENANT, TENANT_ID)
+    assert r.status_code == HTTPStatus.OK, r
 
-    assert (
-        try_auth(primary, issuer, jwt_kid_2, ISSUER_TENANT, TENANT_ID) == HTTPStatus.OK
-    )
-    assert (
-        try_auth(primary, issuer, jwt_kid_2, ISSUER_ANOTHER, ANOTHER_TENANT_ID)
-        == HTTPStatus.UNAUTHORIZED
-    )
+    r = try_auth(primary, issuer, jwt_kid_1, ISSUER_ANOTHER, ANOTHER_TENANT_ID)
+    assert r.status_code == HTTPStatus.OK, r
+
+    r = try_auth(primary, issuer, jwt_kid_1, ISSUER_TENANT, ANOTHER_TENANT_ID)
+    assert r.status_code == HTTPStatus.UNAUTHORIZED, r
+    assert "failed issuer constraint validation" in parse_error_message(r), r
+
+    r = try_auth(primary, issuer, jwt_kid_2, ISSUER_TENANT, TENANT_ID)
+    assert r.status_code == HTTPStatus.OK, r
+
+    r = try_auth(primary, issuer, jwt_kid_2, ISSUER_ANOTHER, ANOTHER_TENANT_ID)
+    assert r.status_code == HTTPStatus.UNAUTHORIZED, r
+    assert "failed issuer constraint validation" in parse_error_message(r), r
 
     network.consortium.remove_jwt_issuer(primary, issuer.name)
 
-    assert (
-        try_auth(primary, issuer, jwt_kid_1, ISSUER_TENANT, TENANT_ID)
-        == HTTPStatus.UNAUTHORIZED
-    )
-    assert (
-        try_auth(primary, issuer, jwt_kid_2, ISSUER_TENANT, TENANT_ID)
-        == HTTPStatus.UNAUTHORIZED
-    )
+    r = try_auth(primary, issuer, jwt_kid_1, ISSUER_TENANT, TENANT_ID)
+    assert r.status_code == HTTPStatus.UNAUTHORIZED, r
+    assert f"key not found for kid {jwt_kid_1}" in parse_error_message(r), r
+
+    r = try_auth(primary, issuer, jwt_kid_2, ISSUER_TENANT, TENANT_ID)
+    assert r.status_code == HTTPStatus.UNAUTHORIZED, r
+    assert f"key not found for kid {jwt_kid_2}" in parse_error_message(r), r
 
     return network
 
@@ -528,41 +530,39 @@ def test_jwt_auth_msft_same_kids_different_issuers(network, args):
 
     set_issuer_with_a_key(primary, network, issuer, jwt_kid, ISSUER_TENANT)
 
-    assert try_auth(primary, issuer, jwt_kid, ISSUER_TENANT, TENANT_ID) == HTTPStatus.OK
-    assert (
-        try_auth(primary, another, jwt_kid, ISSUER_ANOTHER, ANOTHER_TENANT_ID)
-        == HTTPStatus.UNAUTHORIZED
-    )
+    r = try_auth(primary, issuer, jwt_kid, ISSUER_TENANT, TENANT_ID)
+    assert r.status_code == HTTPStatus.OK, r
+
+    r = try_auth(primary, another, jwt_kid, ISSUER_ANOTHER, ANOTHER_TENANT_ID)
+    assert r.status_code == HTTPStatus.UNAUTHORIZED, r
+    assert "failed issuer constraint validation" in parse_error_message(r), r
 
     set_issuer_with_a_key(primary, network, another, jwt_kid, ISSUER_ANOTHER)
 
-    assert try_auth(primary, issuer, jwt_kid, ISSUER_TENANT, TENANT_ID) == HTTPStatus.OK
-    assert (
-        try_auth(primary, another, jwt_kid, ISSUER_ANOTHER, ANOTHER_TENANT_ID)
-        == HTTPStatus.OK
-    )
+    r = try_auth(primary, issuer, jwt_kid, ISSUER_TENANT, TENANT_ID)
+    assert r.status_code == HTTPStatus.OK, r
+
+    r = try_auth(primary, another, jwt_kid, ISSUER_ANOTHER, ANOTHER_TENANT_ID)
+    assert r.status_code == HTTPStatus.OK, r
 
     network.consortium.remove_jwt_issuer(primary, issuer.name)
 
-    assert (
-        try_auth(primary, issuer, jwt_kid, ISSUER_TENANT, TENANT_ID)
-        == HTTPStatus.UNAUTHORIZED
-    )
-    assert (
-        try_auth(primary, another, jwt_kid, ISSUER_ANOTHER, ANOTHER_TENANT_ID)
-        == HTTPStatus.OK
-    )
+    r = try_auth(primary, issuer, jwt_kid, ISSUER_TENANT, TENANT_ID)
+    assert r.status_code == HTTPStatus.UNAUTHORIZED, r
+    assert "failed issuer constraint validation" in parse_error_message(r), r
+
+    r = try_auth(primary, another, jwt_kid, ISSUER_ANOTHER, ANOTHER_TENANT_ID)
+    assert r.status_code == HTTPStatus.OK, r
 
     network.consortium.remove_jwt_issuer(primary, another.name)
 
-    assert (
-        try_auth(primary, issuer, jwt_kid, ISSUER_TENANT, TENANT_ID)
-        == HTTPStatus.UNAUTHORIZED
-    )
-    assert (
-        try_auth(primary, another, jwt_kid, ISSUER_ANOTHER, ANOTHER_TENANT_ID)
-        == HTTPStatus.UNAUTHORIZED
-    )
+    r = try_auth(primary, issuer, jwt_kid, ISSUER_TENANT, TENANT_ID)
+    assert r.status_code == HTTPStatus.UNAUTHORIZED, r
+    assert f"key not found for kid {jwt_kid}" in parse_error_message(r), r
+
+    r = try_auth(primary, another, jwt_kid, ISSUER_ANOTHER, ANOTHER_TENANT_ID)
+    assert r.status_code == HTTPStatus.UNAUTHORIZED, r
+    assert f"key not found for kid {jwt_kid}" in parse_error_message(r), r
 
     return network
 
@@ -587,30 +587,30 @@ def test_jwt_auth_msft_same_kids_overwrite_constraint(network, args):
 
     set_issuer_with_a_key(primary, network, issuer, jwt_kid, COMMNON_ISSUER)
 
-    assert try_auth(primary, issuer, jwt_kid, ISSUER_TENANT, TENANT_ID) == HTTPStatus.OK
-    assert (
-        try_auth(primary, issuer, jwt_kid, ISSUER_ANOTHER, ANOTHER_TENANT_ID)
-        == HTTPStatus.OK
-    )
+    r = try_auth(primary, issuer, jwt_kid, ISSUER_TENANT, TENANT_ID)
+    assert r.status_code == HTTPStatus.OK, r
+
+    r = try_auth(primary, issuer, jwt_kid, ISSUER_ANOTHER, ANOTHER_TENANT_ID)
+    assert r.status_code == HTTPStatus.OK, r
 
     set_issuer_with_a_key(primary, network, issuer, jwt_kid, ISSUER_TENANT)
 
-    assert try_auth(primary, issuer, jwt_kid, ISSUER_TENANT, TENANT_ID) == HTTPStatus.OK
-    assert (
-        try_auth(primary, issuer, jwt_kid, ISSUER_ANOTHER, ANOTHER_TENANT_ID)
-        == HTTPStatus.UNAUTHORIZED
-    )
+    r = try_auth(primary, issuer, jwt_kid, ISSUER_TENANT, TENANT_ID)
+    assert r.status_code == HTTPStatus.OK, r
+
+    r = try_auth(primary, issuer, jwt_kid, ISSUER_ANOTHER, ANOTHER_TENANT_ID)
+    assert r.status_code == HTTPStatus.UNAUTHORIZED, r
+    assert "failed issuer constraint validation" in parse_error_message(r), r
 
     network.consortium.remove_jwt_issuer(primary, issuer.name)
 
-    assert (
-        try_auth(primary, issuer, jwt_kid, ISSUER_TENANT, TENANT_ID)
-        == HTTPStatus.UNAUTHORIZED
-    )
-    assert (
-        try_auth(primary, issuer, jwt_kid, ISSUER_ANOTHER, ANOTHER_TENANT_ID)
-        == HTTPStatus.UNAUTHORIZED
-    )
+    r = try_auth(primary, issuer, jwt_kid, ISSUER_TENANT, TENANT_ID)
+    assert r.status_code == HTTPStatus.UNAUTHORIZED, r
+    assert f"key not found for kid {jwt_kid}" in parse_error_message(r), r
+
+    r = try_auth(primary, issuer, jwt_kid, ISSUER_ANOTHER, ANOTHER_TENANT_ID)
+    assert r.status_code == HTTPStatus.UNAUTHORIZED, r
+    assert f"key not found for kid {jwt_kid}" in parse_error_message(r), r
 
     return network