Skip to content

Commit

Permalink
Implement domain check on key fetch
Browse files Browse the repository at this point in the history
Do not allow issuers to provide keys with mismatching constraints.
Thus, issuers with a particular domain should specify key iss token
constraints for that domain.
  • Loading branch information
maxtropets committed May 29, 2024
1 parent ade5c80 commit 0097276
Show file tree
Hide file tree
Showing 3 changed files with 122 additions and 17 deletions.
46 changes: 44 additions & 2 deletions src/node/rpc/jwt_management.h
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
#include "ccf/ds/hex.h"
#include "ccf/service/tables/jwt.h"
#include "ccf/service/tables/proposals.h"
#include "http/http_jwt.h"

#ifdef SGX_ATTESTATION_VERIFICATION
# include <openenclave/attestation/verifier.h>
Expand All @@ -21,6 +22,34 @@

namespace ccf
{
static bool check_issuer(
const std::string& issuer, const std::string& constraint)
{
const auto issuer_domain = http::parse_url_full(issuer).host;
const auto constraint_domain = http::parse_url_full(constraint).host;

if (constraint_domain.empty())
{
return false;
}

// Either constraint's domain == issuer's domain or it is a subdomain, e.g.:
// limited.facebok.com
// .facebok.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 true;
}

static void remove_jwt_public_signing_keys(kv::Tx& tx, std::string issuer)
{
auto keys =
Expand Down Expand Up @@ -215,11 +244,24 @@ namespace ccf
LOG_INFO_FMT("{}: Storing JWT signing key with kid {}", log_prefix, kid);
new_keys.emplace(kid, der);

if (jwk.issuer.has_value())
if (jwk.issuer)
{
issuer_constraints.emplace(kid, jwk.issuer.value());
if (!check_issuer(issuer, *jwk.issuer))
{
LOG_FAIL_FMT(
"{}: JWKS kid {} with issuer constraint {} fails validation "
"against issuer {}",
log_prefix,
kid,
*jwk.issuer,
issuer);
return false;
}

issuer_constraints.emplace(kid, *jwk.issuer);
}
}

if (new_keys.empty())
{
LOG_FAIL_FMT("{}: no keys left after applying filter", log_prefix);
Expand Down
4 changes: 2 additions & 2 deletions tests/infra/jwt_issuer.py
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@ def __exit__(self, exc_type, exc_value, traceback):


class JwtIssuer:
TEST_JWT_ISSUER_NAME = "test_jwt_issuer"
TEST_JWT_ISSUER_NAME = "https://example.issuer"
TEST_CA_BUNDLE_NAME = "test_ca_bundle_name"

def _generate_cert(self, cn=None):
Expand Down Expand Up @@ -155,7 +155,7 @@ def _create_jwks(self, kid, test_invalid_is_key=False):
if not test_invalid_is_key
else infra.crypto.pub_key_pem_to_der(self.key_pub_pem)
).decode("ascii")
return {"kty": "RSA", "kid": kid, "x5c": [der_b64], "issuer": self.name}
return {"kty": "RSA", "kid": kid, "x5c": [der_b64], "issuer": self.name[::]}

def create_jwks(self, kid=None, test_invalid_is_key=False):
kid_ = kid or self.default_kid
Expand Down
89 changes: 76 additions & 13 deletions tests/jwt_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -69,8 +69,8 @@ def test_refresh_jwt_issuer(network, args):
def test_jwt_mulitple_issuers_same_kids_different_pem(network, args):
primary, _ = network.find_nodes()

issuer1 = infra.jwt_issuer.JwtIssuer("issuer1")
issuer2 = infra.jwt_issuer.JwtIssuer("issuer2")
issuer1 = infra.jwt_issuer.JwtIssuer("https://example.issuer1")
issuer2 = infra.jwt_issuer.JwtIssuer("https://example.issuer2")

set_issuer_with_keys(network, primary, issuer1, ["kid1"])

Expand All @@ -85,13 +85,13 @@ def test_jwt_mulitple_issuers_same_kids_different_pem(network, args):
network.consortium.remove_jwt_issuer(primary, issuer2.name)


@reqs.description("Multiple JWT issuers can't share same kid different pem")
@reqs.description("Multiple JWT issuers can share same kid same pem")
def test_jwt_mulitple_issuers_same_kids_same_pem(network, args):
primary, _ = network.find_nodes()

issuer1 = infra.jwt_issuer.JwtIssuer("issuer1")
issuer1 = infra.jwt_issuer.JwtIssuer("https://example.issuer1")

issuer2 = infra.jwt_issuer.JwtIssuer("issuer2")
issuer2 = infra.jwt_issuer.JwtIssuer("https://example.issuer2")
issuer2.cert_pem = issuer1.cert_pem

set_issuer_with_keys(network, primary, issuer1, ["kid1"])
Expand All @@ -105,7 +105,7 @@ def test_jwt_mulitple_issuers_same_kids_same_pem(network, args):
def test_jwt_same_issuer_constraint_overwritten(network, args):
primary, _ = network.find_nodes()

issuer = infra.jwt_issuer.JwtIssuer("issuer1")
issuer = infra.jwt_issuer.JwtIssuer("https://example.issuer")
keys = issuer.create_jwks_for_kids(["kid1"])

with tempfile.NamedTemporaryFile(prefix="ccf", mode="w+") as metadata_fp:
Expand All @@ -123,7 +123,7 @@ def test_jwt_same_issuer_constraint_overwritten(network, args):
service_keys = get_jwt_keys(args, primary)
assert service_keys["kid1"]["issuers"][issuer.name] == issuer.name

new_constraint = "whatever"
new_constraint = "https://example.issuer/very/specific"
keys["keys"][0]["issuer"] = new_constraint
with tempfile.NamedTemporaryFile(prefix="ccf", mode="w+") as jwks_fp:
json.dump(keys, jwks_fp)
Expand All @@ -135,14 +135,72 @@ def test_jwt_same_issuer_constraint_overwritten(network, args):
service_keys = get_jwt_keys(args, primary)
assert service_keys["kid1"]["issuers"][issuer.name] == new_constraint

network.consortium.remove_jwt_issuer(primary, issuer.name)


@reqs.description("Only able to set keys with issuer constraints matching the url")
def test_jwt_issuer_domain_match(network, args):
"""Check domains match. Additional subdomains permitted. For example, https://limited.facebook.com
may provide keys with issuer constraint https://facebook.com."""

primary, _ = network.find_nodes()

issuer = infra.jwt_issuer.JwtIssuer("https://trusted.issuer.com/something")
keys = issuer.create_jwks_for_kids(["kid1"])

with tempfile.NamedTemporaryFile(prefix="ccf", mode="w+") as metadata_fp:
json.dump({"issuer": issuer.name}, metadata_fp)
metadata_fp.flush()
network.consortium.set_jwt_issuer(primary, metadata_fp.name)

with tempfile.NamedTemporaryFile(prefix="ccf", mode="w+") as jwks_fp:
json.dump(keys, jwks_fp)
jwks_fp.flush()
network.consortium.set_jwt_public_signing_keys(
primary, issuer.name, jwks_fp.name
)

service_keys = get_jwt_keys(args, primary)
assert service_keys["kid1"]["issuers"][issuer.name] == issuer.name

keys["keys"][0]["issuer"] = "https://issuer.com"

with tempfile.NamedTemporaryFile(prefix="ccf", mode="w+") as jwks_fp:
json.dump(keys, jwks_fp)
jwks_fp.flush()
network.consortium.set_jwt_public_signing_keys(
primary, issuer.name, jwks_fp.name
)

garbage = ["", "garbage", "https://another.com", "https://issuer.com.domain"]
for constraint in garbage:
with tempfile.NamedTemporaryFile(prefix="ccf", mode="w+") as jwks_fp:
keys["keys"][0]["issuer"] = constraint
json.dump(keys, jwks_fp)
jwks_fp.flush()
try:
network.consortium.set_jwt_public_signing_keys(
primary, issuer.name, jwks_fp.name
)
except infra.proposal.ProposalNotAccepted:
pass
else:
assert False, f"Constraint {constraint} must not be allowed"


@reqs.description("Multiple JWT issuers registered at once")
def test_jwt_endpoint(network, args):
primary, _ = network.find_nodes()

keys = {
infra.jwt_issuer.JwtIssuer("issuer1"): ["issuer1_kid1", "issuer1_kid2"],
infra.jwt_issuer.JwtIssuer("issuer2"): ["issuer2_kid1", "issuer2_kid2"],
infra.jwt_issuer.JwtIssuer("https://example.issuer1"): [
"issuer1_kid1",
"issuer1_kid2",
],
infra.jwt_issuer.JwtIssuer("https://example.issuer2"): [
"issuer2_kid1",
"issuer2_kid2",
],
}

LOG.info("Register JWT issuer with multiple kids")
Expand All @@ -167,9 +225,11 @@ def test_jwt_endpoint(network, args):
def test_jwt_without_key_policy(network, args):
primary, _ = network.find_nodes()

issuer = infra.jwt_issuer.JwtIssuer("my_issuer")
issuer = infra.jwt_issuer.JwtIssuer("https://example.issuer")
kid = "my_kid_not_key_policy"

network.consortium.remove_jwt_issuer(primary, issuer.name)

LOG.info("Try to add JWT signing key without matching issuer")
with tempfile.NamedTemporaryFile(prefix="ccf", mode="w+") as jwks_fp:
json.dump(issuer.create_jwks(kid), jwks_fp)
Expand Down Expand Up @@ -274,7 +334,9 @@ def test_jwt_with_sgx_key_policy(network, args):
oe_cert_pem = f.read()

kid = "my_kid_with_policy"
issuer = infra.jwt_issuer.JwtIssuer("my_issuer_with_policy", oe_cert_pem)
issuer = infra.jwt_issuer.JwtIssuer(
"https://example.issuer_with_policy", oe_cert_pem
)

oesign = os.path.join(args.oe_binary, "oesign")
oeutil_enc = os.path.join(args.oe_binary, "oeutil_enc.signed")
Expand Down Expand Up @@ -374,8 +436,8 @@ def test_jwt_with_sgx_key_filter(network, args):
with open(oe_cert_path, encoding="utf-8") as f:
oe_cert_pem = f.read()

oe_issuer = infra.jwt_issuer.JwtIssuer("oe_issuer", oe_cert_pem)
non_oe_issuer = infra.jwt_issuer.JwtIssuer("non_oe_issuer_name")
oe_issuer = infra.jwt_issuer.JwtIssuer("https://example.oe_issuer", oe_cert_pem)
non_oe_issuer = infra.jwt_issuer.JwtIssuer("https://example.non_oe_issuer")

oe_kid = "oe_kid"
non_oe_kid = "non_oe_kid"
Expand Down Expand Up @@ -750,6 +812,7 @@ def run_auto(args):
test_jwt_mulitple_issuers_same_kids_different_pem(network, args)
test_jwt_mulitple_issuers_same_kids_same_pem(network, args)
test_jwt_same_issuer_constraint_overwritten(network, args)
test_jwt_issuer_domain_match(network, args)
test_jwt_endpoint(network, args)
test_jwt_without_key_policy(network, args)
if args.enclave_platform == "sgx":
Expand Down

0 comments on commit 0097276

Please sign in to comment.