Arvados

My first batch of review comments for 2328-keep-permission-flags

handler_test.go

if resp.Code != 200 {
t.Errorf("bad response code: %v", resp)
}
if bytes.Compare(resp.Body.Bytes(), TEST_BLOCK) != 0 {
t.Errorf("bad response body: %v", resp)
}

These error messages don't make it clear to the reader what
happened. I think test results should be clear without having to dig
into the test code.

How about "Expected response code 200 from normal unsigned GET without
permissions enabled., but received %v instead." and "Returned body
didn't match written body for normal unsigned GET without permissions
enabled."?

Ditto for following errors like this. Since you have these throughout,
it might make sense to use a helper method and just pass it the type
of request you were testing.

Another example is:

if resp.Code != PermissionError.HTTPCode {
t.Errorf("unsigned request: bad response code: %v", resp)
}

The error message doesn't make clear the following:

• Permissions checking was turned on
• We expected a permission error code

Sorry for my golang n00bness but where are the following variables defined? I didn't see definitions in this file or change:

• TEST_BLOCK
• TEST_HASH
• known_key

Can we rename permission_ttl to permission_ttl_seconds or something similar?
Or maybe even better would be:
permission_ttl = time.Duration(300) * time.Second

Misha Zatsman wrote:

My first batch of review comments for 2328-keep-permission-flags

handler_test.go

if resp.Code != 200 {
t.Errorf("bad response code: %v", resp)
}
if bytes.Compare(resp.Body.Bytes(), TEST_BLOCK) != 0 {
t.Errorf("bad response body: %v", resp)
}

These error messages don't make it clear to the reader what
happened. I think test results should be clear without having to dig
into the test code.

How about "Expected response code 200 from normal unsigned GET without
permissions enabled., but received %v instead." and "Returned body
didn't match written body for normal unsigned GET without permissions
enabled."?

Ditto for following errors like this. Since you have these throughout,
it might make sense to use a helper method and just pass it the type
of request you were testing.

Thanks, I like that idea. I've added ExpectStatusCode and ExpectBody to simplify the tests a bit.

The Go standard library uses a template pattern for a lot of their tests that I'd like to aim to adopt (see e.g. http://golang.org/src/pkg/net/http/response_test.go).

Sorry for my golang n00bness but where are the following variables defined? I didn't see definitions in this file or change:

• TEST_BLOCK
• TEST_HASH
• known_key

It's not your n00bness, I'm using poor style here. TEST_BLOCK and TEST_HASH are defined in keep_test.go, and known_key is defined in perms_test.go.

The scoping rules for Go are that a variable defined at the package level is scoped throughout that entire package. The Keep server is not divided into packages, so everything lives in package main, and any global variable is accessible anywhere in the code. It's not a great way to structure the project and I'm not proud of it, it just helped us get started quickly :-)

Can we rename permission_ttl to permission_ttl_seconds or something similar?
Or maybe even better would be:
permission_ttl = time.Duration(300) * time.Second

Great suggestion, done.

The TestPutHandler unit test is now failing (authenticated PUT), so I'll look into it later to figure out what I screwed up. I'll let you know when it's ready to look at again.

Tim Pierce wrote:

Misha Zatsman wrote:

Can we rename permission_ttl to permission_ttl_seconds or something similar?
Or maybe even better would be:
permission_ttl = time.Duration(300) * time.Second

Great suggestion, done.

Hmm, I'm not seeing this change even though I'm seeing some other ones. Maybe you pulled a Misha and didn't commit or push your latest version? Seems reasonable since you say below it's not ready for review.

The TestPutHandler unit test is now failing (authenticated PUT), so I'll look into it later to figure out what I screwed up. I'll let you know when it's ready to look at again.

Makes sense, but I'll continue looking through your change since I'm out tomorrow.

Misha Zatsman wrote:

Tim Pierce wrote:

Misha Zatsman wrote:

Can we rename permission_ttl to permission_ttl_seconds or something similar?
Or maybe even better would be:
permission_ttl = time.Duration(300) * time.Second

Great suggestion, done.

Hmm, I'm not seeing this change even though I'm seeing some other ones. Maybe you pulled a Misha and didn't commit or push your latest version? Seems reasonable since you say below it's not ready for review.

