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

Support the creation of a new mutable object with a pre-determined signature key #1245

Merged
exarkun merged 28 commits from 3962.pre-determined-rsa-keys into master 2023-01-13 17:28:35 +00:00
Conversation 2 Commits 28 Files Changed 22 +513 -474
exarkun commented 2023-01-06 20:47:58 +00:00 (Migrated from github.com)
Copy Link

For https://tahoe-lafs.org/trac/tahoe-lafs/ticket/3962 in support of https://tahoe-lafs.org/trac/tahoe-lafs/ticket/3961

For https://tahoe-lafs.org/trac/tahoe-lafs/ticket/3962 in support of https://tahoe-lafs.org/trac/tahoe-lafs/ticket/3961
coveralls commented 2023-01-07 13:15:03 +00:00 (Migrated from github.com)
Copy Link

Coverage Status

Coverage: 94.849%. Remained the same when pulling bdad577e41 on exarkun:3962.pre-determined-rsa-keys into d3a40b8430 on tahoe-lafs:master.

[![Coverage Status](https://coveralls.io/builds/55933333/badge)](https://coveralls.io/builds/55933333) Coverage: 94.849%. Remained the same when pulling **bdad577e41a58d771657324c3b7c5fc606d26656 on exarkun:3962.pre-determined-rsa-keys** into **d3a40b84304f6bfd2cca979f9ed90513f62d7fb8 on tahoe-lafs:master**.
exarkun (Migrated from github.com) reviewed 2023-01-11 20:54:30 +00:00
src/allmydata/crypto/rsa.py
exarkun (Migrated from github.com) commented 2023-01-11 20:54:30 +00:00
Copy Link

There should be no change of behavior in this function. All of the changes are to fix complaints mypy has about this function now that it is annotated.

There should be no change of behavior in this function. All of the changes are to fix complaints mypy has about this function now that it is annotated.
exarkun (Migrated from github.com) reviewed 2023-01-11 20:54:57 +00:00
src/allmydata/crypto/rsa.py
exarkun (Migrated from github.com) commented 2023-01-11 20:54:57 +00:00
Copy Link

Try as I might, I could not convince mypy that the private_bytes method exists.

Try as I might, I could not convince mypy that the `private_bytes` method exists.
itamarst (Migrated from github.com) approved these changes 2023-01-12 16:46:50 +00:00
itamarst (Migrated from github.com) left a comment
Copy Link

Thank you, address and merge.

Thank you, address and merge.
src/allmydata/client.py
itamarst (Migrated from github.com) commented 2023-01-12 16:04:14 +00:00
Copy Link

"This valuely independently" is confusing.

"This valuely independently" is confusing.
itamarst (Migrated from github.com) commented 2023-01-12 16:05:20 +00:00
Copy Link

Maybe phrase this positively: "Most users and callers will just want to pass in None here; the most common use-case for passing in a key is writing compliance tests." or something.

Maybe phrase this positively: "Most users and callers will just want to pass in None here; the most common use-case for passing in a key is writing compliance tests." or something.
src/allmydata/scripts/cli.py
@ -187,0 +187,4 @@
("private-key-path", None, None,
"***Warning*** "
"It is possible to use this option to spoil the normal security properties of mutable objects. "
itamarst (Migrated from github.com) commented 2023-01-12 16:25:16 +00:00
Copy Link

Maybe also comment that most people can just ignore this option, since they likely don't need it?

Maybe also comment that most people can just ignore this option, since they likely don't need it?
src/allmydata/scripts/tahoe_put.py
itamarst (Migrated from github.com) commented 2023-01-12 16:25:59 +00:00
Copy Link

Woo

Woo
src/allmydata/test/cli/test_put.py
itamarst (Migrated from github.com) commented 2023-01-12 16:46:01 +00:00
Copy Link

Maybe should also test you can download the resulting data? Just because it gave a matching cap doesn't mean it's stored that way.

Maybe should also test you can download the resulting data? Just because it gave a matching cap doesn't mean it's stored that way.
src/allmydata/test/web/test_web.py
itamarst (Migrated from github.com) commented 2023-01-12 16:46:31 +00:00
Copy Link

Again, might be useful to have testing of download for this code path.

Again, might be useful to have testing of download for this code path.
src/allmydata/web/common.py
@ -745,3 +746,4 @@
return default.encode("utf-8")
return default
itamarst (Migrated from github.com) commented 2023-01-12 16:38:02 +00:00
Copy Link

TIL

TIL
itamarst (Migrated from github.com) commented 2023-01-12 16:40:16 +00:00
Copy Link

So this is saying:

  • If multiple is False, we return bytes
  • If multiple is True, we return tuple of bytes

But then the final version allows returning random other things, regardless of value of multiple. So how does that work?

So this is saying: * If multiple is False, we return bytes * If multiple is True, we return tuple of bytes But then the final version allows returning random other things, regardless of value of multiple. So how does that work?
exarkun (Migrated from github.com) reviewed 2023-01-12 20:28:40 +00:00
src/allmydata/web/common.py
@ -745,3 +746,4 @@
return default.encode("utf-8")
return default
exarkun (Migrated from github.com) commented 2023-01-12 20:28:40 +00:00
Copy Link

You are close. The key is that `Literal[False] doesn't mean "False". It means a statically detectable False. For example:

get_arg(..., multiple=False)

or

x = False
get_arg(..., multiple=x)

but probably not

get_arg(..., multiple=True if x else False)

I don't know the exact extent to which mypy can track this down.

So the first two cases are for statically detectable False or True and the third case covers those and the case where the value couldn't be determined.

Writing this out and reconsidering it, I'm not sure how much sense it makes. I think I got lost in making mypy happy and wasn't thinking carefully enough about the real meaning. A couple things occur to me now:

  • The T case is just as possible for the Literal[True] and Literal[False] cases as for the third case.
  • The None in the return type seems like nonsense. The only way to get None back is if that is the value given for default. It seems like T should cover this but maybe it doesn't because of some special cases applied to None?

sigh... I'll see if I can work this into something more coherent.

You are close. The key is that `Literal[False] doesn't mean "False". It means a statically detectable False. For example: ``` get_arg(..., multiple=False) ``` or ``` x = False get_arg(..., multiple=x) ``` but probably not ``` get_arg(..., multiple=True if x else False) ``` I don't know the exact extent to which mypy can track this down. So the first two cases are for statically detectable False or True and the third case covers those _and_ the case where the value couldn't be determined. Writing this out and reconsidering it, I'm not sure how much sense it makes. I think I got lost in making mypy happy and wasn't thinking carefully enough about the real meaning. A couple things occur to me now: * The `T` case is just as possible for the `Literal[True]` and `Literal[False]` cases as for the third case. * The `None` in the return type seems like nonsense. The only way to get None back is if that is the value given for `default`. It seems like `T` should cover this but maybe it doesn't because of some special cases applied to `None`? sigh... I'll see if I can work this into something more coherent.
exarkun (Migrated from github.com) reviewed 2023-01-12 20:34:18 +00:00
src/allmydata/web/common.py
@ -745,3 +746,4 @@
return default.encode("utf-8")
return default
exarkun (Migrated from github.com) commented 2023-01-12 20:34:18 +00:00
Copy Link

2490f0f58 fwiw

2490f0f58 fwiw
exarkun commented 2023-01-13 17:28:28 +00:00 (Migrated from github.com)
Copy Link

Unclear what the integration test failures are but I see them on the most recent master build too so I guess they aren't a problem with this branch. :/

Unclear what the integration test failures are but I see them on the most recent master build too so I guess they aren't a problem with this branch. :/
Sign in to join this conversation.
Reviewers
No reviewers
itamarst
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#1245
No description provided.
Delete Branch "3962.pre-determined-rsa-keys"

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?