Feature #2328
closedVerify and generate permission hints in Keep
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.
- 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
--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
andDELETE
).--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 permitGET /index
andDELETE
requests from the data manager. - If
--enforce-permissions=false
, the server will fulfill GET requests without regard to permission signatures. It will also prohibitGET /index
andDELETE
requests entirely.
- If
--signature-ttl=num_seconds
to control expiry time of signatures
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
Updated by Tom Clegg over 10 years ago
- Target version set to 2014-05-07 Storing and Organizing Data
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
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
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".
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
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.
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
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.
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
Updated by Tim Pierce over 10 years ago
- Status changed from Resolved to In Progress
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
) andDELETE
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?
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.
Currently the available flags are: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
) andDELETE
do not work unless remote address matches--privileged-ip
command line argumentWhat 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
thenGET /index
andDELETE
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 descriptorCan 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).
Updated by Tim Pierce over 10 years ago
- Status changed from In Progress to Resolved
Updated by Tom Clegg over 10 years ago
Tim Pierce wrote:
To amend the above / clarify my thoughts on this: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 likekeep --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.)