It's in 087773f25. I definitely pushed it, I had to pull it to my laptop when I got home! :-) permission_ttl is now declared as time.Duration and is initialized in main from the --permission-ttl flag.

The TestPutHandler unit test is now failing (authenticated PUT), so I'll look into it later to figure out what I screwed up. I'll let you know when it's ready to look at again.

Makes sense, but I'll continue looking through your change since I'm out tomorrow.

All set in 04ec74cf7d (related -- because permission_ttl is now initialized from main, the unit test has to initialize it explicitly in order to generate valid signatures).

Here are the rest of my comments for handler_test.go

Sorry for the nitpicking.

Many of the following comments ask for more detailed error statements. It's funny because your comments already contain the detail I'm talking about, but the comments won't be seen by the person seeing the test failure without some work, so if we just put the comments in the error message, the person dealing with failing tests will be happier. And if you want to go further you can even delete the comments since they error message is right there by the code.

ExpectStatusCode(t, "unsigned GET", resp, http.StatusOK)
ExpectBody(t, "unsigned GET", resp, string(TEST_BLOCK))

Can we call this "unsigned get with permissions off"?

ExpectStatusCode(t, "unsigned locator", resp, PermissionError.HTTPCode)

"unsigned get with permissions on"?

test_url = "http://localhost:25107/" + SignLocator(TEST_HASH, known_token, expired_ts)
resp = httptest.NewRecorder()
req, _ = http.NewRequest("GET", test_url, nil)
req.Header.Set("Authorization", "OAuth "+known_token)
rest.ServeHTTP(resp, req)

This boilerplate occurs frequently and is long enough that it's hard
for me to grok the differences between each block. How about
parameterizing it and putting it into a method that returns the
response? Something like :

type TestKeepRequest struct {
path, authorization, request_type string
request_body *string
}

func getResponse(t TestKeepRequest) (Response) {
...
}

ExpectStatusCode(t, "permissions off", resp, http.StatusOK)
ExpectBody(t, "permissions off", resp, TEST_HASH)

"unauthenticated PUT without server key"?

// When a permission key is available, the locator returned
// from a PUT request will be signed.

...

// An unauthenticated PUT request returns an unsigned locator
// even when a permission key is available.

These two statements seem to contradict each other. Does the first one need to be updated?

ExpectStatusCode(t, "authenticated PUT", resp, http.StatusOK)

"authenticated PUT with server key"

t.Errorf("authenticated PUT: response '%s' failed signature check",
resp.Body.String())

"authenticated PUT: response '%s' does not contain a valid signature"?

ExpectStatusCode(t, "anon PUT with server key", resp, http.StatusOK)

"unauthenticated PUT with server key"?

ExpectBody(t, "anon PUT with server key", resp, TEST_HASH)

"unauthenticated PUT should return an unsigned locator even with server key"?

// Requests for /index with a prefix are okay even if unauthenticated.

Can I just build the entire index file by iterating through all possible one character prefixes? If so, it seems like we're not actually securing anything. Or does a prefix request get a different kind of response?

ExpectStatusCode(t, "authenticated /index", resp, PermissionError.HTTPCode)

"Authenticated /index request by a non-superuser."

// Even superuser /index requests fail if enforce_permissions is off!

Is this explained http error response the user gets?

If so, we should test for the explanation here.

If not, we should add it to the error response, because I imagine this will be a confusing case for the user to debug: "Why am I getting a permission error, I'm a superuser!"

ExpectStatusCode(t, "superuser /index (permissions off)", resp, PermissionError.HTTPCode)

"Authenticated /index request by a superuser when permissions are turned off."

ExpectStatusCode(t, "superuser /index (permissions on)", resp, http.StatusOK)

"Authenticated /index request by a superuser when permissions are turned on."

// ExpectStatusCode checks whether a response has the specified status code,
// and reports a test failure if not.
func ExpectStatusCode(
t *testing.T,
testname string,
response *httptest.ResponseRecorder,
expected_status int) {
if response.Code != expected_status {
t.Errorf("%s: expected status %s, got %+v",
testname, expected_status, response)
}
}

