From 457511806c4baefd4e7cf7f1d3623db85c485e45 Mon Sep 17 00:00:00 2001 From: Max Date: Tue, 13 Aug 2024 17:23:13 +0100 Subject: [PATCH] Improve JWT auth error msg (#6435) Co-authored-by: Amaury Chamayou Co-authored-by: Amaury Chamayou --- CHANGELOG.md | 4 + src/endpoints/authentication/jwt_auth.cpp | 6 +- .../custom_authorization.py | 188 +++++++++--------- 3 files changed, 100 insertions(+), 98 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2a154cc2f..70de64b11 100644 --- a/CHANGELOG.md +++ b/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) diff --git a/src/endpoints/authentication/jwt_auth.cpp b/src/endpoints/authentication/jwt_auth.cpp index 466adba15..05ceb862f 100644 --- a/src/endpoints/authentication/jwt_auth.cpp +++ b/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; } diff --git a/tests/js-custom-authorization/custom_authorization.py b/tests/js-custom-authorization/custom_authorization.py index edcb03c13..2dbd949e4 100644 --- a/tests/js-custom-authorization/custom_authorization.py +++ b/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,7 +233,7 @@ 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}) @@ -239,7 +241,7 @@ def test_execution_time_limit(network, args): 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 test_cert_auth(network, args): 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 test_cert_auth(network, args): 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 test_cert_auth(network, args): 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