[Author Prev][Author Next][Thread Prev][Thread Next][Author Index][Thread Index]
Re: [tor-bugs] #12533 [Stem]: multiple hidden services and get_conf_map('HiddenServiceOptions') response format
#12533: multiple hidden services and get_conf_map('HiddenServiceOptions') response
format
-------------------------+------------------------------
Reporter: jthayer | Owner: atagar
Type: defect | Status: new
Priority: normal | Milestone:
Component: Stem | Version:
Resolution: | Keywords: controller, easy
Actual Points: | Parent ID:
Points: |
-------------------------+------------------------------
Comment (by atagar):
Hi federico3, sorry about the delay!
----
{{{
from collections import OrderedDict
}}}
Stem includes a python 2.6 compatible copy of OrderedDict...
{{{
try:
# added in python 2.7
from collections import OrderedDict
except ImportError:
from stem.util.ordereddict import OrderedDict
}}}
----
{{{
def get_hidden_services_conf(self):
}}}
This should accept a defaut argument like the Controller's other getter
methods.
----
{{{
"""Get hidden services configuration
}}}
This could do with being a bit more descriptive. Maybe the following?
This provides a mapping of hidden service directories to their attribute's
key/value pairs.
----
{{{
* :class:`RuntimeError` if the configuration contains unexpected entries
}}}
It would probably be better to avoid throwing exceptions that aren't a
stem.ControllerError subclass (makes for more work for callers). That said
though do we really want to error in this case? Tor is free to add new
hidden service values in the future so it seems like we might as well
provide them in our dict. This would be simpler if we provide what Tor
gave us. That is to say, for the pydoc example...
{{{
>>> controller.get_hidden_services_conf()
{
"/var/lib/tor/hidden_service_empty/": {
"HiddenServicePort": [
]
},
"/var/lib/tor/hidden_service_with_two_ports/": {
"HiddenServiceAuthorizeClient": "stealth a, b",
"HiddenServicePort": [
"8020 127.0.0.1:8020", # the ports order is kept
"8021 127.0.0.1:8021"
],
"HiddenServiceVersion": "2"
},
}
}}}
----
{{{
try:
response = self.msg('GETCONF HiddenServiceOptions')
stem.response.convert('GETCONF', response)
log.debug('GETCONF HiddenServiceOptions (runtime: %0.4f)' % (time.time()
- start_time))
except stem.ControllerError as exc:
log.debug('GETCONF HiddenServiceOptions (failed: %s)' % exc)
raise
}}}
Up to you but you might want to consider caching the response (like
get_conf() does). More importantly though this should be re-raising exc.
----
{{{
service_dir_map = OrderedDict()
directory = None
ports = []
}}}
The ports is unused.
----
{{{
k, v = content.split('=', 1)
}}}
What if content doesn't have a '='?
----
{{{
service_dir_map[directory] = dict(
ports = []
)
}}}
Probably little reason not to go with the one-liner...
{{{
service_dir_map[directory] = {'ports': []}
}}}
----
{{{
elif k == 'HiddenServiceVersion':
service_dir_map[directory]['service_version'] = int(v)
}}}
The pydoc example says that the version is a string. Personally I think it
should be (so we're consistent in providing string keys and values).
----
{{{
queryitems.append("HiddenServiceAuthorizeClient=\"%s\"" % v)
}}}
This would be a good spot to use single quotes to avoid the escaping...
{{{
queryitems.append('HiddenServiceAuthorizeClient="%s"' % v)
}}}
----
{{{
query = 'SETCONF ' + ' '.join(queryitems)
response = self.msg(query)
stem.response.convert('SINGLELINE', response)
}}}
By calling SETCONF directly we *might* be failing to invalidate
get_conf()'s cache.
----
{{{
assert 0 <= virtport <= 2 **16
}}}
Stem hasn't used assertions anywhere before. Maybe we should be raising a
ValueError? Stem provides a helper method for checking if it's valid...
{{{
if stem.util.connection.is_valid_port(virtport):
virtport = int(virtport)
else:
raise ValueError("'%s' isn't a valid port number" % virtport)
}}}
I know this violates what I said earlier about throwing
stem.ControllerError subclasses. The reason is that this is purely an
error on the side of our caller, so the best thing we can do to help them
is be really, really noisy about letting them know. :)
----
{{{
if str(virtport) in ports:
return False
if "%d 127.0.0.1:%d" % (virtport, virtport) in ports:
return False
}}}
This makes me wonder about the HiddenServicePort format. You seem to be
expecting either port numbers or '<port> <ip>:<port>' (not sure why it has
the port twice). Unfortunately the Tor docs around this are a bit vague...
https://www.torproject.org/docs/tor-manual.html.en#HiddenServicePort
Might warrant clarification and a ticket to the Tor component. In either
case our get_hidden_services_conf() docs will need to be very clear about
what it provides for the ports value so callers can know how to handle it.
----
{{{
self.set_hidden_services_conf(conf)
return True
}}}
Presently this method isn't documented as raising any exceptions, but
set_hidden_services_conf() could raise. We could either document the
exceptions or suppress them. Probably better to go for the former here.
{{{
def delete_hidden_service(self, dirname, virtport, target=None):
"""Delete a hidden service+port.
:param str dirname: directory name
:param int virtport: virtual port
:param str target: optional ipaddr:port target e.g. '127.0.0.1:8080'
:raises:
"""
}}}
This starts a ':raises:' block but it's empty.
----
{{{
try:
ports.pop(ports.index(str(virtport)))
except ValueError:
ports.pop(ports.index(longport))
}}}
This raises a ValueError if both the virtport and longport doesn't exist.
In this case better to go with a stem.InvalidArguments.
Cheers! -Damian
--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/12533#comment:10>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online
_______________________________________________
tor-bugs mailing list
tor-bugs@xxxxxxxxxxxxxxxxxxxx
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs