ext_authz: support for buffering request body by gsagula · Pull Request #5824 · envoyproxy/envoy

gsagula · 2019-02-04T08:53:59Z

This PR adds support to ext_authz filter for buffering the request data. This is useful when the authorization server needs to check the request body, e.g. HMAC validation.

Fixes #5676

Risk Level: low
Testing: unit
Docs Changes: yes
Release Notes: yes

Signed-off-by: Gabriel <gsagula@gmail.com>

htuch · 2019-02-04T20:43:12Z

Please merge master to pick up #5827.

Signed-off-by: Gabriel <gsagula@gmail.com>

stale · 2019-02-12T03:15:22Z

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

lizan

Sorry for the delay, @dio is sick.

source/extensions/filters/http/ext_authz/ext_authz.cc

api/envoy/config/filter/http/ext_authz/v2/ext_authz.proto

api/envoy/service/auth/v2/attribute_context.proto

source/extensions/filters/common/ext_authz/ext_authz_http_impl.cc

api/envoy/config/filter/http/ext_authz/v2/ext_authz.proto

gsagula · 2019-02-15T18:24:51Z

@lizan Thank you for looking at the PR. I had to work on some other things, but I will work on this by next week.

stale · 2019-02-22T19:01:58Z

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

gsagula · 2019-02-22T21:06:01Z

Sorry, another busy week. I will take a look at this later today or Monday.

Signed-off-by: Gabriel <gsagula@gmail.com>

gsagula · 2019-03-07T17:28:16Z

@lizan I've addressed all your comments. Thanks for reviewing it.

gsagula · 2019-03-07T17:30:54Z

source/extensions/filters/common/ext_authz/check_request_utils.cc

+  const Buffer::Instance* buffer = sdfc->decodingBuffer();
+  if (max_request_bytes > 0 && buffer != nullptr) {
+    const uint64_t len = std::min(buffer->length(), max_request_bytes);
+    std::unique_ptr<char[]> data(new char[len]);


I think I could use an std::string and then just move it. Not sure.

yeah I think you can do that.

Signed-off-by: Gabriel <gsagula@gmail.com>

dio

Thanks for this. I think @lizan has gone through the critical bits. Mine is only small details.

docs/root/intro/version_history.rst

source/extensions/filters/common/ext_authz/ext_authz_http_impl.cc

Signed-off-by: Gabriel <gsagula@gmail.com>

…fered_authz Signed-off-by: Gabriel <gsagula@gmail.com>

gsagula · 2019-03-22T07:57:28Z

@lizan @dio I think I addressed all your comments. Thank you for reviewing it.

brectanus-sigsci · 2019-03-25T17:34:08Z

@gsagula Any update/ETA on this? Seems all comments were addressed.

itstehkman · 2019-03-25T17:38:12Z

Same! I would love for this to be merged, am also waiting on it as it will help me a lot, trying to do HMAC in ext-authz :)

…fered_authz Signed-off-by: Gabriel <gsagula@gmail.com>

dio

Thanks for working on this @gsagula! Looks good.

lizan

sorry for the delay, one last question.

source/extensions/filters/http/ext_authz/ext_authz.cc

gsagula · 2019-03-26T04:14:15Z

@lizan No worries, I've been pretty busy lately too. I found a couple of issues with the partial message feature, but I need to fix the broken tests now. I will try to get it out tomorrow. Thanks!

Signed-off-by: Gabriel <gsagula@gmail.com>

…fered_authz Signed-off-by: Gabriel <gsagula@gmail.com>

Signed-off-by: Gabriel <gsagula@gmail.com>

…fered_authz Signed-off-by: Gabriel <gsagula@gmail.com>

gsagula · 2019-03-28T21:02:17Z

Build failures seem unrelated. I will merge master and retry.

…fered_authz Signed-off-by: Gabriel <gsagula@gmail.com>

lizan · 2019-03-28T23:41:31Z

