Removed exception in getResponseStatus() #13314

zasweq · 2020-09-29T16:35:07Z

Signed-off-by: Zach Reyes zasweq@google.com

Commit Message: Removed exception in getResponseStatus().
Additional Description: Falls within #10878. getResponseStatus() is called on the data plane in the codecs. Thus, for security reasons changed from throw Exception -> Release Assert for the function calls in codebase. For HTTP/2 codec's, changed decodeHeaders() to return a status rather than exception, which logically amounts to the same logic after the try catch around dispatch.
Risk Level: Low
Testing: N/A
Docs Changes: N/A
Release Notes: N/A

Signed-off-by: Zach <zasweq@google.com>

source/common/http/utility.cc

source/common/http/utility.h

Signed-off-by: Zach <zasweq@google.com>

yanavlasov · 2020-09-30T16:21:14Z

source/common/http/http1/codec_impl.cc

@@ -346,6 +346,8 @@ void ResponseEncoderImpl::encodeHeaders(const ResponseHeaderMap& headers, bool e

  // The contract is that client codecs must ensure that :status is present.
  ASSERT(headers.Status() != nullptr);
+  // encodeHeaders() is not supposed to throw any exceptions. This will crash on a release assert if


I think it is possible that it could throw exception and exception and not crash when processing upstream response. The exception would have been caught by the dispatch() method of the client connection. Do we have an integration test where upstream responds with a garbage status?

asraa

Thanks!

source/common/http/utility.cc

source/common/http/http1/codec_impl.cc

yanavlasov · 2020-09-30T16:31:42Z

Please add tests like ProtocolIntegrationTest.UnknownResponsecode but with status strings that are not a number or overflowing uint64_t to cover the new RELEASE_ASSERT

Signed-off-by: Zach <zasweq@google.com>

zasweq · 2020-09-30T20:33:39Z

Yan, just to officially document this, that test crashes on the release assert on this code change, and when I run that integration test against master, it also crashes. Hopefully it's caused by the mocks.

yanavlasov · 2020-10-01T00:15:47Z

Yan, just to officially document this, that test crashes on the release assert on this code change, and when I run that integration test against master, it also crashes. Hopefully it's caused by the mocks.

Yes, it is crashing in the codec used for faking the upstream server. We'll discuss tomorrow how to proceed.

…eger response status. Signed-off-by: Zach <zasweq@google.com>

Signed-off-by: Zach <zasweq@google.com>

stale · 2020-10-12T02:04:47Z

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!

Signed-off-by: Zach <zasweq@google.com>

yanavlasov · 2020-10-21T16:17:58Z

/azp run

azure-pipelines · 2020-10-21T16:18:08Z

Azure Pipelines successfully started running 1 pipeline(s).

test/integration/http2_integration_test.cc

test/integration/integration_test.cc

test/integration/http2_integration_test.cc

yanavlasov · 2020-10-21T17:10:29Z

source/common/http/utility.cc

+  return response_status_or_absl_status.value();
+}
+
+absl::StatusOr<uint64_t> Utility::getResponseStatusOr(const ResponseHeaderMap& headers) {


When this method is only called from the ConnectionImpl::ClientStreamImpl::decodeHeaders(). Given that the H/2 codec verifies validity of the Status header, the only case where this can fail is if the Status header is missing altogether.
I think we should just move the check for Status header presence into the ConnectionImpl::ClientStreamImpl::decodeHeaders() and here just ASSERT success of the absl::SimpleAtoi with the comment that invalid Status values should not get past response parsing in codec.

Interesting. I liked how this got rid of exceptions, but your suggestion is a much cleaner way to remove the exceptions. Take the two things in the if and move one thing (simpleAtoi() call) to an assert and another (status presence) to codebase. However, I do believe the codecs already account for missing status headers similarly to overflowing status, so why even add a check there? @asraa what are your thoughts

Quickly glanced around codebase, couldn't seem to find a missing status integration test? If not there (perhaps ctrl f in GitHub missed it :)) I can add one!

Out of my own interest I wrote an http2 frame method for header frames without statuses breaking the HTTP/2 requirement and wrote an integration test for it. I got "[2020-10-22 05:28:34.375][31][trace][http2] [source/common/http/http2/codec_impl.cc:715] [C2] about to recv frame type=1, flags=5
[2020-10-22 05:28:34.375][31][debug][http2] [source/common/http/http2/codec_impl.cc:890] [C2] invalid frame: Violation in HTTP messaging rule on stream 1". Thus, this is similar to overflowing statuses. Let me know if you want me to check that test in too, because it would potentially hit this line and if the codec didn't validate status presence which is what the test checked for.

Hey Yan, I discussed this with Asra in our standup. Asra wants to keep it consistent, and is indifferent to whether we ASSERT or have a check, however wants to keep it consistent, and not an assert on atoi and a check for headers, because that would be unneeded complexity by splitting them.

Ok, I understand. I think given that we also verify that missing status from the response headers is rejected, I think we could either ENVOY_BUG or RELEASE_ASSERT that status header is present.

So release assert on both conditions in the utility function?

"Both" meaning missing status and overflowed status

Hey - so just to make sure - what you want is to keep the function as is, but instead of throwing up an exception with the if clause, wrap those two booleans (headers present, atoi call okay) in a RELASE_ASSERT. Basically, since we have validated that across both codecs the codecs handle it, we can move it.

"Keep the function as is" meaning what is currently checked into master for getResponseStatus? Both utility function and not having my error handling in codecs.

yanavlasov · 2020-10-21T17:11:58Z

source/common/http/utility.cc

@@ -369,10 +370,18 @@ std::string Utility::makeSetCookieValue(const std::string& key, const std::strin
 }

 uint64_t Utility::getResponseStatus(const ResponseHeaderMap& headers) {
+  absl::StatusOr<uint64_t> response_status_or_absl_status = getResponseStatusOr(headers);


Is there a case where this function can fail parsing the Status header now that there are tests that verify that invalid Status header is rejected by both H/1 and H/2 codecs?

No, but you don't think it's worth removing exceptions (removing meaning converting to error earlier instead of in try catch block) for any future iterations, considering if we keep the exception then it'll still be there in the long run, even though currently not triggerable?

I think it is totally worth removing this exception. We have proven with the test that the code does the right thing and does not accept responses with invalid Status header. Given this I think we should just assert that the parsing of the status number was successful. I think we could use ENVOY_BUG or event RELEASE_ASSERT here, given that we have solid test.

Hey Yan, I've been thinking about this some, and I think that the problem is that getResponseStatus() is called all around the codebase, and the problem is that if we make it an assert rather than an exception for other parts of the codebase, it would introduce a point of failure out of nothing? Let me know what you're thinking :)

Signed-off-by: Zach <zasweq@google.com>

asraa · 2020-10-26T16:00:44Z

To make sure I'm understanding the whole picture, because I am confused reading the threads:

In decodeHeaders() we won't trigger an exception because we know per spec (presence and 3 digits) https://tools.ietf.org/html/rfc7540#section-8.1.2.4, H/2 parser would reject. So I think we can rely on the parser. I think we should RELEASE_ASSERT/ASSERT. I think ASSERT is okay because of the added tests. ENVOY_BUG would be okay as long as we can continue to proceed, but because we have tests, I don't know if it's worth it. The only way to mess it up would be to update/change nghttp2, which would be detected in tests before the change.
In encodeHeaders(), we won't trigger the exception with bad upstream traffic because the codecs have verified valid 3-digit status. I think we should add in protection as a later TODO in case filters misbehave Gracefully handle removal of critical headers by filters #13756

Signed-off-by: Zach <zasweq@google.com>

yanavlasov · 2020-10-27T14:58:25Z

Sorry about the confusion. I was thinking more about this case. It is a bit of a mess.
This error handling was put in place to handle the case of an encoder filter removing or setting invalid ":status" header. It "worked" for the old codecs that used exceptions, since filter's chain was invoked under the dispatch context and this CodecClientException would have been caught in the CodecClient::onData method. It would have caused the stack unwind in the codec's C code which is bad.
In the new exception free codec this exception will cause process termination, since there is no catch any more.
I suppose it is better to leave the exception there (instead of ASSERT or RELEASE_ASSERT), since if someone happens to run buggy filter that removes ":status" they can revert to the old codec and avoid the crash. So i think the current state of this PR is the best.
I think the exception in the getResponseStatus() can only be removed after issue #13756 is fixed. And we should fix it before the old codec is removed completely.

yanavlasov · 2020-10-27T16:38:49Z

Faulty filters will be handled in PR #13470 after which the exception can be removed.
I suggest to just wait for the PR #13470 to land and then replace the exception with the RELEASE_ASSERT with appropriate comments. Do you think you have time to do it? Or would you rather just commit tests and then the exception removal would be completed by someone else?

zasweq · 2020-10-27T17:55:34Z

Hey Yan, thanks for taking a look at this. Weirdly, I forgot to check in the utility source file where I switched it to an ASSERT. Perhaps we can just check in the tests for now, since switching to an ASSERT at a later time would be a one line change :).

