Opened 15 months ago

Last modified 2 weeks ago

#25669 new enhancement

Privcount: blinding and encryption should be finished up

Reported by: nickm Owned by:
Priority: Medium Milestone: Tor: unspecified
Component: Core Tor/Tor Version:
Severity: Normal Keywords: technical-debt, privcount, 035-roadmap-master, 035-triaged-in-20180711, rust, 040-unreached-20190109, 041-accepted-20190115
Cc: teor, nickm, chelseakomlo Actual Points: 0.2
Parent ID: #29009 Points: 0.2
Reviewer: Sponsor:

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
#26941defectassignedPrivcount blinding and encryption: review dependencies
#26942defectclosedteorPrivcount blinding and encryption: Rust code conventions
#26943defectclosednickmPrivcount blinding and encryption: Safety fixes
#26944defectassignedPrivcount blinding and encryption: Add tests
#26945defectassignedPrivcount 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
#26957defectassignedPrivcount blinding and encryption: Derive copy and debug where possible
#26958defectneeds_revisionPrivcount blinding and encryption: run clippy on travis rust nightly
#26973defectclosedteorPrivcount blinding and encryption: add rustfmt CI check

Change History (31)

comment:1 Changed 15 months ago by nickm

Keywords: 034-triage-20180328 added

comment:2 Changed 15 months ago by nickm

Keywords: 034-included-20180328 added

comment:3 Changed 14 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 14 months ago by teor

Parent ID: #22898

comment:5 Changed 12 months ago by nickm

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

comment:7 Changed 11 months ago by nickm

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

comment:8 Changed 11 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 11 months ago by nickm

Keywords: 035-triaged-in-20180711 added

comment:10 Changed 11 months ago by chelseakomlo

Cc: chelseakomlo added
Keywords: rust added

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

Sponsor: SponsorV-can

comment:14 in reply to:  12 Changed 11 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 11 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 11 months ago by chelseakomlo (previous) (diff)

comment:16 Changed 11 months ago by teor

Owner: set to teor
Status: needs_revisionassigned

This is mine now, for revision

comment:17 Changed 11 months ago by teor

Status: assignedneeds_revision

comment:18 Changed 9 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 7 months 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.

comment:20 Changed 5 months ago by teor

Parent ID: #22898#29009

We'll need privcount_shamir in tor for the proof of concept.

comment:21 Changed 5 months ago by teor

Keywords: 040-unreached-20190109 041-proposed added
Milestone: Tor: 0.4.0.x-finalTor: unspecified

These tasks are on the 0.4.1 roadmap for PrivCount and Sponsor V.

comment:22 Changed 5 months ago by teor

Keywords: 041-proposed-on-roadmap added; 041-proposed removed
Milestone: Tor: unspecifiedTor: 0.4.1.x-final

Let's review these tickets at the next meeting using our 041-proposed process.

They're on the roadmap, so the review should focus on ticket size and team capacity (and sponsor expectations).

comment:23 Changed 5 months ago by teor

Actual Points: 0.2
Points: 0.2

comment:24 Changed 5 months ago by teor

Keywords: 041-accepted-20190115 added; 041-proposed-on-roadmap removed

These PrivCount tickets are on the 041 roadmap, we accepted their points estimates in 041 without discussion.

comment:25 Changed 3 months ago by teor

Owner: teor deleted
Status: needs_revisionassigned

comment:26 Changed 3 months ago by teor

Keywords: technical-debt added
Milestone: Tor: 0.4.1.x-finalTor: unspecified
Status: assignedneeds_review

We don't need to do this for the PrivCount proof of concept.

comment:27 Changed 3 months ago by asn

Reviewer: nickm

comment:28 Changed 3 months ago by nickm

Query -- I don't see what I am supposed to review here. Is there a PR somewhere?

comment:29 in reply to:  28 Changed 3 months ago by teor

Reviewer: nickm
Status: needs_reviewnew

Replying to nickm:

Query -- I don't see what I am supposed to review here. Is there a PR somewhere?

Sorry, I don't know how this ticket ended up in needs review. Careless clicking! :-)

comment:30 Changed 2 weeks ago by gaba

Removing sponsor V as we do not have more time to include this tickets in the sponsor.

comment:31 Changed 2 weeks ago by gaba

Sponsor: SponsorV-can

Removing sponsor from tickets that we do not have time to fit in the remain of this sponsorship.

Note: See TracTickets for help on using tickets.