Opened 9 months ago

Last modified 5 weeks ago

#25669 needs_revision enhancement

Privcount: blinding and encryption should be finished up

Reported by: nickm Owned by: teor
Priority: Medium Milestone: Tor: 0.4.0.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: privcount, 035-roadmap-master, 035-triaged-in-20180711, rust
Cc: teor, nickm, chelseakomlo Actual Points:
Parent ID: #22898 Points:
Reviewer: Sponsor: SponsorV-can

Description

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.

Child Tickets

TicketTypeStatusOwnerSummary
#26939defectclosednickmPrivcount blinding and encryption: type fixes
#26940defectclosednickmPrivcount blinding and encryption: doc fixes
#26941defectassignednickmPrivcount blinding and encryption: review dependencies
#26942defectclosedteorPrivcount blinding and encryption: Rust code conventions
#26943defectclosednickmPrivcount blinding and encryption: Safety fixes
#26944defectassignedteorPrivcount blinding and encryption: Add tests
#26945defectassignedteorPrivcount blinding and encryption: always enable i128
#26946defectclosedteorPrivcount blinding and encryption: warning fixes
#26953defectclosedteorPrivcount blinding and encryption: add travis CI
#26955defectclosedteorPrivcount blinding and encryption: rustfmt
#26956defectclosedteorPrivcount blinding and encryption: deny warnings
#26957defectassignedteorPrivcount blinding and encryption: Derive copy and debug where possible
#26958defectneeds_revisionteorPrivcount blinding and encryption: run clippy on travis rust nightly
#26973defectclosedteorPrivcount blinding and encryption: add rustfmt CI check

Change History (19)

comment:1 Changed 9 months ago by nickm

Keywords: 034-triage-20180328 added

comment:2 Changed 9 months ago by nickm

Keywords: 034-included-20180328 added

comment:3 Changed 8 months ago by teor

Keywords: privcount added
Milestone: Tor: 0.3.4.x-finalTor: 0.3.5.x-final

We are deferring this ticket to 0.3.5 because I do not have time to work on it until Mid-May.

comment:4 Changed 8 months ago by teor

Parent ID: #22898

comment:5 Changed 5 months ago by nickm

Keywords: 035-roadmap-master added; 034-roadmap-master 034-triage-20180328 034-included-20180328 removed

comment:7 Changed 5 months ago by nickm

I've implemented the rejection-sampling logic, and noted it in torspec with c590145e6d3212.

comment:8 Changed 5 months ago by teor

I 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

comment:9 Changed 5 months ago by nickm

Keywords: 035-triaged-in-20180711 added

comment:10 Changed 5 months ago by chelseakomlo

Cc: chelseakomlo added
Keywords: rust added

comment:11 Changed 5 months ago by 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?

comment:12 in reply to:  11 ; Changed 5 months ago by 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.

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.

comment:13 Changed 5 months ago by nickm

Sponsor: SponsorV-can

comment:14 in reply to:  12 Changed 5 months ago by chelseakomlo

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.

comment:15 Changed 5 months ago 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

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

Testing

Unit tests

Integration tests

Last edited 5 months ago by chelseakomlo (previous) (diff)

comment:16 Changed 5 months ago by teor

Owner: set to teor
Status: needs_revisionassigned

This is mine now, for revision

comment:17 Changed 5 months ago by teor

Status: assignedneeds_revision

comment:18 Changed 3 months ago by nickm

Milestone: Tor: 0.3.5.x-finalTor: 0.3.6.x-final

Deferring privcount tickets in 0.3.5 to 0.3.6

comment:19 Changed 5 weeks ago by nickm

Milestone: Tor: 0.3.6.x-finalTor: 0.4.0.x-final

Tor 0.3.6.x has been renamed to 0.4.0.x.

Note: See TracTickets for help on using tickets.