Arvados

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)

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.

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.

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.

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

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

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!

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

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.

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