Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#13736 closed defect (fixed)

Kill the DynamicDHGroups feature

Reported by: asn Owned by: yawning
Priority: Medium Milestone: Tor: 0.2.7.x-final
Component: Core Tor/Tor Version: Tor: 0.2.7
Severity: Keywords: tor tor-bridge easy lorax
Cc: yawning Actual Points:
Parent ID: Points: small
Reviewer: Sponsor:

Description

In #4548 we implemented the DynamicDHGroups feature of prop179 which we ended up never using. It's pretty clear now that we will never use it at all, because we have PTs to do this job.

Since the code is non-trivial, I suggest we kill it completely from the current codebase.

Seems like a good volunteer task too.

Child Tickets

Change History (12)

comment:1 Changed 5 years ago by nickm

Keywords: lorax added
Milestone: Tor: 0.2.6.x-finalTor: 0.2.???

Good idea; doesn't require a particular timeframe.

comment:2 Changed 5 years ago by yawning

Cc: yawning added

comment:3 Changed 5 years ago by yawning

Owner: set to yawning
Status: newassigned

comment:4 Changed 5 years ago by nickm

Milestone: Tor: 0.2.???Tor: 0.2.7.x-final

Tentatively move some tickets to 0.2.7

comment:5 Changed 5 years ago by nickm

Status: assignedneeds_review

If we decide to to this, my branch bug13736 is how. If we decide to merge that, we should close #6087 and #6089.

comment:6 Changed 5 years ago by asn

Patch looks solid to me.

Two quick comments:


+  /* Probably not needed any longer XXXX */
+  crypto_set_tls_dh_prime();

This seeems removable, yes. crypto_set_tls_dh_prime() will be called eventually from router.c:init_keys(). Here is a backtrace for clients:

#0  crypto_set_tls_dh_prime () at src/common/crypto.c:1775
#1  0x0000555555687d7a in init_dh_param () at src/common/crypto.c:1842
#2  0x00005555556880a5 in crypto_dh_new (dh_type=3) at src/common/crypto.c:1863
#3  0x00005555556906ec in tor_tls_context_new (is_client=<optimized out>, flags=<optimized out>, key_lifetime=<optimized out>, identity=<optimized out>) at src/common/tortls.c:1396
#4  tor_tls_context_init_one (ppcontext=0x555555954bd0, ppcontext@entry=0x55555592bac0 <client_tls_context>, identity=0x555555954550, key_lifetime=0, flags=15, flags@entry=0, is_client=1) at src/common/tortls.c:1193
#5  0x0000555555690988 in tor_tls_context_init (flags=0, client_identity=0x55555594d790, server_identity=<optimized out>, key_lifetime=17020799) at src/common/tortls.c:1169
#6  0x00005555555c0366 in init_keys () at src/or/router.c:806
#7  0x0000555555588f2d in do_main_loop () at src/or/main.c:1984
#8  0x000055555558be85 in tor_main (argc=<optimized out>, argv=<optimized out>) at src/or/main.c:3078
#9  0x00007ffff6875b45 in __libc_start_main (main=0x555555585570 <main>, argc=3, argv=0x7fffffffe538, init=<optimized out>, fini=<optimized out>, rtld_fini=<optimized out>, stack_end=0x7fffffffe528) at libc-start.c:287
#10 0x00005555555855cb in _start ()

You added an if (1) { block in crypto_set_tls_dh_prime(). I think it's redundant.

comment:7 Changed 5 years ago by nickm

Usually, my reason for an if (1) block is so that the indentation doesn't change and the diff is easier to read.

comment:8 Changed 5 years ago by yawning

Apart from echoing that the crypto_set_tls_dh_prime(); call is removable, this needs tor.1.in/tor.1.txt changes to remove the config option from the documentation.

Apart from that lgtm.

comment:9 Changed 5 years ago by nickm

Updated my branch 'bug13736'. Better now?

comment:10 Changed 5 years ago by yawning

lgtm.

comment:11 Changed 5 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

mergeymergey

comment:12 Changed 5 years ago by isabela

Points: small
Priority: minornormal
Version: Tor: 0.2.7
Note: See TracTickets for help on using tickets.