Repository: syncthing/syncthing (language: go)

Issue:
Allow specifying ports in --generate
On Windows, I am updating my installer to generate the `config.xml` at first installation so I can set the config options appropriate before the user starts Syncthing for the first time.

When I run `syncthing generate`, the Windows firewall prompts me to create a firewall rule. Is this because `syncthing.exe` opens a network connection?

Does `syncthing generate` need to access the network, and if so, can we change this behavior so that `syncthing generate` doesn't perform any network access?


Discussion / hints:
What's the suggested rule? As far as I know there should be no network activity required or intended for `-generate`.
I believe what happens is that when you run `syncthing.exe`, it listens on one or more TCP ports. Windows detects that `syncthing.exe` is listening and prompts the user if they want to create firewall rules for it. If I remove all firewall rules that refer to `syncthing.exe` and run `syncthing generate`, Windows prompts me to create the firewall rules.

So what this seems to mean is that when we run `syncthing generate` on Windows, `syncthing.exe` listens on one or more TCP ports (if even for a short time), because the OS asks to create the rules. (Normally this behavior is helpful, but not when you are trying to automate an installation and configuration process that shouldn't block for various reasons.)
I don't think we can avoid that. We need to pick a free port.
Even when just generating a config and then exiting?
Ah, right, yes. Because on first start / when generating the config we pick a GUI port - 8384 if it's free, but otherwise some other one, in case there are multiple instances of Syncthing or something else in the way. 
Ah. In that case, nothing to fix here. My workaround to avoid this is to check for a firewall rule first.
What happens, then, in the following scenario?

1. User A logs on and starts Syncthing, TCP 8384 is available, so Syncthing uses it.
2. User B logs on and starts Syncthing. Nobody is using TCP 8384, so user B's Syncthing also uses that port.
3. User A logs back on while user B is still logged on.

What happens then?
Can the GUI port be bound to 127.0.0.1? That would not cause a firewall prompt, yes?
I think it does that by default? So I don't think so?

Regardless, agreed the generate logic is not bullet proof if you run multiple instances, but I don't think we are going to fix anything here, as this is not the common case.

Understood. I guess I was trying to make the point that the `generate` implementation doesn't _have to_ make a TCP binding and in fact there are cases where this won't work anyway, so maybe there's an improved approach that could be considered.
And what's the improved approach?
One alternative would be to allow the user to specify the desired port(s) with the `generate` command. If user specifies port(s), then there's no need to perform a binding. (This approach would be more convenient from an automation perspective, where configs could be generated by script for multiple user scenarios.)
Sure, I guess we allow specifying the username/password, so I don't see why we wouldn't allow port.
I started looking into this and think it needs some more clarification.  In principle, what you want to achieve can be done with just a snippet `config.xml` file that you pre-populate only with those options that your installer wants to set.  The purpose for the `--gui-user` and `--gui-password` options is mainly to avoid the need to hash the password externally.  Giving either of those results in a fully normalized `config.xml` being generated, possibly based on any pre-existing file.

Right now, when a `config.xml` file already exists during `syncthing generate`, we just leave it alone as long as none of the above options were supplied.  That could be changed to always read, normalize and write the config file, which would otherwise happen anyway during the first run of `syncthing serve`.  That avoids the need for additional CLI options to set any possible config element.  And your installer can make sure the config snippet is valid before actually running `syncthing serve`.

What may help with the original issue here would be a new `--skip-port-probing` option, which applies for both `generate` and `serve` (first run) subcommands.  Note that the probing is done for the GUI and the listen address, but only when no `config.xml` file previously exists. 
> What may help with the original issue here would be a new `--skip-port-probing` option, which applies for both `generate` and `serve` (first run) subcommands. Note that the probing is done for the GUI and the listen address, but only when no `config.xml` file previously exists.

Yes, I believe this would work perfectly for this use case. Thanks!
Any opinions on whether `syncthing generate` should not touch a pre-existing `config.xml` in any case?  IMHO it would be fine to make it always result in a fully normalized config file.
It seems to me that a case could be made either way. If `generate` always writes out a fully normalized `config.xml` file, it would be good to preserve existing settings (when possible).
Of course, that's the idea. Parse anything that's already there and fill in the rest from the hard coded defaults. It's already like that, but the step is sometimes postponed to `syncthing serve`. 
This seems fine to me. A possible caveat is that if the hand made config doesn't have the expected version (`<config version="32">`) we might run a bunch of migrations that may or may not make sense under the circumstances.

I wouldn't jump to the whole complexity of migrations.  Only try to parse the config as is, then serialize and write it back to XML.  Just as the generate subcommand does now.  Migrations may possibly depend on the database being available?
The config loader does some migrations when it loads a config of an old format. Probably nothing destructive, though.
Sorry you're right.  I thought that was happening in `syncthing/utils.go` but it's actually called when creating the config Wrapper.

Fix patch:
diff --git a/cmd/syncthing/cmdutil/options_common.go b/cmd/syncthing/cmdutil/options_common.go
index 34485e6a563..0751892d8b4 100644
--- a/cmd/syncthing/cmdutil/options_common.go
+++ b/cmd/syncthing/cmdutil/options_common.go
@@ -12,4 +12,5 @@ type CommonOptions struct {
 	ConfDir         string `name:"config" placeholder:"PATH" help:"Set configuration directory (config and keys)"`
 	HomeDir         string `name:"home" placeholder:"PATH" help:"Set configuration and data directory"`
 	NoDefaultFolder bool   `env:"STNODEFAULTFOLDER" help:"Don't create the \"default\" folder on first startup"`
+	SkipPortProbing bool   `help:"Don't try to find free ports for GUI and listen addresses on first startup"`
 }
diff --git a/cmd/syncthing/generate/generate.go b/cmd/syncthing/generate/generate.go
index 4f5c2fa140b..4df7c3d46d1 100644
--- a/cmd/syncthing/generate/generate.go
+++ b/cmd/syncthing/generate/generate.go
@@ -58,13 +58,13 @@ func (c *CLI) Run() error {
 		c.GUIPassword = string(password)
 	}
 
-	if err := Generate(c.ConfDir, c.GUIUser, c.GUIPassword, c.NoDefaultFolder); err != nil {
+	if err := Generate(c.ConfDir, c.GUIUser, c.GUIPassword, c.NoDefaultFolder, c.SkipPortProbing); err != nil {
 		return fmt.Errorf("Failed to generate config and keys: %w", err)
 	}
 	return nil
 }
 
-func Generate(confDir, guiUser, guiPassword string, noDefaultFolder bool) error {
+func Generate(confDir, guiUser, guiPassword string, noDefaultFolder, skipPortProbing bool) error {
 	dir, err := fs.ExpandTilde(confDir)
 	if err != nil {
 		return err
@@ -92,7 +92,7 @@ func Generate(confDir, guiUser, guiPassword string, noDefaultFolder bool) error
 	cfgFile := locations.Get(locations.ConfigFile)
 	cfg, _, err := config.Load(cfgFile, myID, events.NoopLogger)
 	if fs.IsNotExist(err) {
-		if cfg, err = syncthing.DefaultConfig(cfgFile, myID, events.NoopLogger, noDefaultFolder); err != nil {
+		if cfg, err = syncthing.DefaultConfig(cfgFile, myID, events.NoopLogger, noDefaultFolder, skipPortProbing); err != nil {
 			return fmt.Errorf("create config: %w", err)
 		}
 	} else if err != nil {
diff --git a/cmd/syncthing/main.go b/cmd/syncthing/main.go
index 07deb9be88f..98175f80fd4 100644
--- a/cmd/syncthing/main.go
+++ b/cmd/syncthing/main.go
@@ -337,7 +337,7 @@ func (options serveOptions) Run() error {
 	}
 
 	if options.GenerateDir != "" {
-		if err := generate.Generate(options.GenerateDir, "", "", options.NoDefaultFolder); err != nil {
+		if err := generate.Generate(options.GenerateDir, "", "", options.NoDefaultFolder, options.SkipPortProbing); err != nil {
 			l.Warnln("Failed to generate config and keys:", err)
 			os.Exit(svcutil.ExitError.AsInt())
 		}
@@ -551,7 +551,7 @@ func syncthingMain(options serveOptions) {
 	evLogger := events.NewLogger()
 	earlyService.Add(evLogger)
 
-	cfgWrapper, err := syncthing.LoadConfigAtStartup(locations.Get(locations.ConfigFile), cert, evLogger, options.AllowNewerConfig, options.NoDefaultFolder)
+	cfgWrapper, err := syncthing.LoadConfigAtStartup(locations.Get(locations.ConfigFile), cert, evLogger, options.AllowNewerConfig, options.NoDefaultFolder, options.SkipPortProbing)
 	if err != nil {
 		l.Warnln("Failed to initialize config:", err)
 		os.Exit(svcutil.ExitError.AsInt())
diff --git a/lib/config/config.go b/lib/config/config.go
index 67ea8a02425..302cc760c2b 100644
--- a/lib/config/config.go
+++ b/lib/config/config.go
@@ -113,18 +113,16 @@ func New(myID protocol.DeviceID) Configuration {
 	return cfg
 }
 
-func NewWithFreePorts(myID protocol.DeviceID) (Configuration, error) {
-	cfg := New(myID)
-
+func (cfg *Configuration) ProbeFreePorts() error {
 	port, err := getFreePort("127.0.0.1", DefaultGUIPort)
 	if err != nil {
-		return Configuration{}, errors.Wrap(err, "get free port (GUI)")
+		return errors.Wrap(err, "get free port (GUI)")
 	}
 	cfg.GUI.RawAddress = fmt.Sprintf("127.0.0.1:%d", port)
 
 	port, err = getFreePort("0.0.0.0", DefaultTCPPort)
 	if err != nil {
-		return Configuration{}, errors.Wrap(err, "get free port (BEP)")
+		return errors.Wrap(err, "get free port (BEP)")
 	}
 	if port == DefaultTCPPort {
 		cfg.Options.RawListenAddresses = []string{"default"}
@@ -136,7 +134,7 @@ func NewWithFreePorts(myID protocol.DeviceID) (Configuration, error) {
 		}
 	}
 
-	return cfg, nil
+	return nil
 }
 
 type xmlConfiguration struct {
diff --git a/lib/syncthing/utils.go b/lib/syncthing/utils.go
index 5f59af2b64c..bf4752494e1 100644
--- a/lib/syncthing/utils.go
+++ b/lib/syncthing/utils.go
@@ -59,9 +59,12 @@ func GenerateCertificate(certFile, keyFile string) (tls.Certificate, error) {
 	return tlsutil.NewCertificate(certFile, keyFile, tlsDefaultCommonName, deviceCertLifetimeDays)
 }
 
-func DefaultConfig(path string, myID protocol.DeviceID, evLogger events.Logger, noDefaultFolder bool) (config.Wrapper, error) {
-	newCfg, err := config.NewWithFreePorts(myID)
-	if err != nil {
+func DefaultConfig(path string, myID protocol.DeviceID, evLogger events.Logger, noDefaultFolder, skipPortProbing bool) (config.Wrapper, error) {
+	newCfg := config.New(myID)
+
+	if skipPortProbing {
+		l.Infoln("Using default network port numbers instead of probing for free ports")
+	} else if err := newCfg.ProbeFreePorts(); err != nil {
 		return nil, err
 	}
 
@@ -84,11 +87,11 @@ func DefaultConfig(path string, myID protocol.DeviceID, evLogger events.Logger,
 // creates a default one, without the default folder if noDefaultFolder is ture.
 // Otherwise it checks the version, and archives and upgrades the config if
 // necessary or returns an error, if the version isn't compatible.
-func LoadConfigAtStartup(path string, cert tls.Certificate, evLogger events.Logger, allowNewerConfig, noDefaultFolder bool) (config.Wrapper, error) {
+func LoadConfigAtStartup(path string, cert tls.Certificate, evLogger events.Logger, allowNewerConfig, noDefaultFolder, skipPortProbing bool) (config.Wrapper, error) {
 	myID := protocol.NewDeviceID(cert.Certificate[0])
 	cfg, originalVersion, err := config.Load(path, myID, evLogger)
 	if fs.IsNotExist(err) {
-		cfg, err = DefaultConfig(path, myID, evLogger, noDefaultFolder)
+		cfg, err = DefaultConfig(path, myID, evLogger, noDefaultFolder, skipPortProbing)
 		if err != nil {
 			return nil, errors.Wrap(err, "failed to generate default config")
 		}


Test patch: