You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@mesos.apache.org by me...@apache.org on 2016/06/24 08:24:41 UTC
[2/3] mesos git commit: Fixed failed authorization of '/files/*'
endpoints.
Fixed failed authorization of '/files/*' endpoints.
The `FilesProcess` stores an authorization callback
for every virtual path. If no callback is found when
authorizing a request, then it is assumed that path
is not authorizable, so the request is approved.
This led to a bug in which the callbacks for paths
with trailing slashes were not correctly identified,
leading to requests being incorrectly authorized.
This patch trims trailing slashes before comparing
the requested path with the authorized paths.
Review: https://reviews.apache.org/r/49131/
Project: http://git-wip-us.apache.org/repos/asf/mesos/repo
Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/6852825d
Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/6852825d
Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/6852825d
Branch: refs/heads/master
Commit: 6852825da519024de4713b3571e65f1fd5af60ef
Parents: fe43822
Author: Greg Mann <gr...@mesosphere.io>
Authored: Fri Jun 24 00:21:16 2016 -0700
Committer: Adam B <ad...@mesosphere.io>
Committed: Fri Jun 24 00:21:16 2016 -0700
----------------------------------------------------------------------
src/files/files.cpp | 9 +++++++--
src/tests/files_tests.cpp | 40 ++++++++++++++++++++++++++++++++++++++++
2 files changed, 47 insertions(+), 2 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/mesos/blob/6852825d/src/files/files.cpp
----------------------------------------------------------------------
diff --git a/src/files/files.cpp b/src/files/files.cpp
index 20bc6fa..a5a1b86 100644
--- a/src/files/files.cpp
+++ b/src/files/files.cpp
@@ -315,8 +315,13 @@ Future<bool> FilesProcess::authorize(
string requestedPath,
const Option<string>& principal)
{
- if (authorizations.count(requestedPath) > 0) {
- return authorizations[requestedPath](principal);
+ // The path may contain a trailing forward slash. Since we store the
+ // authorization callbacks without the trailing slash, we must remove it here,
+ // if present.
+ string trimmedPath = strings::remove(requestedPath, "/", strings::SUFFIX);
+
+ if (authorizations.count(trimmedPath) > 0) {
+ return authorizations[trimmedPath](principal);
}
do {
http://git-wip-us.apache.org/repos/asf/mesos/blob/6852825d/src/tests/files_tests.cpp
----------------------------------------------------------------------
diff --git a/src/tests/files_tests.cpp b/src/tests/files_tests.cpp
index b8f4f3c..31337e2 100644
--- a/src/tests/files_tests.cpp
+++ b/src/tests/files_tests.cpp
@@ -303,6 +303,9 @@ TEST_F(FilesTest, ResolveTest)
AWAIT_EXPECT_RESPONSE_STATUS_EQ(
BadRequest().status,
process::http::get(upid, "read", "path=one/"));
+ AWAIT_EXPECT_RESPONSE_STATUS_EQ(
+ BadRequest().status,
+ process::http::get(upid, "read", "path=one/two/"));
// Breaking out of sandbox.
AWAIT_EXPECT_RESPONSE_STATUS_EQ(
@@ -320,6 +323,7 @@ TEST_F(FilesTest, BrowseTest)
ASSERT_SOME(os::mkdir("1/3"));
ASSERT_SOME(os::write("1/two", "two"));
ASSERT_SOME(os::write("1/three", "three"));
+ ASSERT_SOME(os::mkdir("2"));
AWAIT_EXPECT_READY(files.attach("1", "one"));
@@ -370,18 +374,54 @@ TEST_F(FilesTest, BrowseTest)
return authorized;
};
+ ASSERT_SOME(os::mkdir("2"));
+
AWAIT_EXPECT_READY(files.attach("1", "one", authorization));
+ AWAIT_EXPECT_READY(files.attach("2", "/two/", authorization));
+
+ // The `FilesProcess` stores authorization callbacks in a map keyed by path.
+ // If no callback is found for the requested path, then it is assumed that
+ // authorization for that path is not enabled - the request is authorized.
+ // Because of this, it is worth testing several permutations of the process's
+ // handling of trailing slashes in path names when authorization is enabled.
+ // We sometimes remove trailing slashes, so it's possible that we could fail
+ // to find the callback in the map.
response = process::http::get(upid, "browse", "path=one");
AWAIT_EXPECT_RESPONSE_STATUS_EQ(OK().status, response);
AWAIT_EXPECT_RESPONSE_BODY_EQ(stringify(expected), response);
+ response = process::http::get(upid, "browse", "path=one/");
+
+ AWAIT_EXPECT_RESPONSE_STATUS_EQ(OK().status, response);
+ AWAIT_EXPECT_RESPONSE_BODY_EQ(stringify(expected), response);
+
+ response = process::http::get(upid, "browse", "path=/two");
+
+ AWAIT_EXPECT_RESPONSE_STATUS_EQ(OK().status, response);
+
+ response = process::http::get(upid, "browse", "path=/two/");
+
+ AWAIT_EXPECT_RESPONSE_STATUS_EQ(OK().status, response);
+
authorized = false;
response = process::http::get(upid, "browse", "path=one");
AWAIT_EXPECT_RESPONSE_STATUS_EQ(Forbidden().status, response);
+
+ response = process::http::get(upid, "browse", "path=one/");
+
+ AWAIT_EXPECT_RESPONSE_STATUS_EQ(Forbidden().status, response);
+
+ response = process::http::get(upid, "browse", "path=/two");
+
+ AWAIT_EXPECT_RESPONSE_STATUS_EQ(Forbidden().status, response);
+
+ response = process::http::get(upid, "browse", "path=/two/");
+
+ AWAIT_EXPECT_RESPONSE_STATUS_EQ(Forbidden().status, response);
}