source/extensions/filters/http/ext_authz/ext_authz.cc

  buffer_data_ = config_->withRequestBody() &&
-                 !(end_stream || (method && isHeaderOnlyMethod(method->value().getStringView())));
-
+                 !(end_stream || Http::Utility::isWebSocketUpgradeRequest(headers) ||


Do filters get upgrade requests? I roughly remember this is handled by HCM now? cc @alyssawilk

Apparently, they do but would be nice to hear from @alyssawilk. If this is ok, the same logic ^^ should be applied to the buffer filter.

Ah nvm I was in the old impression before we have new style websocket.

gsagula · 2019-03-28T23:43:19Z

@lizan @dio I have addressed your concerns I believe. I also added a few tests for the logic around upgrade requests and did some manual testing. Feel free to take another look. Thanks!

Gabriel added 7 commits January 25, 2019 21:44

wip - initial commit, missing tests

7c8f57f

Signed-off-by: Gabriel <gsagula@gmail.com>

wip - saving the work

34796d8

Signed-off-by: Gabriel <gsagula@gmail.com>

added more tests and improved documentation

c16ad7a

Signed-off-by: Gabriel <gsagula@gmail.com>

Merge remote-tracking branch 'upstream/master' into buffered_authz

39fd75f

Signed-off-by: Gabriel <gsagula@gmail.com>

Merge remote-tracking branch 'upstream/master' into buffered_authz

eb06a3d

Signed-off-by: Gabriel <gsagula@gmail.com>

edited comment

a57af7a

Signed-off-by: Gabriel <gsagula@gmail.com>

updated the release notes

3f17795

Signed-off-by: Gabriel <gsagula@gmail.com>

htuch requested a review from saumoh February 4, 2019 15:41

htuch assigned dio Feb 4, 2019

htuch requested a review from dio February 4, 2019 15:42

Merge remote-tracking branch 'upstream/master' into buffered_authz

432be6b

Signed-off-by: Gabriel <gsagula@gmail.com>

stale bot added the stale stalebot believes this issue/PR has not been touched recently label Feb 12, 2019

lizan reviewed Feb 12, 2019

View reviewed changes

stale bot removed the stale stalebot believes this issue/PR has not been touched recently label Feb 12, 2019

lizan added the waiting label Feb 14, 2019

stale bot added the stale stalebot believes this issue/PR has not been touched recently label Feb 22, 2019

stale bot removed the stale stalebot believes this issue/PR has not been touched recently label Feb 22, 2019

wip - fixed segfault and did most of the code review changes

812d3ff

repokitteh-read-only bot removed the waiting label Mar 1, 2019

Gabriel added 4 commits February 28, 2019 21:19

fixed spelling

e157eab

Signed-off-by: Gabriel <gsagula@gmail.com>

fixed check request test

8d747b5

Signed-off-by: Gabriel <gsagula@gmail.com>

fixed check request tests

255f624

Signed-off-by: Gabriel <gsagula@gmail.com>

Merge remote-tracking branch 'upstream/master' into buffered_authz

cf6151b

Signed-off-by: Gabriel <gsagula@gmail.com>

gsagula commented Mar 7, 2019

View reviewed changes

Gabriel added 3 commits March 15, 2019 19:05

removed the logic that copy the buffer slices

d60cd55

Signed-off-by: Gabriel <gsagula@gmail.com>

Merge remote-tracking branch 'upstream/master' into buffered_authz

d2820b9

Signed-off-by: Gabriel <gsagula@gmail.com>

changed the string initialization to the correct size

8939e7d

Signed-off-by: Gabriel <gsagula@gmail.com>

dio suggested changes Mar 18, 2019

View reviewed changes

docs/root/intro/version_history.rst Outdated Show resolved Hide resolved

docs/root/intro/version_history.rst Outdated Show resolved Hide resolved

source/extensions/filters/common/ext_authz/ext_authz_http_impl.cc Show resolved Hide resolved

Gabriel added 2 commits March 21, 2019 21:02

code review changes

075d619

Signed-off-by: Gabriel <gsagula@gmail.com>

Merge branch 'master' of https://github.com/envoyproxy/envoy into buf…

cc03037

…fered_authz Signed-off-by: Gabriel <gsagula@gmail.com>

Merge branch 'master' of https://github.com/envoyproxy/envoy into buf…

f48d0a0

…fered_authz Signed-off-by: Gabriel <gsagula@gmail.com>

dio previously approved these changes Mar 25, 2019

View reviewed changes

lizan reviewed Mar 25, 2019

View reviewed changes

source/extensions/filters/http/ext_authz/ext_authz.cc Outdated Show resolved Hide resolved

Gabriel added 4 commits March 26, 2019 20:30

replaced logic to inspect headers only reuquests

7fc5814

Signed-off-by: Gabriel <gsagula@gmail.com>

Merge remote-tracking branch 'upstream/master' into buffered_authz

e701a76

Signed-off-by: Gabriel <gsagula@gmail.com>

added tests for upgrade requests

9ea7dab

Signed-off-by: Gabriel <gsagula@gmail.com>

Merge branch 'master' of https://github.com/envoyproxy/envoy into buf…

1479c93

…fered_authz Signed-off-by: Gabriel <gsagula@gmail.com>

gsagula dismissed dio’s stale review via 1479c93 March 28, 2019 02:40

Gabriel added 4 commits March 27, 2019 20:56

improved some debug and trace log messages

426dffc

Signed-off-by: Gabriel <gsagula@gmail.com>

removed unused header

2bf9abd

Signed-off-by: Gabriel <gsagula@gmail.com>

Merge branch 'master' of https://github.com/envoyproxy/envoy into buf…

219c2ce

…fered_authz Signed-off-by: Gabriel <gsagula@gmail.com>

Merge branch 'master' of https://github.com/envoyproxy/envoy into buf…

7d9a591

…fered_authz Signed-off-by: Gabriel <gsagula@gmail.com>

Merge branch 'master' of https://github.com/envoyproxy/envoy into buf…

dff99d4

…fered_authz Signed-off-by: Gabriel <gsagula@gmail.com>

lizan reviewed Mar 28, 2019

View reviewed changes

lizan approved these changes Mar 29, 2019

View reviewed changes

lizan merged commit 075edf8 into envoyproxy:master Apr 1, 2019

brectanus-sigsci mentioned this pull request Apr 16, 2019

ext_authz: added ability to detect partial request body data #6583

Merged

jewertow mentioned this pull request Feb 28, 2024

authz: fix external authorization when maxRequestBytes is greater than 1MiB istio/istio#49452

Closed

Conversation

gsagula commented Feb 4, 2019

Uh oh!

htuch commented Feb 4, 2019

Uh oh!

stale bot commented Feb 12, 2019

Uh oh!

lizan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gsagula commented Feb 15, 2019

Uh oh!

stale bot commented Feb 22, 2019

Uh oh!

gsagula commented Feb 22, 2019

Uh oh!

gsagula commented Mar 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gsagula Mar 7, 2019

Choose a reason for hiding this comment

Uh oh!

lizan Mar 8, 2019

Choose a reason for hiding this comment

Uh oh!

dio left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gsagula commented Mar 22, 2019

Uh oh!

brectanus-sigsci commented Mar 25, 2019

Uh oh!

itstehkman commented Mar 25, 2019

Uh oh!

dio left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lizan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gsagula commented Mar 26, 2019

Uh oh!

gsagula commented Mar 28, 2019

Uh oh!

lizan Mar 28, 2019

Choose a reason for hiding this comment

Uh oh!

gsagula Mar 28, 2019

Choose a reason for hiding this comment

Uh oh!

lizan Mar 29, 2019

Choose a reason for hiding this comment

Uh oh!

gsagula commented Mar 28, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

gsagula commented Mar 7, 2019 •

edited

Loading

dio left a comment •

edited

Loading