michelp / pgjwt

PostgreSQL implementation of JWT (JSON Web Tokens)
MIT License
364 stars 60 forks source link

Verify fails when JSON payload contains characters dash or colon #29

Closed ddevienne closed 9 months ago

ddevienne commented 9 months ago

Hi,

First, thanks for this extension. I use it for (FAT, native) client applications connected to PostgreSQL using libpq, to capture in a secure manner the LOGIN and current ROLEs of the client connection. So that those applications can contact over HTTP a mid-tier server that will do on their behalf (in its own higher privileged connection, after checking the roles against an internal security model) DDLs that the client connections cannot do directly (they can only do DMLs).

I unit tested my transposition of your code, and things were fine. But then in production it was failing. I tracked it down to ROLE names with embedded dash '-' and colon ':' characters (spaces OTOH were OK). Those role names appear in the JSON payload used to sign the JWT. The payload (and thus role names) come back OK on verify. BUT... the JWT is reported as invalid.

I'll try to track it down on my own, but since I'm not much of a pg/plSQL dev, you may get there before me :)

Here's the exact JSON payload (and random secret) I'm using, in case that helps

- Payload = '{"user":"ut_reg_user_ddevienne_dw_go1db1auzzughxivgjyvbd","role":"Acme-FOO:EuqSNTjP:dKDu8U9cb0FFx develop","exp":1701874834,"nbf":1701874234}'
- Secret  = '73a53d0e5db95808816fd831e0d01d39'

Update: I may have jumped the gun. Even with - and : replaced by _ in the payload, it sometimes failed. Earlier I thought it ran fine, but repeatedly calling a test that just signs and verify, fails sometimes. Given that I manually add 600s to nbf to set the exp it can't be an expired token. So something else is afoot...

PS: Note that I've adapted your code from an extension, to just functions I create in my own schema. Because you cannot add extensions in cloud-managed PostgreSQL instances, AFAIK. So it is possible the error is in my transposition of your code as a non-extension.

ddevienne commented 9 months ago

I think I figured it out, by decomposing the query.

- NBFrom  = 2023-12-06T16:59:18.0000000
- EXPires = 2023-12-06T17:09:18.0000000
- NOW SS  = 2023-12-06T16:59:17.6991810
- INRANGE = false

We can see the CURRENT_TIMESTAMP is earlier than the moment I sampled the current server-side timestamp using select cast(extract(epoch from current_timestamp) as int8). The rounding is different!

The test tstzrange(...->>'nbf', ...->>'exp') @> CURRENT_TIMESTAMP AS valid should either use a rounded timestamp too, or nbf and exp should be computed server-side by the extension itself, to ensure the values are consistent.

So the test works or not depending on the rounding, which depends when things happen exactly. Pfiou! So this is not a bug per-se in the extension, since I'm the one that put the nbf and exp in the payload. I guess I just round down the value I put in nbf. Live and learn :)

PS: And this has nothing to do with the characters or not in the payload, as I thought it was...

ddevienne commented 9 months ago

For reference, here's the query I used to troubleshoot this ($1 = token, and $2 = secret):

SELECT  cardinality(r) as part_count,
        convert_from(jwt1_url_decode(r[1]), 'utf8')::json AS header,
        convert_from(jwt1_url_decode(r[2]), 'utf8')::json AS payload,
        r[3] AS input_signature,
        jwt2_algorithm_sign(r[1] || '.' || r[2], $2, 'HS256') AS output_signature,
        to_timestamp(jwt1_try_cast_double(convert_from(jwt1_url_decode(r[2]), 'utf8')::json->>'nbf')) as nbf,
        to_timestamp(jwt1_try_cast_double(convert_from(jwt1_url_decode(r[2]), 'utf8')::json->>'exp')) as exp,
        CURRENT_TIMESTAMP,
        tstzrange(
            to_timestamp(jwt1_try_cast_double(convert_from(jwt1_url_decode(r[2]), 'utf8')::json->>'nbf')),
            to_timestamp(jwt1_try_cast_double(convert_from(jwt1_url_decode(r[2]), 'utf8')::json->>'exp'))
        ) @> CURRENT_TIMESTAMP AS valid
  FROM  regexp_split_to_array($1, '\.') r

PS: The 1 or 2 in function names are an artifact of my un-extension conversion