Bug #4219

[Keep] keepstore should not fail permission signature verification just because extra hints are present in URL.

Added by Tom Clegg almost 5 years ago. Updated almost 5 years ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Category:
Keep
Target version:
Start date:
10/15/2014
Due date:
% Done:

100%

Estimated time:
(Total: 2.00 h)
Story points:
0.5

Subtasks

Task #4220: Ignore extra hints when checking signatures. Write tests.ResolvedTom Clegg

Task #4221: Review 4219-verify-with-hintsResolvedTim Pierce

Associated revisions

Revision b19a6058
Added by Tom Clegg almost 5 years ago

Merge branch '4219-verify-with-hints' closes #4219

History

#1 Updated by Tom Clegg almost 5 years ago

  • Category set to Keep
  • Status changed from New to In Progress
  • Assigned To set to Tom Clegg

#2 Updated by Tim Pierce almost 5 years ago

Review @ f39807a3c:

services/keepstore/perms.go:

The API server generates signatures on hash+size, so VerifySignatures needs to include the size hint when verifying.

^([[:xdigit:]]{32}).*\+A([[:xdigit:]]{40})@([[:xdigit:]]{8})

should be:

^([[:xdigit:]]{32}(\+[0-9]+)?).*\+A([[:xdigit:]]{40})@([[:xdigit:]]{8})

Consequently:

blob_hash := matches[1]
sig_hex := matches[3]
exp_hex := matches[4]

We should make sure the tests use hash+size for signing locators.

#3 Updated by Tim Pierce almost 5 years ago

Also, some time ago Peter found regexp.MustCompile to compile a regex or panic if it can't. I don't know why I missed it when I was starting out. We might as well use it here to make everything simpler.

#4 Updated by Tom Clegg almost 5 years ago

The API server generates signatures on hash+size, so VerifySignatures needs to include the size hint when verifying.

I think you'll find on closer inspection that it doesn't.

Also, some time ago Peter found regexp.MustCompile to compile a regex or panic if it can't. I don't know why I missed it when I was starting out. We might as well use it here to make everything simpler.

Ah yes, and now it can be a global, so we don't compile it N times! → 5e2eeea

#5 Updated by Tim Pierce almost 5 years ago

LGTM. But can someone please explain to me what size hints are for in the first place? :-)

#6 Updated by Tim Pierce almost 5 years ago

P.S. we should include tests for validating signatures both with and without size hints, just in case I screw this up again. I should have included them in the first place. You already have an LGTM so feel free to merge this as-is, I'll file a story to add the appropriate tests.

#7 Updated by Tom Clegg almost 5 years ago

Tim Pierce wrote:

P.S. we should include tests for validating signatures both with and without size hints, just in case I screw this up again. I should have included them in the first place. You already have an LGTM so feel free to merge this as-is, I'll file a story to add the appropriate tests.

Added in 1d0d4b1

LGTM. But can someone please explain to me what size hints are for in the first place? :-)

Seeking into >64MiB files in manifests.

#8 Updated by Anonymous almost 5 years ago

  • Status changed from In Progress to Resolved

Applied in changeset arvados|commit:b19a6058168a290fe789b2228c13935edc6e5546.

Also available in: Atom PDF