[Author Prev][Author Next][Thread Prev][Thread Next][Author Index][Thread Index]
Re: [tor-bugs] #4548 [Tor Bridge]: Implement dynamic (rakshasa) primes (part of proposal 179)
#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
_______________________________________________
tor-bugs mailing list
tor-bugs@xxxxxxxxxxxxxxxxxxxx
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs