Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove Old JWT Tables #6302

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.

- Removed the existing metrics endpoint and API (`GET /api/metrics`, `get_metrics_v1`). Stats for request execution can instead be gathered by overriding the `EndpointRegistry::handle_event_request_completed()` method.
- Removed automatic msgpack support from JSON endpoint adapters, and related `include/ccf/serdes.h` file.
- Removed endpoint for `/gov/kv/jwt/public_signing_key` and `/gov/kv/jwt/public_signing_key_issuer`

## [5.0.0-dev18]

Expand Down
52 changes: 1 addition & 51 deletions doc/schemas/gov_openapi.json
Original file line number Diff line number Diff line change
Expand Up @@ -1774,56 +1774,6 @@
}
}
},
"/gov/kv/jwt/public_signing_key": {
"get": {
"deprecated": true,
"operationId": "GetGovKvJwtPublicSigningKey",
"responses": {
"200": {
"content": {
"application/json": {
"schema": {
"$ref": "#/components/schemas/string_to_base64string"
}
}
},
"description": "Default response description"
},
"default": {
"$ref": "#/components/responses/default"
}
},
"summary": "This route is auto-generated from the KV schema.",
"x-ccf-forwarding": {
"$ref": "#/components/x-ccf-forwarding/sometimes"
}
}
},
"/gov/kv/jwt/public_signing_key_issuer": {
"get": {
"deprecated": true,
"operationId": "GetGovKvJwtPublicSigningKeyIssuer",
"responses": {
"200": {
"content": {
"application/json": {
"schema": {
"$ref": "#/components/schemas/string_to_string"
}
}
},
"description": "Default response description"
},
"default": {
"$ref": "#/components/responses/default"
}
},
"summary": "This route is auto-generated from the KV schema.",
"x-ccf-forwarding": {
"$ref": "#/components/x-ccf-forwarding/sometimes"
}
}
},
"/gov/kv/jwt/public_signing_keys_metadata": {
"get": {
"deprecated": true,
Expand Down Expand Up @@ -2838,4 +2788,4 @@
}
},
"servers": []
}
}
13 changes: 0 additions & 13 deletions include/ccf/service/tables/jwt.h
Original file line number Diff line number Diff line change
Expand Up @@ -78,19 +78,6 @@ namespace ccf

static constexpr auto JWT_PUBLIC_SIGNING_KEYS_METADATA =
"public:ccf.gov.jwt.public_signing_keys_metadata";

namespace Legacy
{
static constexpr auto JWT_PUBLIC_SIGNING_KEYS =
"public:ccf.gov.jwt.public_signing_key";
static constexpr auto JWT_PUBLIC_SIGNING_KEY_ISSUER =
"public:ccf.gov.jwt.public_signing_key_issuer";

using JwtPublicSigningKeys =
ccf::kv::RawCopySerialisedMap<JwtKeyId, Cert>;
using JwtPublicSigningKeyIssuer =
ccf::kv::RawCopySerialisedMap<JwtKeyId, JwtIssuer>;
}
}

struct JsonWebKeySet
Expand Down
16 changes: 16 additions & 0 deletions samples/constitutions/default/actions.js
Original file line number Diff line number Diff line change
Expand Up @@ -1494,4 +1494,20 @@ const actions = new Map([
function (args) {},
),
],
[
"remove_old_jwt_tables",
new Action(
function (args) {
// Validate that no arguments are passed
checkNone(args);
},
function (args) {
// Clear the JWT public signing key table
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This and the following comment seem redundant to me

ccf.kv["public:ccf.gov.jwt.public_signing_key"].clear();

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add a trigger snapshot action

// Clear the JWT public signing key issuer table
ccf.kv["public:ccf.gov.jwt.public_signing_key_issuer"].clear();
},
),
],
]);
17 changes: 0 additions & 17 deletions src/endpoints/authentication/jwt_auth.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -136,23 +136,6 @@ namespace ccf
const auto key_id = token.header_typed.kid;
auto token_keys = keys->get(key_id);

