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

Add authentication with multiple jwt providers #694

Merged
merged 42 commits into from
Feb 24, 2022
Merged

Conversation

MeStrak
Copy link
Contributor

@MeStrak MeStrak commented Feb 3, 2022

Please check if the PR fulfills these requirements

  • The commit message follows our guidelines
  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

What kind of change does this PR introduce?

Adds JWT authentication to Whispr, accepting configuration of multiple providers so JWTs can be accepted from multiple applications. Initial implementation for #696.

What is the current behavior?

No authentication.

What is the new behavior?

  • Added authentication module
  • Added a new authenticated query whispsAuthBeta to begin testing the implementation in a deployed environment (no breaking change for the existing API)
  • Auth configuration by storing config in JSON in env vars
  • Configuration can be an array of passport-jwt configurations by using this passport-jwt enhancement: https://www.npmjs.com/package/@mestrak/passport-multi-jwt

What was the testing strategy?

Unit tests

  • Config service for auth configuration covering multiple scenarios including
    • Ensure all JWT extractor functions are correctly configured
    • Ensure both secretOrKey and secretOrKeyProvider configurations work
    • Some simple validation tests
    • Accepts multiple configurations
  • JWT strategy: check that the passport-multi-jwt object is created with multiple configurations

E2E tests

  • E2E testing on authenticated query:
    • with valid JWT configured with a secret
    • with invalid JWT (incorrect secret)
    • with corrupt JWT
    • with secretOrKeyProvider - connect to JWKS server

Load tests

Ran a simple test with the following scenario:

  • Ran whisps load creation script to populate database
  • Ran test with 100 virtual users for 5 minutes, running the whisps query with a limit of 100 records once every iteration
  • Ran the same test again replacing whisps query with whispsAuthBeta
  • Limitations: executed against a single instance running on GitPod - not tested in an auto scale, but the test is good enough to test the raw performance impact from adding auth

Results

Small impact to performance. Average response time increased by approximately 1.6ms for the auth enabled query. The test results can be variable between runs as we are dependent on the Gitpod infrastructure, but I repeated a few times and saw an increase of just over 1ms in most cases.

This means that enabling authentication in production should not have any noticeable impact on system performance; the change is definitely justified considering the security benefits it brings.

See below for detailed test results.

Output with no auth

image

Output with auth

image

How has the change been documented?

Does this PR introduce a breaking change?

Yes 😈

  • Auth configuration is a mandatory environment variable. Without it Whispr will not start.
  • Currently authentication has only been added to a secondary whisps query to allow consumers to start testing their authentication process without breaking existing usage (I might review and adjust this before the final PR to have a custom guard which temporarily allows Auth with no JWT which can be applied globally to log an error if there is no JWT but still return a result).

Other information

This PR does not cover authorization - that needs to be addressed in the future to further secure the API at the record level (for example access per application ID).

@MeStrak MeStrak removed the request for review from David-Wennemaring February 3, 2022 19:20
@MeStrak MeStrak self-assigned this Feb 14, 2022
@MeStrak MeStrak marked this pull request as ready for review February 15, 2022 17:49
Babbafett
Babbafett previously approved these changes Feb 16, 2022
over-flo79
over-flo79 previously approved these changes Feb 23, 2022
@over-flo79 over-flo79 merged commit ee73acc into master Feb 24, 2022
@sanofi-iadc-bot
Copy link
Contributor

🎉 This PR is included in version 4.0.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants