feat(policy): adds new public keys table #1836

strantalis · 2025-01-06T17:54:28Z

Proposed Changes

Implements the following ADR as discussed here #1485.

Adds new public_keys table along with mappings pivot tables for attribute values, definitions and namespaces.
Adds new CRUD RPC's to manage public keys and their associations
Returns new keys field on attribute values, definitions and namespaces

erDiagram

    key_access_server {
        uuid       id                PK
        varchar    uri               UK
        varchar    name              UK "new optional name column"
        jsonb      public_key
        jsonb      metadata
    }

    public_keys {
        uuid        id                      PK 
        boolean     is_active         
        boolean     was_used          
        uuid        key_access_server_id    FK
        varchar(36) key_id
        varchar(50) alg                     "algorithm"
        constraint  unique_key              UK  "enforces unique key_id and algorithm per KAS (key_access_server_id, key_id, alg)"
        constraint  unique_active_key       UK  "enforce only one active key per KAS per algorithm"
        text        public_key
        jsonb       metadata
    }

    attribute_namespace_public_key_map {
        uuid namespace_id  FK
        uuid public_key_id FK
    }

    attribute_definition_public_key_map {
        uuid attribute_definition_id FK
        uuid public_key_id           FK
    }

    attribute_value_public_key_map {
        uuid attribute_value_id FK
        uuid public_key_id      FK
    }

    key_access_server 1 -- 1+ public_keys : "has"
    public_keys 1 -- 1+ attribute_namespace_public_key_map : "maps"
    public_keys 1 -- 1+ attribute_definition_public_key_map : "maps"
    public_keys 1 -- 1+ attribute_value_public_key_map : "maps"

Checklist

I have added or updated unit tests
I have added or updated integration tests (if appropriate)
I have added or updated documentation

Testing Instructions

jakedoublev · 2025-01-06T21:42:50Z

service/policy/objects.proto

