Refresh tokens improvements by s2lomon · Pull Request #13168 · trinodb/trino

s2lomon · 2022-07-13T23:19:49Z

Description

Is this change a fix, improvement, new feature, refactoring, or other?

It's a fix of two potential bugs

Is this a change to the core query engine, a connector, client library, or the SPI interfaces? (be specific)

It improves handling of oauth2 refresh tokens

How would you describe this change to a non-technical end user or system administrator?

It makes UI experience smother and prevents us from rejecting possibly correct TokenPairs

Related issues, pull requests, and links

Documentation

(x) No documentation is needed.
( ) Sufficient documentation is included in this PR.
( ) Documentation PR is available with #prnumber.
( ) Documentation issue #issuenumber is filed, and can be handled later.

Release notes

() No release notes entries required.
(x) Release notes entries required with the following suggested text

Improvements to UI when refresh tokens are enabled
Fix potential bug in scenarios when refresh-tokens are not obtained from IdP, but are expected
Fix a bug with outdated refresh-token on UI

# Section
* Fix some things. ({issue}`issuenumber`)

core/trino-main/src/main/java/io/trino/server/security/oauth2/JweTokenSerializer.java

core/trino-main/src/main/java/io/trino/server/ui/OAuth2WebUiAuthenticationFilter.java

lukasz-walkiewicz · 2022-07-18T14:14:42Z

core/trino-main/src/main/java/io/trino/server/ui/OAuth2WebUiAuthenticationFilter.java

Not really related but since you're already making changes in this area we could consider adding additional tokens logging, probably behind flag, to make debugging easier. Similar to Hydra's LOG_LEAK_SENSITIVE_VALUES https://www.ory.sh/docs/ecosystem/logging

We can discuss this offline, as it is a potential security issue. Recommendations are that we should not log anything security related, so I'm a bit hesitant.

I agree with @s2lomon we shouldn't be logging any security tokens, passwords, or other sensitive information. Image that the logs can always be read by a malicious party and act accordingly.

lukasz-walkiewicz · 2022-07-18T14:16:01Z

core/trino-main/src/main/java/io/trino/server/ui/OAuth2WebUiAuthenticationFilter.java

cmt msg: into account?

lukasz-walkiewicz · 2022-07-18T14:16:16Z

core/trino-main/src/main/java/io/trino/server/security/oauth2/JweTokenSerializer.java

Typo in cmt msg

should be fixed.

lukasz-walkiewicz · 2022-07-18T14:16:31Z

core/trino-main/src/main/java/io/trino/server/ui/OAuth2WebUiAuthenticationFilter.java

Typo in cmt msg

Additional than in the commit message.

Praveen2112

LGTM some minor cleanups.

Praveen2112 · 2022-08-03T15:07:33Z

core/trino-main/src/main/java/io/trino/server/ui/OAuth2WebUiAuthenticationFilter.java

Isn't this redundant ?

no, because this method is a void one (as we modify the response in Filter, rather than creating and returning it). So I need to return somewhere, to stop further processing. Still I will change it to if-else as it seems clearer than this return;

Praveen2112 · 2022-08-03T15:08:01Z

core/trino-main/src/main/java/io/trino/server/ui/OAuth2WebUiAuthenticationFilter.java

Should this be a part of the next commit ?

I think it belongs here, as before we were mixing up handling error from refreshToken flow for api and non api calls.

Praveen2112 · 2022-08-03T15:11:48Z

core/trino-main/src/main/java/io/trino/server/ui/OAuth2WebUiAuthenticationFilter.java

Additional than in the commit message.

Praveen2112 · 2022-08-03T15:14:14Z

core/trino-main/src/main/java/io/trino/server/ui/OAuth2WebUiAuthenticationFilter.java

Can we revert this change isValidPrincipal ?

It's just a method reordering, as I'm adding few new ones. I've not added this method (although it seems as if I did on diff)

- Extract method to keep existing conventions for response creation - Filter out outdated tokens instead of trying to obtain claims for them

Previously, only requests for ui pages were taken into account for the token refresh. After this change, every call for ui - including the api ones to fill existing charts etc. will benefit from refresh tokens. The end result is that when refresh token is enabled, user should never be asked to relog, as long as the site is active and any secure communication goes through to coordinator.

There might be a scenario when refresh token won't be issued, even if it's expected from trino coordinator. Such cases should be handled gracefully.