* master: (83 commits) tls: Typesafe tls slots (envoyproxy#13789) docs(example): Correct URL for caching example page (envoyproxy#13810) [fuzz] Made health check fuzz more efficient (envoyproxy#13747) rtds: properly scope rtds stats (envoyproxy#13764) http: fixing a bug with IPv6 hosts (envoyproxy#13798) connection: Remember transport socket read resumption requests and replay them when re-enabling read. (envoyproxy#13772) network: adding some accessors for ALPN work. (envoyproxy#13785) docs: added a step about how to handle platform specific extensions (envoyproxy#13759) Fix identation in ip transparency code snippet (envoyproxy#13743) wasm: enable WAVM's stack unwinding feature (envoyproxy#13792) log: set route name for direct response (envoyproxy#13683) Use nghttp2 as external dependsncy in protocol_constraints_lib (envoyproxy#13763) [Windows] Update windows dev docs (envoyproxy#13741) cel: patch thread safety issue (envoyproxy#13739) Windows: Fix ssl_socket_test (envoyproxy#13264) apple dns: add fake api test suite (envoyproxy#13780) overload: scale selected timers in response to load (envoyproxy#13475) examples: Add dynamic configuration (control plane) sandbox (envoyproxy#13746) Removed exception in getResponseStatus() (envoyproxy#13314) network: add timeout for transport connect (envoyproxy#13610) ... Signed-off-by: Michael Puncel <mpuncel@squareup.com>

Removed exception in getResponseStatus()

6c1b18e

Signed-off-by: Zach <zasweq@google.com>

akonradi suggested changes Sep 29, 2020

View reviewed changes

source/common/http/utility.cc Outdated Show resolved Hide resolved

source/common/http/utility.h Outdated Show resolved Hide resolved

mattklein123 assigned yanavlasov Sep 30, 2020

Responded to Alex's comments

5d54f1e

Signed-off-by: Zach <zasweq@google.com>

yanavlasov requested changes Sep 30, 2020

View reviewed changes

asraa reviewed Sep 30, 2020

View reviewed changes

source/common/http/utility.cc Outdated Show resolved Hide resolved

source/common/http/http1/codec_impl.cc Outdated Show resolved Hide resolved

Added crashing integration test and responded to Asra's comments

b4564b2

Signed-off-by: Zach <zasweq@google.com>

zasweq added 2 commits October 1, 2020 16:33

Changed back to exception. Added integration test for overflowing int…

1d723cf

…eger response status. Signed-off-by: Zach <zasweq@google.com>

Save progress

0900801

Signed-off-by: Zach <zasweq@google.com>

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

Dump of work

582a966

Signed-off-by: Zach <zasweq@google.com>

stale bot removed the stale stalebot believes this issue/PR has not been touched recently label Oct 19, 2020

zasweq added 2 commits October 19, 2020 04:26

Removed protocol integration test

dcfddd3

Signed-off-by: Zach <zasweq@google.com>

Added integration test for overflowing status in h2

5f345b3

Signed-off-by: Zach <zasweq@google.com>

yanavlasov requested changes Oct 21, 2020

View reviewed changes

zasweq added 3 commits October 23, 2020 17:55

Save progress

f34ac08

Signed-off-by: Zach <zasweq@google.com>

Merge branch 'master' into http-util-remove-exceptions

5d49ae6

Overflowing and missing status added to protocol integration

0e260cd

Signed-off-by: Zach <zasweq@google.com>

zasweq added 2 commits October 26, 2020 18:13

Merge branch 'master' into http-util-remove-exceptions

ebd0f1a

Switched from exception to ASSERT

8f2e640

Signed-off-by: Zach <zasweq@google.com>

yanavlasov approved these changes Oct 27, 2020

View reviewed changes

yanavlasov merged commit b9ed0a9 into envoyproxy:master Oct 28, 2020

zasweq mentioned this pull request Oct 28, 2020

[fuzz] Added status frame support to h2 fuzz test #13807

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Removed exception in getResponseStatus() #13314

Removed exception in getResponseStatus() #13314

zasweq commented Sep 29, 2020

yanavlasov Sep 30, 2020

asraa left a comment

yanavlasov commented Sep 30, 2020

zasweq commented Sep 30, 2020

yanavlasov commented Oct 1, 2020

stale bot commented Oct 12, 2020

yanavlasov commented Oct 21, 2020

azure-pipelines bot commented Oct 21, 2020

yanavlasov Oct 21, 2020

zasweq Oct 22, 2020

zasweq Oct 22, 2020

zasweq Oct 22, 2020

zasweq Oct 23, 2020

yanavlasov Oct 23, 2020

zasweq Oct 23, 2020

zasweq Oct 23, 2020

zasweq Oct 23, 2020

zasweq Oct 23, 2020

yanavlasov Oct 21, 2020

zasweq Oct 22, 2020

yanavlasov Oct 23, 2020

zasweq Oct 24, 2020

asraa commented Oct 26, 2020

yanavlasov commented Oct 27, 2020

yanavlasov commented Oct 27, 2020

zasweq commented Oct 27, 2020

Removed exception in getResponseStatus() #13314

Removed exception in getResponseStatus() #13314

Conversation

zasweq commented Sep 29, 2020

Choose a reason for hiding this comment

asraa left a comment

Choose a reason for hiding this comment

yanavlasov commented Sep 30, 2020

zasweq commented Sep 30, 2020

yanavlasov commented Oct 1, 2020

stale bot commented Oct 12, 2020

yanavlasov commented Oct 21, 2020

azure-pipelines bot commented Oct 21, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asraa commented Oct 26, 2020

yanavlasov commented Oct 27, 2020

yanavlasov commented Oct 27, 2020

zasweq commented Oct 27, 2020