Opened 6 years ago

Closed 4 years ago

#3958 closed defect (wontfix)

pytorctl should learn to try both CookieAuth and PasswordAuth

Reported by: arma Owned by: mikeperry
Priority: Medium Milestone:
Component: Torctl Version:
Severity: Keywords:
Cc: Actual Points:
Parent ID: #3476 Points:
Reviewer: Sponsor:

Description

pytorctl apparently suffers from the same problem Vidalia did in #3898, except backwards -- where Vidalia would try cookie if offered and give up if it fails, apparently pytorctl tries password if offered and gives up if it fails.

https://gitweb.torproject.org/pytorctl.git/blob/HEAD:/TorCtl.py#l532

Child Tickets

Change History (12)

comment:1 follow-up: Changed 6 years ago by atagar

  • Owner set to atagar
  • Status changed from new to assigned

Not quite. TorCtl tries the *first* authentication type present in the 'METHODS=' entry. Iirc this is COOKIE if both are offered so it'll have the same issue that's discussed in 3898.

When both are offered we should:

  1. try the password if we already have it at that point in execution (ie, it's an arg to connect)
  2. attempt to read the cookie file and use that
  3. prompt the user for a password

This attempts in order of relative attempt speed, prompting the user as a last resort.

comment:2 in reply to: ↑ 1 Changed 6 years ago by rransom

Replying to atagar:

Not quite. TorCtl tries the *first* authentication type present in the 'METHODS=' entry. Iirc this is COOKIE if both are offered so it'll have the same issue that's discussed in 3898.

When both are offered we should:

  1. try the password if we already have it at that point in execution (ie, it's an arg to connect)
  2. attempt to read the cookie file and use that
  3. prompt the user for a password

This attempts in order of relative attempt speed, prompting the user as a last resort.

How can we ensure that bandwidth authorities, exit scanners, and other unattended processes that use PyTorCtl never try to 'prompt the user for a password'? (That would cause Nagios scripts to think that the process is running, even if it's doing nothing.)

comment:3 Changed 6 years ago by atagar

TorCtl has three methods for connecting...

  • connect() which may be interative
  • preauth_connect() which just returns the auth type and unauthenticated connection, letting the caller handle that
  • the connection can be made manually and authenticated, which is how it was done before the above existed

If starting non-interactively is important then they shouldn't be using the connect() method.

This change will need to break backward compatibility for preauth_connect callers since the return value needs to be an auth type tuple, but arm is probably the only caller so I doubt Mike will mind much.

comment:4 Changed 6 years ago by arma

  • Parent ID set to #3476

comment:5 Changed 6 years ago by atagar

  • Owner changed from atagar to mikeperry

Fixed in my bug3958 branch, for details and testing done see the commit description:
https://gitweb.torproject.org/atagar/pytorctl.git/commitdiff/3608416cbf00c18aa7c2d1584861246740c549bd

I'll make a similar change in arm so this'll be fixed for its next release without waiting on a merge. Sending this over to Mike for review.

comment:6 Changed 6 years ago by atagar

  • Status changed from assigned to needs_review

second trac update to put in into 'needs review' cuz trac isn't smart enough to do it with a reassign in one go

comment:8 Changed 6 years ago by mikeperry

FYI I can't merge this branch because it contains the diffs for #3679, which we still are not sure about due to #4015.

comment:9 Changed 6 years ago by atagar

FYI I can't merge this branch

This is what 'git cherry-pick' is for. The changes are completely separate and should merge cleanly.

comment:10 Changed 5 years ago by atagar

Tor closes its socket after a failed authentication attempt. This patch seems to work in practice because cookie auth failures are almost always with reading the cookie so we fail before issuing an AUTHENTICATE call.

However, if the auth cookie somehow contains the _wrong_ auth value then...

  • torctl will send the cookie value to tor which is rejected, closing the socket
  • torctl will then fall back to sending the user's passphrase, which regardless of if it's correct or not will also fail because the socket is closed

From what I recall of vidalia's handling I suspect it has a similar bug. I'm attempting to address this with stem by having socket objects that can reconnect after a failed connection attempt - see the ControlSocket and sublasses of...
https://gitweb.torproject.org/stem.git/blob/HEAD:/stem/socket.py

All that said, this patch still gets TorCtl to a better place than it once was.

Cheers! -Damian

comment:11 Changed 5 years ago by atagar

Oh, a bug with this patch: if the PROTOCOLINFO response has multiple auth types and the authenticate() caller doesn't provide a passphrase then we abort without trying cookie auth (see lines 1014-1016). This can be trivially fixed by changing...

if AUTH_TYPE.PASSWORD in self._authTypes and secret == "":

to

if secret == "" and self._authTypes == (AUTH_TYPE.PASSWORD,):

This probably also means an issue with the edge case where the user generated an empty password...
tor --hash-password ""

so I'd suggest changing the secret default value to None.

comment:12 Changed 4 years ago by atagar

  • Resolution set to wontfix
  • Status changed from needs_review to closed
Note: See TracTickets for help on using tickets.