Opened 14 months ago

Closed 7 days ago

#28863 closed defect (fixed)

updateFallbackDirs.py thinks it is python 3 compatible but it is not

Reported by: starlight Owned by:
Priority: Medium Milestone:
Component: Core Tor/Fallback Scripts Version:
Severity: Normal Keywords: fallback-ci, 043-must
Cc: teor Actual Points: 0.6
Parent ID: #30971 Points:
Reviewer: asn Sponsor:

Description

This comment would lead one to believe the script will run with python 3 but problems remain:

# Optionally uses ipaddress (python 3 builtin) or py2-ipaddress (package)
# for netblock analysis.

After running 2to3-3.7 on commit 6BC5C06D additional manual revisions were required per the attached patch. A subtle certificate validation problem exists, not enough time to fix so disabled it. Have good CA files in both system and Python directories and openssl s_client has no problem.

OUTPUT_CANDIDATES = True

is broken, wasn't prepared to spend the time hacking it.

Child Tickets

TicketStatusOwnerSummaryComponent
#32632closedteorFallback Scripts Travis: Use the latest dependenciesCore Tor/Fallback Scripts

Attachments (1)

updateFallbackDirs.py-python3.patch (2.0 KB) - added by starlight 14 months ago.
touch up to apply after 2to3.7

Download all attachments as: .zip

Change History (20)

Changed 14 months ago by starlight

touch up to apply after 2to3.7

comment:1 Changed 14 months ago by starlight

The TLS certificate validation problem could be the result of running with torsocks. Two years+ ago updateFallbackDirs.py ran fine this way, but much code has been added and changed since. Did not try running over direct connection.

comment:2 in reply to:  1 ; Changed 14 months ago by teor

Milestone: Tor: 0.4.0.x-final
Parent ID: #28793

Thanks for this patch.

We don't have CI for this script yet, so we can easily make changes that break some platforms. (I might wait to merge this patch until we have CI for python 2 and 3.)

Replying to starlight:

The TLS certificate validation problem could be the result of running with torsocks. Two years+ ago updateFallbackDirs.py ran fine this way, but much code has been added and changed since. Did not try running over direct connection.

Running the script over Tor was never supported, because the script is designed to measure direct consensus download speeds. We documented this requirement in April 2016:

# This script should be run from a stable, reliable network connection,
# with no other network activity (and not over tor).
# If this is not possible, please disable:
# PERFORM_IPV4_DIRPORT_CHECKS and PERFORM_IPV6_DIRPORT_CHECKS

I am not sure about the https change, the only https URL I can see is:

ONIONOO = 'https://onionoo.torproject.org/'

This URL validates fine in my (Tor) browser.

I also see this note in the Python 2 urllib2 documentation:

Note The urllib2 module has been split across several modules in Python 3 named urllib.request and urllib.error. The 2to3 tool will automatically adapt imports when converting your sources to Python 3.

https://docs.python.org/2/library/urllib2.html

Is your patch missing the imports for python 3?

comment:3 Changed 14 months ago by teor

Status: newneeds_revision

comment:4 in reply to:  2 Changed 14 months ago by starlight

Replying to teor:

Thanks for this patch.

welcome

...

Running the script over Tor was never supported, because the script is designed to measure direct consensus download speeds.

Sure, but it runs nice anyway.

I am not sure about the https change, the only https URL I can see is:

ONIONOO = 'https://onionoo.torproject.org/'

this is where it happens; on searching found many folk adapting python 2 to 3 baffled by similar behaviors; didn't have time to dig deeper

This URL validates fine in my (Tor) browser.

and with "torsocks openssl s_client -connect xxx", works fine

I also see this note in the Python 2 urllib2 documentation:

Note The urllib2 module has been split across several modules in Python 3 named urllib.request and urllib.error. The 2to3 tool will automatically adapt imports when converting your sources to Python 3.

https://docs.python.org/2/library/urllib2.html

Is your patch missing the imports for python 3?

