Project

General

Profile

Actions

Bug #4219

closed

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

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

Status:
Resolved
Priority:
Normal
Assigned To:
Category:
Keep
Target version:
Story points:
0.5

Subtasks 2 (0 open2 closed)

Task #4220: Ignore extra hints when checking signatures. Write tests.ResolvedTom Clegg10/15/2014Actions
Task #4221: Review 4219-verify-with-hintsResolvedTim Pierce10/15/2014Actions
Actions #1

Updated by Tom Clegg over 9 years ago

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

Updated by Tim Pierce over 9 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.

Actions #3

Updated by Tim Pierce over 9 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.

Actions #4

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

Actions #5

Updated by Tim Pierce over 9 years ago

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

Actions #6

Updated by Tim Pierce over 9 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.

Actions #7

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

Actions #8

Updated by Anonymous over 9 years ago

  • Status changed from In Progress to Resolved

Applied in changeset arvados|commit:b19a6058168a290fe789b2228c13935edc6e5546.

Actions

Also available in: Atom PDF