-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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.
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
oauth2 filter: Make OAuth scopes configurable. #14034
Changes from 29 commits
80acc89
c276e54
d26dd3a
1577ed6
47f1fa4
906a91f
94fa8b1
2fdf64a
7c689b9
6549060
5e709f9
c2567ec
657e21f
a7acd87
5381f74
4d9eab9
5e126b9
b64e322
7359ad3
62f1ff4
b3c2071
a18a184
2aa7274
a097339
47598a2
6cca203
a5d64d0
5aef557
b956a6e
e8393a7
feb36f7
54af113
8d0ece6
8795f12
990899d
f769059
b0b90ad
6cd68d5
d01b49e
bbfe4fb
96156bc
86fbfae
aa78e42
6bf0a56
81b02b6
854b797
36fabd0
9576515
4cd22a2
5dd7bd8
8771252
b89bcae
7c7fb6c
fb4a4b7
85a911e
7e6b6c7
a52935a
0026696
b1d1fc0
c72c15c
783b773
21a81cf
22a820b
e73172e
e0a80c4
723475a
ab6e2ff
ca66519
51185ab
59945aa
ce172b7
40aaf60
c8d39ea
446df5e
57cfd34
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -48,7 +48,7 @@ constexpr const char* CookieTailHttpOnlyFormatString = | |
";version=1;path=/;Max-Age={};secure;HttpOnly"; | ||
|
||
const char* AuthorizationEndpointFormat = | ||
"{}?client_id={}&scope=user&response_type=code&redirect_uri={}&state={}"; | ||
"{}?client_id={}&scope={}&response_type=code&redirect_uri={}&state={}"; | ||
|
||
constexpr absl::string_view UnauthorizedBodyMessage = "OAuth flow failed."; | ||
|
||
|
@@ -61,6 +61,8 @@ constexpr absl::string_view REDIRECT_LOGGED_IN = "oauth.logged_in"; | |
constexpr absl::string_view REDIRECT_FOR_CREDENTIALS = "oauth.missing_credentials"; | ||
constexpr absl::string_view SIGN_OUT = "oauth.sign_out"; | ||
|
||
const std::string& DEFAULT_AUTH_SCOPE = "user"; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd make this absl::string_view for consistency with the others unless you need this to be a std::string |
||
|
||
template <class T> | ||
std::vector<Http::HeaderUtility::HeaderData> headerMatchers(const T& matcher_protos) { | ||
std::vector<Http::HeaderUtility::HeaderData> matchers; | ||
|
@@ -90,6 +92,8 @@ FilterConfig::FilterConfig( | |
redirect_matcher_(proto_config.redirect_path_matcher()), | ||
signout_path_(proto_config.signout_path()), secret_reader_(secret_reader), | ||
stats_(FilterConfig::generateStats(stats_prefix, scope)), | ||
auth_scopes_((!proto_config.auth_scopes().empty()) ? proto_config.auth_scopes() | ||
: DEFAULT_AUTH_SCOPE), | ||
forward_bearer_token_(proto_config.forward_bearer_token()), | ||
pass_through_header_matchers_(headerMatchers(proto_config.pass_through_matcher())) { | ||
if (!cluster_manager.get(oauth_token_endpoint_.cluster())) { | ||
|
@@ -275,9 +279,9 @@ Http::FilterHeadersStatus OAuth2Filter::decodeHeaders(Http::RequestHeaderMap& he | |
const std::string escaped_redirect_uri = | ||
Http::Utility::PercentEncoding::encode(redirect_uri, ":/=&?"); | ||
|
||
const std::string new_url = | ||
fmt::format(AuthorizationEndpointFormat, config_->authorizationEndpoint(), | ||
config_->clientId(), escaped_redirect_uri, escaped_state); | ||
const std::string new_url = fmt::format( | ||
AuthorizationEndpointFormat, config_->authorizationEndpoint(), config_->clientId(), | ||
config_->authScopes(), escaped_redirect_uri, escaped_state); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does the auth scope need to be URL encoded here or somewhere else? |
||
response_headers->setLocation(new_url); | ||
decoder_callbacks_->encodeHeaders(std::move(response_headers), true, REDIRECT_FOR_CREDENTIALS); | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why space separate rather than make this a
repeated string
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@htuch basically my lack of knowledge of how to handle this in proto. And also because I've been seen other OAuth solutions that use space separate as a solution, so I had that in mind.
If you want to request the change that's fine, I'll work on that. Any guidance would be much appreciated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this betrays my lack of knowledge on the OAuth space; generally in proto we prefer structure, but if there is some standard format then this is OK. CC @snowp
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please add the default value ("user") in the comment?
Can this be an empty string?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@adisuissa I've just added the default value in the comment, thanks for your suggestion
if the property is not provided, it will be compared to an empty string in the FilterConfig getter property. Then it will default to 'user' in this case since 'scope' is a mandatory parameter for OAuth
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm working to implement
repeated string auth_scopes = 9;
, but having issues with the generated cpp code. Looks like the accessors described in https://developers.google.com/protocol-buffers/docs/reference/cpp-generated#repeatedstring are not available.Posted a question in Slack's #envoy-users channel, if anyone have time to check it I'd appreciate
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You probably need to re-run
./tools/proto_format/proto_format.sh fix
as the actual protos are read from the "generated_api_shadow" directory (see here).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of special casing 'scope', would it make sense to make this:
map<string, string> additional_authorize_parameters = 9 ?
This would allow passing other parameters (such as audience) using the same mechanism
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @adisuissa, this fixed the issue and now the code is compiling.
But now I'm facing an error to run automated tests, even in the upstream master branch or in my feature branch after rebasing it (this was not happening before).
This error is thrown in both master as well, without any changes added: ERROR: /source/test/mocks/server/BUILD:234:14: C++ compilation of rule
'//test/mocks/server:transport_socket_factory_context_mocks' failed (Exit 1) gcc failed: error executing command /usr/bin/gcc -U_FORTIFY_SOURCE -fstack-protector -Wall -Wunused-but-set-parameter -Wno-free-nonheap-object -fno-omit-frame-pointer '-std=c++0x' -MD -MF ... (remaining 580 argument(s) skipped).
I'm running the tests for oauth filter only as follows:
./ci/run_envoy_docker.sh 'bazel test test/extensions/filters/http/oauth2/...'.
The build runs fine:
./ci/run_envoy_docker.sh 'bazel build source/extensions/filters/http/oauth2/...'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've just updated to the latest master and tried
./ci/run_envoy_docker.sh 'bazel test test/extensions/filters/http/oauth2/...'
(without your changes) and it works.I don't see the entire error from your comment. Would you be able to copy-paste it to a slack chat (my user is "Adi Peleg" in the Envoy slack)?