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.

Sign up for GitHub

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

Jump to bottom

Remove Old JWT Tables #6302

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open

Remove Old JWT Tables #6302

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
af8353f
Remove Old JWT Tables
sidmore Jun 27, 2024
c938fa5
fix format error
sidmore Jun 27, 2024
f3492bd
fix prettier error
sidmore Jun 27, 2024
4d90934
Update samples/constitutions/default/actions.js
sidmore Jul 1, 2024
42622ac
Merge branch 'main' into remove_old_jwt_tables
sidmore Jul 1, 2024
f99b756
Add Test for verification of JWT tables
sidmore Jul 2, 2024
6314bbc
update doc version
sidmore Jul 2, 2024
d554c7f
Merge branch 'main' into remove_old_jwt_tables
sidmore Jul 2, 2024
b13fd66
Update ChangeLog
sidmore Jul 5, 2024
a4c1ff9
Merge branch 'main' into remove_old_jwt_tables
sidmore Jul 8, 2024
f9cd14d
Update gov_openapi.json
sidmore Jul 10, 2024
c52fb57
Merge branch 'main' into remove_old_jwt_tables
sidmore Jul 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 0 additions & 12 deletions include/ccf/service/tables/jwt.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,18 +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 = kv::RawCopySerialisedMap<JwtKeyId, Cert>;
using JwtPublicSigningKeyIssuer =
kv::RawCopySerialisedMap<JwtKeyId, JwtIssuer>;
}
}

struct JsonWebKeySet
Expand Down
24 changes: 24 additions & 0 deletions samples/constitutions/default/actions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1494,4 +1494,28 @@ const actions = new Map([
function (args) {},
),
],
[
"remove_old_jwt_tables",
new Action(
function (args) {
// Validate that no arguments are passed
if (
args !== undefined &&
sidmore marked this conversation as resolved.
Show resolved Hide resolved
args !== null &&
Object.keys(args).length > 0
) {
throw new Error(
"remove_old_jwt_tables does not accept any arguments",
);
}
sidmore marked this conversation as resolved.
Show resolved Hide resolved
},
function (args) {
// Clear the JWT public signing key table
Copy link
Contributor

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
View file
Open in desktop
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
View file
Open in desktop
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(
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 @@ -75,11 +56,6 @@ 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<JwtPublicSigningKeys>(Tables::JWT_PUBLIC_SIGNING_KEYS_METADATA);

Expand Down
11 changes: 1 addition & 10 deletions src/service/network_tables.h
View file
Open in desktop
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
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -875,3 +875,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, issuer):
Copy link
Member

Choose a reason for hiding this comment

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

Looks like we're not actually calling this anywhere. We should add a call to this in the lts_compatibility test (or one of the tests in recovery.py that recovers from an old ledger), to fully confirm that we can have a service where these tables are present before the call, removed after the call, and not repopulated if we do a jwt refresh.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree. We could also try make a simple new test like that

  • populate old tables
  • try out proposal
  • make sure old tables are gone

You can look up tests in jwt_test.py and custom_authorization.py

proposal_body, careful_vote = self.make_proposal(
"remove_old_jwt_tables", issuer=issuer
)
proposal = self.get_any_active_member().propose(remote_node, proposal_body)
return self.vote_using_majority(remote_node, proposal, careful_vote)
Loading