oauth2: Fix panic when oauth upstream is unhealthy #34094
Conversation
Signed-off-by: Benedikt Werner <1benediktwerner@gmail.com>
|
Hi @benediktwerner, welcome and thank you for your contribution. We will try to review your Pull Request as quickly as possible. In the meantime, please take a look at the contribution guidelines if you have not done so already. |
| // Regression test(issue #22678) where (incorrectly)using server's init manager(initialized state) | ||
| // to add init target by the secret manager led to the assertion failure. | ||
| // Verify the behavior when the callback param is missing the state query param. | ||
| TEST_P(OauthIntegrationTest, MissingStateParam) { |
There was a problem hiding this comment.
I moved this up from below so it's grouped with the other OauthIntegrationTests instead of OauthIntegrationTestWithBasicAuth.
| @@ -263,6 +263,47 @@ TEST_F(OAuth2ClientTest, RequestAccessTokenInvalidResponse) { | |||
| [&](auto* callback) { callback->onSuccess(request, std::move(mock_response)); })); | |||
| } | |||
|
|
|||
| TEST_F(OAuth2ClientTest, RequestAccessTokenNetworkError) { | |||
There was a problem hiding this comment.
I moved this up from below so it's grouped with the other access token tests instead of the refresh token tests.
| } | ||
|
|
||
| void OAuth2ClientImpl::asyncRefreshAccessToken(const std::string& refresh_token, | ||
| const std::string& client_id, | ||
| const std::string& secret, AuthType auth_type) { | ||
| ASSERT(state_ == OAuthState::Idle); |
There was a problem hiding this comment.
Did you add, or is there an existing test for immediate response in case of refresh token request?
Signed-off-by: Benedikt Werner <1benediktwerner@gmail.com>
|
There is a build error: https://source.cloud.google.com/results/invocations/49bee7ae-a361-46e0-b9ce-9feec302bb92 /wait |
Signed-off-by: Benedikt Werner <1benediktwerner@gmail.com>
As discussed on envoy-security@googlegroups.com, Envoy currently panics when receiving an Oauth callback while the Oauth has no healthy upstream (e.g. because Envoy can't resolve its hostname), because the onSuccess callback is called immediately during the request dispatch in that case, before the oauth state is changed to pending.
The fix is to set the state before sending the request. Since the state isn't used anywhere else, there's no reason to only set it afterwards.
Unfortunately, I wasn't able to test this properly because I ran into various rather random-looking Bazel failures both building locally or using the CI Docker script (on a Mac).
Failure using the CI Docker script
``` ERROR: /build/bazel_root/base/external/envoy/bazel/foreign_cc/BUILD:11:15: no such package '@com_github_axboe_liburing//': error globbing [**] op=FILES: /build/bazel_root/base/external/com_github_axboe_liburing/man/IO_URING_VERSION_MAJOR.3 (Too many levels of symbolic links) and referenced by '@envoy//bazel/foreign_cc:liburing' ```Failure using the CI Docker script on the v1.30.1 tag
``` ERROR: /source/WORKSPACE:13:19: no such package '@v8//': error globbing [tools/testrunner/**/*.py] op=FILES: /build/bazel_root/base/external/v8/tools/testrunner/testdata/testroot6/out/build (No such file or directory) and referenced by '//external:wee8' ```Failure building directly on Mac OS
``` compiling lib/findprog-in.c... ./lib/findprog-in.c:149:25: error: call to undeclared function 'eaccess'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration] if (eaccess (progpathname, X_OK) == 0) ^ ./lib/findprog-in.c:149:25: note: did you mean 'access'? /Library/Developer/CommandLineTools/SDKs/MacOSX.sdk/usr/include/unistd.h:431:6: note: 'access' declared here int access(const char *, int); ^ ./lib/findprog-in.c:301:21: error: call to undeclared function 'eaccess'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration] if (eaccess (progpathname, X_OK) == 0) ^ 2 errors generated. Compilation failed. _____ END BUILD LOGS _____ rules_foreign_cc: Build wrapper script location: bazel-out/darwin_arm64-opt-exec-2B5CBBC6/bin/external/rules_foreign_cc/toolchains/make_tool_foreign_cc/wrapper_build_script.sh rules_foreign_cc: Build script location: bazel-out/darwin_arm64-opt-exec-2B5CBBC6/bin/external/rules_foreign_cc/toolchains/make_tool_foreign_cc/build_script.sh rules_foreign_cc: Build log location: bazel-out/darwin_arm64-opt-exec-2B5CBBC6/bin/external/rules_foreign_cc/toolchains/make_tool_foreign_cc/BootstrapGNUMake.log ```Commit Message: oauth2: Fix panic when oauth upstream is unhealthy
Risk Level: Low
Testing: unit test
Docs Changes: N/A
Release Notes: Added
Platform Specific Features: N/A