search

supabase / wrappers

Postgres Foreign Data Wrapper development framework in Rust.
http://fdw.dev
Apache License 2.0
562 stars 56 forks source link

The stripe wrapper is unable to lookup my secret key ID when selecting from the wrapped tables #284

Closed vickkhera closed 2 months ago

vickkhera commented 5 months ago

Bug report

Describe the bug

The stripe wrapper is unable to lookup my secret key ID when selecting from the wrapper tables.

To Reproduce

Steps to reproduce the behavior, please provide code snippets or a repository:

Add the secret key to the vault (inside seed.sql):

INSERT INTO vault.secrets(name,description,secret,id) VALUES ('stripe','Stripe TEST key','sk_test_XXXXX','97954c5a-3cce-405d-803d-7a9c5949e9a8');

NOTE: You have to hard-code the id here since there's no way to introspect it within the migration file. I need it to work on all environments (DEV/PREVIEW/PROD) so I just hard-coded it.

Create a migration file, and run npx supabase db reset to load it.

CREATE EXTENSION IF NOT EXISTS wrappers WITH SCHEMA extensions;
CREATE FOREIGN DATA WRAPPER stripe_wrapper
  HANDLER stripe_fdw_handler
  VALIDATOR stripe_fdw_validator;
CREATE SERVER stripe_server
  FOREIGN DATA WRAPPER stripe_wrapper
  OPTIONS (api_key_id '97954c5a-3cce-405d-803d-7a9c5949e9a8');

CREATE SCHEMA stripe;
create foreign table stripe.customers (
  id text,
  email text,
  name text,
  description text,
  created timestamp,
  attrs jsonb
)
  server stripe_server
  options (
    object 'customers',
    rowid_column 'id'
  );

Then interactively in psql CLI:

postgres=> select * from stripe.customers;
ERROR:  invalid secret id "97954c5a-3cce-405d-803d-7a9c5949e9a8": SpiTupleTable positioned before the start or after the end
DETAIL:  Wrappers

postgres=> select name,decrypted_secret from vault.decrypted_secrets where id='97954c5a-3cce-405d-803d-7a9c5949e9a8';
  name  |         decrypted_secret
--------+----------------------------------
 stripe | sk_test_XXXXX
(1 row)

As you can see, the vault key ID does exist despite the error. I cannot find what this "SpiTupleTable..." error means on google.

Expected behavior

I expect the query on stripe.customers to return a list of my customers (dev mode, so there are only a handful that should come back). Same error on every other wrapped stripe table.

Screenshots

Error is in text above.

System information

Additional context

Add any other context about the problem here.

Supabase postgres container version 15.1.1.41

imor commented 5 months ago

The error occurs because the api_key_id options should be set to the value of the key_id column of the vault.secrets table, not the id column. Since you want to create the foreign table and set the api key in a migration you can do something like the following:

create extension if not exists wrappers with schema extensions;

create foreign data wrapper stripe_wrapper
  handler stripe_fdw_handler
  validator stripe_fdw_validator;

-- not yet setting the api_key_id option, it will be set later in the set_stripe_secret function
create server stripe_server
  foreign data wrapper stripe_wrapper;

create schema if not exists stripe;

create foreign table stripe.customers (
  id text,
  email text,
  name text,
  description text,
  created timestamp,
  attrs jsonb
)
  server stripe_server
  options (
    object 'customers',
    rowid_column 'id'
);

-- this function inserts a new stripe secret in vault and updates 
-- the foreign server's api_key_id with the returned key_id
create or replace function set_stripe_secret() returns void as $$
  declare
    api_key_id uuid;
  begin
    with inserted_key_id as (
      insert into vault.secrets (name, description, secret)
      values (
        'stripe',
        'stripe test key',
        'sk_test_XXXXX'
      )
      returning key_id
    )
    select key_id into api_key_id from inserted_key_id;
    execute 'alter server stripe_server options (api_key_id ' || quote_literal(api_key_id) || ')';
  end;
$$ language plpgsql;

select set_stripe_secret();

select * from stripe.customers;

Be careful about embedding a production stripe secret in a migration file though.

vickkhera commented 5 months ago

Thanks for pointing out my error on the proper ID field. I must say using the key_id as the reference into the vault does make it very hard to use when I'm running migrations to a preview and production environment. I'm curious what the security implications are of just using the name column from the vault? It is unique (unlike key_id).

I was hoping I could just force the key_id to be my chosen value, but the vault does not permit me to set that column. I ended up with this solution, so I can use it on local development (with a seed.sql) and deploy on my working preview and production environments.

The migration file:

