[ARVADOS] created: 521457373a8f1e46f44a43311be2d9242ad5d0a9

git at public.curoverse.com git at public.curoverse.com
Fri May 23 17:27:12 EDT 2014


        at  521457373a8f1e46f44a43311be2d9242ad5d0a9 (commit)


commit 521457373a8f1e46f44a43311be2d9242ad5d0a9
Author: Tim Pierce <twp at curoverse.com>
Date:   Wed May 21 10:58:18 2014 -0400

    2755: add support for signed locators in the Python SDK.
    
    * arvados.Keep.put() saves the response body (which may contain a
      signed locator) and returns it to the caller.
    
    * arvados.Keep.get() passes the full signed locator to the remote Keep
      server. The bare MD5 hash is still used for caching and for
      shuffled_service_roots
    
    * run_test_server.run_keep() takes arguments 'blob_signing_key' and
      'enforce_permissions', for testing permission signatures in unit
      tests.
    
    * test_keep_client: new unit tests for permissions:
        - with --enforce-permissions=true:
            - GET with a signed locator works
            - GET with an unsigned locator fails
            - unauthenticated GET fails
        - with --enforce-permissions=false:
            - GET with a signed locator works
    	- GET with an unsigned locator works
    	- unauthenticated GET works
    
    Bug fixes to permission handling in the Keep server:
    
    * Locator hints may appear in any order; be flexible. Parse them in
      GetBlockHandler rather than in the REST router.
    
    * Returned locators are terminated with newline (consistent with
      Warehouse, and more friendly for human debugging).
    
    * The locator returned from a PUT request always has a size hint.
    
    * The correct Authorization header keyword is "OAuth2", not
      "OAuth". D'oh.
    
    * Updated unit tests to accommodate newlines, size hints and OAuth2.
    
    Refs #2755.

diff --git a/sdk/python/arvados/keep.py b/sdk/python/arvados/keep.py
index e414d26..4e800f7 100644
--- a/sdk/python/arvados/keep.py
+++ b/sdk/python/arvados/keep.py
@@ -55,6 +55,7 @@ class KeepClient(object):
         def __init__(self, todo):
             self._todo = todo
             self._done = 0
+            self._response = None
             self._todo_lock = threading.Semaphore(todo)
             self._done_lock = threading.Lock()
 
@@ -73,12 +74,23 @@ class KeepClient(object):
             with self._done_lock:
                 return (self._done < self._todo)
 
-        def increment_done(self):
+        def save_response(self, response_body):
             """
-            Report that the current thread was successful.
+            Records a response body (a locator, possibly signed) returned by
+            the Keep server.  It is not necessary to save more than
+            one response, since we presume that any locator returned
+            in response to a successful request is valid.
             """
             with self._done_lock:
                 self._done += 1
+                self._response = response_body
+
+        def response(self):
+            """
+            Returns the body from the response to a PUT request.
+            """
+            with self._done_lock:
+                return self._response
 
         def done(self):
             """
@@ -89,9 +101,9 @@ class KeepClient(object):
 
     class KeepWriterThread(threading.Thread):
         """
