[tor-bugs] #28940 [Obfuscation/Pluggable transport]: Add support for LOG to goptlib

[Tor Bug Tracker & Wiki]

#28940: Add support for LOG to goptlib
---------------------------------------------+-----------------------------
 Reporter:  dcf                              |          Owner:  dcf
     Type:  enhancement                      |         Status:
                                             |  needs_revision
 Priority:  Medium                           |      Milestone:
Component:  Obfuscation/Pluggable transport  |        Version:
 Severity:  Normal                           |     Resolution:
 Keywords:  goptlib                          |  Actual Points:
Parent ID:  #28180                           |         Points:
 Reviewer:                                   |        Sponsor:
---------------------------------------------+-----------------------------
Changes (by dcf):

 * status:  assigned => needs_revision


Comment:

 Thanks for making this branch, ahf.

 First off: I'm really confused about the format of the LOG message. The
 discussion at #21879 and #28181 doesn't seem to have been updated after
 decisions got made on IRC. In addition to LOG, there's also STATUS? It
 would be nice if the format were in a spec, or at least in textual form in
 a ticket somewhere. Especially with regard to what byte values are allowed
 and which must be escaped, and how.

 Here's some quick review.
  *
 https://github.com/ahf/goptlib/commit/2d4cadf91f7da4fb43d8c1e85bb3c851fb660b0c
 (removing advice on how to spy on the stdout output stream) is
 inappropriate. The LOG feature does not replace this feature. While
 debugging, you might want to see for example CMETHOD line that won't just
 be logged verbatim by tor or whatever the controlling program is. And we
 won't be able to rely on the LOG feature for a long time yet.
  * I do not want goptlib to expose a `log.Logger` interface. That is a
 high-level application matter, not something for a low-level library like
 goptlib. If an application needs a `log.Logger`-compatible interface, it
 can write its own wrapper. I feel that there should be exactly one new
 exported function:\\
 {{{
 func Log(severity int, message string)
 }}}
    plus appropriate constants for severities. This is a tiny new feature;
 it doesn't need a new source file.
  * Don't use names like `kvline_escape_value`. Use camel-case names as
 https://golang.org/doc/effective_go.html#mixed-caps
  * I don't want a full-fledged key–value encoding library if we don't need
 it. I would rather have `Log` defined as something like\\
 {{{
 line("LOG", "SEVERITY=" + encodeValue(severity), "MESSAGE=" +
 encodeValue(message))
 }}}
    In the `encodeValue` helper function, don't do a check like
 `kvline_value_needs_escape`. Just escape everything always.
  * No need to
 [https://github.com/ahf/goptlib/commit/c70bebb95b2a9585a31013bbee74fecc593802ea
 #diff-8ab1b4d8f6dfb248d21e27a7fcfee762R29 call bytes.Buffer.Grow]. It will
 grow automatically as you write to it.
  * In the test code, please also test:
    * Values containing `'\0'` (should panic?)
    * Values containing bytes >= `'\x80'` (e.g. UTF-8) (should panic?)
    * Values containing `'='`
    * Empty string
  Unfortunately, per https://spec.torproject.org/pt-spec Section 3.3, we
 are limited to US-ASCII minus `'\x00'` and `'\n'` in ''all'' PT output
 lines, so we need to test what happens when that is violated, even if the
 KV quoted string output allows other values.

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