ran fine after the changes with python 3.7 (output at https://lists.torproject.org/pipermail/tor-relays/2018-December/016735.html); might work ok via direct but need adjustment to function using torsocks, but out of scope as you say

comment:5 Changed 13 months ago by teor

Status: needs_revisionneeds_information

I'd like to merge this patch, but I don't want to disable security checks without understanding the problem better.

Has someone written a good summary of the issue with python 3, and the security impact of this change?

comment:6 Changed 13 months ago by starlight

I would not recommend keeping TLS disabled, was just tired after hours of hacking at it and trying to get the script running in an environment where compiling Python3 was the path of least resistance. Should be fixable but is probably a couple of hours of searching and hacking. While I respect Python I've vowed to do everything I would use Python for in modern C++ going forward, don't have enough space in my brain to be good at both.

comment:7 Changed 13 months ago by nickm

Keywords: postfreeze-ok added

Mark some tickets as postfreeze-ok, to indicate that I think they are okay to accept in 0.4.0 post-freeze. Does not indicate that they are all necessary to do postfreeze.

comment:8 Changed 13 months ago by teor

Keywords: postfreeze-ok removed
Milestone: Tor: 0.4.0.x-final
Version: Tor: 0.3.5.5-alpha

Hi starlight,

In #27914, we split the fallback scripts out into https://git.torproject.org/fallback-scripts.git . We're still working on syncing it with https://github.com/torproject/fallback-scripts.git (#29101).

After #29101 closes, we can check the security implications, then apply your patch to fallback-scripts. We might also get CI working before we merge any more script changes (#28797).

(Removing tags, because this ticket no longer has any connection to tor releases.)

comment:9 Changed 7 months ago by teor

Parent ID: #28793#28797

We're not going to have time to do this any time soon.

comment:10 Changed 7 months ago by teor

Keywords: fallback-ci added

comment:11 Changed 6 months ago by teor

Keywords: 043-must added
Parent ID: #28797#30971

We need to fix this at the end of 2019, because python 2 is not supported in 2020.

comment:12 Changed 2 months ago by teor

I've done some research, here's what I've discovered:

  • python 2's code freeze starts on 1 January 2020, the final release will be around April at pycon 2020
  • if we want to continue testing using python 2 during the transition, we should use python-modernize or python-future, not 2to3
  • python 3.6 is the earliest supported python 3; python 3.2, 3.3, and 3.5 added features that make porting easier
  • we should try to use if PY2 for any code that won't run in both versions, in case there is a python 4 some day

comment:13 Changed 8 weeks ago by teor

Actual Points: 0.5
Status: needs_informationneeds_review

See my PR:

I tried to make minimal changes to get the 3 fallback scripts working with python 3.6 and later. I also made sure I maintained compatibility with python 2. We should see if any of the CI fails, it has a lot more python versions.

We might fix check_existing and OUTPUT_CANDIDATES modes in #31021, but they aren't a high priority, because we don't use them very often.

comment:14 Changed 8 weeks ago by asn

Reviewer: asn

comment:15 Changed 8 weeks ago by teor

(CI is passing, I rewrote the lookup* and generate* scripts to make them more reliable.)

comment:16 Changed 7 weeks ago by teor

Status: needs_reviewneeds_revision

I need to add future statements at the start of the scripts:

from __future__ import print_function

And for the update script:

from __future__ import division
Last edited 7 weeks ago by teor (previous) (diff)

comment:17 Changed 7 weeks ago by teor

Actual Points: 0.50.6
Status: needs_revisionneeds_review

I made this change by adding all the __future__ imports to every python script.

Last edited 7 weeks ago by teor (previous) (diff)

comment:18 Changed 7 days ago by teor

Status: needs_reviewmerge_ready

No-one has reviewed these changes, so I'll just merge them after CI passes.

comment:19 Changed 7 days ago by teor

Resolution: fixed
Status: merge_readyclosed

Merged to master.

Note: See TracTickets for help on using tickets.