-        Write a blob of data to the given Keep server. Call
-        increment_done() of the given ThreadLimiter if the write
-        succeeds.
+        Write a blob of data to the given Keep server. On success, call
+        save_response() of the given ThreadLimiter to save the returned
+        locator.
         """
         def __init__(self, **kwargs):
             super(KeepClient.KeepWriterThread, self).__init__()
@@ -129,7 +141,7 @@ class KeepClient(object):
                                       (str(threading.current_thread()),
                                        self.args['data_hash'],
                                        self.args['service_root']))
-                        return limiter.increment_done()
+                        return limiter.save_response(content.strip())
                     logging.warning("Request fail: PUT %s => %s %s" %
                                     (url, resp['status'], content))
                 except (httplib2.HttpLib2Error, httplib.HTTPException) as e:
@@ -277,7 +289,7 @@ class KeepClient(object):
 
         try:
             for service_root in self.shuffled_service_roots(expect_hash):
-                url = service_root + expect_hash
+                url = service_root + locator
                 api_token = config.get('ARVADOS_API_TOKEN')
                 headers = {'Authorization': "OAuth2 %s" % api_token,
                            'Accept': 'application/octet-stream'}
@@ -289,7 +301,7 @@ class KeepClient(object):
 
             for location_hint in re.finditer(r'\+K@([a-z0-9]+)', locator):
                 instance = location_hint.group(1)
-                url = 'http://keep.' + instance + '.arvadosapi.com/' + expect_hash
+                url = 'http://keep.' + instance + '.arvadosapi.com/' + locator
                 blob = self.get_url(url, {}, expect_hash)
                 if blob:
                     slot.set(blob)
@@ -348,8 +360,9 @@ class KeepClient(object):
         for t in threads:
             t.join()
         have_copies = thread_limiter.done()
+        # If we're done, return the response from Keep
         if have_copies == want_copies:
-            return (data_hash + '+' + str(len(data)))
+            return thread_limiter.response()
         raise arvados.errors.KeepWriteError(
             "Write fail for %s: wanted %d but wrote %d" %
             (data_hash, want_copies, have_copies))
diff --git a/sdk/python/run_test_server.py b/sdk/python/run_test_server.py
index dbb4ff0..4b65d72 100644
--- a/sdk/python/run_test_server.py
+++ b/sdk/python/run_test_server.py
@@ -123,15 +123,22 @@ def stop():
 
     os.chdir(cwd)
 
-def _start_keep(n):
+def _start_keep(n, keep_args):
     keep0 = tempfile.mkdtemp()
-    kp0 = subprocess.Popen(["bin/keep", "-volumes={}".format(keep0), "-listen=:{}".format(25107+n)])
+    keep_cmd = ["bin/keep",
+                "-volumes={}".format(keep0),
+                "-listen=:{}".format(25107+n)]
+
+    for arg, val in keep_args.iteritems():
+        keep_cmd.append("{}={}".format(arg, val))
+
+    kp0 = subprocess.Popen(keep_cmd)
     with open("tmp/keep{}.pid".format(n), 'w') as f:
         f.write(str(kp0.pid))
     with open("tmp/keep{}.volume".format(n), 'w') as f:
         f.write(keep0)
 
-def run_keep():
+def run_keep(blob_signing_key=None, enforce_permissions=False):
     stop_keep()
 
     cwd = os.getcwd()
@@ -146,8 +153,16 @@ def run_keep():
     if not os.path.exists("tmp"):
         os.mkdir("tmp")
 
-    _start_keep(0)
-    _start_keep(1)
+    keep_args = {}
+    if blob_signing_key:
+        with open("tmp/keep.blob_signing_key", "w") as f:
+            f.write(blob_signing_key)
+        keep_args['--permission-key-file'] = 'tmp/keep.blob_signing_key'
+    if enforce_permissions:
+        keep_args['--enforce-permissions'] = 'true'
+
+    _start_keep(0, keep_args)
+    _start_keep(1, keep_args)
 
 
     os.environ["ARVADOS_API_HOST"] = "127.0.0.1:3001"
@@ -172,6 +187,8 @@ def _stop_keep(n):
     if os.path.exists("tmp/keep{}.volume".format(n)):
         with open("tmp/keep{}.volume".format(n), 'r') as r:
             shutil.rmtree(r.read(), True)
+    if os.path.exists("tmp/keep.blob_signing_key"):
+        os.remove("tmp/keep.blob_signing_key")
 
 def stop_keep():
     cwd = os.getcwd()
diff --git a/sdk/python/test_keep_client.py b/sdk/python/test_keep_client.py
index aa79b0d..8fac34d 100644
--- a/sdk/python/test_keep_client.py
+++ b/sdk/python/test_keep_client.py
@@ -65,3 +65,105 @@ class KeepTestCase(unittest.TestCase):
         self.assertEqual(arvados.Keep.get(blob_locator),
                          blob_str,
                          'wrong content from Keep.get(md5(<binarydata>))')
+
+class KeepPermissionTestCase(unittest.TestCase):
+    @classmethod
+    def setUpClass(cls):
+        try:
+            del os.environ['KEEP_LOCAL_STORE']
+        except KeyError:
+            pass
+        run_test_server.run()
+        run_test_server.run_keep(blob_signing_key='abcdefghijk0123456789',
+                                 enforce_permissions=True)
+
+    @classmethod
+    def tearDownClass(cls):
+        run_test_server.stop()
+        run_test_server.stop_keep()
+
+    def test_KeepBasicRWTest(self):
+        run_test_server.authorize_with('active')
+        foo_locator = arvados.Keep.put('foo')
+        self.assertRegexpMatches(
+            foo_locator,
+            r'^acbd18db4cc2f85cedef654fccc4a4d8\+3\+A[a-f0-9]+@[a-f0-9]+$',
+            'invalid locator from Keep.put("foo"): ' + foo_locator)
+        self.assertEqual(arvados.Keep.get(foo_locator),
+                         'foo',
+                         'wrong content from Keep.get(md5("foo"))')
+
+        # With Keep permissions enabled, a GET request without a locator will fail.
+        bar_locator = arvados.Keep.put('bar')
+        self.assertRegexpMatches(
+            bar_locator,
+            r'^37b51d194a7513e45b56f6524f2d51f2\+3\+A[a-f0-9]+@[a-f0-9]+$',
+            'invalid locator from Keep.put("bar"): ' + bar_locator)
+        self.assertRaises(arvados.errors.NotFoundError,
+                          arvados.Keep.get,
+                          "37b51d194a7513e45b56f6524f2d51f2")
+
+        # A request without an API token will also fail.
+        del arvados.config.settings()["ARVADOS_API_TOKEN"]
+        self.assertRaises(arvados.errors.NotFoundError,
+                          arvados.Keep.get,
+                          bar_locator)
+
+# KeepOptionalPermission: starts Keep with --permission-key-file
+# but not --enforce-permissions (i.e. generate signatures on PUT
+# requests, but do not require them for GET requests)
+#
+class KeepOptionalPermission(unittest.TestCase):
+    @classmethod
+    def setUpClass(cls):
+        try:
+            del os.environ['KEEP_LOCAL_STORE']
+        except KeyError:
+            pass
+        run_test_server.run()
+        run_test_server.run_keep(blob_signing_key='abcdefghijk0123456789',
+                                 enforce_permissions=False)
+
+    @classmethod
+    def tearDownClass(cls):
+        run_test_server.stop()
+        run_test_server.stop_keep()
+
+    def test_KeepBasicRWTest(self):
+        run_test_server.authorize_with('active')
+        foo_locator = arvados.Keep.put('foo')
+        self.assertRegexpMatches(
+            foo_locator,
+            r'^acbd18db4cc2f85cedef654fccc4a4d8\+3\+A[a-f0-9]+@[a-f0-9]+$',
+            'invalid locator from Keep.put("foo"): ' + foo_locator)
+        self.assertEqual(arvados.Keep.get(foo_locator),
+                         'foo',
+                         'wrong content from Keep.get(md5("foo"))')
+
+    def test_KeepUnsignedLocatorTest(self):
+        # Since --enforce-permissions is not in effect, GET requests
+        # do not require signatures.
+        run_test_server.authorize_with('active')
+        foo_locator = arvados.Keep.put('foo')
+        self.assertRegexpMatches(
+            foo_locator,
+            r'^acbd18db4cc2f85cedef654fccc4a4d8\+3\+A[a-f0-9]+@[a-f0-9]+$',
+            'invalid locator from Keep.put("foo"): ' + foo_locator)
+        self.assertEqual(arvados.Keep.get("acbd18db4cc2f85cedef654fccc4a4d8"),
+                         'foo',
+                         'wrong content from Keep.get(md5("foo"))')
+
+    def test_KeepUnauthenticatedTest(self):
+        # Since --enforce-permissions is not in effect, GET requests
+        # need not be authenticated.
+        run_test_server.authorize_with('active')
+        foo_locator = arvados.Keep.put('foo')
+        self.assertRegexpMatches(
+            foo_locator,
+            r'^acbd18db4cc2f85cedef654fccc4a4d8\+3\+A[a-f0-9]+@[a-f0-9]+$',
+            'invalid locator from Keep.put("foo"): ' + foo_locator)
+
+        del arvados.config.settings()["ARVADOS_API_TOKEN"]
+        self.assertEqual(arvados.Keep.get("acbd18db4cc2f85cedef654fccc4a4d8"),
+                         'foo',
+                         'wrong content from Keep.get(md5("foo"))')
diff --git a/services/keep/src/keep/handler_test.go b/services/keep/src/keep/handler_test.go
index 8e7bfea..c96086f 100644
--- a/services/keep/src/keep/handler_test.go
+++ b/services/keep/src/keep/handler_test.go
@@ -15,6 +15,7 @@ import (
 	"net/http"
 	"net/http/httptest"
 	"regexp"
+	"strings"
 	"testing"
 	"time"
 )
@@ -159,7 +160,9 @@ func TestPutHandler(t *testing.T) {
 
 	ExpectStatusCode(t,
 		"Unauthenticated request, no server key", http.StatusOK, response)
-	ExpectBody(t, "Unauthenticated request, no server key", TEST_HASH, response)
+	ExpectBody(t,
+		"Unauthenticated request, no server key",
+		TEST_HASH_PUT_RESPONSE, response)
 
 	// ------------------
 	// With a server key.
@@ -183,10 +186,11 @@ func TestPutHandler(t *testing.T) {
 	ExpectStatusCode(t,
 		"Authenticated PUT, signed locator, with server key",
 		http.StatusOK, response)
-	if !VerifySignature(response.Body.String(), known_token) {
+	response_locator := strings.TrimSpace(response.Body.String())
+	if !VerifySignature(response_locator, known_token) {
 		t.Errorf("Authenticated PUT, signed locator, with server key:\n"+
 			"response '%s' does not contain a valid signature",
-			response.Body.String())
+			response_locator)
 	}
 
 	// Unauthenticated PUT, unsigned locator
@@ -203,7 +207,7 @@ func TestPutHandler(t *testing.T) {
 		http.StatusOK, response)
 	ExpectBody(t,
 		"Unauthenticated PUT, unsigned locator, with server key",
-		TEST_HASH, response)
+		TEST_HASH_PUT_RESPONSE, response)
 }
 
 // Test /index requests:
@@ -407,7 +411,7 @@ func IssueRequest(router *mux.Router, rt *RequestTester) *httptest.ResponseRecor
 	body := bytes.NewReader(rt.request_body)
 	req, _ := http.NewRequest(rt.method, rt.uri, body)
 	if rt.api_token != "" {
-		req.Header.Set("Authorization", "OAuth "+rt.api_token)
+		req.Header.Set("Authorization", "OAuth2 "+rt.api_token)
 	}
 	router.ServeHTTP(response, req)
 	return response
diff --git a/services/keep/src/keep/keep.go b/services/keep/src/keep/keep.go
index d65e445..1f80678 100644
--- a/services/keep/src/keep/keep.go
+++ b/services/keep/src/keep/keep.go
@@ -271,7 +271,7 @@ func MakeRESTRouter() *mux.Router {
 	rest.HandleFunc(
 		`/{hash:[0-9a-f]{32}}`, GetBlockHandler).Methods("GET", "HEAD")
 	rest.HandleFunc(
-		`/{hash:[0-9a-f]{32}}+A{signature:[0-9a-f]+}@{timestamp:[0-9a-f]+}`,
+		`/{hash:[0-9a-f]{32}}+{hints}`,
 		GetBlockHandler).Methods("GET", "HEAD")
 	rest.HandleFunc(`/{hash:[0-9a-f]{32}}`, PutBlockHandler).Methods("PUT")
 
@@ -330,8 +330,28 @@ func GetBlockHandler(resp http.ResponseWriter, req *http.Request) {
 
 	log.Printf("%s %s", req.Method, hash)
 
-	signature := mux.Vars(req)["signature"]
-	timestamp := mux.Vars(req)["timestamp"]
+	hints := mux.Vars(req)["hints"]
+
+	// Parse the locator string and hints from the request.
+	// TODO(twp): implement a Locator type.
+	var signature, timestamp string
+	if hints != "" {
+		signature_pat, _ := regexp.Compile("^A([[:xdigit:]]+)@([[:xdigit:]]{8})$")
+		for _, hint := range strings.Split(hints, "+") {
+			if match, _ := regexp.MatchString("^[[:digit:]]+$", hint); match {
+				// Server ignores size hints
+			} else if match, _ := regexp.MatchString("^K([[:alnum:]]+)$", hint); match {
+				// Server ignores location hints
+			} else if m := signature_pat.FindStringSubmatch(hint); m != nil {
+				signature = m[1]
+				timestamp = m[2]
+			} else {
+				// Not a valid locator: return 404
+				http.Error(resp, NotFoundError.Error(), NotFoundError.HTTPCode)
+				return
+			}
+		}
+	}
 
 	// If permission checking is in effect, verify this
 	// request's permission signature.
@@ -343,8 +363,8 @@ func GetBlockHandler(resp http.ResponseWriter, req *http.Request) {
 			http.Error(resp, ExpiredError.Error(), ExpiredError.HTTPCode)
 			return
 		} else {
-			validsig := MakePermSignature(hash, GetApiToken(req), timestamp)
-			if signature != validsig {
+			req_locator := req.URL.Path[1:] // strip leading slash
+			if !VerifySignature(req_locator, GetApiToken(req)) {
 				http.Error(resp, PermissionError.Error(), PermissionError.HTTPCode)
 				return
 			}
@@ -384,11 +404,15 @@ func PutBlockHandler(resp http.ResponseWriter, req *http.Request) {
 	//
 	if buf, err := ReadAtMost(req.Body, BLOCKSIZE); err == nil {
 		if err := PutBlock(buf, hash); err == nil {
-			// Success; sign the locator and return it to the client.
+			// Success; add a size hint, sign the locator if
+			// possible, and return it to the client.
+			return_hash := fmt.Sprintf("%s+%d", hash, len(buf))
 			api_token := GetApiToken(req)
-			expiry := time.Now().Add(permission_ttl)
-			signed_loc := SignLocator(hash, api_token, expiry)
-			resp.Write([]byte(signed_loc))
+			if PermissionSecret != nil && api_token != "" {
+				expiry := time.Now().Add(permission_ttl)
+				return_hash = SignLocator(return_hash, api_token, expiry)
+			}
+			resp.Write([]byte(return_hash + "\n"))
 		} else {
 			ke := err.(*KeepError)
 			http.Error(resp, ke.Error(), ke.HTTPCode)
@@ -650,13 +674,15 @@ func IsValidLocator(loc string) bool {
 	return false
 }
 
-// GetApiToken returns the OAuth token from the Authorization
+// GetApiToken returns the OAuth2 token from the Authorization
 // header of a HTTP request, or an empty string if no matching
 // token is found.
 func GetApiToken(req *http.Request) string {
 	if auth, ok := req.Header["Authorization"]; ok {
-		if strings.HasPrefix(auth[0], "OAuth ") {
-			return auth[0][6:]
+		if pat, err := regexp.Compile(`^OAuth2\s+(.*)`); err != nil {
+			log.Println(err)
+		} else if match := pat.FindStringSubmatch(auth[0]); match != nil {
+			return match[1]
 		}
 	}
 	return ""
diff --git a/services/keep/src/keep/keep_test.go b/services/keep/src/keep/keep_test.go
index 6642c72..7a787c1 100644
--- a/services/keep/src/keep/keep_test.go
+++ b/services/keep/src/keep/keep_test.go
@@ -12,6 +12,7 @@ import (
 
 var TEST_BLOCK = []byte("The quick brown fox jumps over the lazy dog.")
 var TEST_HASH = "e4d909c290d0fb1ca068ffaddf22cbd0"
+var TEST_HASH_PUT_RESPONSE = "e4d909c290d0fb1ca068ffaddf22cbd0+44\n"
 
 var TEST_BLOCK_2 = []byte("Pack my box with five dozen liquor jugs.")
 var TEST_HASH_2 = "f15ac516f788aec4f30932ffb6395c39"
diff --git a/services/keep/src/keep/perms.go b/services/keep/src/keep/perms.go
index 0d1b091..f7caca4 100644
--- a/services/keep/src/keep/perms.go
+++ b/services/keep/src/keep/perms.go
@@ -83,10 +83,10 @@ func SignLocator(blob_locator string, api_token string, expiry time.Time) string
 // VerifySignature returns true if the signature on the signed_locator
 // can be verified using the given api_token.
 func VerifySignature(signed_locator string, api_token string) bool {
-	if re, err := regexp.Compile(`^(.*)\+A(.*)@(.*)$`); err == nil {
+	if re, err := regexp.Compile(`^([a-f0-9]+(\+[0-9]+)?)\+A(.*)@(.*)$`); err == nil {
 		if matches := re.FindStringSubmatch(signed_locator); matches != nil {
 			blob_locator := matches[1]
-			timestamp_hex := matches[3]
+			timestamp_hex := matches[4]
 			if expire_ts, err := ParseHexTimestamp(timestamp_hex); err == nil {
 				// Fail signatures with expired timestamps.
 				if expire_ts.Before(time.Now()) {

-----------------------------------------------------------------------


hooks/post-receive
-- 




More information about the arvados-commits mailing list