Opened 10 months ago

Last modified 3 months ago

#28863 needs_information defect

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:
Parent ID: #30971 Points:
Reviewer: 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
#31020newValidate the fallback scripts CI output using grep and stemCore Tor/Fallback Scripts
#31021newAdd fallback CI for the check_existing and OUTPUT_CANDIDATES modesCore Tor/Fallback Scripts

Attachments (1)

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

Download all attachments as: .zip

Change History (12)

Changed 10 months ago by starlight

touch up to apply after 2to3.7

comment:1 Changed 10 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 10 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 10 months ago by teor

Status: newneeds_revision

comment:4 in reply to:  2 Changed 10 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 9 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 9 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 9 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 9 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 4 months ago by teor

Parent ID: #28793#28797

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

comment:10 Changed 4 months ago by teor

Keywords: fallback-ci added

comment:11 Changed 3 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.

Note: See TracTickets for help on using tickets.