Opened 4 months ago

Closed 3 months ago

#25449 closed defect (fixed)

Bump snowflake/go-webrtc for trac 21312

Reported by: arlolra Owned by: tbb-team
Priority: Medium Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords: snowflake, TorBrowserTeam201803R
Cc: dcf Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

Please see the attached.

Incidentally, this should also fix,
https://trac.torproject.org/projects/tor/ticket/24203#comment:11

However, note that this now means there won't be a snowflake.log anymore, unless we provide a path to -log in the torrc-defaults-appendix

Child Tickets

Attachments (1)

0001-Bump-snowflake-go-webrtc-for-trac-21312.patch (1.4 KB) - added by arlolra 4 months ago.

Download all attachments as: .zip

Change History (9)

comment:1 in reply to:  description Changed 4 months ago by gk

Replying to arlolra:

However, note that this now means there won't be a snowflake.log anymore, unless we provide a path to -log in the torrc-defaults-appendix

I think that's fine or do you think we should make it easier for debugging for now and add that flag?

comment:2 Changed 4 months ago by arlolra

Given #21304 is still open, it's probably prudent to leave it off for the time being.

comment:3 Changed 4 months ago by dcf

I'm fine with no logging by default. If someone has a problem, hopefully we can ask them to turn on logging.

My removal of default logging in 12922a232b was arguably rash: while obviously eventually we would have to stop logging by default, requiring a -log option with a path prevents writing to a log file that's inside the pt_state directory, which, in sandbox situations, may be the only place the transport plugin is allowed to write. obfs4proxy just has a boolean option -enableLogging that doesn't take a path, unconditionally writing to pt_state/obfs4proxy.log. Perhaps something like that would be better, even if it's less convenient for development.

comment:4 Changed 4 months ago by arlolra

Perhaps something like that would be better, even if it's less convenient for development.

Maybe just add another flag, -logToStateDir or the like

comment:5 Changed 4 months ago by arlolra

--- a/client/snowflake.go
+++ b/client/snowflake.go
@@ -10,6 +10,7 @@ import (
        "net"
        "os"
        "os/signal"
+       "path"
        "strings"
        "sync"
        "syscall"
@@ -126,12 +127,22 @@ func main() {
        brokerURL := flag.String("url", "", "URL of signaling broker")
        frontDomain := flag.String("front", "", "front domain")
        logFilename := flag.String("log", "", "name of log file")
+       logToStateDir := flag.Bool("logToStateDir", false, "supersedes log file")
        max := flag.Int("max", DefaultSnowflakeCapacity,
                "capacity for number of multiplexed WebRTC peers")
        flag.Parse()
 
        webrtc.SetLoggingVerbosity(1)
        log.SetFlags(log.LstdFlags | log.LUTC)
+
+       if *logToStateDir {
+               stateDir, err := pt.MakeStateDir()
+               if err != nil {
+                       log.Fatal(err)
+               }
+               *logFilename = path.Join(stateDir, "snowflake.log")
+       }
+
        if *logFilename != "" {
                logFile, err := os.OpenFile(*logFilename,
                        os.O_CREATE|os.O_APPEND|os.O_WRONLY, 0600)

comment:6 in reply to:  4 Changed 3 months ago by dcf

Status: newmerge_ready

Replying to arlolra:

Perhaps something like that would be better, even if it's less convenient for development.

Maybe just add another flag, -logToStateDir or the like

I opened #25471 to talk about the logging issue separately.

Per comment:2, I think we're fine to go ahead with disabled logging for now. attachment:0001-Bump-snowflake-go-webrtc-for-trac-21312.patch looks good to me.

People are going to like this change. I'm using it right now and it's light years better than it was.

comment:7 Changed 3 months ago by gk

Keywords: TorBrowserTeam201803R added
Status: merge_readyneeds_review

comment:8 Changed 3 months ago by gk

Resolution: fixed
Status: needs_reviewclosed

Applied to master with commit 42720c512520ecef5437d23e405d8ce011764661, thanks!

Note: See TracTickets for help on using tickets.