Story #8936

[Keepstore+API] Feed configured TTL into the real Keep block signing key

Added by Brett Smith over 3 years ago. Updated over 3 years ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Radhika Chippada
Category:
-
Target version:
Start date:
04/18/2016
Due date:
% Done:

100%

Estimated time:
(Total: 0.00 h)
Story points:
1.0

Description

The real signing key should be generated from both the random configured signing key, and the configured block TTL. This way, when the sysadmin changes the TTL, the effective signing key changes, and clients are in a better position to detect that and retry write operations if needed.

Update:

  • The API server key generation logic (app/models/blob.rb#generate_signature), see note-4 below. Make sure to add a test that changing the TTL causes the permission signature to change.
  • The Keepstore/SDK key generation logic (sdk/go/keepclient/perms.go). Make sure to update the "known good signature" fixture(s).
  • The install guide documentation - Where the TTL is mentioned, add a note that it effectively becomes part of the signing key, and will cause clients to retry or fail if it is changed while they are in progress.
  • The "Upgrading to master" wiki page, to note that you must upgrade both keepstore and API server at the same time, with no operations in progress and nothing in arv-put resume caches -- otherwise operations will fail.

Subtasks

Task #8989: Review branch 8936-ttl-in-signing-keyResolvedRadhika Chippada

Task #9038: Review branch 8936-ttl-in-signing-key for proper handling of ttl in keep-block-check and keep-rsync.ResolvedTom Clegg

Task #9039: Update the Upgrading_to_master wiki pageResolvedRadhika Chippada

Associated revisions

Revision 3c88abd3
Added by Radhika Chippada over 3 years ago

closes #8936
Merge branch '8936-ttl-in-signing-key'

Revision 8cc6fc04
Added by Radhika Chippada over 3 years ago

closes #8936
Merge branch '8936-ttl-in-signing-key'

History

#1 Updated by Brett Smith over 3 years ago

  • Description updated (diff)
  • Subject changed from [Keepstore/API] Feed configured TTL into the real Keep block signing key to [Keepstore+API] Feed configured TTL into the real Keep block signing key

#2 Updated by Brett Smith over 3 years ago

  • Description updated (diff)

#3 Updated by Brett Smith over 3 years ago

  • Target version set to Arvados Future Sprints

#4 Updated by Tom Clegg over 3 years ago

Old signature algorithm

HMAC((block_md5_hex "@" client_token "@" expiry_epoch_hex), permission_key)

New signature algorithm

HMAC((block_md5_hex "@" client_token "@" expiry_epoch_hex "@" ttl_seconds_hex), permission_key)

#5 Updated by Brett Smith over 3 years ago

  • Description updated (diff)
  • Story points set to 1.0

#6 Updated by Tom Clegg over 3 years ago

  • Description updated (diff)

#7 Updated by Brett Smith over 3 years ago

  • Assigned To set to Radhika Chippada
  • Target version changed from Arvados Future Sprints to 2016-04-27 sprint

#8 Updated by Radhika Chippada over 3 years ago

  • Status changed from New to In Progress

#9 Updated by Radhika Chippada over 3 years ago

Branch 8936-ttl-in-signing-key is ready for review.

I will update the wiki page https://dev.arvados.org/projects/arvados/wiki/Upgrading_to_master after the code is merged into master.

#10 Updated by Radhika Chippada over 3 years ago

After the branch is merged, I will update the wiki page https://dev.arvados.org/projects/arvados/wiki/Upgrading_to_master with the following (with the correct commit hash).

2016-04-18: commit:6a202a27 makes blob-signing-ttl part of blob-signing-key. 
* This will cause clients to retry or fail if changed while they are in progress.

#11 Updated by Tom Clegg over 3 years ago

Docs:

Proposed an alternate version (af754a6 on 8936-ttl-in-signing-key-TC) of the doc/comment updates in 6a202a2. I'd say "TTL will become part of the key" is neither technically correct (if you look closely enough at the HMAC) nor a plain way to say what the user needs to know, i.e., the rule for matching up your TTL configs has changed from "should not be smaller" to "absolutely must be equal".

Code:

blobSigningTTL should be renamed to blobSignatureTTL, to match the corresponding variables elsewhere.

makePermSignature() should take blobSignatureTTL as a string argument (and SignLocator() should have the native-type-to-string conversion), consistent with the other args.

The conversion should be hexadecimal instead of decimal, consistent with expiry time (this is arbitrary but it seems reasonable to be consistent if there's no reason not to).
  • Rails.configuration.blob_signature_ttl.to_s(16)
  • strconv.FormatInt(int64(blobSignatureTTL.Seconds()), 16)

Instead of time.Duration(1) * time.Second, how about just time.Second? Logically, multiplying durations isn't meaningful, so 123 * time.Second seems more sensible when the constant isn't 1 -- e.g., 2*7*24*time.Hour. Likewise, 0*time.Second in flags.Duration() should just be 0.

"If not provided, this will be retrieved from the keepservers." ...this should say "from the API server's discovery document".

keep-rsync needs two TTL flags: one for source and one for destination.

API tests are failing for me in source:services/api/test/unit/blob_test.rb -- I'm guessing this is related to the fact that (if I'm reading correctly) the API server's tests use blob_signature_ttl=1209600 while the Go tests use blob_signature_ttl=1. In that case the Go tests should be updated to use the more realistic TTL, and the "known good" signature should be updated accordingly.

[1/0] BlobTest#test_generate_predictable_invincible_signature = 1.14 s
  1) Failure:
