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.

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

Switch to token-based authentication #2157

Closed
22 tasks done
VakarisZ opened this issue Aug 3, 2022 · 1 comment · Fixed by #3085
Closed
22 tasks done

Switch to token-based authentication #2157

VakarisZ opened this issue Aug 3, 2022 · 1 comment · Fixed by #3085

Comments

@VakarisZ
Copy link
Contributor

VakarisZ commented Aug 3, 2022

Follow up to #2130

By-reference vs by-value tokens

The main libraries for session-based authentication are Flask-Login and more robust version of it Flask-Security.
Although, based on this article setting up flask-login is not trivial for a setup such as ours and the author just ended up with JWT + blocklist approach.
All things considered, I lean towards session-based approach, because it seems like a better fit for the use case of the project.
Managing JWT blocklist could become a pain and a bottleneck, because for each agent run we'll have to create a JWT blocklist entry. Those entries will then have to be removed after the original JWT is outdated?
example implementations

Token in authorization header

Another option to strongly consider is moving the session token to authorization header: https://flask-login.readthedocs.io/en/latest/#custom-login-using-request-loader
This would provide unified interface for the UI and the agents. Also, this would prevent CSRF. The downside is the manual token handling (which we already do with JWT).

There are a couple of possible ways to handle this:

  1. Add authorization header:
    + Requests remain stateless
    + Faster, no need to keep fetching CSRF token
    - More custom code is required on the client side. We need to store the token in local storage and include it in requests

  2. Include CSRF token in all requests:
    + Less custom code, only hook all requests to fetch and include CSRF
    - Requests become stateful. This has potential to make unit/bb tests and agent communication harder

  3. Add custom header and use only json content type:
    + Easy
    - Stateful
    - Good enough?

All things considered I think solution 3 is the easiest. If we enforce the content type and custom header on the island side, it should be enough protection.

Edit: after discussion with @mssalvatore we decided to go with 1, because stateless API is easier to document and use for third parties and it's more orthodox solution.

More on CSRF protection

CSRF protection (add custom request headers. The CSRF is also irrelevant as long as we use application/json content type)
Here's what Flask-security has to say about it: https://flask-security-too.readthedocs.io/en/stable/patterns.html#csrf

Prototype location

Working example with key changes are here. Registration/login/logout are working.

Main branch: 2157-switch-to-token-based-auth

Tasks for session based token

  • Move user to database (0d) @ilija-lazoroski
    • Define user model (inherits UserMixin example model)
    • Define a single role model (inherits role mixin)
    • Remove IUserRepository
  • Change authentication service on the island
    • Setup flask-security (add MongoEngine connection, example here) (0d) @ilija-lazoroski
    • Implement registration (Allow only single user to register) (0d) - @shreyamalviya @ilija-lazoroski
      • Emit CLEAR_SIMULATION_DATA, RESET_AGENT_CONFIGURATION, SET_ISLAND_MODE - UNSET` on registration
      • Reset repository encryptor on registration
    • Implement login. Try using verify_and_update_password. Make sure verify_and_update_password is using salt correctly (basically make sure it does bcrypt.checkpw behind the scenes). Make sure login creates a user with the "user" role. (0d) - @shreyamalviya
  • Change monkey/monkey_island/cc/ui/src/services/AuthService.js component. It needs to include Authentication-token header. Login and registration requests will also need specific query param to retrieve an authentication token. (0d) @VakarisZ
    • Implement logout
  • Show the new format of login/registration/logout errors (0d) @cakekoa
  • Secure all presently secured endpoints with auth_token_required (0d) - @shreyamalviya
  • Secure all presently secured endpoints with roles_accepted decorator. Roles will allow us to segregate endpoints that are agent-only, user-only or both (0d) - @shreyamalviya @ilija-lazoroski
  • Unlock repository encryptor on on login (0d)
  • Update blackbox tests to use new auth mechanism (0d) @ilija-lazoroski
  • Research do we actually need SECRET_KEY and password salt (0d) @ilija-lazoroski
  • Consolidate all login-related functionality under an "AuthenticationService" (0d) - @ilija-lazoroski
  • Investigate the API, make sure we are CSRF protected, make sure that other configuration options set for the app are adequate (maybe we need to change more defaults) (0d) @ilija-lazoroski
  • Change the docs for resetting the credentials (0d) @cakekoa
  • Remove vulture entries related to this issue

Note: These tasks will take more than expected because we won't resist the temptations to refactor the services and front end, which could use improvements.

@mssalvatore mssalvatore changed the title Switch to session based authentication Switch to token-based authentication Aug 8, 2022
This was referenced Feb 24, 2023
ilija-lazoroski added a commit that referenced this issue Mar 3, 2023
@VakarisZ VakarisZ mentioned this issue Mar 3, 2023
8 tasks
VakarisZ pushed a commit that referenced this issue Mar 3, 2023
VakarisZ pushed a commit that referenced this issue Mar 3, 2023
VakarisZ pushed a commit that referenced this issue Mar 7, 2023
VakarisZ pushed a commit that referenced this issue Mar 7, 2023
VakarisZ pushed a commit that referenced this issue Mar 7, 2023
VakarisZ pushed a commit that referenced this issue Mar 7, 2023
cakekoa pushed a commit that referenced this issue Mar 7, 2023
@ilija-lazoroski ilija-lazoroski mentioned this issue Mar 8, 2023
8 tasks
mssalvatore added a commit that referenced this issue Mar 8, 2023
cakekoa added a commit that referenced this issue Mar 8, 2023
Disables the default login, logout, and register endpoints

Issue #2157
PR #3071
mssalvatore pushed a commit that referenced this issue Mar 8, 2023
Disables the default login, logout, and register endpoints

Issue #2157
PR #3071
cakekoa added a commit that referenced this issue Mar 8, 2023
cakekoa added a commit that referenced this issue Mar 8, 2023
shreyamalviya pushed a commit that referenced this issue Mar 9, 2023
mssalvatore pushed a commit that referenced this issue Mar 17, 2023
* Island: Do manual "is user already registered" check

Issue #2157
PR #3124
@mssalvatore mssalvatore added this to the v2.1.0 milestone Mar 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants