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

Helper party api key #37

Merged
merged 6 commits into from
Jun 13, 2024
Merged

Helper party api key #37

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
82305e1
add helper_parties and helper_party_network tables
eriktaubeneck Jun 9, 2024
1253378
add helper_party_api_keys table
eriktaubeneck Jun 8, 2024
3b9474b
add development api keys to .env.development
eriktaubeneck Jun 9, 2024
1d6cb5c
hashed_api_key should be non null
eriktaubeneck Jun 9, 2024
152dcd3
Merge branch 'main' into helper-party-api-key
eriktaubeneck Jun 10, 2024
5de1101
add modified_at and modified_reason to api keys
eriktaubeneck Jun 13, 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
5 changes: 5 additions & 0 deletions server/.env.development
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,11 @@
# DO NOT ADD SECRETS TO THIS FILE. This is a good place for defaults.
# If you want to add secrets use `.env.development.local` instead.

# local dev API keys
HELPER_PARTY_1_API_KEY="f9c1cced33c932da94d520682d30b1a558711865d32634008488abf38d6f75a0"
HELPER_PARTY_2_API_KEY="d64fd1cc084e2633d8e0719cb4691e81b1961949bc2aadecee14fdf53bd6f139"
HELPER_PARTY_3_API_KEY="46198655cdac1d67e3e6aaf905c9d6702c899c607c2baedf353fc3106ec92929"