func ExpectBody(
t *testing.T,
testname string,
response *httptest.ResponseRecorder,
expected_body string) {
if response.Body.String() != expected_body {
t.Errorf("%s: expected response body '%s', got %+v",
testname, expected_body, response)
}
}

Can we reorder the arguments so that testname and expected_* appear next to each other to make it easier for readers to grok.

Misha Zatsman wrote:

Here are the rest of my comments for handler_test.go

Sorry for the nitpicking.

Many of the following comments ask for more detailed error statements. It's funny because your comments already contain the detail I'm talking about, but the comments won't be seen by the person seeing the test failure without some work, so if we just put the comments in the error message, the person dealing with failing tests will be happier. And if you want to go further you can even delete the comments since they error message is right there by the code.

DON'T apologize -- this is excellent feedback, and readability is important. I refactored the tests according to your suggestions and I think they're much easier to follow and understand. Do let me know if you think I missed something important.

// When a permission key is available, the locator returned
// from a PUT request will be signed.

...

// An unauthenticated PUT request returns an unsigned locator
// even when a permission key is available.

These two statements seem to contradict each other. Does the first one need to be updated?

Edited to clarify (the server will sign locators in the response body if the request is authenticated and the server has a permission key).

// Requests for /index with a prefix are okay even if unauthenticated.

Can I just build the entire index file by iterating through all possible one character prefixes? If so, it seems like we're not actually securing anything. Or does a prefix request get a different kind of response?

Yes, you can do that. This is an excellent point. In fact, if permissions are enforced, then having the locators alone won't do an attacker much good anyway, so it's not clear what this is supposed to protect against. I'll check in with Tom about the goal of this restriction.

// Even superuser /index requests fail if enforce_permissions is off!

Is this explained http error response the user gets?

If so, we should test for the explanation here.

If not, we should add it to the error response, because I imagine this will be a confusing case for the user to debug: "Why am I getting a permission error, I'm a superuser!"

It's an interesting point. Remember that the chief reason for having an --enforce-permissions flag at all is to permit a smooth migration from an unauthenticated Keep server to a fully authenticated one. When the new Keep server is installed, all of the users will have only unsigned locators; as they continue to use Arvados, the system will replace each URL in their collections with signed locators. --enforce-permissions can be activated once all of the old locators have been signed.

So I'm not too fussed about the clarity of the error message, since we don't expect Keep servers without permissions enabled to be long-lived in any event.

// Requests for /index with a prefix are okay even if unauthenticated.

Can I just build the entire index file by iterating through all possible one character prefixes? If so, it seems like we're not actually securing anything. Or does a prefix request get a different kind of response?

Yes, you can do that. This is an excellent point. In fact, if permissions are enforced, then having the locators alone won't do an attacker much good anyway, so it's not clear what this is supposed to protect against. I'll check in with Tom about the goal of this restriction.

I checked this with Tom. The intended behavior is that all /index requests should be restricted only to the superuser. Non-superusers may not issue any kind of /index request, whether with a prefix or not. IndexHandler and TestIndexHandler have been updated to reflect this behavior. eef1936

Tim Pierce wrote:

Misha Zatsman wrote:

Here are the rest of my comments for handler_test.go

Sorry for the nitpicking.

Many of the following comments ask for more detailed error statements. It's funny because your comments already contain the detail I'm talking about, but the comments won't be seen by the person seeing the test failure without some work, so if we just put the comments in the error message, the person dealing with failing tests will be happier. And if you want to go further you can even delete the comments since they error message is right there by the code.

DON'T apologize -- this is excellent feedback, and readability is important. I refactored the tests according to your suggestions and I think they're much easier to follow and understand. Do let me know if you think I missed something important.

Thanks. So far the only thing I noticed that wasn't addressed from my suggestions is that the comments in the test are still more detailed than the strings you pass in for error messages. For example in the first test with permissions the comment says:

// Authenticated request, signed locator

While the error string just says:

ExpectStatusCode(t, "signed GET (permissions on)", http.StatusOK, response)

And has no mention of the fact that the request is authenticated.

I think the easiest thing to do would be just to paste your comments in to the error strings.

I'm still going through the rest of your change, but just wanted to get this comment out.

Not quite finished, should finish later today, but here's what I have so far:

handler_test.go
-------------

