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

fix:InMemoryChannel ingress: Reject unauthorized requests #8123

Closed
wants to merge 2 commits into from
Closed

fix:InMemoryChannel ingress: Reject unauthorized requests #8123

wants to merge 2 commits into from

Conversation

7h3-3mp7y-m4n
Copy link
Contributor

Fixes #7981

Proposed Changes

  • Replace the logic of deprecated function VerifyJWTFromRequest with VerifyRequest in event_recevier
  • Handle both AuthN and AuthZ in the request
  • Check policy using InMemoryChannelNamespaceLister

Pre-review Checklist

  • At least 80% unit test coverage
  • E2E tests for any new behavior
  • Docs PR for any user-facing impact
  • Spec PR for any new API feature
  • Conformance test for any change to the spec

Release Note

Docs

@knative-prow knative-prow bot requested review from creydr and matzew July 31, 2024 21:21
@knative-prow knative-prow bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jul 31, 2024
@knative-prow Knative Prow
Copy link

knative-prow bot commented Jul 31, 2024

Hi @7h3-3mp7y-m4n. Thanks for your PR.

I'm waiting for a knative member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

creydr
creydr reviewed Aug 6, 2024
View reviewed changes
Copy link
Member

@creydr creydr left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Hello @7h3-3mp7y-m4n,
thanks for working on this. I left a comment. If you have any questions do not hesitate to reach out

pkg/channel/event_receiver.go Outdated Show resolved Hide resolved
@7h3-3mp7y-m4n
Copy link
Contributor Author

Hello @7h3-3mp7y-m4n, thanks for working on this. I left a comment. If you have any questions do not hesitate to reach out

Thanks for helping me. I'll reach out to you if I get stuck in some trouble

@knative-prow Knative Prow
Copy link

knative-prow bot commented Aug 11, 2024

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: 7h3-3mp7y-m4n
Once this PR has been reviewed and has the lgtm label, please assign leo6leo for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

creydr
creydr requested changes Aug 12, 2024
View reviewed changes
Copy link
Member

@creydr creydr left a comment

Choose a reason for hiding this comment

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

Hello @7h3-3mp7y-m4n,
thanks for the update on this. This looks better. Now you need to declare and set the getPoliciesFor callback for the InMemoryChannel, so we have this set. Therefor I would do the following:

  1. Declare a new EventReceiverOptions function in event_receiver.go, which allows to set the getPoliciesFor callback on a EventReceiver object. (this allows us to not touch the method signature of NewEventReceiver())
  2. Add an implementation for this for the InMemoryChannel and pass it via NewFanoutEventHandler() in dispatcher/inmemorychannel.go (here and here). This "implementation" would then get an InMemoryChannel by its namespace and name and simply return its AppliedEventPoliciesStatus.

I hope this helps. If you need an my information, please let us know.

pkg/channel/event_receiver.go
Comment on lines +30 to +31
//v1 "knative.dev/eventing/pkg/client/listers/messaging/v1"

Copy link
Member

Choose a reason for hiding this comment

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

can you remove this unneeded lines?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

okay.

pkg/channel/event_receiver.go
@@ -251,19 +254,22 @@ func (r *EventReceiver) ServeHTTP(response nethttp.ResponseWriter, request *neth
response.WriteHeader(nethttp.StatusBadRequest)
return
}

polices, err := r.getPoliciesFor(channel.Namespace, channel.Name)
Copy link
Member

Choose a reason for hiding this comment

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

we need to check, that getPoliciesFor is not nil (as this is set by the outside). As said in #8123 (comment), we the could fail fast as the callback is not defined (log a error message and return with a 500 http code)

creydr
creydr reviewed Aug 12, 2024
View reviewed changes
pkg/channel/event_receiver.go
http://www.apache.org/licenses/LICENSE-2.0
http://www.apache.org/licenses/LICENSE-2.0
Copy link
Member

Choose a reason for hiding this comment

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

can you run gofmt on your PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

on it.

@creydr creydr mentioned this pull request Aug 12, 2024
Closed
5 tasks
@7h3-3mp7y-m4n
Copy link
Contributor Author

Hello @7h3-3mp7y-m4n, thanks for the update on this. This looks better. Now you need to declare and set the getPoliciesFor callback for the InMemoryChannel, so we have this set. Therefor I would do the following:

  1. Declare a new EventReceiverOptions function in event_receiver.go, which allows to set the getPoliciesFor callback on a EventReceiver object. (this allows us to not touch the method signature of NewEventReceiver())
  2. Add an implementation for this for the InMemoryChannel and pass it via NewFanoutEventHandler() in dispatcher/inmemorychannel.go (here and here). This "implementation" would then get an InMemoryChannel by its namespace and name and simply return its AppliedEventPoliciesStatus.

I hope this helps. If you need an my information, please let us know.

on it. Thanks for helping :)

@creydr
Copy link
Member

creydr commented Aug 15, 2024

@7h3-3mp7y-m4n any updates on this? Do you need any more help to get this done?
As this seems to be the last main open implementation point to get authorization done, do you have an ETA for this?

@creydr creydr mentioned this pull request Aug 16, 2024
Merged
1 task
@creydr
Copy link
Member

creydr commented Aug 16, 2024

Duplicate with #8162

@creydr creydr closed this Aug 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pierDipi pierDipi pierDipi left review comments

@creydr creydr creydr requested changes

@matzew matzew Awaiting requested review from matzew

Assignees

@creydr creydr

Labels
needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

InMemoryChannel ingress: Reject unauthorized requests
3 participants
@7h3-3mp7y-m4n @creydr @pierDipi