# for local dev, we turn off auth, unless specifically working on auth
BYPASS_AUTH=true
DUMMY_EMAIL="demo@draft.test"
Expand Down
74 changes: 74 additions & 0 deletions server/data/supabaseTypes.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,41 @@ export type Database = {
}
Relationships: []
}
helper_party_api_keys: {
Row: {
created_at: string
expires_at: string
hashed_api_key: string
helper_party_uuid: string
revoked: boolean
uuid: string
}
Insert: {
created_at?: string
expires_at?: string
hashed_api_key: string
helper_party_uuid: string
revoked?: boolean
uuid?: string
}
Update: {
created_at?: string
expires_at?: string
hashed_api_key?: string
helper_party_uuid?: string
revoked?: boolean
uuid?: string
}
Relationships: [
{
foreignKeyName: "helper_party_api_keys_helper_party_uuid_fkey"
columns: ["helper_party_uuid"]
isOneToOne: false
referencedRelation: "helper_parties"
referencedColumns: ["uuid"]
},
]
}
helper_party_network_members: {
Row: {
created_at: string
Expand Down Expand Up @@ -109,6 +144,45 @@ export type Database = {
}
Relationships: []
}
helper_party_query_status_updates: {
Row: {
helper_party_uuid: string
query_uuid: string
started_at: string
status: Database["public"]["Enums"]["status"]
uuid: string
}
Insert: {
helper_party_uuid: string
query_uuid: string
started_at?: string
status: Database["public"]["Enums"]["status"]
uuid?: string
}
Update: {
helper_party_uuid?: string
query_uuid?: string
started_at?: string
status?: Database["public"]["Enums"]["status"]
uuid?: string
}
Relationships: [
{
foreignKeyName: "helper_party_query_status_updates_helper_party_uuid_fkey"
columns: ["helper_party_uuid"]
isOneToOne: false
referencedRelation: "helper_parties"
referencedColumns: ["uuid"]
},
{
foreignKeyName: "helper_party_query_status_updates_query_uuid_fkey"
columns: ["query_uuid"]
isOneToOne: false
referencedRelation: "queries"
referencedColumns: ["uuid"]
},
]
}
queries: {
Row: {
created_at: string
Expand Down
13 changes: 13 additions & 0 deletions server/supabase/migrations/20240609010604_helper_party_api_keys.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
create table
helper_party_api_keys (
Copy link
Collaborator

Choose a reason for hiding this comment

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

Most of the time I see API keys bound to clients that use them. Helper party has the power to revoke them. but they also want to know who requested them and why, so they can validate both at the request time. If you allow more than one key per party, that may be worth exploring, because otherwise it gets hard to manage

Copy link
Member Author

Choose a reason for hiding this comment

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

This isn't super clear from the PR, but it will be more clear in the next one: the point of these keys for the report collector (draft/server) to provide a key to the helper party (draft/sidecar), that allows that helper party to POST a status update about a query to the server. This is issued from the report collector to the helper party, so that the helper party can validate that POSTs coming in from helper parties are valid.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I see. So, generally systems that are allowed to talk to each other using bi-directional channels, that is: sending actions/mutating the state on both sides are much harder to reason about. Both sides need to have "write" permission on the other side which leads to security complications and maintainability issues.

For the short term, it can work but I'd love us to reconsider this approach after the test

Copy link
Member Author

Choose a reason for hiding this comment

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

yea, a more detailed design review here would be great. at a very high level, I'm not sure we can avoid bi-directional all together, but I certainly want to (and think we can) avoid managing state in both places. As I see it:

  1. Management Plane --> Control Plane: Start query signal, Stop Query signal, etc
  2. Control Plane --> Management Plane: Current state, logs, observability

So different signals, but in both directions, we need authentication from between these two planes (as they are operated by different parties.)

uuid uuid default gen_random_uuid() primary key,
helper_party_uuid uuid references helper_parties not null,
hashed_api_key varchar(255) not null,
created_at timestamp default current_timestamp not null,
expires_at timestamp default current_timestamp + interval '1 year' not null,
revoked boolean default false not null
Copy link
Collaborator

Choose a reason for hiding this comment

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

can you elaborate why we need an extra flag here? One may think that to revoke a key, you can set expires_at in the past and wait until this change is propagated

Copy link
Member Author

Choose a reason for hiding this comment

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

Main reason is so that we retain more information, e.g., revoking a key is different than it expiring, and we'd want to provide different error messages based on those different facts.

Copy link
Collaborator

Choose a reason for hiding this comment

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

My main concern is the validity check - you must not forget to check both and if expires_at is still in the future and revoked is set, that looks weird. Maybe, as an alternative, you could use status or modify_reason/modified_at? Validity check can only look at expires_at and that is simpler

Copy link
Member Author

Choose a reason for hiding this comment

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

If we have a status column, then it becomes a bit less clear to a future developer that they need to do a validity check on expires_at and not status, and similarly with modify_reason/modify_at you need to assure that it also changes expiry.

Here's another idea. It would require slightly more work, but would be easier to worry about. Create two tables:

helper_party_api_key_creation_events and helper_party_api_key_revocation_events. The first table would have everything in this PR except the revoked column, and the second table would have: uuid, helper_party_api_key_creation_event_uuid (join column), helper_party_uuid, created_at.

Then, we'd create a view (possibly materialized), which would be used for the validity checks

select 
    c.helper_party_uuid, 
  , c.hashed_api_key
  , c.expires_at < current_timestamp() and r.uuid is null as valid
  , case 
      when r.uuid is not null as 'revoked'
      when c.expires_at >= current_timestamp() as 'expired'
      else null
from 
  helper_party_api_key_creation_events c
left outer join
  helper_party_api_key_revocation_events r
on c.uuid = r.helper_party_api_key_creation_event_uuid

This does a few nice things:

  1. Maintains all information (e.g., nothing is overwritten or modified, just computed as a result of events.)
  2. Minimizes the risk of incorrect validation, as stored directly on the table.
  3. Minimized the risk of incorrectly invalidating, as a future developer just needs to create that event, rather than make sure to properly modify the helper_party_api_key table.

The only potential downside is that it moves some of business logic into the database. For this, it seems like there are essentially 3 places where logic can live (and things can go wrong):

  1. When events are created or modified (revoked). In your example, this would require making sure that when a revoke action happens, the developer needs to correctly update the expiration.
  2. When api keys are validated. This requires potentially checking expires_at and revoked.
  3. In the database, as a view computed over actions.

After writing this all out, I think I like 3/ the best. Also, this type of event driven architecture is a possible solution to the problem you mention about state management across between the management plane and multiple control planes.

Copy link
Collaborator

Choose a reason for hiding this comment

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

With the event sourcing, you typically have only one timestamp which is the timestamp of the event. Then you can build a snapshot and/or the latest status by applying these events according to their timestamp and looking at the resulting object. So you technically can have only one table that records two event types - create/revoke and if revoke is the last event, then key is no longer valid.

This could be more powerful than what we want here - I was thinking that expiry_at + modify_reason = "REVOKE" is enough information for an engineer to see what is going on, even if reason is unstructured. I don't want to overcompicate things here, so if you are not inclined with this, I think the current proposal is better than even sourcing because it is less complex

);

alter table helper_party_api_keys enable row level security;

-- do not add any authenticated access to api_keys, require service_role and handle in application
8 changes: 8 additions & 0 deletions server/supabase/seed.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,3 +20,11 @@ INSERT INTO public.helper_party_networks (uuid, display_name) VALUES ('a8c892ae-
INSERT INTO public.helper_party_network_members (helper_party_uuid, helper_party_network_uuid) VALUES ('de218b52-1ec7-4a4d-9bf9-f9070b2c3a93', 'a8c892ae-8cee-472f-95f0-e25b1fec9759');
INSERT INTO public.helper_party_network_members (helper_party_uuid, helper_party_network_uuid) VALUES ('b8848f0f-65c4-499f-82b4-1e3a119ba31e', 'a8c892ae-8cee-472f-95f0-e25b1fec9759');
INSERT INTO public.helper_party_network_members (helper_party_uuid, helper_party_network_uuid) VALUES ('91993b4a-4131-4b9f-a132-d4a5839e3c6c', 'a8c892ae-8cee-472f-95f0-e25b1fec9759');

--
-- Data for Name: helper_party_api_keys; Type: TABLE DATA; Schema: public; Owner: postgres
--

INSERT INTO public.helper_party_api_keys (uuid, helper_party_uuid, hashed_api_key) VALUES ('13d80a42-4b40-4987-a338-b394674ab399', 'de218b52-1ec7-4a4d-9bf9-f9070b2c3a93', '243262243130244470514a2e527665766e57614a4f5835782f505a534f6a674f335866444737505178585851656e3866796e52467563516d66414547');
INSERT INTO public.helper_party_api_keys (uuid, helper_party_uuid, hashed_api_key) VALUES ('1d61b974-7ac8-4baa-b0b0-a83cd29c46e2', 'b8848f0f-65c4-499f-82b4-1e3a119ba31e', '243262243130247770623576564767534f61772f516441334a51734c2e7373494e3652714369355a554a414e2e7343796d4673394c475247584c5375');
INSERT INTO public.helper_party_api_keys (uuid, helper_party_uuid, hashed_api_key) VALUES ('31c229e3-8150-4f9b-91e6-ac413198f4ff', '91993b4a-4131-4b9f-a132-d4a5839e3c6c', '24326224313024724746664c3146614b4b6e68715169714e6b58573165485a37662f6d3857563271364a336845564139352e5746465677616b774d71');
Loading