search

torproject / stem

Python controller library for Tor
https://stem.torproject.org/
GNU Lesser General Public License v3.0
257 stars 75 forks source link

Adds support for ONION_CLIENT_AUTH_ADD, ONION_CLIENT_AUTH_REMOVE and ONION_CLIENT_AUTH_VIEW #67

Closed mig5 closed 3 years ago

mig5 commented 4 years ago

Hello @atagar,

Per #66, here is an implementation of the client-side Client Auth control port commands for v3 onions.

I added some integration tests which passed for me.

This is my first time contributing to Stem so please do let me know if I did anything incorrectly.

Cheers, mig5 (from the OnionShare project)

mig5 commented 4 years ago

Here's a PoC script for a bit of manual testing if required. Note that if you use a real onion you'd have to add the base32 public key to its HiddenService folder per the setup docs.

https://gist.github.com/mig5/07cc7558c00111fa8696368589019fb2

atagar commented 4 years ago

Thanks mig5, skimmed this patch and it looks great! I particularly appreciate the integ test and interpreter additions.

I'm in the middle of reviewing a large branch that adds asyncio support so it'll probably be a couple weeks before I circle back and merge this. Sorry for the delay!

mig5 commented 4 years ago

@atagar no problem and no rush! Thanks!

BTW, in case you missed it, I recently had a chat with nickm in IRC regarding the corresponding Tor core patch https://github.com/torproject/tor/pull/1941

Nick pointed out that the actual response messages (text) from the control port are not strictly defined in the Control spec. Only the codes.

Although that patch is obviously nice in that it makes the use of the term 'address' consistent in the responses, it brought up a wider issue for Nick:

1) Should the control spec explicitly state the mandatory response string text, (potentially breaking the conformity of existing apps that implement the spec as it stands today), or

2) Should Stem's (and other apps) tests only check for the response code alone (250, etc) rather than the corresponding text, so that they aren't bound to what the controller responds as it stands today. (Which could potentially break your tests one day if a change to the text occurs in core -as it very may do with my core patch linked above!)

If you think 2) is more flexible for Stem, I'd be happy to try and work on adjusting your tests to care only about the response code rather than the full message. Of course nothing would change in terms of handling the response message in the app itself - just the tests.

atagar commented 4 years ago

If ONION_CLIENT_AUTH responses have anything of substance beyond the return code it should be detailed within the specification. PROTOCOLINFO is a good example of this.

So... both #1 and #2? The spec only describes response codes so that's all Stem should attempt to parse (#2). However, if the responses have additional data then Tor should fix our specification and then we'll read it.

atagar commented 3 years ago

Hi mig5, I just commented and closed your other ticket (https://github.com/torproject/stem/issues/66) which on reflection more accurately concerned this. If you'd like any adjustments to it please feel free to reopen.