unsigned_locator := "http://localhost:25107/" + TEST_HASH

Can we move this to the var block with the other values we use for testing?

var (
expiration = time.Now().Add(permission_ttl)

How about valid_timestamp instead of expiration for a variable name?

// superuser /index/prefix request
// => OK

Can we add tests to verify that /index/prefix fails for non-superusers and unauthenticated users as well please?

keep.go
-------

"Comma-separated list of directories to use for Keep volumes, e.g. -volumes=/var/keep1,/var/keep2. If empty or not supplied, Keep will scan mounted filesystems for volumes w

This line (and several others around it) is really long. Can you wrap lines longer than 80 chars please?

"File with the API token used by the Data Manager. All DELETE requests or unqualified GET /index requests must carry this token.")

Is "unqualified GET /index" a reference to the old implementation that allowed /index/prefix requests? If so this should be updated. Otherwise I think it needs to be clarified because I don't know what it means.

} else {
log.Printf("reading data_manager_token: %s\n", err)

...

} else {
log.Printf("reading data_manager_token: %s\n", err)

I think we should fail fast if we encounter an error. In this case the caller has given us a path that doesn't work for some reason, and if we don't fail, she might not notice that she has a typo in her path.

So I think these should be log.Fatals

// If --enforce-permissions is true, we must have a permission key to continue.
if enforce_permissions && PermissionSecret == nil {
log.Fatal("--enforce-permissions requires a permission key")
}

Can we add a test for this condition?

Issues addressed at b08f164, including more explicit test names in the test failure output...

Misha Zatsman wrote:

Not quite finished, should finish later today, but here's what I have so far:

handler_test.go
-------------

unsigned_locator := "http://localhost:25107/" + TEST_HASH

Can we move this to the var block with the other values we use for testing?

Yup, done.

var (
expiration = time.Now().Add(permission_ttl)

How about valid_timestamp instead of expiration for a variable name?

Good call, done.

// superuser /index/prefix request
// => OK

Can we add tests to verify that /index/prefix fails for non-superusers and unauthenticated users as well please?

Yup, added. This should now test every possible permutation of authentication, prefix and superuser.

keep.go
-------

"Comma-separated list of directories to use for Keep volumes, e.g. -volumes=/var/keep1,/var/keep2. If empty or not supplied, Keep will scan mounted filesystems for volumes w

This line (and several others around it) is really long. Can you wrap lines longer than 80 chars please?

Sure. I mostly have been trying to stick to 80 columns but wasn't sure what was best to do with very long unbroken strings like this. I went through the code and folded every line where I could find a reasonable way to do it -- there are still a few that break 80 characters. If you still consider this a problem I'll finish them.

"File with the API token used by the Data Manager. All DELETE requests or unqualified GET /index requests must carry this token.")

Is "unqualified GET /index" a reference to the old implementation that allowed /index/prefix requests? If so this should be updated. Otherwise I think it needs to be clarified because I don't know what it means.

Yes, that's what it is, sorry. Took out "unqualified".

} else {
log.Printf("reading data_manager_token: %s\n", err)

...

} else {
log.Printf("reading data_manager_token: %s\n", err)

I think we should fail fast if we encounter an error. In this case the caller has given us a path that doesn't work for some reason, and if we don't fail, she might not notice that she has a typo in her path.

I think you're right. These are now log.Fatalf.

// If --enforce-permissions is true, we must have a permission key to continue.
if enforce_permissions && PermissionSecret == nil {
log.Fatal("--enforce-permissions requires a permission key")
}

Can we add a test for this condition?

Not easily, since the code is in main(). We could move this check into a helper function and add a test for the helper function. As time goes on I do want to move as much logic out of main() as possible, precisely to maximize this test coverage.

Thanks, all the changes look great

// If --enforce-permissions is true, we must have a permission key to continue.
if enforce_permissions && PermissionSecret nil {
log.Fatal("--enforce-permissions requires a permission key")
}

Can we add a test for this condition?

Not easily, since the code is in main(). We could move this check into a helper function and add a test for the helper function. As time goes on I do want to move as much logic out of main() as possible, precisely to maximize this test coverage.

Ok, that sounds like a good plan, but more work than we want to add to this change, so let's add a TODO to move this code (and whatever else we can) out of main() and add tests to boost test coverage.

more keep.go:
-------------

// If --enforce-permissions is true, we must have a permission key to continue.
if enforce_permissions && PermissionSecret nil {
log.Fatal("--enforce-permissions requires a permission key")
}

How about changing this to:

if PermissionSecret == nil {
if enforce_permissions {
log.Fatal("--enforce-permissions requires a permission key")
} else {
log.Warning("Running without a PermissionSecret. Block locators returned " +
"by this server will not be signed, and will be rejected by " +
"a server that enforces permissions.")
}
}

// NewRESTRouter
// Returns a mux.Router that passes GET and PUT requests to the
// appropriate handlers.
//

Why does this function name start with "new"?

• If it's because it creates a new object, let's call it CreateRESTRouter
• if it's new because it adds functionality vs the previous router that we had, let's just name it with the functionality that it added. e.g. PermissionsRESTRouter.

rest.HandleFunc(
`/index/{prefix:[0-9a-f]{0,32}}`, IndexHandler).Methods("GET", "HEAD")

My initial reading of this I thought we'd want the quantifier on prefix to be {1,32} or maybe even {1,31}, but now I can see arguments for {0,31} too.
Whatever the right choice ends up being, could you add a comment explaining it please?

My guess would be that the right answer is actually {0,32} like you have, but I still think we should explain why we allow lengths of 0 and 32. Maybe something like "We allow a prefix of length 0 for calls that actually want the whole index but included a trailing slash. We allow a prefix of length 32 for users that just want to check on the disk usage of a given block."

func GetBlockHandler(w http.ResponseWriter, req *http.Request) {

What do you think about changing the name of the response writer from w to resp?

I remember you telling me that it's common in go to use short variable names for short functions, but I feel like this function is now long enough that users won't be able to remember what w is (indeed my brain's first theory was that it was a 'w' for some kind of write flag).

http.Error(w, err.Error(), err.(*KeepError).HTTPCode)

Is it possible that this type assertion could fail? e.g. if we had some kind of unexpected file system problem like someone unplugging the disk.

If so, it seems like what we'd want to do is something like:

keep_error, valid_assertion = err.(*KeepError)
if valid_assertion {
http.Error(w, err.Error(), keep_error.HTTPCode)
} else {
http.Error(w, err.Error(), 404)
}

api_token := GetApiToken(req)
if !enforce_permissions ||
api_token == "" ||
data_manager_token != GetApiToken(req) {
http.Error(w, PermissionError.Error(), PermissionError.HTTPCode)
return
}

You call GetApiToken() twice in this segment. Can you replace the second call with api_token please?

I was hoping to find a slick way to avoid the GetApiToken() call if we don't need it, but it seems like that you can't put an assignment after a ||.

func GetApiToken(req *http.Request) string {
if auth, ok := req.Header["Authorization"]; ok {
if strings.HasPrefix(auth⁰, "OAuth ") {
return auth⁰[6:]
}
}
return ""
}

If I send an HTTP request to the keep server and place a bogus authorization header in the request, it will get overwritten somewhere, right? Can we document that somewhere in this file?

Brownie points if we can test this somehow, but that test probably belongs in another server. Maybe we can add a TODO to write it. Obviously this is functionality we want to confirm regularly.

// IsExpired returns true if the given Unix timestamp (expressed as a
// hexadecimal string) is in the past.

Please add to this comment indicating that true will also be returned if we fail to parse the timestamp_hex.

That's all, I read through the whole change!

Tim Pierce wrote:

...

func GetApiToken(req *http.Request) string {
if auth, ok := req.Header["Authorization"]; ok {
if strings.HasPrefix(auth⁰, "OAuth ") {
return auth⁰[6:]
}
}
return ""
}

If I send an HTTP request to the keep server and place a bogus authorization header in the request, it will get overwritten somewhere, right? Can we document that somewhere in this file?

I don't think I follow -- what do you mean by overwritten? The contents of the Authorization header don't get recorded anywhere permanently. Or are you thinking that a user might try to supply her own Authorization header but that the Keep client would replace it before sending the request? (I'm not sure I see how that would happen, either.)

My mistake, I got confused about how Authorization headers work.

LGTM!