Arvados

Looking at aeff59b

I don't see a check for "valid signature but timestamp is in the past". That should fail. (Maybe you should invent a test case here, and then we can add an identical test to the apiserver test suite?)

SignLocator's API would be nicer if it could accept expiry time in a native representation, rather than expecting the caller to know about the internal hexstring(unixtimestamp(expirytime)) representation.

Minor doc complaints:

"Keep returns a 401 error." → I wonder if we should consider 403 if the timestamp is in the past.

"generated...for the user with the supplied API token" → depending on how you read it, this sounds like tokens are per-user rather than per-token. Perhaps just say "generated...for the supplied API token".

Tom Clegg wrote:

Looking at aeff59b

I don't see a check for "valid signature but timestamp is in the past". That should fail. (Maybe you should invent a test case here, and then we can add an identical test to the apiserver test suite?)

Good idea - added TestVerifySignatureExpired.

SignLocator's API would be nicer if it could accept expiry time in a native representation, rather than expecting the caller to know about the internal hexstring(unixtimestamp(expirytime)) representation.

Agreed, the API should generally use native types wherever it makes sense. Done.

I also decided that GeneratePerms (now makePermSignature) should not be considered private and not tested by itself, since the only thing Keep cares about is knowing that it can sign a locator and that it can verify a signature on one.

Minor doc complaints:

"Keep returns a 401 error." → I wonder if we should consider 403 if the timestamp is in the past.

Hm, I think we could? VerifyPermission could return an error instead of a bool, so it could distinguish between nil/BadSignature/Expired.

"generated...for the user with the supplied API token" → depending on how you read it, this sounds like tokens are per-user rather than per-token. Perhaps just say "generated...for the supplied API token".

Done. 8c5b4f6

Tom Clegg wrote:

Looks great. Only two more suggestions:

The SignLocator argument "timestamp" is a bit vague. The comments (and perhaps the argument name itself) could mention that it's an expiry time.

Good idea, changed.

And doc bug:

If a request to Keep includes a locator with a valid signature and is accompanied by the proper API token, the user has permission to perform any action on that object (GET, PUT or DELETE).

The signature only gives GET permission.

Ah! That is a conceptual bug, not a doc bug (knowing that these permission signatures should be checked only on GET requests). Thanks for catching that, fixed the comments. 395b4e7

Tom Clegg wrote:

Looks great. Only two more suggestions:

The SignLocator argument "timestamp" is a bit vague. The comments (and perhaps the argument name itself) could mention that it's an expiry time.

Good idea, changed.

And doc bug:

If a request to Keep includes a locator with a valid signature and is accompanied by the proper API token, the user has permission to perform any action on that object (GET, PUT or DELETE).

The signature only gives GET permission.

Ah! That is a conceptual bug, not a doc bug (knowing that these permission signatures should be checked only on GET requests). Thanks for catching that, fixed the comments.

Fixes in 395b4e7

Some questions before I start looking at the code:

Tom Clegg wrote:

The proposed permissions model for Keep is as follows:

A Keep locator string may end with a permission hint in the form "+Asignature@timestamp". The signature string here is a cryptographic hash consisting of alphanumeric characters, and timestamp is a Unix timestamp expressed as a 32-bit integer.

"May end" or "must end"? This says "may", but the following text ("keep server expects...") seems to indicate "must".

A valid permission hint grants the user the ability to GET, PUT or DELETE an object in Keep; permission is all-or-nothing.

The permission hint may be generated either by Keep (after the user writes a block) or by the API server (if the user has can_read permission on the specified object). Keep and API server share a secret that is used to generate signatures. Permission hints do not incorporate secrets from the user; if another user acquires a locator that includes a valid permission hint, they have permission to GET, PUT or DELETE that object.

The above seems to indicate that if I copy a locator that you used, I can get/put/delete the object the locator points to. But text below seems to indicate that the user's API token is an input to signature in the permission hint in the locator, which would seem to mean that I can't use another user's locator since a distinct api token will be computed

Desired command line arguments to control permissions:

• --enable-permissions= true/false to enable or disable permission enforcement
  • For transition, maybe support a --generate-permissions flag and a --enforce-permissions flag.

What's the current state of this --generate-permissions flag? Did it make it into the change? Can you update the this doc to indicate whether it's in there or just an idea for down the road?

• Alternatively, make Keep always generate permission tokens, but only enforce them when the perms flag is turned on.
• Permissions are always required for DELETE

• --privileged-ip specifies IP addresses to be considered superuser.
  • If permissions are disabled, index (GET /index.txt) and DELETE do not work unless remote address matches --privileged-ip command line argument

