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: introduce auth scheme and jumping to next authentication #982

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

feat: introduce auth scheme and jumping to next authentication #982

wants to merge 1 commit into from

Conversation

Marlinc
Copy link
Contributor

@Marlinc Marlinc commented Jul 4, 2022 •
edited
Loading

Some (legacy) application don't use Bearer as auth scheme in the Authorization header, in the current implementation this means Oathkeeper cannot be used to handle these legacy applications. The bearer token (and cookie) authenticator currently also don't support handling multiple authentication configurations (for example an access token in a header but also in a query parameter).

This change adds an optional auth_scheme option to the bearer token authenticator that allows changing the scheme it accepts, it also supports setting an empty scheme for applications that provide a token directly in a header. For compatibility it defaults to Bearer when the header is set to Authorization. The second change is that the session store endpoint can now return a HTTP 406 Not Acceptable response to tell Oathkeeper to jump to the next authentication rule.

This changes were previously discussed on Slack.

I will add some documentation too about these new features.

Related issue(s)

Checklist

  • I have read the contributing guidelines.
  • I have referenced an issue containing the design document if my change
    introduces a new feature.
  • I am following the
    contributing code guidelines.
  • I have read the security policy.
  • I confirm that this pull request does not address a security
    vulnerability. If this pull request addresses a security. vulnerability, I
    confirm that I got green light (please contact
    [email protected]) from the maintainers to push
    the changes.
  • I have added tests that prove my fix is effective or that my feature
    works.
  • I have added or changed the documentation.

Further Comments

@Marlinc Marlinc changed the title rfeat: service points changes feat: introduce auth scheme and jumping to next authentication Jul 4, 2022
@Marlinc Marlinc force-pushed the mutator-features branch 3 times, most recently from f64c7a0 to fb908e1 Compare July 4, 2022 13:49
@codecov
Copy link

codecov bot commented Jul 4, 2022 •
edited
Loading

Codecov Report

Merging #982 (2e345a4) into master (1738e61) will decrease coverage by 11.47%.
The diff coverage is 100.00%.

❗ Current head 2e345a4 differs from pull request most recent head 64e332f. Consider uploading reports for the commit 64e332f to get more accurate results

@@             Coverage Diff             @@
##           master     #982       +/-   ##
===========================================
- Coverage   77.79%   66.31%   -11.48%     
===========================================
  Files          81      107       +26     
  Lines        3873     4771      +898     
===========================================
+ Hits         3013     3164      +151     
- Misses        587     1325      +738     
- Partials      273      282        +9
Impacted Files Coverage Δ
helper/bearer.go 100.00% <100.00%> (ø)
pipeline/authn/authenticator_cookie_session.go 83.33% <100.00%> (+4.08%) ⬆️
metrics/prometheus.go 93.33% <0.00%> (-2.42%) ⬇️
rule/fetcher_default.go 76.65% <0.00%> (-1.92%) ⬇️
credentials/fetcher_default.go 64.89% <0.00%> (-1.07%) ⬇️
pipeline/mutate/mutator_hydrator.go 65.09% <0.00%> (-1.00%) ⬇️
metrics/middleware.go 97.36% <0.00%> (-0.51%) ⬇️
driver/registry_memory.go 89.91% <0.00%> (-0.18%) ⬇️
cmd/serve.go 100.00% <0.00%> (ø)
driver/health/event_manager_default.go 94.11% <0.00%> (ø)
... and 44 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@CLAassistant
Copy link

CLAassistant commented Jul 4, 2022 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@Marlinc Marlinc force-pushed the mutator-features branch 5 times, most recently from 41d2dc6 to 0a4adec Compare July 6, 2022 08:51
@Marlinc Marlinc marked this pull request as ready for review July 6, 2022 09:30
@Marlinc Marlinc requested a review from aeneasr as a code owner July 6, 2022 09:30
@Marlinc Marlinc force-pushed the mutator-features branch from 0a4adec to 2e345a4 Compare July 13, 2022 07:15
aeneasr
aeneasr reviewed Sep 10, 2022
View reviewed changes
Copy link
Member

@aeneasr aeneasr left a comment

Choose a reason for hiding this comment

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

Thank you @Marlinc and sorry. for the late review! I'm wondering if it would make more sense to use a generic authenticator in such a case? The bearer token authenticator is supposed to solve https://datatracker.ietf.org/doc/html/rfc6750 which is expecting the Bearer to be in the header. If that's not the case, wouldn't it be a different type of authentication (same mechanism maybe, but a different HTTP auth mechanism).

We could add another authenticator that handles this (e.g. a more generic one)

@aeneasr
Copy link
Member

aeneasr commented Sep 10, 2022

This would potentially be solved by: #949

@vinckr vinckr mentioned this pull request May 10, 2023
Draft
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aeneasr aeneasr aeneasr left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Marlinc @CLAassistant @aeneasr