BlobTest#test_generate_predictable_invincible_signature [/home/tom/go/src/git.curoverse.com/arvados.git/services/api/test/unit/blob_test.rb:28]:
<"acbd18db4cc2f85cedef654fccc4a4d8+3+A44362129a92a48d02b2e0789c597f970f3b1faf3@7fffffff"> expected but was
<"acbd18db4cc2f85cedef654fccc4a4d8+3+Ab63876105ae00891386769bba38f917e7a594bfc@7fffffff">.

Nit: I think it would be better to have source:services/api/app/models/blob.rb follow the existing pattern we use with blob_signing_key -- i.e., generate_signature() takes a mandatory ttl argument, and verify_signature() and sign_locator() pass Rails.configuration.blob_signature_ttl. As it stands, a caller can pass a ttl argument to sign_locator(), and this gets used to generate the expiry timestamp, but it doesn't get passed along to generate_signature() so the Rails.configuration value is used instead. This seems a bit weird. I'm calling it a nit because it doesn't seem to have any practical consequences until/unless we move generate_signature() out into the Ruby SDK.

#12 Updated by Radhika Chippada over 3 years ago

Docs: Proposed an alternate version (af754a6 on 8936-ttl-in-signing-key-TC) ...

Thanks. I merged this into my branch.

blobSigningTTL should be renamed to blobSignatureTTL, to match the corresponding variables elsewhere.

Done

makePermSignature() should take blobSignatureTTL as a string argument (and SignLocator() should have the native-type-to-string conversion), consistent with the other args.

Done

The conversion should be hexadecimal instead of decimal, consistent with expiry time (this is arbitrary but it seems reasonable to be consistent if there's no reason not to).

Done

Instead of time.Duration(1) * time.Second, how about just time.Second?

Done

"If not provided, this will be retrieved from the keepservers." ...this should say "from the API server's discovery document".

Done

keep-rsync needs two TTL flags: one for source and one for destination.

The destination side does not use any signing features. So, instead I renamed the src ttl so that it is not confusing

API tests are failing for me in source:services/api/test/unit/blob_test.rb

Updated to use a specific duration rather than expecting any specific value

Nit: I think it would be better to have source:services/api/app/models/blob.rb follow the existing pattern we use with blob_signing_key ...

Done

#13 Updated by Tom Clegg over 3 years ago

blobSigningTTL should be renamed to blobSignatureTTL, to match the corresponding variables elsewhere.

Done

Nearly. Please update keep-rsync and keep-block-check command line flags. :)

keep-rsync needs two TTL flags: one for source and one for destination.

The destination side does not use any signing features. So, instead I renamed the src ttl so that it is not confusing

Ah, indeed. Can we call the flag "-src-blob-signature-ttl" too, to make it more obvious that it's related only to the other "-src-..." arguments?

API tests are failing for me in source:services/api/test/unit/blob_test.rb
...
the Go tests should be updated to use the more realistic TTL, and the "known good" signature should be updated accordingly

Updated to use a specific duration rather than expecting any specific value

Looks like you did the opposite change, though? IMO we should use the realistic TTL of 2 weeks for our "known good" example, instead of 1 second, which is a special case in that it encodes correctly to "1" even if some code mistakenly uses decimal instead of hexadecimal.

Need to add ". " at the end of the new sentence here in keepstore.go, so we don't get "...existing signaturesSee ..."

-               "Lifetime of blob permission signatures. "+
+               "Lifetime of blob permission signatures. Modifying the ttl will invalidate all existing signatures"+
                        "See services/api/config/application.default.yml.")

The rest LGTM, thanks!

#14 Updated by Radhika Chippada over 3 years ago

Please update keep-rsync and keep-block-check command line flags. :)

Thanks for noticing. I updated these two as well.

keep-rsync needs two TTL flags: one for source and one for destination. ... Can we call the flag "-src-blob-signature-ttl" too, to make it more obvious that it's related only to the other "-src-..." arguments?

Done

API tests are failing for me in source:services/api/test/unit/blob_test.rb ... Looks like you did the opposite change, though?

Updated the go tests to use a blob-signature-ttl other than 1s.

Need to add ". " at the end of the new sentence here in keepstore.go, so we don't get "...existing signaturesSee ..."

Updated

#15 Updated by Radhika Chippada over 3 years ago

  • Status changed from In Progress to Resolved
  • % Done changed from 0 to 100

Applied in changeset arvados|commit:3c88abd3cb33cbe80bb81a7cca779fe668036c9e.

#16 Updated by Radhika Chippada over 3 years ago

  • Status changed from Resolved to In Progress

#17 Updated by Radhika Chippada over 3 years ago

The upgrading-to-master wiki page will be updated as follows:

Modifying blob_signature_ttl invalidates existing signatures,  which can cause programs that are in progress such as arv-put, arv-get, Crunch jobs et al to fail.
To avoid errors, update only when no such processes are running. In addition, keepstore and API servers must use the same value for blob-signature-ttl.

#18 Updated by Tom Clegg over 3 years ago

8936-ttl-in-signing-key @ f5617be LGTM, thanks.

#19 Updated by Radhika Chippada over 3 years ago

  • Status changed from In Progress to Resolved

Applied in changeset arvados|commit:8cc6fc047506e3bead524f18416b78fe068c70fd.

Also available in: Atom PDF