crate icon indicating copy to clipboard operation
crate copied to clipboard

Add support of JWT token authentication

Open BaurzhanSakhariev opened this issue 1 year ago • 2 comments

Summary of the changes:

  1. Added new authentication method jwt to HostBasedAuthentication.

    • Currently only HTTP, node won't be started if it has entries with jwt and no protocol or non-http protocol
  2. Turned Credentials token (introduced in https://github.com/crate/crate/commit/ffc7f8a3d9fe53bf8d1a38f8f2a2feecfcc62732) from String to DecodedJWT.

    Reason - we have to do unsafe decode to do CrateDB user lookup. If we pass through String token in Credentials, we would need to do verify(String) which does decoding again. See also https://github.com/auth0/java-jwt/issues/279.

    It's fine to use unsafe decoded data, as we verify signature and double check claims at the end of workflow.

    withIssuer/withClaim essentially check authenticated user's metadata with token data.

  3. Limited supported algorithms to RSA, all 3 256/384/512 but no ECDSA.

    • Microsoft and Google support only RSA256. RSA384/512 are also supported in our case because it's same effort as not supporting them - we anyway have RSAKeyProvider, so code is same.
  4. Added possibility to fetch user from Bearer token in _sql endpoint.

    • It's not authentication, same with password. Same like we don't check password, we don't verify signature there. Only try to get user from header and if not found, fall back to default setting. See SqlHttpHandler change.

TODO:

  • [x] docs
  • [x] full integration test (including setting up an endpoint returning public key)

BaurzhanSakhariev avatar Feb 13 '24 17:02 BaurzhanSakhariev

Hi @seut, could you please check current draft?

There are still docs missing and need to write full blown integration test but otherwise functional is ready.

BaurzhanSakhariev avatar Feb 15 '24 15:02 BaurzhanSakhariev

Pushed another commit to resolve Algorithm not from header but from JWK endpoint.

BaurzhanSakhariev avatar Feb 19 '24 13:02 BaurzhanSakhariev

Hi @seut, @mfussenegger, PR is ready for another review.

One thing which might need a closer look - checking token per request.

If token issuer adds and signs "exp" claim it's meant to be checked. If we don't it per-request, a stale token can be used while conenction is alive.

https://github.com/crate/crate/pull/15551/commits/f446957c59826024aa794257c9e1302d247caf72 basically enforces authenticate call for tokens.

However, we still allow caching authorizedUser to handle chunks. It basically means do the check for the first chunk and consider follow up chunks as the same request. It's needed for:

  1. Handling huge files - if we allowed access once, download shouldn't stop midway.
  2. Even regular request gets followed by LastHttpContent EMPTY_LAST_CONTENT.

BaurzhanSakhariev avatar Feb 26 '24 14:02 BaurzhanSakhariev

unrelated https://jenkins.crate.io/job/CrateDB/job/crate_on_pr/13243/testReport/junit/io.crate.integrationtests/LogicalReplicationITest/test_drop_and_recreate_subscription_while_published_table_is_written_to/

retest this please

BaurzhanSakhariev avatar Feb 27 '24 11:02 BaurzhanSakhariev

another one LR failure https://jenkins.crate.io/job/CrateDB/job/crate_on_pr/13244/testReport/junit/io.crate.integrationtests/LogicalReplicationITest/test_write_to_subscribed_empty_partitioned_table_is_forbidden/

retest this please

BaurzhanSakhariev avatar Feb 27 '24 11:02 BaurzhanSakhariev