From f45132794c83b07b6c84db7f989b6404b3d4c0ae Mon Sep 17 00:00:00 2001 From: Max Tropets Date: Sun, 2 Jun 2024 21:52:07 +0000 Subject: [PATCH] CR changes --- include/ccf/service/tables/jwt.h | 3 + src/endpoints/authentication/jwt_auth.cpp | 15 +- src/node/rpc/jwt_management.h | 37 +++- .../custom_authorization.py | 192 +++++------------- tests/jwt_test.py | 2 + 5 files changed, 100 insertions(+), 149 deletions(-) diff --git a/include/ccf/service/tables/jwt.h b/include/ccf/service/tables/jwt.h index 0d5e5ab7985e..d6437793245b 100644 --- a/include/ccf/service/tables/jwt.h +++ b/include/ccf/service/tables/jwt.h @@ -82,6 +82,9 @@ namespace ccf "public:ccf.gov.jwt.public_signing_key"; static constexpr auto JWT_PUBLIC_SIGNING_KEY_ISSUER = "public:ccf.gov.jwt.public_signing_key_issuer"; + + using JwtPublicSigningKeyIssuer = + kv::RawCopySerialisedMap; } static constexpr auto JWT_PUBLIC_SIGNING_KEY_CERTS = diff --git a/src/endpoints/authentication/jwt_auth.cpp b/src/endpoints/authentication/jwt_auth.cpp index 241b10428af8..32d0c81a212a 100644 --- a/src/endpoints/authentication/jwt_auth.cpp +++ b/src/endpoints/authentication/jwt_auth.cpp @@ -32,7 +32,7 @@ namespace bool validate_issuer( const http::JwtVerifier::Token& token, std::string issuer) { - LOG_INFO_FMT( + LOG_DEBUG_FMT( "Verify token.iss {} and token.tid {} against published key issuer {}", token.payload_typed.iss, token.payload_typed.tid, @@ -196,6 +196,19 @@ namespace ccf else { auto identity = std::make_unique(); + + if (validated_issuer.empty()) + { + auto fallback_issuers = + tx.ro( + ccf::Tables::Legacy::JWT_PUBLIC_SIGNING_KEY_ISSUER); + const auto& issuer = fallback_issuers->get(key_id); + if (issuer) + { + validated_issuer = *issuer; + } + } + identity->key_issuer = validated_issuer; identity->header = std::move(token.header); identity->payload = std::move(token.payload); diff --git a/src/node/rpc/jwt_management.h b/src/node/rpc/jwt_management.h index 3604c9e227ad..8fb04efd4a6c 100644 --- a/src/node/rpc/jwt_management.h +++ b/src/node/rpc/jwt_management.h @@ -22,6 +22,25 @@ namespace ccf { + static void legacy_remove_jwt_public_signing_keys( + kv::Tx& tx, std::string issuer) + { + auto keys = + tx.rw(Tables::Legacy::JWT_PUBLIC_SIGNING_KEYS); + auto key_issuer = tx.rw( + Tables::Legacy::JWT_PUBLIC_SIGNING_KEY_ISSUER); + + key_issuer->foreach( + [&issuer, &keys, &key_issuer](const auto& k, const auto& v) { + if (v == issuer) + { + keys->remove(k); + key_issuer->remove(k); + } + return true; + }); + } + static bool check_issuer( const std::string& issuer, const std::string& constraint) { @@ -34,17 +53,12 @@ namespace ccf } // Either constraint's domain == issuer's domain or it is a subdomain, e.g.: - // limited.facebok.com - // .facebok.com + // limited.facebook.com + // .facebook.com if (issuer_domain != constraint_domain) { - const auto start_seek = - issuer_domain.size() - (constraint_domain.size() + 1); - const auto count_seek = constraint_domain.size() + 1; const auto pattern = "." + constraint_domain; - - return start_seek > 0 // at least one letter preceeds .issuer.domain - && issuer_domain.substr(start_seek, count_seek) == pattern; + return issuer_domain.ends_with(pattern); } return true; @@ -52,6 +66,11 @@ namespace ccf static void remove_jwt_public_signing_keys(kv::Tx& tx, std::string issuer) { + // Unlike resetting JWT keys for a particular issuer, removing keys can be + // safely done on both table revisions, as soon as the application shouldn't + // use them anyway after being ask about that explicitly. + legacy_remove_jwt_public_signing_keys(tx, issuer); + auto keys = tx.rw(Tables::JWT_PUBLIC_SIGNING_KEY_CERTS); auto key_issuers = @@ -313,7 +332,7 @@ namespace ccf value.constraint = it->second; } - LOG_INFO_FMT( + LOG_DEBUG_FMT( "Save JWT issuer for kid {} where issuer: {}, issuer constraint: {}", kid, value.issuer, diff --git a/tests/js-custom-authorization/custom_authorization.py b/tests/js-custom-authorization/custom_authorization.py index 096b63a9f35c..0448237ba21c 100644 --- a/tests/js-custom-authorization/custom_authorization.py +++ b/tests/js-custom-authorization/custom_authorization.py @@ -79,6 +79,42 @@ def temporary_js_limits(network, primary, **kwargs): network.consortium.set_js_runtime_options(primary, **default_kwargs) +def set_issuer_with_a_key(primary, network, issuer, kid, constraint): + with tempfile.NamedTemporaryFile(prefix="ccf", mode="w+") as metadata_fp: + jwt_cert_der = infra.crypto.cert_pem_to_der(issuer.cert_pem) + der_b64 = base64.b64encode(jwt_cert_der).decode("ascii") + data = { + "issuer": issuer.issuer_url, + "auto_refresh": False, + "jwks": { + "keys": [ + { + "kty": "RSA", + "kid": kid, + "x5c": [der_b64], + "issuer": constraint, + } + ] + }, + } + json.dump(data, metadata_fp) + metadata_fp.flush() + network.consortium.set_jwt_issuer(primary, metadata_fp.name) + + +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( + "/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") def test_stack_size_limit(network, args): primary, _ = network.find_nodes() @@ -311,25 +347,8 @@ def test_jwt_auth(network, args): jwt_kid = "my_key_id" LOG.info("Add JWT issuer with initial keys") - with tempfile.NamedTemporaryFile(prefix="ccf", mode="w+") as metadata_fp: - jwt_cert_der = infra.crypto.cert_pem_to_der(issuer.cert_pem) - der_b64 = base64.b64encode(jwt_cert_der).decode("ascii") - data = { - "issuer": issuer.name, - "jwks": { - "keys": [ - { - "kty": "RSA", - "kid": jwt_kid, - "x5c": [der_b64], - "issuer": issuer.name, - } - ] - }, - } - json.dump(data, metadata_fp) - metadata_fp.flush() - network.consortium.set_jwt_issuer(primary, metadata_fp.name) + + set_issuer_with_a_key(primary, network, issuer, jwt_kid, issuer.name) LOG.info("Calling jwt endpoint after storing keys") with primary.client("user0") as c: @@ -369,19 +388,6 @@ def test_jwt_auth(network, args): return network -def try_auth(primary, issuer, kid, iss, tid): - with primary.client("user0") as c: - LOG.info("Calling JWT with kid from issuer for tenant") - r = 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("JWT authentication as by MSFT Entra (single tenant)") def test_jwt_auth_msft_single_tenant(network, args): """For a specific tenant, only tokens with this issuer+tenant can auth.""" @@ -396,26 +402,7 @@ def test_jwt_auth_msft_single_tenant(network, args): issuer = infra.jwt_issuer.JwtIssuer(name="https://login.microsoftonline.com") jwt_kid = "my_key_id" - with tempfile.NamedTemporaryFile(prefix="ccf", mode="w+") as metadata_fp: - jwt_cert_der = infra.crypto.cert_pem_to_der(issuer.cert_pem) - der_b64 = base64.b64encode(jwt_cert_der).decode("ascii") - data = { - "issuer": issuer.name, - "auto_refresh": False, - "jwks": { - "keys": [ - { - "kty": "RSA", - "kid": jwt_kid, - "x5c": [der_b64], - "issuer": ISSUER_TENANT, - } - ] - }, - } - json.dump(data, metadata_fp) - metadata_fp.flush() - network.consortium.set_jwt_issuer(primary, metadata_fp.name) + set_issuer_with_a_key(primary, network, issuer, jwt_kid, ISSUER_TENANT) assert ( try_auth(primary, issuer, jwt_kid, ISSUER_TENANT, "garbage_tenant") @@ -447,7 +434,7 @@ def test_jwt_auth_msft_multitenancy(network, args): COMMNON_ISSUER = "https://login.microsoftonline.com/{tenantid}/v2.0" TENANT_ID = "9188050d-6c67-4c5b-b112-36a304b66da" ISSUER_TENANT = f"https://login.microsoftonline.com/{TENANT_ID}/v2.0" - ANOTHER_TENANT_ID = "ANOTHER-6c67-4c5b-b112-36a304b66da" + ANOTHER_TENANT_ID = "deadbeef-6c67-4c5b-b112-36a304b66da" ISSUER_ANOTHER = f"https://login.microsoftonline.com/{ANOTHER_TENANT_ID}/v2.0" issuer = infra.jwt_issuer.JwtIssuer(name="https://login.microsoftonline.com") @@ -528,65 +515,30 @@ def test_jwt_auth_msft_same_kids_different_issuers(network, args): TENANT_ID = "9188050d-6c67-4c5b-b112-36a304b66da" ISSUER_TENANT = f"https://login.microsoftonline.com/{TENANT_ID}/v2.0" - ANOTHER_TENANT_ID = "ANOTHER-6c67-4c5b-b112-36a304b66da" + ANOTHER_TENANT_ID = "deadbeef-6c67-4c5b-b112-36a304b66da" ISSUER_ANOTHER = f"https://login.microsoftonline.com/{ANOTHER_TENANT_ID}/v2.0" issuer = infra.jwt_issuer.JwtIssuer(name=ISSUER_TENANT) another = infra.jwt_issuer.JwtIssuer(name=ISSUER_ANOTHER) + # Immitate same key sharing + another.cert_pem, another.key_priv_pem = issuer.cert_pem, issuer.key_priv_pem + jwt_kid = "my_key_id" - with tempfile.NamedTemporaryFile(prefix="ccf", mode="w+") as metadata_fp: - jwt_cert_der = infra.crypto.cert_pem_to_der(issuer.cert_pem) - der_b64 = base64.b64encode(jwt_cert_der).decode("ascii") - data = { - "issuer": issuer.issuer_url, - "auto_refresh": False, - "jwks": { - "keys": [ - { - "kty": "RSA", - "kid": jwt_kid, - "x5c": [der_b64], - "issuer": ISSUER_TENANT, - } - ] - }, - } - json.dump(data, metadata_fp) - metadata_fp.flush() - network.consortium.set_jwt_issuer(primary, metadata_fp.name) + 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) + try_auth(primary, another, jwt_kid, ISSUER_ANOTHER, ANOTHER_TENANT_ID) == HTTPStatus.UNAUTHORIZED ) - with tempfile.NamedTemporaryFile(prefix="ccf", mode="w+") as metadata_fp: - jwt_cert_der = infra.crypto.cert_pem_to_der(issuer.cert_pem) - der_b64 = base64.b64encode(jwt_cert_der).decode("ascii") - data = { - "issuer": another.issuer_url, - "auto_refresh": False, - "jwks": { - "keys": [ - { - "kty": "RSA", - "kid": jwt_kid, - "x5c": [der_b64], - "issuer": ISSUER_ANOTHER, - } - ] - }, - } - json.dump(data, metadata_fp) - metadata_fp.flush() - network.consortium.set_jwt_issuer(primary, metadata_fp.name) + 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, issuer, jwt_kid, ISSUER_ANOTHER, ANOTHER_TENANT_ID) + try_auth(primary, another, jwt_kid, ISSUER_ANOTHER, ANOTHER_TENANT_ID) == HTTPStatus.OK ) @@ -597,7 +549,7 @@ def test_jwt_auth_msft_same_kids_different_issuers(network, args): == HTTPStatus.UNAUTHORIZED ) assert ( - try_auth(primary, issuer, jwt_kid, ISSUER_ANOTHER, ANOTHER_TENANT_ID) + try_auth(primary, another, jwt_kid, ISSUER_ANOTHER, ANOTHER_TENANT_ID) == HTTPStatus.OK ) @@ -608,7 +560,7 @@ def test_jwt_auth_msft_same_kids_different_issuers(network, args): == HTTPStatus.UNAUTHORIZED ) assert ( - try_auth(primary, issuer, jwt_kid, ISSUER_ANOTHER, ANOTHER_TENANT_ID) + try_auth(primary, another, jwt_kid, ISSUER_ANOTHER, ANOTHER_TENANT_ID) == HTTPStatus.UNAUTHORIZED ) @@ -627,32 +579,13 @@ def test_jwt_auth_msft_same_kids_overwrite_constraint(network, args): COMMNON_ISSUER = "https://login.microsoftonline.com/{tenantid}/v2.0" TENANT_ID = "9188050d-6c67-4c5b-b112-36a304b66da" ISSUER_TENANT = f"https://login.microsoftonline.com/{TENANT_ID}/v2.0" - ANOTHER_TENANT_ID = "ANOTHER-6c67-4c5b-b112-36a304b66da" + ANOTHER_TENANT_ID = "deadbeef-6c67-4c5b-b112-36a304b66da" ISSUER_ANOTHER = f"https://login.microsoftonline.com/{ANOTHER_TENANT_ID}/v2.0" issuer = infra.jwt_issuer.JwtIssuer(name=ISSUER_TENANT) jwt_kid = "my_key_id" - with tempfile.NamedTemporaryFile(prefix="ccf", mode="w+") as metadata_fp: - jwt_cert_der = infra.crypto.cert_pem_to_der(issuer.cert_pem) - der_b64 = base64.b64encode(jwt_cert_der).decode("ascii") - data = { - "issuer": issuer.issuer_url, - "auto_refresh": False, - "jwks": { - "keys": [ - { - "kty": "RSA", - "kid": jwt_kid, - "x5c": [der_b64], - "issuer": COMMNON_ISSUER, - } - ] - }, - } - json.dump(data, metadata_fp) - metadata_fp.flush() - network.consortium.set_jwt_issuer(primary, metadata_fp.name) + 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 ( @@ -660,26 +593,7 @@ def test_jwt_auth_msft_same_kids_overwrite_constraint(network, args): == HTTPStatus.OK ) - with tempfile.NamedTemporaryFile(prefix="ccf", mode="w+") as metadata_fp: - jwt_cert_der = infra.crypto.cert_pem_to_der(issuer.cert_pem) - der_b64 = base64.b64encode(jwt_cert_der).decode("ascii") - data = { - "issuer": issuer.issuer_url, - "auto_refresh": False, - "jwks": { - "keys": [ - { - "kty": "RSA", - "kid": jwt_kid, - "x5c": [der_b64], - "issuer": ISSUER_TENANT, - } - ] - }, - } - json.dump(data, metadata_fp) - metadata_fp.flush() - network.consortium.set_jwt_issuer(primary, metadata_fp.name) + 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 ( diff --git a/tests/jwt_test.py b/tests/jwt_test.py index d73e3ec5e086..ddcb2066074d 100644 --- a/tests/jwt_test.py +++ b/tests/jwt_test.py @@ -187,6 +187,8 @@ def test_jwt_issuer_domain_match(network, args): else: assert False, f"Constraint {constraint} must not be allowed" + network.consortium.remove_jwt_issuer(primary, issuer.name) + @reqs.description("Multiple JWT issuers registered at once") def test_jwt_endpoint(network, args):