Add ability to generate access tokens #86
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
There was a problem hiding this comment.
the SQL looks great!
lets wait for pgsodium review from michel and auth flow review from stojan before approving
I ran into some minor UI stuff during testing (video below)
Screen.Recording.2023-08-25.at.9.09.03.AM.mov
The 3 I noticed were:
Loading...not visible in dark mode- clicking
Create Tokendid not redirect or have any visible effect Error loading access tokensafter creating an access token
There was a problem hiding this comment.
Looks good, though the password hashing is really not necessary for this use case and can actually severely tank performance of your database if someone puts their mind to it (which isn't that difficult). So I would just use SHA256 in the name of security (CIA model -- Availability is impacted by using password hashing when you don't need it without effective rate limiting or features providing for fairness).
| create policy access_tokens_select_policy | ||
| on app.access_tokens | ||
| as permissive | ||
| for select | ||
| to public | ||
| using ( auth.uid() = user_id ); | ||
|
|
||
| create policy access_tokens_insert_policy | ||
| on app.access_tokens | ||
| as permissive | ||
| for insert | ||
| to authenticated | ||
| with check ( auth.uid() = user_id ); | ||
|
|
||
| create policy access_tokens_delete_policy | ||
| on app.access_tokens | ||
| as permissive | ||
| for delete | ||
| to authenticated | ||
| using ( auth.uid() = user_id ); |
There was a problem hiding this comment.
May be easier for maintenance to have one permissive policy for all that enforces the auth.uid() = user_id binding, and then add restrictive policies if something needs stopping (like deletes or the such).
There was a problem hiding this comment.
If you look carefully, the topmost policy is for the public role and the other two are for authenticated. So, adding a for all policy and then restricting individual operations will need more policies: two for all policies, three policies for restricting insert, update and, delete to public and two policies for restricting select and update to authenticated. Which makes seven policies in total.
| exception when unique_violation then | ||
| raise exception 'Token with name `%s` already exists', token_name; | ||
| end; |
There was a problem hiding this comment.
This will quite literally never happen, so I would omit it and panic when you see the insert violation, as the token being inserted will never repeat while this universe exists.
There was a problem hiding this comment.
Ah sorry, it's about the name. It's fine, then though I don't see the point in enforcing name uniqueness -- it's just for the UX. By making the token's prefix available in the database, debugging will be easier for people.
There was a problem hiding this comment.
I have removed the unique constraint on (token_name, user_id) and the exception clause as well.
| user_id uuid = auth.uid(); | ||
| account app.accounts = account from app.accounts account where id = user_id; | ||
| token bytea = gen_random_bytes(16); | ||
| token_hash bytea = pgsodium.crypto_pwhash_str(token); |
There was a problem hiding this comment.
Ditto, you don't need to do this. Just hash it with SHA256.
There was a problem hiding this comment.
Moved to a SHA256 impl now.
| account app.accounts = account from app.accounts account where id = user_id; | ||
| token bytea = gen_random_bytes(16); | ||
| token_hash bytea = pgsodium.crypto_pwhash_str(token); | ||
| token_text text = encode(token, 'hex'); |
There was a problem hiding this comment.
It's often useful to encode some identifying information in the token's prefix. For example: dbdev_<token>.
Also what you may want to store in the database is the token's prefix (first 4-5 chars) to help people identify it quickly. It may also be a bit more compact to encode as Base64-URL or something.
There was a problem hiding this comment.
Implemented this, tokens now look like this:
Still a bit long due to token_id being uuids. I'm trying to convert them to an integer which should make the tokens smaller.
| for token_hash in | ||
| select t.token_hash from app.access_tokens t where t.user_id = user_id | ||
| loop | ||
| token_valid := pgsodium.crypto_pwhash_str_verify(token_hash, token); |
There was a problem hiding this comment.
Again, no need to do password hash verification, which is also VERY CPU intensive. When you switch to SHA256, make sure you don't just do = as that's not constant-time comparison. I need to look into how this can be achieved in Postgres easily as I'm not finding it but @michelp this may be a good API to add to pgsodium.
In general though, this is really a fringe security benefit for this use case.
There was a problem hiding this comment.
Oh also I suppose libsodium uses Argon2 which is incredibly CPU and memory intensive (depending on the settings). So this is really an easy way to crash the database -- just start generating / verifying tokens all the time.
There was a problem hiding this comment.
Moved to a SHA256 based impl.
| 'aud', 'authenticated', | ||
| 'role', 'authenticated', | ||
| 'sub', user_id, | ||
| 'iat', issued_at, | ||
| 'exp', expiry_at |
There was a problem hiding this comment.
Always useful to include the iss claim that will identify who issued the JWT.
There was a problem hiding this comment.
Also, you may want to identify the key that signed it with the kid claim in the JWT's header. This can be useful to easily identity and rotate the key (in Supabase PostgREST we're just about to add support for this).
There was a problem hiding this comment.
I read the signing key with current_setting('app.settings.jwt_secret', true), but how do I get the key id?
There was a problem hiding this comment.
Added iss claim value of database.dev.
|
@olirice I have fixed the invisible |
| ))} | ||
| {isAccessTokensSuccess && accessTokens.length <= 0 && ( | ||
| <p className="dark:text-white"> | ||
| No access tokens found. Click "New Token" to create one. |
This PR adds a new feature to allow users to generate access tokens which will be used by the
dbdev logincommand (in a separate PR).In the UI the following changes have been made:
Access Tokenshas been added.Access Tokenswill take the user to the access tokens page (initially empty).New Tokenwill open the new token form.Cancelwill take the user back to the access tokens page. Filling a token name and clickingCreate Tokenwill take the user to the newly created token. On this page the user can copy the token to be pasted into the terminal whendbdev logincommand will be issued. This will also be the only opportunity user will get to copy the token, this is because tokens in the db are stored hashed and can't be shown in plaintext again.Closewill take the user back to the access tokens page. Here user can clickRevoketo delete/revoke a token.In the db tokens are stored in
app.access_tokenstable. RLS policies are setup so that users can only create, read, or delete their own tokens. Tokens are stored salted and hashed in the db. The token returned to the user in theapp.new_access_tokenis in plaintext. This token also contains the user id prefixed to the token. This is done so that a single token can act as both a "user id and password" when theapp.redeem_access_tokenfunction is given this token. Theapp.redeem_access_tokenfunction validates the token presented to it and issues a jwt with anauthenticatedclaim. This jwt can then be used by the client (dbdevin our case) to make api calls.