+message Key {
+  string id = 1;
+
+  bool is_active = 2;


Might be better to use google.protobuf.BoolValuehere so you can differentiate between set and false, set and true, and unset? https://protobuf.dev/reference/protobuf/google.protobuf/#bool-value This is particularly useful when marshaling/unmarshaling to JSON as an unset field value will become false (Go zero value) without this rather than skipped in the JSON.

Same with was_used?

service/policy/objects.proto

jakedoublev · 2025-01-06T21:49:59Z

service/policy/objects.proto

@@ -262,6 +307,21 @@ message KeyAccessServer {
  common.Metadata metadata = 100;
 }

+message Key {
+  string id = 1;


Suggested change

string id = 1;

// the database record ID, not the key ID (`kid`)

string id = 1;

Should we add a comment that this is not the kid but the row ID in the database?

jakedoublev · 2025-01-06T21:51:07Z

service/policy/objects.proto

-message KasPublicKeySet {
-  repeated KasPublicKey keys = 1;
-}
+message KasPublicKeySet { repeated KasPublicKey keys = 1; }


Looking at this makes me wonder - do we want to get rid of the concept of a public key set alongside the deprecation of grants? If so, can we document that?

Yeah that should go away. I believe I only leveraged KasPublicKey. I will go through and add deprecation comments to that and kas grants.

jakedoublev · 2025-01-06T21:52:37Z

service/policy/objects.proto

+              "additional segments. Each segment must start and end with an "
+              "alphanumeric character, can contain hyphens, alphanumeric "
+              "characters, and slashes.",
+    expression : "this.matches('^https?://"


Was splitting up the regex like this a requirement from a new linter? I am not sure the formatting improvement is worth the readability impact to the regex. 🤔

I must have a formatter locally that did this.

jakedoublev · 2025-01-06T21:55:13Z

service/policy/unsafe/unsafe.proto

@@ -186,6 +186,19 @@ message UnsafeDeleteAttributeValueResponse {
  policy.Value value = 1;
 }

+// WARNING!!
+message UnsafeDeletePublicKeyRequest {


If delete is unsafe, is update also unsafe?

Update only allows modification if is_active. Not sure if thats an unsafe action though.

service/policy/kasregistry/key_access_server_registry.proto

jakedoublev · 2025-01-06T21:58:45Z

service/policy/kasregistry/key_access_server_registry.proto

+message UpdateKeyRequest {
+  // Required
+  string id = 1;
+  bool active = 2;


Is the supported update functionality strictly deactivation?

Should we make the paths clearer between deactivation, reactivation, and a true update (change of KID, alg, or PEM) even if those are all unsafe?

I think adding deactivation and reactivation makes sense.

@cassandrabailey293 @jp-ayyappan When managing public keys should we allow modification of that resource or should we force an admin to create a new key so they can retain it for audit purposes in the database.

jakedoublev · 2025-01-06T22:07:29Z

service/integration/attributes_test.go

@@ -1267,6 +1267,18 @@ func (s *AttributesSuite) Test_RemoveKeyAccessServerFromAttribute_Returns_Succes
 	s.Equal(aKas, resp)
 }

+func (s *AttributesSuite) Test_GetAttribute_Returns_Only_Active_PublicKeys() {


I see why this makes sense, but I wonder how you're envisioning policy administration of these associations once created. For example, if I'm a CLI user getting a unique violation error because I'm trying to create a key that already existed but was deactivated, how do I find that key? I think in my mind GetAttribute could maybe give everything in every state, but List APIs all take active state filters in, and the lookup GetByFQN is always strictly active for all policy it returns.

service/policy/db/migrations/20241125220354_keys_table.sql

jakedoublev · 2025-01-06T22:14:50Z

service/policy/db/models.go

@@ -55,6 +55,21 @@ func (ns NullAttributeDefinitionRule) Value() (driver.Value, error) {
 	return string(ns.AttributeDefinitionRule), nil
 }

+type ActiveDefinitionPublicKeysView struct {


With the database migrated, would you please run make policy-erd-gen to generate the updated ERD?

service/logger/audit/constants.go

jakedoublev · 2025-01-06T22:29:41Z

service/policy/db/key_access_server_registry.go

+	}
+
+	// Get freshly created key
+	ck, err := c.GetPublicKey(ctx, &kasregistry.GetKeyRequest{Id: id})


We've been trying to avoid doing immediate GETs during CREATEs to prevent data consistency issues in a system with multiple DB replicas (planning in advance). Would you mind employing either:

a transaction

platform/service/policy/db/policy.go

Line 37 in 9e7d5b9

func (c *PolicyDBClient) RunInTx(ctx context.Context, query func(txClient *PolicyDBClient) error) error {

populating everything in the response from the RETURNING SQL and request?

wrapped it in a transaction

jakedoublev · 2025-01-06T22:39:18Z

service/policy/db/migrations/20241125220354_keys_table.sql

+    km.definition_id;
+
+-- Trigger for attribute_value_key_map
+CREATE TRIGGER trigger_update_was_used_value


Can you please help me understand the tradeoff you chose keeping this permanent state in sync with a column and transactions instead of doing the JOINs as needed when checking if a key is assigned to a policy object?

jakedoublev · 2025-01-06T22:40:59Z

service/policy/db/migrations/20241125220354_keys_table.sql

@@ -0,0 +1,233 @@
+-- +goose Up


Could you please add a markdown file service/policy/db/migrations/20241125220354_keys_table.md that links to the ADR so we have that historical link between the migration and the relational change?

jakedoublev

This is a lot of work. Thanks for tackling this @strantalis.

strantalis added 5 commits November 26, 2024 16:11

save changes

581233e

save changes

199c362

save changes

53d027f

save

ad0d30b

adds unit tests and audit logs

356afcb

strantalis changed the title ~~Keys table~~ feat(policy): adds new public keys table Jan 6, 2025

strantalis added 3 commits January 6, 2025 13:28

Merge branch 'main' into keys-table

d3d3320

fix linting errors

e20942a

typos

2058880

strantalis requested review from jrschumacher, ryanulit and jakedoublev January 6, 2025 18:52