[tor-bugs] #12087 [Pluggable transport]: goptlib could provide IsClient()/IsServer().

[Tor Bug Tracker & Wiki]

#12087: goptlib could provide IsClient()/IsServer().
-------------------------------------+------------------------------
     Reporter:  yawning              |      Owner:  asn
         Type:  enhancement          |     Status:  needs_review
     Priority:  minor                |  Milestone:
    Component:  Pluggable transport  |    Version:
   Resolution:                       |   Keywords:  goptlib, library
Actual Points:                       |  Parent ID:
       Points:                       |
-------------------------------------+------------------------------

Comment (by yawning):

 Replying to [comment:2 dcf]:
 > I'm fine with applications having to peek at environment variables if
 they need this functionality, at least in the short term.
 >
 > I'm thinking about the application's interface to these functions. I
 guess it would have to look like this?
 > {{{
 > if pt.IsClient() {
 >         // do client stuff
 > } else if pt.IsServer() {
 >         // do server stuff
 > } else {
 >         // handle error? pt.EnvError?
 > }
 > }}}

 obfs4proxy does:
 {{{
 launched := false
 if pt.IsClient() {
     launched = clientSetup()
 } else if pt.IsServer() {
     launched = serverSetup()
 }
 if !launched {
     log.Fatal("Something evil happened.")
 }
 }}}

 `clientSetup()`/`serverSetup()` will finish the pt config protocol so the
 `log.Fatal()` doesn't affect anything.

 > Since "client mode" and "server mode" are the only two possibilities,
 maybe it makes sense to have only one function to test the mode, rather
 than two, so that there can be no ambiguity or disagreement between the
 two functions? In other words,
 > {{{
 > if pt.IsClient() {
 >         // do client stuff
 > } else {
 >         // do server stuff
 > }
 > }}}
 > If there are two separate functions, then we should document and
 implement one to be the inverse of the other, rather than having two
 separate functions testing two different environment variables. I'm not
 such a fan of that idea, because the second function amounts to a
 convenience function. It would look like:
 > {{{
 > func IsServer() bool {
 >         return !IsClient()
 > }
 > }}}

 How about something like:
 {{{
 type ManagedMode int

 const (
     ManagedClient ManagedMode = iota
     ManagedServer
 )

 func ManagedMode() (ManagedMode, error) {
     // Examine both client and server transports to handle pathological
 inputs, EnvError as appropriate.
 }
 }}}

 That would let ManagedMode() examine all the things that should be tested
 and do error handling.

 > I'd like there to be additional test cases. The code should do something
 reasonable in all of them.
 >  1. TOR_PT_CLIENT_TRANSPORTS and TOR_PT_SERVER_TRANSPORTS both unset.
 >  2. TOR_PT_CLIENT_TRANSPORTS unset, TOR_PT_SERVER_TRANSPORTS set.
 >  3. TOR_PT_CLIENT_TRANSPORTS set, TOR_PT_SERVER_TRANSPORTS unset.
 >  4. TOR_PT_CLIENT_TRANSPORTS and TOR_PT_SERVER_TRANSPORTS both set.

 With the IsClient()/IsServer() model, unless the method signature gets
 changed to return `(bool, error)`, recovering from both set is left up to
 waiting for `ClientSetup`/`ServerSetup` to fail after `IsFoo()` returns an
 approximate guess.

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