if (!token_keys)
{
auto fallback_keys = tx.ro<Tables::Legacy::JwtPublicSigningKeys>(
ccf::Tables::Legacy::JWT_PUBLIC_SIGNING_KEYS);
auto fallback_issuers = tx.ro<Tables::Legacy::JwtPublicSigningKeyIssuer>(
ccf::Tables::Legacy::JWT_PUBLIC_SIGNING_KEY_ISSUER);

auto fallback_key = fallback_keys->get(key_id);
if (fallback_key)
{
token_keys = std::vector<OpenIDJWKMetadata>{OpenIDJWKMetadata{
.cert = *fallback_key,
.issuer = *fallback_issuers->get(key_id),
.constraint = std::nullopt}};
}
}

if (!token_keys)
{
error_reason =
Expand Down
24 changes: 0 additions & 24 deletions src/node/rpc/jwt_management.h
Original file line number Diff line number Diff line change
Expand Up @@ -23,25 +23,6 @@

namespace ccf
{
static void legacy_remove_jwt_public_signing_keys(
ccf::kv::Tx& tx, std::string issuer)
{
auto keys =
tx.rw<JwtPublicSigningKeys>(Tables::Legacy::JWT_PUBLIC_SIGNING_KEYS);
auto key_issuer = tx.rw<Tables::Legacy::JwtPublicSigningKeyIssuer>(
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_constraint(
const std::string& issuer, const std::string& constraint)
{
Expand Down Expand Up @@ -76,11 +57,6 @@ namespace ccf
static void remove_jwt_public_signing_keys(
ccf::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<JwtPublicSigningKeys>(Tables::JWT_PUBLIC_SIGNING_KEYS_METADATA);

Expand Down
11 changes: 1 addition & 10 deletions src/service/network_tables.h
Original file line number Diff line number Diff line change
Expand Up @@ -157,20 +157,11 @@ namespace ccf
const JwtIssuers jwt_issuers = {Tables::JWT_ISSUERS};
const JwtPublicSigningKeys jwt_public_signing_keys_metadata = {
Tables::JWT_PUBLIC_SIGNING_KEYS_METADATA};
const Tables::Legacy::JwtPublicSigningKeys legacy_jwt_public_signing_keys =
{Tables::Legacy::JWT_PUBLIC_SIGNING_KEYS};
const Tables::Legacy::JwtPublicSigningKeyIssuer
legacy_jwt_public_signing_key_issuer = {
Tables::Legacy::JWT_PUBLIC_SIGNING_KEY_ISSUER};

inline auto get_all_jwt_tables() const
{
return std::make_tuple(
ca_cert_bundles,
jwt_issuers,
jwt_public_signing_keys_metadata,
legacy_jwt_public_signing_keys,
legacy_jwt_public_signing_key_issuer);
ca_cert_bundles, jwt_issuers, jwt_public_signing_keys_metadata);
}

//
Expand Down
7 changes: 7 additions & 0 deletions tests/infra/consortium.py
Original file line number Diff line number Diff line change
Expand Up @@ -876,3 +876,10 @@ def check_for_service(self, remote_node, status, recovery_count=None):
assert (
recovery_count is None or current_recovery_count == recovery_count
), f"Current recovery count {current_recovery_count} is not expected {recovery_count}"

def remove_old_jwt_tables(self, remote_node):
proposal_body, careful_vote = self.make_proposal(
"remove_old_jwt_tables"
)
proposal = self.get_any_active_member().propose(remote_node, proposal_body)
return self.vote_using_majority(remote_node, proposal, careful_vote)
87 changes: 87 additions & 0 deletions tests/recovery.py
Original file line number Diff line number Diff line change
Expand Up @@ -528,6 +528,93 @@ def test_share_resilience(network, args, from_snapshot=False):
recovered_network.service_load.set_network(recovered_network)
return recovered_network

@reqs.description("Remove JWT Tables")
def test_recovered_ledger_remove_jwt_tables(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Love the test. Could we, however:

  • remove unnecessary parts except the required initialisation stuff
  • probably move the common part (with test_recover_service_from_files) to a separate function

As is, it's a copy-pasted test_recover_service_from_files with added JWT logic at the end with redundant (to the JWT logic) recovery checks, which doesn't make a lot of sense.

args, directory, expected_recovery_count, test_receipt=True
):
service_dir = os.path.join(
os.path.dirname(os.path.realpath(__file__)), "testdata", directory
)

old_common = os.path.join(service_dir, "common")
new_common = infra.network.get_common_folder_name(args.workspace, args.label)
copy_tree(old_common, new_common)

network = infra.network.Network(args.nodes, args.binary_dir)

args.previous_service_identity_file = os.path.join(old_common, "service_cert.pem")

network.start_in_recovery(
args,
ledger_dir=os.path.join(service_dir, "ledger"),
committed_ledger_dirs=[os.path.join(service_dir, "ledger")],
snapshots_dir=os.path.join(service_dir, "snapshots"),
common_dir=new_common,
)

network.recover(args, expected_recovery_count=expected_recovery_count)

primary, _ = network.find_primary()

# The member and user certs stored on this service are all currently expired.
# Remove user certs and add new users before attempting any user requests
primary, _ = network.find_primary()

user_certs = [
os.path.join(old_common, file)
for file in os.listdir(old_common)
if file.startswith("user") and file.endswith("_cert.pem")
]
user_ids = [
infra.crypto.compute_cert_der_hash_hex_from_pem(open(cert).read())
for cert in user_certs
]
for user_id in user_ids:
LOG.info(f"Removing expired user {user_id}")
network.consortium.remove_user(primary, user_id)

new_user_local_id = "recovery_user"
new_user = network.create_user(new_user_local_id, args.participants_curve)
LOG.info(f"Adding new user {new_user.service_id}")
network.consortium.add_user(primary, new_user.local_id)

infra.checker.check_can_progress(primary, local_user_id=new_user_local_id)

if test_receipt:
r = primary.get_receipt(2, 3)
verify_receipt(r.json(), network.cert)

# Get State
network.get_latest_ledger_public_state()
ledger_directories = primary.remote.ledger_paths()
ledger = ccf.ledger.Ledger(ledger_directories)

for chunk in ledger:
for tx in chunk:
txid = TxID(tx.gcm_header.view, tx.gcm_header.seqno)
tables = tx.get_public_domain().get_tables()
pub_keys = tables["public:ccf.gov.jwt.public_signing_key"]
assert (pub_keys is not None), "PubKeys is not None"
pub_key_issuer = tables["public:ccf.gov.jwt.public_signing_key_issuer"]
assert (pub_key_issuer is not None), "PubKeys is not None"

# Submit Proposal to remove old JWT Tables
network.consortium.remove_old_jwt_tables(primary)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

stop the network here to trigger a flush to disk

# Get New State after submitting proposal to remove older JWT tables
network.get_latest_ledger_public_state()
ledger_directories = primary.remote.ledger_paths()
ledger = ccf.ledger.Ledger(ledger_directories)

for chunk in ledger:
for tx in chunk:
txid = TxID(tx.gcm_header.view, tx.gcm_header.seqno)
tables = tx.get_public_domain().get_tables()
pub_keys = tables["public:ccf.gov.jwt.public_signing_key"]
assert (pub_keys is None), "PubKeys is None"
pub_key_issuer = tables["public:ccf.gov.jwt.public_signing_key_issuer"]
assert (pub_key_issuer is None), "PubKeys is None"


@reqs.description("Recover a service from malformed ledger")
@reqs.recover(number_txs=2)
Expand Down
1 change: 1 addition & 0 deletions tests/suite/test_suite.py
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,7 @@
# recovery:
recovery.test_recover_service,
recovery.test_recover_service_aborted,
recovery.test_recovered_ledger_remove_jwt_tables,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you tried this? It seems like it can't work, as the new test's parameters are not compatible with this test suite.

To make it work, I assume you should've added that test to the recovery.py, along with the test you copied the initialisation from.

# rekey:
e2e_logging.test_rekey,
# election:
Expand Down
Loading