[tor-bugs] #12088 [Pluggable transport]: goptlib should provide a method for querying the state location.

[Tor Bug Tracker & Wiki]

#12088: goptlib should provide a method for querying the state location.
-------------------------------------+------------------------------
     Reporter:  yawning              |      Owner:  dcf
         Type:  enhancement          |     Status:  needs_review
     Priority:  normal               |  Milestone:
    Component:  Pluggable transport  |    Version:
   Resolution:                       |   Keywords:  goptlib, library
Actual Points:                       |  Parent ID:
       Points:                       |
-------------------------------------+------------------------------

Comment (by yawning):

 Replying to [comment:2 dcf]:
 > Thanks for this patch. My thinking is that the API should work a bit
 differently. I don't think ClientSetup and ServerSetup should silently
 create a directory as a side effect, and I don't think
 TOR_PT_STATE_LOCATION should be required to be set for transports that
 don't use the state directory.

 Ok.

 > Here's what I'm thinking for the API:
 > {{{
 > // Return the directory name in the TOR_PT_STATE_LOCATION environment
 variable,
 > // creating it if it doesn't exist. Returns non-nil error if
 > // TOR_PT_STATE_LOCATION is not set or if there is an error creating the
 > // directory.
 > func MakeStateDir() (string, error) {
 >       dir, err := getenvRequired("TOR_PT_STATE_LOCATION")
 >       if err != nil {
 >               return "", err
 >       }
 >       err = os.MkdirAll(dir, 0700)
 >       return dir, err
 > }
 > }}}
 > (With no changes to ClientInfo and ServerInfo, and nothing involving
 TOR_PT_STATE_LOCATION in ClientSetup and ServerSetup.)
 >
 > I don't know what's the best way to deal with errors from os.MkdirAll.
 The patch in comment:1 crafts a new ENV-ERROR, which is reasonable, even
 though failure to create a directory is not really an environment error.
 It is a bit awkward that the function above will print an error to stdout
 in one case, but not in the other. On the other hand, it doesn't matter so
 much, because any error is likely to be a fatal error; i.e.
 > {{{
 > stateDir, err := pt.MakeStateDir()
 > if err != nil {
 >       log.Fatalf("failed to create state directory: %s", err)
 > }
 > }}}
 > After all, there are many other errors that can happen during the
 execution of a transport (like the ORPort unexpectedly closing), and
 there's currently no way to report generic errors back up to tor other
 than simply terminating. Failure to create a directory could be considered
 one of these generic errors.

 PT error reporting is a bit of a mess.  Another problem with the proposed
 API given our current error handling limitations is that if the pt
 implementation defers calling `pt.MakeStateDir()` till after it sends the
 appropriate `DONE` line back over stdout that the error feedback won't get
 passed to the user at all beyond "yet another frustrating case of 'pt
 randomly died at startup'".  I know wfn had a patch to redirect stderr to
 the tor log which probably solves this in the long run, so perhaps it is
 reasonable to defer till then.

 > What's your impression, how well would this approach work for obfs4?

 Apart from concerns about error handling, obfs4proxy only uses the state
 dir as "the place where the log file goes, and it's disabled by default"
 so it works fine for me.  I'm getting the feeling that exporting a routine
 that pt implementations can call to ENV-ERROR may be nice as a convenience
 thing (For obfs4proxy, I would probably manually ENV-ERROR the mkdir error
 just so it appears in the logs before terminating to save user
 frustration).

 > I think we need some tests for this new function:
 >  1. TOR_PT_STATE_LOCATION not set.
 >  1. Already existing directory.
 >  1. Nonexistent directory, parent exists.
 >  1. Nonexistent directory, parent doesn't exist.
 >  1. Name already exists, but is an ordinary file.
 >  1. Directory name that cannot be created (e.g., no write permission in
 parent).

 Tests 2 through 6 seem to be more like "testing os.Mkdirall()", I'll be
 happy to write such tests, but it feels a bit redundant given
 pkg/os/path_test.go tests most of those conditions.

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/12088#comment:3>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online