11xor6

LGTM

lukasz-walkiewicz

LGTM 👍

When our internal token has been issued for lesser time than the refresh token from idp or if idp would start to reject already issued refresh-tokens we need to capture every possible exception and restart whole login.

Praveen2112 · 2022-08-09T07:32:36Z

CI hit - reported as #13556, #13557, #13507

Praveen2112 · 2022-08-09T07:34:36Z

@s2lomon Can you please update the RN here.

Praveen2112 · 2022-08-09T07:36:37Z

Thanks for fixing this !!

colebow · 2022-08-10T08:28:01Z

@s2lomon pinging again on the topic of a release note - if you'd like to propose one, I think a bugfix like this would be worth mentioning, especially if any users were running into these bugs.

s2lomon · 2022-08-10T09:50:58Z

@s2lomon pinging again on the topic of a release note - if you'd like to propose one, I think a bugfix like this would be worth mentioning, especially if any users were running into these bugs.

@colebow Sure sorry for that. Take a look at my propositions, as it's fixing few bugs that I've found.

cla-bot bot added the cla-signed label Jul 13, 2022

s2lomon requested review from Praveen2112 and lukasz-walkiewicz July 13, 2022 23:20

Praveen2112 reviewed Jul 14, 2022

View reviewed changes

s2lomon force-pushed the refresh-tokens-improvements branch from 40fe6c9 to d4eef71 Compare July 14, 2022 12:46

s2lomon requested a review from Praveen2112 July 14, 2022 12:46

Praveen2112 reviewed Jul 14, 2022

View reviewed changes

core/trino-main/src/main/java/io/trino/server/ui/OAuth2WebUiAuthenticationFilter.java Outdated Show resolved Hide resolved

s2lomon force-pushed the refresh-tokens-improvements branch from f374374 to 579c9b6 Compare July 16, 2022 09:14

lukasz-walkiewicz reviewed Jul 18, 2022

View reviewed changes

Praveen2112 force-pushed the refresh-tokens-improvements branch from 579c9b6 to b0e7761 Compare July 28, 2022 07:53

Praveen2112 requested a review from 11xor6 July 28, 2022 09:36

s2lomon requested review from Praveen2112 and lukasz-walkiewicz August 3, 2022 09:27

Praveen2112 approved these changes Aug 3, 2022

View reviewed changes

s2lomon added 3 commits August 4, 2022 15:13

Cleanup OAuth2WebUi implementation

93dcb53

- Extract method to keep existing conventions for response creation - Filter out outdated tokens instead of trying to obtain claims for them

Include refresh-token only when issued

3c324a0

There might be a scenario when refresh token won't be issued, even if it's expected from trino coordinator. Such cases should be handled gracefully.

s2lomon force-pushed the refresh-tokens-improvements branch from b0e7761 to d5371c2 Compare August 4, 2022 13:20

11xor6 approved these changes Aug 5, 2022

View reviewed changes

lukasz-walkiewicz approved these changes Aug 8, 2022

View reviewed changes

Fix refresh flow on ui with outdated token

f9a17d3

When our internal token has been issued for lesser time than the refresh token from idp or if idp would start to reject already issued refresh-tokens we need to capture every possible exception and restart whole login.

s2lomon force-pushed the refresh-tokens-improvements branch from d5371c2 to f9a17d3 Compare August 8, 2022 11:52

Praveen2112 merged commit 5c1da02 into trinodb:master Aug 9, 2022

github-actions bot added this to the 393 milestone Aug 9, 2022

colebow mentioned this pull request Aug 10, 2022

Add Trino 393 release notes #13519

Merged

Conversation

s2lomon commented Jul 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related issues, pull requests, and links

Documentation

Release notes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Praveen2112 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

11xor6 left a comment

Choose a reason for hiding this comment

Uh oh!

lukasz-walkiewicz left a comment

Choose a reason for hiding this comment

Uh oh!

Praveen2112 commented Aug 9, 2022

Uh oh!

Praveen2112 commented Aug 9, 2022

Uh oh!

Praveen2112 commented Aug 9, 2022

Uh oh!

colebow commented Aug 10, 2022

Uh oh!

s2lomon commented Aug 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

5 participants

s2lomon commented Jul 13, 2022 •

edited

Loading

s2lomon commented Aug 10, 2022 •

edited

Loading