[tor-bugs] #4548 [Tor Bridge]: Implement dynamic (rakshasa) primes (part of proposal 179)

[Tor Bug Tracker & Wiki]

#4548: Implement dynamic (rakshasa) primes (part of proposal 179)
------------------------+---------------------------------------------------
 Reporter:  asn         |          Owner:                    
     Type:  defect      |         Status:  needs_review      
 Priority:  normal      |      Milestone:  Tor: 0.2.3.x-final
Component:  Tor Bridge  |        Version:                    
 Keywords:              |         Parent:  #3972             
   Points:              |   Actualpoints:                    
------------------------+---------------------------------------------------

Comment(by asn):

 Replying to [comment:6 nickm]:
 > Remaining issues, in addition to those above, after second review:
 >
 >  * If this new option is going to be on-by-default, then clients really
 shouldn't pay attention to it, since they shouldn't actually need to have
 a group at all.

 True. I'm only doing dynamic DH stuff to bridges now.

 >  * DH_GENERATOR should probably be internal to crypto.c; I don't see a
 great reason to have it in crypto.

 Done.

 >  * spelling error in crypto_set_tls_dh_prime: "moduluss"

 Done.

 >  * Why not call crypto_store_dynamic_dh_modulus from
 crypto_set_tls_dh_prime immediately after generating and checking the new
 modulus?

 Because I'm stupid. Done.

 >  * Checking a file status right before opening it is prone to race-
 conditions; it's better just to open the file and see if you get an error.
 There should be functions in util.c to do this. (This one could get
 cleaned up later)

 I didn't find such functions in util.c. We need a FILE* to pass to
 BN_print_fp().
 I thought of using open() or fdopen() with O_CREAT and O_EXCL, but open()
 seems to be a POSIX thing.

 >  * The branch is super-long: the "git log -p" output is over 6x as long
 as the actual diff with the changes in it.  I think this implies I should
 do some rebasing and squashing pre-merge; suggestions there would be
 welcome.

 Hm. Let's see:
 Leave edec9409 on its own, since it's Jake's stuff.
 Leave 659381e0 on its own, since it introduces config.c stuff.
 Group up 375e55ea, 1797e0a3, fb38e58d and 0e71be5d as code improvements.
 Leave 21babd15 on its own, since it adds stuff to the manual page.
 Group up 42bda231 and 8a726dd0, since they are dependent on each other.
 Group up 2ef68980 and 94076d9e, as migration to crypto.c
 Group up 5f3f41c2, bdeb797a, 782c907c, 7c37a664 and 1d1d5ae7 as 'minor
 improvements'.
 Group up 4938bcc0, 1df6b5a7, b3160197, f477ddcc as 'more minor
 improvements'.

 I'm not sure if my logic makes sense, and I'm not sure of the golden ratio
 between "too many commits" and "too few commits", but I hope I helped you.
 If you need me to create a new branch, with different commit logic, tell
 me.

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