goptlib should provide a method for querying the state location.
The only place pluggable transports are allowed to write to is the TOR_PT_STATE_LOCATION. goptlib should support querying (and maybe optionally) creating the directory.
Activity
Proposed patch:
Trac:
Status: new to needs_reviewReplying to dcf:
Proposed patch:
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.
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.
What's your impression, how well would this approach work for obfs4?
I think we need some tests for this new function:
- TOR_PT_STATE_LOCATION not set.
- Already existing directory.
- Nonexistent directory, parent exists.
- Nonexistent directory, parent doesn't exist.
- Name already exists, but is an ordinary file.
- Directory name that cannot be created (e.g., no write permission in parent).
- Author
Replying to 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 appropriateDONEline 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:
- TOR_PT_STATE_LOCATION not set.
- Already existing directory.
- Nonexistent directory, parent exists.
- Nonexistent directory, parent doesn't exist.
- Name already exists, but is an ordinary file.
- 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.
Replying to yawning:
Replying to dcf:
I think we need some tests for this new function:
- TOR_PT_STATE_LOCATION not set.
- Already existing directory.
- Nonexistent directory, parent exists.
- Nonexistent directory, parent doesn't exist.
- Name already exists, but is an ordinary file.
- 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.
But that's the point of tests, right? Don't think too much about what the code is doing internally, just think about what its effects are supposed to be. obfs4's first version of GetStateLocation would not have passed all these tests, after all, so guarding against future refactoring changes seems to make sense.
- Author
I took your API/implementation and wrote the unit tests for it. For the #6 (closed) test I did what path_test did and made sure that creating a subdirectory of a file fails.
Replying to yawning:
I took your API/implementation and wrote the unit tests for it. For the #6 (closed) test I did what path_test did and made sure that creating a subdirectory of a file fails.
Thank you very much. Can you also make a patch showing how this API would be used in obfs4? I don't want it to be too cumbersome over ClientInfo.StateLocation.
- Author
Replying to dcf:
Replying to yawning:
I took your API/implementation and wrote the unit tests for it. For the #6 (closed) test I did what path_test did and made sure that creating a subdirectory of a file fails.
Thank you very much. Can you also make a patch showing how this API would be used in obfs4? I don't want it to be too cumbersome over ClientInfo.StateLocation.
Something like this:
-func ptInitializeLogging(enable bool) { +func ptInitializeLogging(enable bool) error { if enable { - dir, err := ptGetStateDir() - if err != nil || dir == "" { - return + // pt.MakeStateDir will ENV-ERROR for us. + dir, err := pt.MakeStateDir() + if err != nil { + return err } + // While we could just exit, log an ENV-ERROR so it will propagate to + // the tor log. f, err := os.OpenFile(path.Join(dir, obfs4LogFile), os.O_CREATE|os.O_APPEND|os.O_WRONLY, 0600) if err != nil { - log.Fatalf("[ERROR] Failed to open log file: %s", err) + envError := []byte(fmt.Sprintf("ENV-ERROR Failed to open log file: %s", err)) + pt.Stdout.Write(envError) + return err } log.SetOutput(f) } else { log.SetOutput(ioutil.Discard) } + + return nil } Replying to yawning:
Replying to dcf:
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 appropriateDONEline 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.About error reporting: suppose your transport needs to create its own subdirectories in the state dir. You call pt.MakeStateDir, then you manually create the directories inside it. Suppose the creation of one of these directories fails. Is that an ENV-ERROR? If not, then a failure to create the state dir itself probably should also not be an ENV-ERROR. (Except to the extent that we abuse ENV-ERROR as a general-purpose error logging function.)
CMETHOD-ERROR/SMETHOD-ERROR could be a good way to report these kinds of generic errors that happen at startup. "CMETHOD-ERROR foo failed to create state directory." The only thing is I bet application authors want to handle all the state dir stuff once at the top of their program, before they enter the CMETHOD processing loop. It kind of makes me wish for a GENERIC-ERROR that could appear at any time during negotiation.
- Author
Replying to dcf:
About error reporting: suppose your transport needs to create its own subdirectories in the state dir. You call pt.MakeStateDir, then you manually create the directories inside it. Suppose the creation of one of these directories fails. Is that an ENV-ERROR? If not, then a failure to create the state dir itself probably should also not be an ENV-ERROR. (Except to the extent that we abuse ENV-ERROR as a general-purpose error logging function.)
This is true. As far as what the API does, I would prefer it to always ENV-ERROR, or never so the error handling in the applications can do the right thing (and if the latter, I will more than likely continue to abuse ENV-ERROR as a general purpose error reporting mechanism till something better comes around, because providing 0 feedback to the actual user is terrible).
CMETHOD-ERROR/SMETHOD-ERROR could be a good way to report these kinds of generic errors that happen at startup. "CMETHOD-ERROR foo failed to create state directory." The only thing is I bet application authors want to handle all the state dir stuff once at the top of their program, before they enter the CMETHOD processing loop. It kind of makes me wish for a GENERIC-ERROR that could appear at any time during negotiation.
I kind of like the #9957 (moved) approach here ("Spam errors to stderr, tor will propagate the complaints to interested parties"). It's simple, and the code is technically done, so it's more likely to exist sooner than adding new statements to the PT protocol.
Replying to yawning:
I took your API/implementation and wrote the unit tests for it. For the #6 (closed) test I did what path_test did and made sure that creating a subdirectory of a file fails.
This patch looks good to me. You can commit it if you're happy with it too.
- Author
Ty for the review.
Trac:
Status: needs_review to closed
Resolution: N/A to fixed - Trac closed
closed