What if permissions are enabled? Are requests from privileged ips treated any differently in that case?

• --signature-key-fd=fdnum to read signing key from given file descriptor

Can you explain what this means a little more please?

Good questions, I'll update the docs to reflect the current implementation more clearly. Answers below:

Misha Zatsman wrote:

Some questions before I start looking at the code:

Tom Clegg wrote:

The proposed permissions model for Keep is as follows:

A Keep locator string may end with a permission hint in the form "+Asignature@timestamp". The signature string here is a cryptographic hash consisting of alphanumeric characters, and timestamp is a Unix timestamp expressed as a 32-bit integer.

"May end" or "must end"? This says "may", but the following text ("keep server expects...") seems to indicate "must".

The server can be configured to enforce permission signatures (--enforce-permissions=true) or to ignore them. If the server is configured to enforce permissions, then GET requests must use signed locator ids; otherwise, the server will accept unsigned locators. If that helps clarify it, I can add it to the comments.

A valid permission hint grants the user the ability to GET, PUT or DELETE an object in Keep; permission is all-or-nothing.

The permission hint may be generated either by Keep (after the user writes a block) or by the API server (if the user has can_read permission on the specified object). Keep and API server share a secret that is used to generate signatures. Permission hints do not incorporate secrets from the user; if another user acquires a locator that includes a valid permission hint, they have permission to GET, PUT or DELETE that object.

The above seems to indicate that if I copy a locator that you used, I can get/put/delete the object the locator points to. But text below seems to indicate that the user's API token is an input to signature in the permission hint in the locator, which would seem to mean that I can't use another user's locator since a distinct api token will be computed

You're right, I screwed up this description in editing. If another user gets ahold of your API token, they can generate new valid signatures for a locator, but just getting a signed locator from another user will not allow them to access it. Thanks for the catch, I'll fix that.

Desired command line arguments to control permissions:

• --enable-permissions= true/false to enable or disable permission enforcement
  • For transition, maybe support a --generate-permissions flag and a --enforce-permissions flag.

What's the current state of this --generate-permissions flag? Did it make it into the change? Can you update the this doc to indicate whether it's in there or just an idea for down the road?

• --permission-key specifies the server key used to generate permission signatures
• --enforce-permissions tells Keep whether to verify signatures on GET requests

If a permission key is available, Keep will return a signed locator for every PUT request, so starting Keep with --permission-key implies that it should generate permissions.

I'll correct the docs to reflect this.

• Alternatively, make Keep always generate permission tokens, but only enforce them when the perms flag is turned on.
• Permissions are always required for DELETE

• --privileged-ip specifies IP addresses to be considered superuser.
  • If permissions are disabled, index (GET /index.txt) and DELETE do not work unless remote address matches --privileged-ip command line argument

What if permissions are enabled? Are requests from privileged ips treated any differently in that case?

The implementation that Tom and I hashed out is:

• If --enforce-permissions=false then GET /index and DELETE are forbidden.
• Instead of --privileged-ip, Keep supports a --data-manager-token flag. If Keep is started with --data-manager-token, that represents the OAuth token that will be used by the Data Manager, and any request that is authenticated with that token (i.e. has "Authorization: OAuth data-manager-token" in the headers will be able to perform these privileged operations.
• In other words, in order to perform one of these privileged requests, the server must have been started with both --enforce-permissions=true and --data-manager-token, and the request must include the data manager's OAuth token.

• --signature-key-fd=fdnum to read signing key from given file descriptor

Can you explain what this means a little more please?

This was Tom's suggestion for the command line argument to supply Keep with a permission key, to avoid exposing it via ps -auwx or the like. Keep would be started with something like keep --signature-key-fd=3 3</tmp/signature_key.sha1. The code I submitted here does the less secure thing and just supplies the key on the command line. Tom and I agreed that's okay for now and we can decide on how best to provision Keep with secure keys (e.g. maybe over websockets).

Tim Pierce wrote:

This was Tom's suggestion for the command line argument to supply Keep with a permission key, to avoid exposing it via ps -auwx or the like. Keep would be started with something like keep --signature-key-fd=3 3</tmp/signature_key.sha1. The code I submitted here does the less secure thing and just supplies the key on the command line. Tom and I agreed that's okay for now and we can decide on how best to provision Keep with secure keys (e.g. maybe over websockets).

• --signature-key foo is good to unblock dev process.
• Environment variable is good to unblock first release.
• Read from file descriptor is a secure solution (or to be precise, can be used in a secure solution.)
• Actually taking on the work of key distribution is TBD.

(Added note to #2763, i.e., doesn't have to hold up this merge.)