tahoe-lafs/tahoe-lafs
tahoe-lafs
/
tahoe-lafs
2
0
Fork 0
Code Issues Pull Requests 23 Packages Projects Releases 1 Wiki Activity

HTTP API: secrets infrastructure #1166

Merged
itamarst merged 18 commits from 3848.http-api-start-immutables into master 2021-12-22 18:39:51 +00:00
Conversation 1 Commits 18 Files Changed 5 +306 -37
itamarst commented 2021-12-16 16:48:45 +00:00 (Migrated from github.com)
Copy Link

Fixes https://tahoe-lafs.org/trac/tahoe-lafs/ticket/3848

Part of review should be validating this matches documented behavior in the HTTP protocol spec.

Fixes https://tahoe-lafs.org/trac/tahoe-lafs/ticket/3848 Part of review should be validating this matches documented behavior in the HTTP protocol spec.
coveralls commented 2021-12-17 14:31:57 +00:00 (Migrated from github.com)
Copy Link

Coverage Status

Coverage increased (+0.03%) to 95.71% when pulling 58a71517c1 on 3848.http-api-start-immutables into bc8889f32f on master.

[![Coverage Status](https://coveralls.io/builds/45194877/badge)](https://coveralls.io/builds/45194877) Coverage increased (+0.03%) to 95.71% when pulling **58a71517c1fdc74b735876541d2dfa0ddfb2e5c4 on 3848.http-api-start-immutables** into **bc8889f32ffef729d96e9161759ddb66554906b2 on master**.
meejah (Migrated from github.com) reviewed 2021-12-17 18:13:45 +00:00
src/allmydata/storage/http_server.py
@ -24,40 +28,95 @@ from cbor2 import dumps
from .server import StorageServer
meejah (Migrated from github.com) commented 2021-12-17 18:13:45 +00:00
Copy Link

this should probably use timing_safe_compare (or however we call it here) I think, or clients may be able to learn the correct swissnum.

this should probably use `timing_safe_compare` (or however we call it here) I think, or clients may be able to learn the correct swissnum.
itamarst (Migrated from github.com) reviewed 2021-12-20 16:17:58 +00:00
src/allmydata/storage/http_server.py
@ -24,40 +28,95 @@ from cbor2 import dumps
from .server import StorageServer
itamarst (Migrated from github.com) commented 2021-12-20 16:17:58 +00:00
Copy Link

Good point, done.

Good point, done.
exarkun (Migrated from github.com) approved these changes 2021-12-20 18:19:21 +00:00
exarkun (Migrated from github.com) left a comment
Copy Link

Thanks. A few comments inline. Please at least address the comments about docstrings, the rest are optional. Then merge (unless you want another review).

Thanks. A few comments inline. Please at least address the comments about docstrings, the rest are optional. Then merge (unless you want another review).
docs/proposed/http-storage-node-protocol.rst
@ -372,0 +372,4 @@
There are also, for some endpoints, secrets sent via ``X-Tahoe-Authorization`` headers.
If these are:
1. Missing.
exarkun (Migrated from github.com) commented 2021-12-20 17:06:06 +00:00
Copy Link

A couple lines above the text says "If credentials are not presented ... the request receives a 401". That should be updated, I suppose.

A couple lines above the text says "If credentials are not presented ... the request receives a 401". That should be updated, I suppose.
src/allmydata/storage/http_server.py
exarkun (Migrated from github.com) commented 2021-12-20 17:15:13 +00:00
Copy Link

Looks like this is no longer a TODO?

Looks like this is no longer a TODO?
src/allmydata/test/test_storage_http.py
@ -14,36 +14,217 @@ if PY2:
from future.builtins import filter, map, zip, ascii, chr, hex, input, next, oct, open, pow, round, super, bytes, dict, list, object, range, str, max, min # noqa: F401
exarkun (Migrated from github.com) commented 2021-12-20 17:16:18 +00:00
Copy Link

This might be a good place to use Hypothesis

This might be a good place to use Hypothesis
exarkun (Migrated from github.com) commented 2021-12-20 17:18:01 +00:00
Copy Link

This test might benefit from Hypothesis. Hypothesis can generate zero to three secrets and the test can verify they round-trip through an encoder/decoder pair.

This test might benefit from Hypothesis. Hypothesis can generate zero to three secrets and the test can verify they round-trip through an encoder/decoder pair.
exarkun (Migrated from github.com) commented 2021-12-20 17:19:14 +00:00
Copy Link

Likewise, Hypothesis would help here - generate sets of required secrets and different sets of secrets to provide and make just have a single assertRaises.

Likewise, Hypothesis would help here - generate sets of required secrets and different sets of secrets to provide and make just have a single `assertRaises`.
exarkun (Migrated from github.com) commented 2021-12-20 17:21:28 +00:00
Copy Link

I'd probably use Hypothesis here but it's a bit more tedious since it requires a strategy for all those not-a-valid-secret cases (although if you lean on assume then that isn't necessarily much work, but it's slower at runtime).

Avoiding Hypothesis, I'd make a separate test method for each case so you can see which cases fail from one run.

I'd probably use Hypothesis here but it's a bit more tedious since it requires a strategy for all those not-a-valid-secret cases (although if you lean on `assume` then that isn't necessarily much work, but it's slower at runtime). Avoiding Hypothesis, I'd make a separate test method for each case so you can see which cases fail from one run.
exarkun (Migrated from github.com) commented 2021-12-20 17:22:31 +00:00
Copy Link

This would be a nice testtools Fixture.

This would be a nice testtools Fixture.
itamarst (Migrated from github.com) reviewed 2021-12-20 18:47:40 +00:00
docs/proposed/http-storage-node-protocol.rst
@ -372,0 +372,4 @@
There are also, for some endpoints, secrets sent via ``X-Tahoe-Authorization`` headers.
If these are:
1. Missing.
itamarst (Migrated from github.com) commented 2021-12-20 18:47:40 +00:00
Copy Link

I think it's correct? There are three scenarios:

  1. Wrong swissnum -> 401
  2. Missing/additional secrets -> 400
  3. Wrong secret -> 401
I think it's correct? There are three scenarios: 1. Wrong swissnum -> 401 2. Missing/additional secrets -> 400 3. Wrong secret -> 401
exarkun (Migrated from github.com) reviewed 2021-12-20 18:54:24 +00:00
docs/proposed/http-storage-node-protocol.rst
@ -372,0 +372,4 @@
There are also, for some endpoints, secrets sent via ``X-Tahoe-Authorization`` headers.
If these are:
1. Missing.
exarkun (Migrated from github.com) commented 2021-12-20 18:54:24 +00:00
Copy Link

Oh, indeed. I mixed up swissnum with additional secrets.

Still, do we want "missing swissnum" to be a 400, like "missing other secrets" is? I guess "401" is conventional for a missing "Authorization" header... 🤷

Oh, indeed. I mixed up swissnum with additional secrets. Still, do we want "missing swissnum" to be a 400, like "missing other secrets" is? I guess "401" is conventional for a missing "Authorization" header... :shrug:
itamarst (Migrated from github.com) reviewed 2021-12-21 16:11:37 +00:00
docs/proposed/http-storage-node-protocol.rst
@ -372,0 +372,4 @@
There are also, for some endpoints, secrets sent via ``X-Tahoe-Authorization`` headers.
If these are:
1. Missing.
itamarst (Migrated from github.com) commented 2021-12-21 16:11:37 +00:00
Copy Link

In theory none of these error code should happen in real world.

In theory none of these error code should happen in real world.
itamarst (Migrated from github.com) reviewed 2021-12-22 16:51:35 +00:00
src/allmydata/test/test_storage_http.py
@ -14,36 +14,217 @@ if PY2:
from future.builtins import filter, map, zip, ascii, chr, hex, input, next, oct, open, pow, round, super, bytes, dict, list, object, range, str, max, min # noqa: F401
itamarst (Migrated from github.com) commented 2021-12-22 16:51:35 +00:00
Copy Link

Decided to just split it up.

Decided to just split it up.
Sign in to join this conversation.
Reviewers
No reviewers
exarkun
Labels
Clear labels   
Benchmarking and Performance

   
HTTP Storage Protocol

   
Nevow Removal

   
Python 3 Porting

   
not-for-merge
No Label
Benchmarking and Performance
HTTP Storage Protocol
Nevow Removal
Python 3 Porting
not-for-merge
Milestone
Clear milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
No Assignees
1 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: tahoe-lafs/tahoe-lafs#1166
No description provided.
Delete Branch "3848.http-api-start-immutables"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?