Privcount: blinding and encryption should be finished up
We're supposed to do this in 0.3.4. I don't know if there is anything here left to do, but in case there is (like merging to tor, testing more, etc) this is the master ticket.
Activity
changed milestone to %Tor: unspecified
Trac:
Parent: N/A to #22898 (moved)The code is here: https://github.com/nmathewson/privcount_shamir/tree/master/rust
It needs two changes:
- use rejection sampling to avoid a non-uniform distribution of values, and amend the spec:
- now that i128 is in stable, amend the i128 checks:
Trac:
Status: new to needs_revisionI found a typo in a comment, and added a note on GitHub.
I'm not sure how to handle the i128 feature - we can rely on it on common platforms with rust 1.26 or later, but rare platforms may miscompile i128 code: https://doc.rust-lang.org/book/second-edition/appendix-06-newest-features.html#a128-bit-integers https://github.com/rust-lang/rust/issues/45676
Replying to chelseakomlo:
I glanced over this with an eye towards the Rust conventions we've been working towards in tor- would it be helpful to do a Rust style/format review now, or wait for some of these other fixes first?
I don't think there are any major changes left, so I think we would benefit from a rust style review at this point.
To solve i128 miscompilation, we should disable i128 for:
- rust stable versions 1.25 and earlier, which don't support i128, and
- rust platforms where the unit tests fail because i128 is miscompiled.
(We do the same thing for optimised C code which doesn't work on some platforms.)
I'll look up the rust incantations to define a feature (tor_enable_i128) and submit a patch, hopefully later this week.
Replying to teor:
Replying to chelseakomlo:
I glanced over this with an eye towards the Rust conventions we've been working towards in tor- would it be helpful to do a Rust style/format review now, or wait for some of these other fixes first?
I don't think there are any major changes left, so I think we would benefit from a rust style review at this point.
Sounds good! I will look this over by next Monday, and will use our Rust conventions to compare and also notice where/if we are lacking any guidelines for Rust.
Overall, this is looking good. This review is purely a Rust convention and code quality review, I didn't actually look deeply at how logic is implemented. Doctests and more unit tests would help in being able to do a deeper review for logic though.
As a more meta point, there is a lot of good and very detailed information in our Rust coding standards: tor/docs/HACKING/CodingStandardsRust.md. I would recommend taking a look, particularly for safety if/when this code integrates with tor C code.
Let me know if you want to talk over any of this on irc/etc.
== Types
- In several places, it looks like
usizeis being used interchangeably for integer types (such astest_combination). I would recommend doing a review to ensure if using explicit fixed-size representations would be safer in these cases, as usize is of pointer size, so its size changes depending on the architecture. https://doc.rust-lang.org/std/primitive.usize.html.
== Documentation
- Doctests should probably be added to all public functions- this is helpful for both documenting the code and also verifying that the documentation is valid.
- Readme needs to be updated: https://github.com/nmathewson/privcount_shamir
- Per our Rust coding standards (in tor/docs/HACKING/CodingStandardsRust.md,
#[deny(missing_docs)]must be included
== External Dependencies
- I see that this crate depends on several external crates.
rust-cryptostates that it doesn't have strong security guarantees- is there something else that we should be using? https://crates.io/crates/rust-crypto. - Should we have an auditing process for when we choose to import/use new external crates?
== Rust code conventions
- Snake case in the function name: https://github.com/nmathewson/privcount_shamir/blob/master/rust/src/server.rs#L69
== Integration
- How will this code be executed? Will something in core Tor call into these public functions? If so, where is the FFI layer?
- Should any logging be added to this module?
- Will this code call into Tor's C code at all? It doesn't currently but I was curious if there are plans for this in the case of the code moving into the core tor codebase.
== Safety
- Ensure that if this code is being called from C, there aren't unexpected places that would lead to UB. For example, everything needs to be explicitly handled in the case of errors: https://github.com/nmathewson/privcount_shamir/blob/master/rust/src/encrypt.rs#L122 as one example, but I see
assert!in several places as well asassert_eq!, etc: https://github.com/nmathewson/privcount_shamir/blob/f679aa90ef3ce0d264c50f7e010b2b8022b96c7c/rust/src/shamir.rs#L90 - Are there bounds checks that should happen in these places? https://github.com/nmathewson/privcount_shamir/blob/f679aa90ef3ce0d264c50f7e010b2b8022b96c7c/rust/src/shamir.rs#L50 and https://github.com/nmathewson/privcount_shamir/blob/34db770b2bed5ee7217bc01a530e39b1a90bad17/rust/src/data.rs#L87
- Handle error cases and don't call unwrap directly. https://github.com/nmathewson/privcount_shamir/blob/fe039e5600c25b99acd4bd23b0e609874789fd79/rust/src/client.rs#L25 and https://github.com/nmathewson/privcount_shamir/blob/fe039e5600c25b99acd4bd23b0e609874789fd79/rust/src/client.rs#L100 for example.
== Testing === Unit tests
- I see a couple modules without unit tests (client, data). Per our coding standards, all code must be unit and integration tests. For extremely basic functions, doctests are likely sufficient (such as to string methods) but this should be in only in extremely simple cases and not the default.
- Ensure test names are descriptive of what they are testing, or add comments: https://github.com/nmathewson/privcount_shamir/blob/f679aa90ef3ce0d264c50f7e010b2b8022b96c7c/rust/src/shamir.rs#L131
- Remove print statements in tests :) https://github.com/nmathewson/privcount_shamir/blob/f679aa90ef3ce0d264c50f7e010b2b8022b96c7c/rust/src/shamir.rs#L138 === Integration tests
- Are there any extreme cases that should be tested here? I.e, high values, low values, large values, etc.
- Can you add a more descriptive name to each test case? I.e, if those are valid test cases, maybe specify that in the test name.
- Try to pick explicit naming where possible-
n_trsisn't totally clear: https://github.com/nmathewson/privcount_shamir/blob/master/rust/tests/basic_integration.rs#L26
- In several places, it looks like
