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

Bearer Token Authentication #38

Merged
merged 4 commits into from
Apr 26, 2021
Merged

Bearer Token Authentication #38

merged 4 commits into from
Apr 26, 2021

Conversation

cimnine
Copy link
Contributor

@cimnine cimnine commented Apr 13, 2021

Summary

This PR implements JWT Bearer token authentication alongside Basic auth.
It's implemented as middleware and adds information to the Request's Context:

  • authentication-method is either Bearer or Basic
  • bearer-token allows access to the jwt.Claims instance if authentication-method=Bearer
  • user allows access to the user's name if authentication-method=Basic

The endpoints /healthz and /metrics (new) don't require authentication.
Every other endpoint does and if no valid credentials or tokens are provided, then an HTTP 403 error is returned.

I've added the promtheus library for metrics collection to the project. Besides the metrics it collects automatically I've created the following two manual metrics:

  • osb_total_broker_api_requests_total:
    The total number of processed service broker api requests, not including healthz and metrics requests.
  • osb_failed_authentication_attempts_total:
    The total number of failed authentication attempts.

I've manually reviewed the library I used whether https://auth0.com/blog/critical-vulnerabilities-in-json-web-token-libraries/ would be a problem. I conclude it's not a problem: (1) The none algorithm results in an error. (2) The secrets are always associated to a specific algorithm. It's therefore not possible to use a public key as signature for an HMAC.

TODO

  • Adjust and extend E2E Tests
  • Organize commits into meaningful pieces
  • Allow JWK store be fetched from an URL

Checklist

  • Keep pull requests small so they can be easily reviewed.
  • Categorize the PR by setting a good title and adding one of the labels:
    bug, enhancement, documentation, change, breaking,
    as they show up in the changelog
  • Update the documentation.
  • Update tests.
  • Link this PR to related issues.

@cimnine cimnine added the enhancement New feature or request label Apr 13, 2021
@cimnine cimnine force-pushed the APPCAT-44-BearerToken branch 4 times, most recently from 0039b6b to b7c0b79 Compare April 16, 2021 10:01
@cimnine cimnine changed the title Copy NewBrokerAPI from brokerapi.New Bearer Token Authentication Apr 22, 2021
Copy link
Member

@simu simu left a comment

Choose a reason for hiding this comment

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

The implementation itself LGTM, I concur that the JWT library that we're using isn't affected by https://auth0.com/blog/critical-vulnerabilities-in-json-web-token-libraries/

@cimnine cimnine force-pushed the APPCAT-44-BearerToken branch 2 times, most recently from 2fff130 to cbdef93 Compare April 23, 2021 09:44
@cimnine cimnine force-pushed the APPCAT-44-BearerToken branch 2 times, most recently from bd9147b to c1a2a7a Compare April 23, 2021 16:08
@cimnine cimnine marked this pull request as ready for review April 23, 2021 16:10
@cimnine cimnine requested review from simu and ccremer April 23, 2021 16:12
pkg/config/config.go Show resolved Hide resolved
Copy link
Contributor

@ccremer ccremer left a comment

Choose a reason for hiding this comment

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

What is the current test strategy in order to verify the new auth stack? The PR doesn't seem to ship testing for JWT or I missed it

docs/modules/ROOT/pages/index.adoc Outdated Show resolved Hide resolved
@cimnine cimnine requested a review from ccremer April 26, 2021 11:25
@cimnine
Copy link
Contributor Author

cimnine commented Apr 26, 2021

Thank you for the feedback. I've extended the existing Auth-tests to also test for Bearer-Token based Auth. The coverage of the auth process should thereby be pretty solid.

@cimnine cimnine merged commit 1587c29 into master Apr 26, 2021
@cimnine cimnine deleted the APPCAT-44-BearerToken branch April 26, 2021 12:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants