Feature #2328

Verify and generate permission hints in Keep

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

Status:
Resolved
Priority:
Normal
Assigned To:
Tim Pierce
Category:
-
Start date:
04/30/2014
Due date:
% Done:

100%

Estimated time:
(Total: 32.50 h)
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

Task #2728: Review 2328-keep-permission-hintsResolvedTim Pierce

Task #2653: Permissions command-line flagsResolvedTim Pierce

Task #2652: Keep interface for generating and verifying tokensResolvedTim Pierce

Task #2722: Review 2328-signatures-for-integration-testing branchResolvedTim Pierce

Task #2651: Finalize spec for permissions tokensResolvedTim Pierce

Associated revisions

Revision cd08497d
Added by Tom Clegg over 7 years ago

Merge branch '2328-signatures-for-integration-testing'

refs #2328

Revision 8023ae39 (diff)
Added by Tim Pierce over 7 years ago

Added permission helper functions.

GeneratePerms returns a string representing the signed permission hint
for the blob identified by blob_hash, api_token and timestamp.

SignLocator takes a blob_locator, an api_token and a timestamp, and
returns a signed locator string.

VerifySignature returns true if the signature on the signed_locator can
be verified using the given api_token.

Refs #2328.

Revision aeff59bf (diff)
Added by Tim Pierce over 7 years ago

Update docs. (refs #2328)

Revision 8c5b4f6d (diff)
Added by Tim Pierce over 7 years ago

Incorporating code review. (refs #2328)

Revision 395b4e72 (diff)
Added by Tim Pierce over 7 years ago

Resolve code review (closes #2328)

Fix doc comments and argument names for the expiry timestamp arguments
for SignLocator and makePermSignature.

Revision 7c8bfeb8 (diff)
Added by Tim Pierce over 7 years ago

Add --permissions-key flag.

The --permissions-key flag initializes the PermissionSecret to the
string of bytes in its argument. (refs #2328)

Revision bdc9139d (diff)
Added by Tim Pierce over 7 years ago

Check GET permissions in the HTTP handler.

Move the check for the permission signature to the HTTP handler, so the
back end can still call GetBlock without having to move permissions all
the way down the stack. Refs #2328

Revision 2b8857f6 (diff)
Added by Tim Pierce over 7 years ago

Added permission flags and unit tests.

New flags:
--enforce-permissions enables permission checking for GET requests.
--permission-ttl sets the expiration time on signed locators returned
by PUT.
--data-manager-token defines a privileged token for the Data Manager
to issue DELETE and "GET /index" requests.

PUT now responds with a signed locator if a permission key has been
set.

Unit test TestGetHandler tests the GetBlockHandler both when permission
checking is off, and tests signed, unsigned and expired requests when
permission checking is enabled.

Refs #2328

Revision b63b4b51 (diff)
Added by Tim Pierce over 7 years ago

Reset enforce_permissions between tests. (refs #2328)

Revision 297f48f2 (diff)
Added by Tim Pierce over 7 years ago

Require enforce_permissions and API token as necessary.

Unqualified /index requests require enforce_permissions to be enabled
and an API token to be supplied with the request.

SignLocator should return an unsigned locator if no API token was
supplied.

Refs #2328

Revision 1a4846b2 (diff)
Added by Tim Pierce over 7 years ago

Split out HTTP handler tests into their own file.

Refs #2328

Revision d5c8f2dc (diff)
Added by Tim Pierce over 7 years ago

Added handler_test.go. (refs #2328)

Revision f71c9e59
Added by Tim Pierce over 7 years ago

Merge branch '2328-keep-permission-flags' (closes #2328)

History

#1 Updated by Tom Clegg over 7 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

#2 Updated by Tom Clegg over 7 years ago

  • Description updated (diff)

#3 Updated by Tom Clegg over 7 years ago

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

#4 Updated by Tom Clegg over 7 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

#5 Updated by Tom Clegg over 7 years ago

  • Story points changed from 3.0 to 2.0

#6 Updated by Tim Pierce over 7 years ago

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

#7 Updated by Tim Pierce over 7 years ago

  • Story points changed from 3.0 to 4.0

#8 Updated by Tom Clegg over 7 years ago

  • Description updated (diff)

#9 Updated by Tim Pierce over 7 years ago

  • Description updated (diff)

#10 Updated by Tim Pierce over 7 years ago

  • Description updated (diff)

#11 Updated by Tom Clegg over 7 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".

#12 Updated by Tim Pierce over 7 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

#13 Updated by Tom Clegg over 7 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.

#14 Updated by Tim Pierce over 7 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

#15 Updated by Tim Pierce over 7 years ago

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

Applied in changeset arvados|commit:395b4e72d33c5b4df931c029a45870d354c32312.

#16 Updated by Tim Pierce over 7 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

#17 Updated by Tim Pierce over 7 years ago

  • Status changed from Resolved to In Progress

#18 Updated by Misha Zatsman over 7 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?

#19 Updated by Tim Pierce over 7 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).

#20 Updated by Tim Pierce over 7 years ago

  • Status changed from In Progress to Resolved

#21 Updated by Tim Pierce over 7 years ago

  • Description updated (diff)

#22 Updated by Tom Clegg over 7 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.)

#23 Updated by Tim Pierce over 7 years ago

  • Description updated (diff)

Also available in: Atom PDF