Project

General

Profile

Actions

Feature #2328

closed

Verify and generate permission hints in Keep

Added by Tom Clegg over 10 years ago. Updated over 10 years ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Tim Pierce
Category:
-
Story points:
4.0
Release relationship:
Auto

Description

The proposed permissions model for Keep is as follows:

A Keep locator string may end with a permission hint in the form "+Asignature@timestamp". If a Keep server is configured to enforce permissions (via the --enforce-permissions flag), then GET requests for blocks must supply a block locator with a valid permission hint. If --enforce-permissions=false, then the server will accept both signed and unsigned locators, and will not attempt to verify signatures.

The signature string here is a cryptographic hash consisting of alphanumeric characters, and timestamp is a 32-bit Unix timestamp expressed as an 8-digit hexadecimal number.

A valid permission hint grants the user the ability to GET that object in Keep.

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). The hint is a SHA-1 hash generated from a combination of a server key (shared by Keep and the API server), the user's API token, and an expiration timestamp.

To verify a permission hint, Keep generates a new hint for the requested object (using the locator string, the timestamp, the permission secret and the user's API token, which must appear in the request headers) and compares it against the hint included in the request. If the permissions do not match, or if the API token is not present, Keep returns a 401 error.

GET and HEAD:
  • Keep server expects each read request to come with an API token (in HTTP headers) and a string +Asignature@timestamp (appended to the block hash)
  • if the timestamp is in the past, 403.
  • if the signature is invalid, 401.
PUT:
  • Keep server expects each write request to come with an API token (in HTTP headers)
  • Keep server generates a +Asignature@timestamp string and appends it to the hash in the response after a successful PUT
Reference: The following command line arguments are used to configure permission generation and enforcement:
  • --permission-key-file=path specifies a file which contains the permission key. If this argument is supplied, PUT requests will be required to include an OAuth token, and Keep will respond to PUT requests with a signed locator string.
  • --data-manager-token-file=path specifies a file which contains an OAuth token that identifies the Data Manager. Requests that include this OAuth token will be allowed to perform privileged operations (GET /index and DELETE).
  • --enforce-permissions= true/false to enable or disable permission enforcement.
    • If --enforce-permissions=true, the server will reject any GET requests for blocks that lack a valid signature, and will permit GET /index and DELETE requests from the data manager.
    • If --enforce-permissions=false, the server will fulfill GET requests without regard to permission signatures. It will also prohibit GET /index and DELETE requests entirely.
  • --signature-ttl=num_seconds to control expiry time of signatures

Subtasks 5 (0 open5 closed)

Task #2728: Review 2328-keep-permission-hintsResolvedTim Pierce04/30/2014Actions
Task #2653: Permissions command-line flagsResolvedTim Pierce05/05/2014Actions
Task #2652: Keep interface for generating and verifying tokensResolvedTim Pierce04/30/2014Actions
Task #2722: Review 2328-signatures-for-integration-testing branchResolvedTim Pierce04/30/2014Actions
Task #2651: Finalize spec for permissions tokensResolvedTim Pierce04/30/2014Actions
Actions #1

Updated by Tom Clegg over 10 years ago

  • Subject changed from Use Keep permission signature hints in SDKs, collections.get, collections.create to Verify and generate Keep permission signature hints in SDKs, collections.get, collections.create, Keep read, Keep write
  • Story points changed from 2.0 to 3.0
Actions #2

Updated by Tom Clegg over 10 years ago

  • Description updated (diff)
Actions #3

Updated by Tom Clegg over 10 years ago

  • Target version set to 2014-05-07 Storing and Organizing Data
Actions #4

Updated by Tom Clegg over 10 years ago

  • Subject changed from Verify and generate Keep permission signature hints in SDKs, collections.get, collections.create, Keep read, Keep write to Verify and generate permission hints in Keep
Actions #5

Updated by Tom Clegg over 10 years ago

  • Story points changed from 3.0 to 2.0
Actions #6

Updated by Tim Pierce over 10 years ago

  • Description updated (diff)
  • Assigned To set to Tim Pierce
  • Story points changed from 2.0 to 3.0
Actions #7

Updated by Tim Pierce over 10 years ago

  • Story points changed from 3.0 to 4.0
Actions #8

Updated by Tom Clegg over 10 years ago

  • Description updated (diff)
Actions #9

Updated by Tim Pierce over 10 years ago

  • Description updated (diff)
Actions #10

Updated by Tim Pierce over 10 years ago

  • Description updated (diff)
Actions #11

Updated by Tom Clegg over 10 years ago

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".

Actions #12

Updated by Tim Pierce over 10 years ago

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

Actions #13

Updated by Tom Clegg over 10 years ago

  • Description updated (diff)

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.

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.

Actions #14

Updated by Tim Pierce over 10 years ago

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

Actions #15

Updated by Tim Pierce over 10 years ago

  • Status changed from New to Resolved
  • % Done changed from 21 to 100

Applied in changeset arvados|commit:395b4e72d33c5b4df931c029a45870d354c32312.

Actions #16

Updated by Tim Pierce over 10 years ago

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

Actions #17

Updated by Tim Pierce over 10 years ago

  • Status changed from Resolved to In Progress
Actions #18

Updated by Misha Zatsman over 10 years ago

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?

Actions #19

Updated by Tim Pierce over 10 years ago

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?

Currently the available flags are:
  • --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).

Actions #20

Updated by Tim Pierce over 10 years ago

  • Status changed from In Progress to Resolved
Actions #21

Updated by Tim Pierce over 10 years ago

  • Description updated (diff)
Actions #22

Updated by Tom Clegg over 10 years ago

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).

To amend the above / clarify my thoughts on this:
  • --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.)

Actions #23

Updated by Tim Pierce over 10 years ago

  • Description updated (diff)
Actions

Also available in: Atom PDF