-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Enable ESP to invoke Firebase Security rules. #54
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
Changes from 1 commit
348f061
6339697
13caf68
b0e48c7
6f10a20
c6ae0fa
01b50bf
1a6704c
6351cfa
32d0023
75d6e7a
b573770
9887846
7a2abe7
b871e40
71c0d92
50955e3
35a61df
4ef505a
9dddcac
c35348d
4ccc606
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -40,6 +40,8 @@ struct UserInfo { | |
| // Authorized party of the incoming JWT. | ||
| // See http://openid.net/specs/openid-connect-core-1_0.html#IDToken | ||
| std::string authorized_party; | ||
| // String of claims | ||
| char *claims; | ||
|
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. Can we make it std::string?
Contributor
Author
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. Done. |
||
|
|
||
| // Returns audiences as a comma separated strings. | ||
| std::string AudiencesAsString() const { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -213,7 +213,9 @@ void AuthChecker::LookupJwtCache() { | |
| if (cache_hit) { | ||
| CheckAudience(true); | ||
| } else { | ||
| env_->LogInfo("Before Parse JWT"); | ||
|
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. Remember remove these logs before submit.
Contributor
Author
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. Done
Contributor
Author
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. Done |
||
| ParseJwt(); | ||
| env_->LogInfo("After Parse JWT"); | ||
|
Contributor
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. remove logging?
Contributor
Author
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. Done
Contributor
Author
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. Looks like the merge put them back in. I will delete them before the next commit. 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. Remove these logs before submit.
Contributor
Author
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. Done |
||
| } | ||
| } | ||
|
|
||
|
|
@@ -243,6 +245,8 @@ void AuthChecker::CheckAudience(bool cache_hit) { | |
| context_->set_auth_audience(audience); | ||
| context_->set_auth_authorized_party(user_info_.authorized_party); | ||
|
|
||
| context_->set_auth_claims(user_info_.claims); | ||
|
|
||
| // Remove http/s header and trailing '/' for issuer. | ||
| std::string issuer = utils::GetUrlContent(user_info_.issuer); | ||
| if (!context_->method()->isIssuerAllowed(issuer)) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -14,92 +14,271 @@ | |
| // | ||
| //////////////////////////////////////////////////////////////////////////////// | ||
| #include "contrib/endpoints/src/api_manager/check_security_rules.h" | ||
| #include "contrib/endpoints/src/api_manager/auth/lib/json_util.h" | ||
| #include <sstream> | ||
| #include <iostream> | ||
|
|
||
| #include <string> | ||
|
|
||
| #include "contrib/endpoints/include/api_manager/api_manager.h" | ||
| #include "contrib/endpoints/include/api_manager/request.h" | ||
|
|
||
| using ::google::api_manager::auth::GetProperty; | ||
| using ::google::api_manager::auth::GetStringValue; | ||
| using ::google::api_manager::utils::Status; | ||
| using ::google::protobuf::util::error::Code; | ||
|
|
||
| namespace google { | ||
| namespace api_manager { | ||
|
|
||
| namespace { | ||
|
|
||
| const char kFirebaseServerStaging[] = | ||
|
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. The whole class should be an anonymous namespace. You should not remove it.
Contributor
Author
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. done. |
||
| "https://staging-firebaserules.sandbox.googleapis.com/"; | ||
|
|
||
| // An AuthzChecker object is created for every incoming request. It does | ||
| // authorizaiton by calling Firebase Rules service. | ||
| class AuthzChecker : public std::enable_shared_from_this<AuthzChecker> { | ||
| public: | ||
| AuthzChecker(std::shared_ptr<context::RequestContext> context, | ||
| std::function<void(Status status)> continuation); | ||
| const char kFirebaseService[] = | ||
|
Contributor
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. where are these two constants used?
Contributor
Author
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 will delete them once i have done more testing. |
||
| "/google.firebase.rules.v1.FirebaseRulesService"; | ||
|
|
||
|
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. This comment applies to the class. Why do you remove it?
Contributor
Author
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. Added it back. Initially I had code that was creating the object in the service context but this did not work due to circular dependences. I removed it as a part of that change and forgot to add it back. |
||
| const char kFailedFirebaseReleaseFetch[] = "Failed to fetch Firebase Release"; | ||
| const char kFailedFirebaseTest[] = "Failed to execute Firebase Test"; | ||
| const char kInvalidResponse[] = "Invalid JSON response from Firebase Service"; | ||
|
|
||
|
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. If AuthzChecker class is used only in check_seucurity_rules.cc/h, we prefer to put the class definition in .cc file. This way, any place that "include" check_security_rules.h will only include CheckSecurityRules() function definition (not AuthzChecker class).
Contributor
Author
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. Done. |
||
| AuthzChecker::AuthzChecker(ApiManagerEnvInterface *env, | ||
| auth::ServiceAccountToken *sa_token) | ||
| : env_(env), | ||
| sa_token_(sa_token) {} | ||
|
|
||
| void AuthzChecker::Check( | ||
| std::shared_ptr<context::RequestContext> context, | ||
| std::function<void(Status status)> final_continuation) { | ||
| // TODO: Check service config to see if "useSecurityRules" is specified. | ||
| // If so, call Firebase Rules service TestRuleset API. | ||
|
|
||
| void Check(); | ||
| if (!context->service_context()->RequireAuth() || | ||
| context->method() == nullptr || !context->method()->auth()) { | ||
| env_->LogDebug( | ||
| std::string("Autherization and JWT validation was not performed") | ||
| + " skipping authorization"); | ||
| final_continuation(Status::OK); | ||
| return; | ||
| } | ||
|
|
||
| private: | ||
| // Helper function to send a http GET request. | ||
| void HttpFetch(const std::string &url, const std::string &request_body, | ||
| std::function<void(Status, std::string &&)> continuation); | ||
| // Fetch the Release attributes. | ||
| auto pChecker = GetPtr(); | ||
|
Contributor
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. Google c++ style guide, not to use camel case in variable name
Contributor
Author
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. Hmm. This code is again similar to check_auth.cc below. Let me know if you want me to fix it in both places. void AuthChecker::DiscoverJwksUri(const std::string &url) {
Contributor
Author
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. Done |
||
| HttpFetch(GetReleaseUrl(context), std::string("GET"), std::string(""), | ||
|
Contributor
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. no need for std::string("GET"), just "GET", compiler will convert it
Contributor
Author
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. done. |
||
| [context, final_continuation, pChecker] (Status status, | ||
| std::string &&body) { | ||
| std::string ruleset_id; | ||
| if (status.ok()) { | ||
| pChecker->env_->LogDebug( | ||
| std::string("GetReleasName succeeded with ") + body); | ||
| std::tie(status, ruleset_id) = pChecker->ParseReleaseResponse(&body); | ||
| } else { | ||
| pChecker->env_->LogError(std::string("GetReleaseName for ") | ||
| + pChecker->GetReleaseUrl(context) | ||
| + " with status " + status.ToString()); | ||
| status = Status(Code::INTERNAL, kFailedFirebaseReleaseFetch); | ||
| } | ||
|
|
||
| // Get Auth token for accessing Firebase Rules service. | ||
| const std::string &GetAuthToken(); | ||
| // If the parsing of the release body is successful, then call the | ||
| // Test Api for firebase rules service. | ||
| if (status.ok()) { | ||
| pChecker->CheckAuth(ruleset_id, context, final_continuation); | ||
| } else { | ||
| final_continuation(status); | ||
| } | ||
| }); | ||
| } | ||
|
|
||
| // Request context. | ||
| std::shared_ptr<context::RequestContext> context_; | ||
| void AuthzChecker::CheckAuth( | ||
| std::string ruleset_id, | ||
| std::shared_ptr<context::RequestContext> context, | ||
| std::function<void(Status status)> continuation) { | ||
| auto pChecker = GetPtr(); | ||
|
|
||
| // Pointer to access ESP running environment. | ||
| ApiManagerEnvInterface *env_; | ||
| HttpFetch(std::string(kFirebaseServerStaging) + "v1/" + ruleset_id + | ||
|
Contributor
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. use a separate function for URL |
||
| ":test?alt=json", | ||
| std::string("POST"), BuildTestRequestBody(context), | ||
| [context, continuation, pChecker, ruleset_id] | ||
| (Status status, std::string &&body) { | ||
|
|
||
| // The final continuation function. | ||
| std::function<void(Status status)> on_done_; | ||
| }; | ||
| if (status.ok()) { | ||
| pChecker->env_->LogDebug( | ||
| std::string("Test API succeeded with ") + body); | ||
| status = pChecker->ParseTestResponse(context, &body); | ||
| } else { | ||
| pChecker->env_->LogError(std::string("Test API failed with ") | ||
| + status.ToString()); | ||
| status = Status(Code::INTERNAL, kFailedFirebaseTest); | ||
| } | ||
|
|
||
| AuthzChecker::AuthzChecker(std::shared_ptr<context::RequestContext> context, | ||
| std::function<void(Status status)> continuation) | ||
| : context_(context), | ||
| env_(context_->service_context()->env()), | ||
| on_done_(continuation) {} | ||
| continuation(status); | ||
| }); | ||
| } | ||
|
|
||
| void AuthzChecker::Check() { | ||
| // TODO: Check service config to see if "useSecurityRules" is specified. | ||
| // If so, call Firebase Rules service TestRuleset API. | ||
| std::pair<Status, std::string> AuthzChecker::ParseReleaseResponse( | ||
|
Contributor
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. can we pass out ruleset_id in the argument. e.g.
Contributor
Author
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. Done.
Contributor
Author
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. Done |
||
| std::string *json_str) { | ||
|
Contributor
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. pass in const std::string& if you are not modifying json_str
Contributor
Author
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. Done |
||
| grpc_json *json = grpc_json_parse_string_with_len( | ||
| const_cast<char *>(json_str->data()), json_str->length()); | ||
|
|
||
| if (!json) { | ||
| return std::make_pair(Status(Code::INVALID_ARGUMENT, kInvalidResponse), | ||
| ""); | ||
| } | ||
|
|
||
| const char *ruleset_id = GetStringValue(json, "rulesetName"); | ||
| std::pair<Status, std::string> result = std::make_pair(Status::OK, | ||
| ruleset_id); | ||
| if (ruleset_id == nullptr) { | ||
| env_->LogError("Empty ruleset Id received from firebase service"); | ||
| result =std::make_pair(Status(Code::INTERNAL, kInvalidResponse), ""); | ||
| } else { | ||
| env_->LogInfo(std::string("Received ruleset Id: ") + ruleset_id); | ||
| } | ||
|
|
||
| grpc_json_destroy(json); | ||
| return result; | ||
| } | ||
|
|
||
| Status AuthzChecker::ParseTestResponse( | ||
| std::shared_ptr<context::RequestContext> context, | ||
| std::string *json_str) { | ||
| grpc_json *json = grpc_json_parse_string_with_len( | ||
| const_cast<char *>(json_str->data()), json_str->length()); | ||
|
|
||
|
|
||
| if (!json) { | ||
| return Status(Code::INVALID_ARGUMENT, | ||
| "Invalid JSON response from Firebase Service"); | ||
| } | ||
|
|
||
| Status status = Status::OK; | ||
| Status invalid = Status(Code::INTERNAL, kInvalidResponse); | ||
|
|
||
| const grpc_json *testResults = GetProperty(json, "testResults"); | ||
| if (testResults == nullptr) { | ||
| env_->LogError("TestResults are null"); | ||
| status = invalid; | ||
| } else { | ||
| const char *result = GetStringValue(testResults->child, "state"); | ||
| if (result == nullptr) { | ||
| env_->LogInfo("Result state is empty"); | ||
| status = invalid; | ||
| } else if (std::string(result) != "SUCCESS") { | ||
|
Contributor
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.
Contributor
Author
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 have declared a const char[] kTestSuccess with the rest of the cont strings. Let me know if this is not what you had in mind. |
||
| status = Status(Code::PERMISSION_DENIED, | ||
| std::string("Unauthorized ") | ||
| + context->request()->GetRequestHTTPMethod() | ||
| + " access to resource " + context->request()->GetRequestPath(), | ||
|
Contributor
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 response have detail info we can pass back to help users?
Contributor
Author
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. No. Unfortunatley, the Test API that we use only provided with a response that looks like this: { or { The response itself is a HTTP 200 OK message. |
||
| Status::AUTH); | ||
| } | ||
| } | ||
|
|
||
| grpc_json_destroy(json); | ||
| return status; | ||
| } | ||
|
|
||
| const std::string &AuthzChecker::GetAuthToken() { | ||
| // TODO: Get Auth token for accessing Firebase Rules service. | ||
| static std::string empty; | ||
| return empty; | ||
| std::string AuthzChecker::GetOperation(std::string httpMethod) { | ||
|
Contributor
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. need to be a class member function?
Contributor
Author
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. Made it static function.
Contributor
Author
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. Made it static. |
||
| if (httpMethod == "POST") { | ||
| return std::string("create"); | ||
| } | ||
|
|
||
| if (httpMethod == "GET" || httpMethod == "HEAD" || httpMethod == "OPTIONS") { | ||
| return std::string("get"); | ||
| } | ||
|
|
||
| if (httpMethod == "DELETE") { | ||
| return std::string("delete"); | ||
| } | ||
|
|
||
| return std::string("update"); | ||
| } | ||
|
|
||
| std::string AuthzChecker::BuildTestRequestBody( | ||
| std::shared_ptr<context::RequestContext> context) { | ||
|
|
||
| std::shared_ptr<std::ostringstream> ss(new std::ostringstream); | ||
|
Contributor
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. not need to use new. just allocate it from stack.
Contributor
Author
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. Done.
Contributor
Author
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. Done |
||
|
|
||
| int openCount = 0; | ||
| *(ss.get()) << "{" | ||
| << Stringify("test_cases") + ": " | ||
| << "[ {"; | ||
| ++openCount; | ||
| AddToBody("service_name", context->service_context()->service_name(), false, ss); | ||
| AddToBody("resource_path", context->request()->GetRequestPath(), | ||
| false, ss); | ||
| AddToBody("operation", GetOperation(context->request()->GetRequestHTTPMethod()), | ||
| false, ss); | ||
| AddToBody("expectation", "ALLOW", false, ss); | ||
| AddToBody("variables", ss); | ||
| ++openCount; | ||
| AddToBody("request", ss); | ||
| ++openCount; | ||
| AddToBody("auth", ss); | ||
| ++openCount; | ||
|
|
||
| *(ss.get()) << Stringify("token") + ": " << context->auth_claims(); | ||
|
|
||
| while(openCount-- > 0) { | ||
| *(ss.get()) << "} "; | ||
| } | ||
|
|
||
| *(ss.get()) << "] }"; | ||
| return ss->str(); | ||
| } | ||
|
|
||
| void AuthzChecker::AddToBody(const std::string &key, | ||
| std::shared_ptr<std::ostringstream> ss) { | ||
| *(ss.get()) << Stringify(key.c_str()) + ": " + "{"; | ||
| } | ||
| void AuthzChecker::AddToBody(const std::string &key, const std::string &value, | ||
| bool end, std::shared_ptr<std::ostringstream> ss) { | ||
| *(ss.get()) << Stringify(key.c_str()) + ": " + Stringify(value.c_str()); | ||
| if (!end) { | ||
| *(ss.get()) << ", "; | ||
| } | ||
| } | ||
|
|
||
| const std::string AuthzChecker::GetReleaseName( | ||
|
Contributor
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. no need to be a class member function
Contributor
Author
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. Same reason as above. There is no meaning to this method outside the context of the class.
Contributor
Author
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. Changed this to static |
||
| std::shared_ptr<context::RequestContext> request_context) { | ||
| return request_context->service_context()->service_name() + ":" | ||
| + request_context->service_context()->service().apis(0).version(); | ||
| } | ||
|
|
||
| const std::string AuthzChecker::GetReleaseUrl( | ||
| std::shared_ptr<context::RequestContext> request_context) { | ||
| return std::string(kFirebaseServerStaging) + "v1/projects/" | ||
| + request_context->service_context()->project_id() + "/releases/" | ||
| + GetReleaseName(request_context); | ||
| } | ||
|
|
||
| void AuthzChecker::HttpFetch( | ||
| const std::string &url, const std::string &request_body, | ||
| const std::string &url, const std::string &method, | ||
| const std::string &request_body, | ||
| std::function<void(Status, std::string &&)> continuation) { | ||
|
|
||
| env_->LogInfo(std::string("Issue HTTP Request to url :") + url + | ||
| " method : " + method + " body: " + request_body); | ||
|
|
||
| std::unique_ptr<HTTPRequest> request(new HTTPRequest([continuation]( | ||
| Status status, std::map<std::string, std::string> &&, | ||
| std::string &&body) { continuation(status, std::move(body)); })); | ||
|
|
||
| if (!request) { | ||
| continuation(Status(Code::INTERNAL, "Out of memory"), ""); | ||
| return; | ||
| } | ||
|
|
||
| request->set_method("POST") | ||
| std::string token = GetAuthToken(); | ||
| request->set_method(method) | ||
| .set_url(url) | ||
| .set_auth_token(GetAuthToken()) | ||
| .set_header("Content-Type", "application/json") | ||
| .set_auth_token(token); | ||
|
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. set_auth_token(GetAuthToken()). Then you don't need "token" variable.
Contributor
Author
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. Done. |
||
| if (method != "GET") { | ||
| request->set_header("Content-Type", "application/json") | ||
| .set_body(request_body); | ||
| } | ||
|
|
||
| env_->RunHTTPRequest(std::move(request)); | ||
| } | ||
|
|
||
| } // namespace | ||
|
|
||
| void CheckSecurityRules(std::shared_ptr<context::RequestContext> context, | ||
| std::function<void(Status status)> continuation) { | ||
| std::shared_ptr<AuthzChecker> authzChecker = | ||
| std::make_shared<AuthzChecker>(context, continuation); | ||
| authzChecker->Check(); | ||
| std::shared_ptr<AuthzChecker> checker(new AuthzChecker( | ||
|
Contributor
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. why don't you use make_shared?
Contributor
Author
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. Done |
||
| context->service_context()->env(), | ||
| context->service_context()->service_account_token())); | ||
| checker->Check(context, continuation); | ||
| } | ||
|
|
||
| } // namespace api_manager | ||
|
|
||
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.
do we need this one?
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.
Yes. We do. The UserInfo is where the decoded JWT claims are stored so it can be used in the later part of the work flow. This actually is the same flow as it is used for JWT audience and issuer fields.
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.
Changed it to std::string