Opened 8 years ago

Closed 8 years ago

Last modified 7 years ago

#4662 closed defect (fixed)

Revert changes from bug4312 and thereafter until 0.2.4.x

Reported by: nickm Owned by:
Priority: Medium Milestone: Tor: 0.2.3.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: tor-relay
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

Our efforts to get the likes of #4626 and #4857 solved before 0.2.3.x have proven pretty hard, and given how close we are to beta (or rc) territory, I think it's time that we consider reverting the code here.

I have a pair of new branches "revert_4312" and "reapply_4312" in my public repository. "revert_4312" has the necessary commits to revert every part of bug 4312. "reapply_4312" is there so we can re-apply it later in 0.2.4.x, when we are older and wiser and hopefully have some more time.

Child Tickets

TicketStatusOwnerSummaryComponent
#4672closedf77f9bd and 40a87c4 re-commited into master after #4662Core Tor/Tor

Change History (15)

comment:1 Changed 8 years ago by nickm

Status: newneeds_review

Here's how I made the branches. To find commits which should maybe be reverted, I looked for things that changed tortls.c, connection_or.c, or compat_libevent.ch:

[528]$ git log --no-merges --reverse --format='%h %s' tor-0.2.3.8-alpha..master src/common/tortls.c src/or/connection_or.c src/common/compat_libevent.[ch] > revertable
[529]$ cp revertable revert
[530]$ vim revert

I then edited the file "revert" to have only the commits that we wanted to remove. Those are:

69a821e Refactor the SSL_set_info_callback() callbacks.
4fd79f9 Detect renegotiation when it actually happens.
ecd239e Detect and deny excess renegotiations attempts.
340809d Get rid of tor_tls_block_renegotiation().
e2b3527 Also handle needless renegotiations in SSL_write().
e097bff Fix issues pointed out by nickm.
406ae1b Use callback-driven approach to block renegotiations.
f77f9bd appease check-spaces
7920ea5 Refactor tor_event_base_once to do what we actually want
633071e Avoid a double-mark in connection_or_close_connection_cb
e8dde3a Fix some wide lines in tortls.c
9a88c0c use event_free() wrapper; fix bug 4582
617617e Don't schedule excess_renegotiations_callback unless it's set
40a87c4 indent; add comment
aba25a6 Make pending libevent actions cancelable
e27a26d Set renegotiation callbacks immediately on tls inititation

Note that we are *NOT* reverting the following commits even though they affect those files:

b42ff65 Use random bytes as our certificate serial numbers.
7b02d1a Clarify function documentation.
f89c619 Use the preferred address and port when initiating a connection.
529820f Use correct address family where necessary for bridges on IPv6.
2376a6a Merge node_get_{prim,pref,pref_ipv6}_addr with their _orport counterparts.
682a85f Don't just tell the controller "foo" on id mismatch

I then created the set of reverts automatically using:

% git checkout master
% git checkout -b revert_4312
% git revert --no-edit `cut -f 1  -d ' ' revert`

I then made a new set of commits to reapply the old changes using:

% git checkout -b reapply_4312
% git cherry-pick --no-edit `cut -f 1  -d ' ' revert`

So if I missed anything, let me know: it'll be easy to regenerate two new branches that have the right set of commits.

comment:2 Changed 8 years ago by nickm

Also, if we do this, we should make a new ticket for "merge the 4312 code again", and make all remaining bugs that affected that code child tickets of that ticket, targeting 0.2.4.x. We should triage the TLS/v2-handshake bugs carefully to see which fall into which category.

comment:3 Changed 8 years ago by asn

Looking at the list of to-be-reverted commits, do we need to revert f77f9bd or 40a87c4?
We will also need to rip the relevant text out of the Changelog.

comment:4 Changed 8 years ago by asn

Apart from comment:3 it "Looks good!".

