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

feat: Traefik decision api support #904

Merged
merged 19 commits into from
Feb 14, 2022

Conversation

dadrus
Copy link
Contributor

@dadrus dadrus commented Feb 1, 2022

@aeneasr: This is the PR I was talking to you about yesterday

Related issue(s)

This PR focuses on the decision API only. So I don't know, whether other traefik specific use cases are supported.

There are a couple of discussions, issues and stale PRs related to this PR:

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 security@ory.sh) 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

Even there are a couple of ideas outlined in the referenced issues/PRs on how oathkeeper could be enhanced to have a more generic implementation to support different use cases (like other API gateways or proxies), this PR does not take them into account. Rather is focuses on Traefik support only. I'm convinced, it is however a good starting point to move forward and to resolve at least a couple of requests here at github, as well as at Slack. Further enhancements can then be done in further iterations. I also didn't want to touch any parts of the configuration, as we should first make use of the updated ory-x configuration capabilities.

Tests and documentation will be added as soon as we have a common understanding on how to proceed.

Beyond that, the given PR enables full support for the Traefik's ForwardAuth middleware.

Please note that this implementation does not differentiate between different strategies (where to get the information about the upstream service uri, host, scheme and method). If no X-Forwarded-* header is set for a particular attribute, it is taken from the Request URL, otherwise the corresponding value from the header is taken. This way one can overwrite any, or none of the aforesaid attributes which is a non-breaking change to the current behavior.

I have not implemented a separate decision end-point by intention, as it would uselessly blow the code base by mainly copying&pasting code (implementation & tests) and make the project less maintainable. I'm also not really sure, whether an introduction of different strategies makes sense. But if it is according to the community, I personally favor a strategy based approach (via configuration) much more (which depends on the new ory-x version), compared to specific endpoints.

@dadrus dadrus requested a review from aeneasr as a code owner February 1, 2022 07:34
@codecov
Copy link

codecov bot commented Feb 1, 2022

Codecov Report

Merging #904 (6e91ee0) into master (cb01565) will decrease coverage by 1.12%.
The diff coverage is 90.90%.

❗ Current head 6e91ee0 differs from pull request most recent head 487e2fa. Consider uploading reports for the commit 487e2fa to get more accurate results

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #904      +/-   ##
==========================================
- Coverage   65.72%   64.60%   -1.13%     
==========================================
  Files         104      103       -1     
  Lines        4633     4732      +99     
==========================================
+ Hits         3045     3057      +12     
- Misses       1314     1402      +88     
+ Partials      274      273       -1     
Impacted Files Coverage Δ
driver/registry.go 100.00% <ø> (ø)
proxy/proxy.go 72.34% <ø> (ø)
credentials/verifier_default.go 80.00% <57.14%> (+0.23%) ⬆️
api/decision.go 95.55% <100.00%> (+4.44%) ⬆️
driver/registry_memory.go 89.91% <100.00%> (ø)
pipeline/errors/error_redirect.go 77.14% <100.00%> (+2.14%) ⬆️
proxy/request_handler.go 77.61% <100.00%> (ø)
x/compare.go 100.00% <100.00%> (ø)
...tpclient/client/api/is_instance_alive_responses.go 20.83% <0.00%> (-6.95%) ⬇️
...rnal/httpclient/client/api/list_rules_responses.go 19.14% <0.00%> (-6.57%) ⬇️
... and 7 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bd1b03a...487e2fa. Read the comment docs.

@dduzgun-security
Copy link

This PR would be a solution for the discussion #899 for the integration of Traefik and Oathkeeper

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.

Looking solid, just needs some tests :)

@dadrus
Copy link
Contributor Author

dadrus commented Feb 4, 2022

@aeneasr: In principle, this PR is complete. I also added traefik to the list of supported integrations. The documentation is however kind of thin. If there would be a section for integration examples, I would add some. I'm also not sure, where to put the information about how the url to be tested is created by oathkeeper (request url + header combinations)

@dadrus dadrus requested a review from aeneasr February 4, 2022 08:13
@dadrus
Copy link
Contributor Author

dadrus commented Feb 4, 2022

@aeneasr: I'm not sure, whether the new guide about integrating oathkeeper with traefik is properly referenced in the sidebar.json. Please take a look.

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! Great stuff :)

Regarding the docs, we are currently in the process of moving all the docs to github.com/ory/docs to resolve several major problems in the CI infrastructure. You can either keep them in this branch and I merge them into ory/docs once this is merged, or we make a PR against this PR ( ory/docs#557 ) in this directory: https://github.com/ory/docs/tree/next-generation/docs/oathkeeper

@@ -42,7 +43,7 @@ func (v *VerifierDefault) Verify(

kid, ok := token.Header["kid"].(string)
if !ok || kid == "" {
return nil, errors.WithStack(herodot.ErrInternalServerError.WithReason("The JSON Web Token must contain a kid header value but did not."))
return nil, errors.WithStack(herodot.ErrBadRequest.WithReason("The JSON Web Token must contain a kid header value but did not."))
Copy link
Member

Choose a reason for hiding this comment

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

Changing all of these is kind of a breaking change (although not an terrible one). Given that our service configuration returns something incorrect here, why would we report to the client calling this endpoint that it's a bad request?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

According to my understanding of the code, this is actually not about our service configuration, it is about the data sent by the client. If the client sent a malformed request (here the an access token not containing the expected data), it should imho be answered accordingly.

Another reason was logging. Internal server error is about unrecoverable errors, which imply some kind of a bug, which should be addressed. Here, we don't have things like this. So I wanted avoiding the be filled with error log statements, which are no errors.

Do you agree, or do you still want to have these changes reverted?

Copy link
Member

Choose a reason for hiding this comment

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

I see, in case that we verify credentials, Unauthorized error would also be acceptable. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The main idea is not polluting the logs with error messages, which are actually not errors. I consider this as best practice.
Ah, if I remember correctly, to have the error handler working with traefik (to redirect to the login page), I had to change some of the responses to Unauthorized (which relate to authentication cases).

Back to your proposal. So you would rather like to see all changed responses to be Unauthorized? Fine for me ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please let me know if this is what you would like to have. I'll then update the PR accordingly.

@dadrus
Copy link
Contributor Author

dadrus commented Feb 10, 2022

Let's do a PR against the ory/docs PR.

@aeneasr
Copy link
Member

aeneasr commented Feb 11, 2022

Ok :) The PR in ory/docs is merged so it can be straight against master!

@dadrus
Copy link
Contributor Author

dadrus commented Feb 11, 2022

The last commit (see above) includes the merge with current master and removes the documentation. Latter is moved to the other PR (also referenced above)

@dadrus dadrus requested a review from aeneasr February 11, 2022 16:18
aeneasr pushed a commit to ory/docs that referenced this pull request Feb 14, 2022
This PR contains the documentation for the updates done in ory/oathkeeper#904.
@aeneasr aeneasr merged commit bfde9df into ory:master Feb 14, 2022
@dadrus dadrus deleted the traefik_decision_api_support branch February 14, 2022 17:51
@designermonkey
Copy link

Hi there, forgive me if this sounds pushy (I'm at the edge of my seat for this functionality). As I don't know how your release schedules work, would someone be able to tell me when this will be available as a docker container for me to use?

@designermonkey
Copy link

I just spotted the v0.38.20-beta.1 tag.

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

Successfully merging this pull request may close these issues.

4 participants