InMemoryChannel: reject request for wrong audience by Leo6Leo · Pull Request #7449 · knative/eventing

Leo6Leo · 2023-11-14T14:26:35Z

Fixes #7289

Proposed Changes

If OIDC authentication is enabled, imc will check if the audience of the received token matches the audience of the broker. If not it will respond with a 401

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

Backporting

[] Upstream
[] Midstream

Release Note

Docs

knative-prow · 2023-11-14T14:26:38Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

codecov · 2023-11-23T22:14:15Z

Codecov Report

Attention: 34 lines in your changes are missing coverage. Please review.

Comparison is base (3fcc78a) 76.70% compared to head (c01f073) 76.48%.
Report is 2 commits behind head on main.

❗ Current head c01f073 differs from pull request most recent head 27f4517. Consider uploading reports for the commit 27f4517 to get more accurate results

Files	Patch %	Lines
pkg/channel/event_receiver.go	17.14%	27 Missing and 2 partials ⚠️
...iler/inmemorychannel/dispatcher/inmemorychannel.go	66.66%	2 Missing and 1 partial ⚠️
pkg/channel/fanout/fanout_event_handler.go	80.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #7449      +/-   ##
==========================================
- Coverage   76.70%   76.48%   -0.23%     
==========================================
  Files         259      259              
  Lines       14254    14315      +61     
==========================================
+ Hits        10934    10949      +15     
- Misses       2765     2807      +42     
- Partials      555      559       +4

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Co-authored-by: Christoph Stäbler <cstabler@redhat.com>

creydr · 2023-11-24T09:01:37Z

@Cali0707 as I worked together with @Leo6Leo on this, my review is probably a bit "biased" 😁 and should be done by someone else.
Could you review this PR?

Regarding the failing TestBrokerSupportsOIDC auth test: this is because the broker ingress does not yet authenticate the requests to the underlying channel, while with this PR, the channel requires a valid token if oidc is enabled. #7473 should fix it.

Co-authored-by: Calum Murray <cmurray@redhat.com>

…ect-request-for-imc

Signed-off-by: Leo Li <leoli@redhat.com>

Leo6Leo · 2023-12-04T15:05:44Z

/cc @Cali0707 @pierDipi

Ready for review

Cali0707

Just one small nit @Leo6Leo

Signed-off-by: Leo Li <leoli@redhat.com>

Cali0707

/lgtm
/approve

Thanks @Leo6Leo !!

knative-prow · 2023-12-04T19:16:10Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Cali0707, Leo6Leo

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [Cali0707]

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

Cali0707 · 2023-12-04T19:16:47Z

/hold

In case you have any final opinions @creydr, I noticed you still have 1 change requested in the GitHub UI

creydr · 2023-12-05T07:10:34Z

Thanks @Cali0707 for checking this. My review points were addressed 👍

/unhold

Implement the auth check in the event receiver

4127b0a

knative-prow Bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 14, 2023

knative-prow Bot requested review from Cali0707 and aslom November 14, 2023 14:26

knative-prow Bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Nov 14, 2023

creydr and others added 2 commits November 23, 2023 17:08

Reject request for wrong audience in IMC event_receiver (#1)

ae495fb

Merge branch 'main' into reject-request-for-imc

a508d28

Running the rekt test

256a41d

Co-authored-by: Christoph Stäbler <cstabler@redhat.com>

knative-prow Bot added the area/test-and-release Test infrastructure, tests or release label Nov 23, 2023

Leo6Leo marked this pull request as ready for review November 24, 2023 06:10

knative-prow Bot requested review from aliok and evankanderson November 24, 2023 06:10

Cali0707 reviewed Nov 24, 2023

View reviewed changes

Comment thread pkg/channel/event_receiver.go

Comment thread pkg/channel/fanout/fanout_event_handler.go Outdated

Comment thread test/auth/oidc_test.go

Leo6Leo and others added 5 commits November 24, 2023 13:44

Merge branch 'main' into reject-request-for-imc

afae6d8

Update pkg/channel/fanout/fanout_event_handler.go

115605f

Co-authored-by: Calum Murray <cmurray@redhat.com>

Merge remote-tracking branch 'origin/reject-request-for-imc' into rej…

85f3f24

…ect-request-for-imc

Update the variable naming

095bf13

Signed-off-by: Leo Li <leoli@redhat.com>

Refactor the jwt event auth header verification

05b1eb2

Signed-off-by: Leo Li <leoli@redhat.com>

creydr requested changes Nov 27, 2023

View reviewed changes

Comment thread pkg/auth/token_verifier.go Outdated

Comment thread pkg/auth/token_verifier.go Outdated

knative-prow Bot assigned creydr Nov 27, 2023

Leo6Leo added 2 commits December 4, 2023 00:48

Merge branch 'main' into reject-request-for-imc

250b3df

Revert the refactoring

c01f073

Signed-off-by: Leo Li <leoli@redhat.com>

knative-prow Bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Dec 4, 2023

Leo6Leo changed the title ~~[WIP] InMemoryChannel: reject request for wrong audience~~ InMemoryChannel: reject request for wrong audience Dec 4, 2023

knative-prow Bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 4, 2023

knative-prow Bot requested review from Cali0707 and pierDipi December 4, 2023 15:05

Leo6Leo mentioned this pull request Dec 4, 2023

Refactor the code that rejects for wrong audience #7487

Closed

Cali0707 reviewed Dec 4, 2023

View reviewed changes

Comment thread pkg/channel/event_receiver.go Outdated

Fix the linter issue

27f4517

Signed-off-by: Leo Li <leoli@redhat.com>

Cali0707 approved these changes Dec 4, 2023

View reviewed changes

knative-prow Bot assigned Cali0707 Dec 4, 2023

knative-prow Bot added the lgtm Indicates that a PR is ready to be merged. label Dec 4, 2023

knative-prow Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 4, 2023

knative-prow Bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 4, 2023

knative-prow Bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 5, 2023

knative-prow Bot merged commit 83125a9 into knative:main Dec 5, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

InMemoryChannel: reject request for wrong audience#7449

InMemoryChannel: reject request for wrong audience#7449
knative-prow[bot] merged 12 commits into
knative:mainfrom
Leo6Leo:reject-request-for-imc

Leo6Leo commented Nov 14, 2023 •

edited

Loading

Uh oh!

knative-prow Bot commented Nov 14, 2023

Uh oh!

codecov Bot commented Nov 23, 2023 •

edited

Loading

Uh oh!

creydr commented Nov 24, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Leo6Leo commented Dec 4, 2023

Uh oh!

Cali0707 left a comment

Uh oh!

Uh oh!

Cali0707 left a comment

Uh oh!

knative-prow Bot commented Dec 4, 2023

Uh oh!

Cali0707 commented Dec 4, 2023 •

edited

Loading

Uh oh!

creydr commented Dec 5, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

Leo6Leo commented Nov 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed Changes

Pre-review Checklist

Backporting

Uh oh!

knative-prow Bot commented Nov 14, 2023

Uh oh!

codecov Bot commented Nov 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

creydr commented Nov 24, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Leo6Leo commented Dec 4, 2023

Uh oh!

Cali0707 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Cali0707 left a comment

Choose a reason for hiding this comment

Uh oh!

knative-prow Bot commented Dec 4, 2023

Uh oh!

Cali0707 commented Dec 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

creydr commented Dec 5, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Leo6Leo commented Nov 14, 2023 •

edited

Loading

codecov Bot commented Nov 23, 2023 •

edited

Loading

Cali0707 commented Dec 4, 2023 •

edited

Loading