I did a diff between master and your branch and I couldn't see anything related to #4312 et al. forgotten in. I also studied git log master a couple of times to find any possible forgotten commits. Finally, I did some v2 handshakes with a maint-0.2.2 client and it seemed to work.

Let's also wait and see what effects this branch will have on gabelmoo.

comment:5 Changed 8 years ago by Sebastian

gabelmoo lost the fast flag mid-day yesterday. Whatever my analysis was wrt asn't attempt to fix 4626 and this branch here can just go down the toilet, because I have no feel for how gabelmoo should behave without that flag. Preliminary results seem to indicate that even on 0.2.3.8-alpha, I'm sending way fewer bytes than I'm receiving. Maybe people only publish to me now, and I don't give out many consensuses/descriptors anymore because I'm not Fast.

comment:6 Changed 8 years ago by Sebastian

Even going back to 0.2.2.34 doesn't mean I'm pushing more bytes. I feel like the change in flags threw us(at least me) off massively.

comment:7 Changed 8 years ago by arma

Even so, I think it might be helpful to revert so we can put out an 0.2.3.9-alpha that we do not know has problems.

wanoskarnet has pointed out a variety of problems in the reneg rate limiting stuff, and having a workable alpha release is holding up putting out a stable release.

I don't care whether we decide to delay it until 0.2.4 or just later in the 0.2.3 cycle. But I want to put out another alpha and expect it to mostly work. :)

comment:8 Changed 8 years ago by Sebastian

So, IIUIC, with that branch (asn/bug4626_callback_conditions_theory), stuff does work. The thing that doesn't work is to actually limit renegotiations, but that is just as bad as it was before, so we don't actually lose anything. Of course we can also revert the branch, but maybe we get that revert wrong. hrm.

comment:9 Changed 8 years ago by arma

I'm running asn's b6c18f81f71 on moria1 and as far as I can tell it's working fine.

comment:10 Changed 8 years ago by asn

This is probably the wrong ticket to mention this, but bug4626_callback_conditions_theory (aka b6c18f81f71) should not be considered The Commit That Fixes Everything, if we want an -alpha any time soon.

There are still bugs lurking in the code, like for example:

<wanoskarnet> ugh. ssl3_read_bytes(). seems like if SSL_ST_OK after calling handshake then it going to get record. if any one record there then it returns number of bytes n
ot a error. most times it's impossible because non-blocking io, but non impossible. SSL_read() can return non error if reneg for some extremal cases.

I'm just mentioning this here, because I see Roger still considering b6c18f81f71.

Roger, how did revert_4312 work out for moria1?

comment:11 in reply to:  10 Changed 8 years ago by arma

Replying to asn:

This is probably the wrong ticket to mention this, but bug4626_callback_conditions_theory (aka b6c18f81f71) should not be considered The Commit That Fixes Everything, if we want an -alpha any time soon.

Ok. I would like an alpha rsn.

Roger, how did revert_4312 work out for moria1?

Fine. I just switched moria1 to it.

I agree that somebody should help me clean out the changelog stanzas (or just do it) once we revert.

comment:12 Changed 8 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

Merged revert_4312, and removed the 4312 stuff from the changelog.

Now I am going to make a new "restore/revise 4312" ticket to revert to restore_4312, bug4626_callback_conditions_theory, etc.

comment:13 Changed 8 years ago by asn

f77f9bd and 40a87c4 were also reverted.

f77f9bd was "appease check-spaces" and got reverted by 53f535aeb863204470379b2da4631770fa10b13f. It was a commit by Sebastian that made check-spaces happy.

40a87c4 was "indent; add comment" and got reverted by 75134c6c86e54c10fd9e11c4345aadcdabc0f8fb. It was a commit by Nick that improved the new certificate serial number generation code.

comment:14 Changed 7 years ago by nickm

Keywords: tor-relay added

comment:15 Changed 7 years ago by nickm

Component: Tor RelayTor
Note: See TracTickets for help on using tickets.