Re: [tor-bugs] #3629 [arm]: Arm/Tor Deb Torrc Configuration

["Tor Bug Tracker & Wiki" <torproject-admin@xxxxxxxxxxxxxx>]

#3629: Arm/Tor Deb Torrc Configuration
-------------------------+--------------------------------------------------
 Reporter:  atagar       |          Owner:  ioerror
     Type:  enhancement  |         Status:  new    
 Priority:  normal       |      Milestone:         
Component:  arm          |        Version:         
 Keywords:               |         Parent:         
   Points:               |   Actualpoints:         
-------------------------+--------------------------------------------------

Comment(by ioerror):

 Replying to [comment:13 kragen]:
 > Commentary on the above tarball:
 >
 > Overall, it seems like it has some extra complexity that could be
 removed, and a worrisome tendency to discard information about errors and
 continue, rather than reporting the error and stopping.
 >

 Sounds good. I'll change that.

 > I think there is no point in exiting if you're not in the right group if
 you're root; it will add just as much inconvenience to legitimate users as
 to attackers.
 >

 Sure, I've added a check for that case.

 > I think the indirection between {{{HELPER_NAME}}} and
 {{{TOR_ARM_REPLACE_TORRC}}} does not provide any benefit, so you should
 remove it, and the .h file that it exists in.
 >

 Hrm, I'm not sold on this but I'll consider it.

 > In the Makefile, I think rather than just ignoring failure from {{{mkdir
 -p}}}, you should check for the failure that you expect, e.g.:
 >
 > {{{
 > VARLIBDIR=$(DESTDIR)/var/lib/tor-arm
 > ...
 > [ -d $(VARLIBDIR) ] || mkdir -p $(VARLIBDIR)
 > }}}

 Sounds good. I've done that as well as updated it to ensure that
 $(VARLIBDIR) is owned by root:$GROUP

 > Also, your makefile is full of problems if DESTDIR contains spaces.  Not
 sure if you want to do something about that; it's a tricky problem to
 solve in the context of make.
 >

 Suggestions? Does Debian ensure that at package build that DESTDIR is
 "safe" in this regard?

 > In the Python, I think you should get rid of all of the try: except:
 that don't specify which exceptions to catch, because those will produce
 misleading error messages if something unexpected goes wrong.
 >

 OK.

 > Also, I already suggested rewriting _checkId more or less as follows:
 >
 > {{{
 > def _checkId(group=GROUP):
 >     return os.geteuid() == 0 and grp.getgrnam(group).gr_gid in
 os.getgroups()
 > }}}
 > If you want to print a friendly error message, you could do that in
 `main` where you're printing all the other friendly error messages:
 >
 > {{{
 > try:
 >     group_ok = _checkId(GROUP)
 > except KeyError:
 >     print "It doesn't appear that " + GROUP + " is a valid group on this
 system!"
 > if group_ok:
 > }}}
 >

 The new code does this.

 > Your overWriteTorConfig will leave the system with a blank or incomplete
 Tor config file if the machine loses power at the wrong time, which could
 produce security problems.

 Hrm, I think you might have reviewed old code - I am copying a file over
 the old file (after backing up the first one), that should be atomic, no?

 >
 > {{{nf.flush()}}} is implied by {{{nf.close()}}}; there's no point in
 doing it explicitly. Maybe you meant to fsync()?  But you didn't.
 >

 I've removed it.

 > I hope this commentary is helpful!

 Very! Thank you!

-- 
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/3629#comment:14>
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