Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: improve ImplicitBearerToken #698

Merged
merged 12 commits into from
Aug 17, 2023
Merged

feat: improve ImplicitBearerToken #698

merged 12 commits into from
Aug 17, 2023

Conversation

oleobal
Copy link
Collaborator

@oleobal oleobal commented Jul 28, 2023 •
edited

Description

  • Solve the issue of sometimes issuing tokens that are about to expire, by just issuing an new token every time and relying on feat: add Client.logout and context manager substra#381 to clean them up

  • Add a new server.allowImplicitLogin option, allowing node admins to disable the option altogether in order to improve security practices.

  • Extend /active-api-tokens -X DELETE to also be able to delete ImplicitBearerToken, adding an id field to ImplicitBearerToken for this purpose. This is to enable users to terminate their sessions, as per security recommendations (this is leveraged by feat: add Client.logout and context manager substra#381)

Closes FL-1067, FL-1140

Companion to Substra/substra-documentation#336

How has this been tested?

Tried it on my machine

Checklist

  • changelog was updated with notable changes
  • documentation was updated

@linear
Copy link

linear bot commented Jul 28, 2023

FL-1067 Token expired just after login with username and password

Context

On Lipome, see slack thread.

Tanguy got an error "expired token", some minutes after login in with user and password

Our interpretation of the issue:

  • the backend, with TOKEN_STRATEGY=reuse (which is the only setting now, the alternative strategy has been removed), will reuse tokens. This is to allow running multiple scripts each using client.login at the same time
  • This means the backend can give you a token that is about to expire

Specification

There are several ways around this:

  • issue tokens more aggressively, based on a some heuristic (time since last issuance?)
  • maintain several tokens at the same time

Acceptance criteria

Using client.login should give out tokens with a useful lifetime left

@github-actions github-actions bot added the documentation Improvements or additions to documentation label Jul 29, 2023
@oleobal oleobal changed the title fix: stop issuing old tokens feat: improve ImplicitBearerToken Jul 29, 2023
@oleobal oleobal mentioned this pull request Jul 31, 2023
Merged
7 tasks
@oleobal oleobal marked this pull request as ready for review August 1, 2023 09:05
@oleobal oleobal requested a review from a team as a code owner August 1, 2023 09:05
SdgJlbl
SdgJlbl reviewed Aug 2, 2023
View reviewed changes
backend/backend/views.py Outdated Show resolved Hide resolved
@github-actions github-actions bot added the api label Aug 4, 2023
@oleobal oleobal force-pushed the fix/issue-old-tokens branch 3 times, most recently from 798adda to 4bf67b7 Compare August 8, 2023 09:57
@oleobal
Copy link
Collaborator Author

oleobal commented Aug 8, 2023

/e2e --tests sdk,frontend

@Owlfred
Copy link

Owlfred commented Aug 8, 2023 •
edited

End to end tests: ❌ FAILURE

Jobs status:

  • Camelyon: ⏭️
  • Dispatch Jobs: ✔️
  • Distributed / distributed-sdk,frontend: ❌
  • MNIST: ⏭️
  • Standalone / standalone-sdk,frontend: ❌

Sorry, try again.

@oleobal oleobal requested a review from SdgJlbl August 8, 2023 10:16
@oleobal
Copy link
Collaborator Author

oleobal commented Aug 8, 2023

/e2e --tests sdk,frontend

@Owlfred
Copy link

Owlfred commented Aug 8, 2023 •
edited

End to end tests: ✔️ SUCCESS

“Carpe diem. Seize the day, boys.” ― John Keating, Dead Poets Society

@oleobal oleobal mentioned this pull request Aug 8, 2023
Merged
@linear
Copy link

linear bot commented Aug 8, 2023

FL-1140 Add logout endpoint to the backend

SdgJlbl
SdgJlbl approved these changes Aug 9, 2023
View reviewed changes
Copy link
Contributor

@SdgJlbl SdgJlbl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR

backend/backend/views.py
Comment on lines +84 to +85
tokens = model.objects.filter(id=request.GET.get("id"))
if len(tokens) == 1 and request.user == tokens[0].user:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor but if we expect only one object, or raise, get does the job

Copy link
Collaborator Author

@oleobal oleobal Aug 9, 2023 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My worry is since len(tokens) == 0 is a common outcome of the above request, I'd be using exceptions for control flow.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

better ask for forgiveness than permission ☯️

@oleobal oleobal mentioned this pull request Aug 9, 2023
Merged
7 tasks
@oleobal oleobal force-pushed the fix/issue-old-tokens branch 4 times, most recently from ff22581 to 5f0590e Compare August 10, 2023 12:01
@oleobal
Copy link
Collaborator Author

oleobal commented Aug 14, 2023

Waiting for Substra/substra#381 to merge

@oleobal oleobal mentioned this pull request Aug 14, 2023
Closed
@oleobal
fix: prevent handing out tokens about to expire
ffa1aaf 
Signed-off-by: Olivier Léobal <olivier.leobal@owkin.com>
@oleobal
add server.allowImplicitLogin option
5b8c17d 
Signed-off-by: Olivier Léobal <olivier.leobal@owkin.com>
@oleobal
Update documentation
dea4afe 
Signed-off-by: Olivier Léobal <olivier.leobal@owkin.com>
@oleobal
Update documentation
8b33f6b 
Signed-off-by: Olivier Léobal <olivier.leobal@owkin.com>
@oleobal
switch to HTTP 410
a892951 
Signed-off-by: Olivier Léobal <olivier.leobal@owkin.com>
@oleobal
Improve error when implicit login disabled
1c28679 
Signed-off-by: Olivier Léobal <olivier.leobal@owkin.com>
@oleobal
Allow deleting ImplicitBearerTokens via the API
4d07451 
Signed-off-by: Olivier Léobal <olivier.leobal@owkin.com>
@oleobal
Update authentication.md
d983a60 
Signed-off-by: Olivier Léobal <olivier.leobal@owkin.com>
@oleobal
set allowImplicitLogin in skaffold
c191a22 
Signed-off-by: Olivier Léobal <olivier.leobal@owkin.com>
@oleobal
Remove token rotation mechanism
74c4371 
Signed-off-by: Olivier Léobal <olivier.leobal@owkin.com>
@oleobal
Update tests
dc6c3a0 
Signed-off-by: Olivier Léobal <olivier.leobal@owkin.com>
@oleobal
Copy link
Collaborator Author

oleobal commented Aug 16, 2023

/e2e --tests sdk,substrafl --refs substra=feat/client-logout

@Owlfred
Copy link

Owlfred commented Aug 16, 2023 •
edited

End to end tests: ✔️ SUCCESS

@oleobal
switch allowImplicitLogin to true by default
a5a397e 
Signed-off-by: Olivier Léobal <olivier.leobal@owkin.com>
@oleobal
Copy link
Collaborator Author

oleobal commented Aug 17, 2023

/e2e --tests sdk,substrafl --refs substra=feat/client-logout

@Owlfred
Copy link

Owlfred commented Aug 17, 2023 •
edited

End to end tests: ✔️ SUCCESS

“Carpe diem. Seize the day, boys.” ― John Keating, Dead Poets Society

@oleobal oleobal merged commit 2ef40a3 into main Aug 17, 2023
10 checks passed
@oleobal oleobal deleted the fix/issue-old-tokens branch August 17, 2023 10:12
@Milouu Milouu mentioned this pull request Sep 5, 2023
Merged
Milouu added a commit that referenced this pull request Sep 7, 2023
@Milouu
chore: release 0.41.0 (#725)
9a7259d 
### Added

- New `SECRET_KEY` optional environment variable
([#671](#671))
- `/api-token-auth/` and the associated tokens can now be disabled
through the `EXPIRY_TOKEN_ENABLED` environment variable and
`server.allowImplicitLogin` chart value
([#698](#698))
- Tokens issued by `/api-token-auth/` can now be deleted like other API
tokens, through a `DELETE` request on the `/active-api-tokens` endpoint
([#698](#698))

### Changed

- Increase the number of tasks displayable in frontend workflow
[#697](#697)
- BREAKING: Change the format of many API responses from
`{"message":...}` to `{"detail":...}`
([#705](#705))

### Removed

- BREAKING: `SECRET_KEY_PATH` and `SECRET_KEY_LOAD_AND_STORE`
environment variables
([#671](#671))
- Removed logic for storing `SECRET_KEY` at startup, in order to
increase stability; it should be done at a higher level, i.e. the chart
([#671](#671))

## Fixed

- `/api-token-auth/` sometimes handing out tokens that are about to
expire ([#698](#698))

Signed-off-by: Milouu <milan.roustan@owkin.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SdgJlbl SdgJlbl SdgJlbl approved these changes

Assignees
No one assigned
Labels
api documentation Improvements or additions to documentation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@oleobal @Owlfred @SdgJlbl