-- not yet setting the api_key_id option, it will be set later by looking up the key_id in the vault
CREATE SERVER stripe_server FOREIGN DATA WRAPPER stripe_wrapper OPTIONS (api_key_id 'placeholder');

-- on PREVIEW and PROD, we fetch the key_id from the vault which was set before this migration. Only if we find it, we update
-- the wrapper server to use it.  This allows us to run this on DEV where the key_id is not yet set by the seed file.
CREATE PROCEDURE install_stripe_secret() AS $$
    DECLARE
        vault_key_id TEXT;
    BEGIN
        SELECT key_id::TEXT FROM vault.secrets WHERE name='stripe' INTO vault_key_id;
        IF vault_key_id IS NOT NULL THEN
            EXECUTE 'ALTER SERVER stripe_server OPTIONS (SET api_key_id ' || quote_literal(vault_key_id) || ')';
        END IF;
    END
$$ LANGUAGE PLPGSQL;
COMMENT ON PROCEDURE install_stripe_secret() IS 'Look up the Stripe key_id from the vault and set it in the server wrapper';

REVOKE ALL ON PROCEDURE install_stripe_secret FROM PUBLIC,authenticated,anon;

CALL install_stripe_secret();

NOTE: I use a 'placeholder' in the original key_id because there is not "ADD OR SET" for the options in the ALTER command. If I use ADD, then that function will emit an error if it is ever run again; if I use SET then it must have a value already but I can re-run my function as I need. I chose the latter option.

Then in my seed.sql for local dev, I have to also call the function, since it is processed after the migrations.

INSERT INTO vault.secrets(name,description,secret) VALUES ('stripe','Stripe TEST key','sk_test_XXXXX');
CALL install_stripe_secret();

This solves my problem.

All in all, it would be much simpler to make this service reference a vault entry name rather than a computed column value which is different in each environment. I can control the name and make it the same on all environments, which I feel is the point of having the vault be our substitute for not having access to environment variables yet.

imor commented 5 months ago

Thanks @vickkhera for the suggestion to use name instead of key_id. I agree that it would make it easier to manage secrets if they are referenced by name. I don't see any security implications of this right now. But I'm leaving this open for others to mull this over and implement a solution in which users can write something like the following:

-- note the use of a new `api_key_name` option instead of `api_key_id`.
create server stripe_server
  foreign data wrapper stripe_wrapper
  options (api_key_name 'stripe secret name');
robcurtis commented 5 months ago

This is timely - I’m encountering the exact same issue right now. Even better would be if we could reference an env from directly within a migration file. I.e. {{env.API_KEY}}.

This would avoid any risk of exposing secrets to version control via migration file.

burmecia commented 5 months ago

The reason we're using key_id instead of key_name is because the name is not unique. You can save multiple api keys in vault with same name, but the foreign server needs exactly only one api key. So I think we'd better keep using key_id.

vickkhera commented 5 months ago

The reason we're using key_id instead of key_name is because the name is not unique. You can save multiple api keys in vault with same name, but the foreign server needs exactly only one api key. So I think we'd better keep using key_id.

This is not true.

postgres=> \d vault.secrets
                                        Table "vault.secrets"
   Column    |           Type           | Collation | Nullable |               Default
-------------+--------------------------+-----------+----------+-------------------------------------
 id          | uuid                     |           | not null | gen_random_uuid()
 name        | text                     |           |          |
 description | text                     |           | not null | ''::text
 secret      | text                     |           | not null |
 key_id      | uuid                     |           |          | (pgsodium.create_key()).id
 nonce       | bytea                    |           |          | pgsodium.crypto_aead_det_noncegen()
 created_at  | timestamp with time zone |           | not null | CURRENT_TIMESTAMP
 updated_at  | timestamp with time zone |           | not null | CURRENT_TIMESTAMP
Indexes:
    "secrets_pkey" PRIMARY KEY, btree (id)
    "secrets_name_idx" UNIQUE, btree (name) WHERE name IS NOT NULL
Foreign-key constraints:
    "secrets_key_id_fkey" FOREIGN KEY (key_id) REFERENCES pgsodium.key(id)
Triggers:
    secrets_encrypt_secret_trigger_secret BEFORE INSERT OR UPDATE OF secret ON vault.secrets FOR EACH ROW EXECUTE FUNCTION vault.secrets_encrypt_secret_secret()

There is a UNIQUE index on secrets name column, specifically secrets_name_idx.

burmecia commented 4 months ago

There is a UNIQUE index on secrets name column, specifically secrets_name_idx.

oh sorry I missed that index, then adding the api_key_name option looks a good solution, will sort it out soon.