Re: [tor-bugs] #25669 [Core Tor/Tor]: Privcount: blinding and encryption should be finished up

["Tor Bug Tracker & Wiki" <blackhole@xxxxxxxxxxxxxx>]

#25669: Privcount: blinding and encryption should be finished up
-------------------------------------------------+-------------------------
 Reporter:  nickm                                |          Owner:  (none)
     Type:  enhancement                          |         Status:
                                                 |  needs_revision
 Priority:  Medium                               |      Milestone:  Tor:
                                                 |  0.3.5.x-final
Component:  Core Tor/Tor                         |        Version:
 Severity:  Normal                               |     Resolution:
 Keywords:  privcount, 035-roadmap-master, 035   |  Actual Points:
  -triaged-in-20180711, rust                     |
Parent ID:  #22898                               |         Points:
 Reviewer:                                       |        Sponsor:
                                                 |  SponsorV-can
-------------------------------------------------+-------------------------

Comment (by chelseakomlo):

 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 `usize` is being used interchangeably
 for integer types (such as `test_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-crypto`
 states 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 as
 `assert_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_trs` isn't totally clear:
 https://github.com/nmathewson/privcount_shamir/blob/master/rust/tests/basic_integration.rs#L26

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/25669#comment:15>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online

_______________________________________________
tor-bugs mailing list
tor-bugs@xxxxxxxxxxxxxxxxxxxx
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs