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 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.

 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.

 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.

 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)
 }}}
 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.

 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.

 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:
 }}}

 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.

 {{{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 hope this commentary is helpful!

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