various small fixes #1151

0pcom · 2022-04-11T19:46:56Z

Changes:

fix hidden flags nil pointer error on mac & windows ( --all flag error)
optimized run-source & run-vpnsrv makefile directives and related subdirectives
fix visor read config from stdin for systray

On that last one, runVisor now can accept a config passed to it as input (or nil). Running with systray was trying to read the config a second time, calling for initConfig . When the config comes via stdin there is no second read possible.

Additions:

prepare and prepare-systray makefile directives
run-systray makefile directive
run-systray-test makefile directive

How to test this PR:

make run-systray-test

…root

0pcom · 2022-04-11T23:14:18Z

In tests I discovered that the systray app is not appropriate to run as root on linux and does not function as intended. It possibly could but it cannot directly launch the browser as root. I have disabled systray if the root user is detected on macOS or linux.

privilege elevation for the vpn client / server is still yet to be addressed.

local/.gitignore

ersonp · 2022-04-12T08:15:33Z

In tests I discovered that the systray app is not appropriate to run as root on linux and does not function as intended. It possibly could but it cannot directly launch the browser as root. I have disabled systray if the root user is detected on macOS or linux.

privilege elevation for the vpn client / server is still yet to be addressed.

we need to set cap_net_admin by sudo setcap 'cap_net_admin+p' ./apps/vpn-client and sudo setcap 'cap_net_admin+p' ./apps/vpn-server

andi think we should provide a WARN log in case if the systray app is started as root.

cmd/apps/skysocks-client/skysocks-client.go

cmd/apps/skychat/skychat.go

cmd/apps/vpn-client/vpn-client.go

cmd/apps/skychat/skychat.go

ersonp · 2022-04-12T09:04:33Z

One small change is needed for skywire windows. In windows config gen the only app that needs to be included is vpn-client as all other apps are not supported.

… are not found at path specified in config during config gen with small exceptions

0pcom · 2022-04-12T19:22:48Z

On the systray app I changed the behavior from being disabled as root to only hiding the buttons that launch the browser when the process is run as root.

on the config gen @ersonp I changed it to detect if the applications are at the bin path, and if not they are added to the list of apps to disabled

exceptions to this behavior are in two instances;

the config is being written to stdout (-n flag)
the cli command was started with go run

most of the other stuff mentioned has been cleaned up

… as root to non root-owned path and fail before attempted non-root write to root-owned path for skywire-cli config gen and skywire-visor

0pcom · 2022-04-13T04:14:51Z

I've prevented writing to non-root owned path as root and vice versa. With skywire-cli config gen and with skywire-visor

@ersonp I had to revert the -o flag for config gen to the way I had it before your recent PR.

At issue is flags.Changed not working right.. my workaround was to have the output be actually unset and set the value after it's detected that it contains an empty string, as well as to record that it was unset into a boolean to be used for overriding the output of the -p flag default config path

I was otherwise unable to get the -o flag to override the -p flag for config gen.

Tomorrow I need to add a flag which will generate one default config at a set location in the user space, which references the users.db and local folder along the same path as the config.

ersonp · 2022-04-14T11:49:12Z

Can we just limit the scope of this PR to flags and reading from STDIN because the changes with the path is not actually solving the problem but just shifting it. I think it would be better if we fixed it via the read and write functions of all the files in the /local folder. If I remember correctly this issue was solved previously and only reappeared a little while ago (as far as I can tell). So I think it would be better if we took a deeper look at it in a different PR.

…ging more descriptive around these errors. Add dtection of unpublised changes since commit in logging

0pcom · 2022-04-14T15:59:47Z

@ersonp I've changed it to warn about writing with elevated permissions here:

I want to maintain the failure in the reverse instance as not failing means it runs but doesn't work, and just repeats "waiting for RPC get ready"

jdknives · 2022-04-14T20:17:58Z

cmd/skywire-cli/commands/config/gen.go

@@ -261,6 +335,32 @@ var genConfigCmd = &cobra.Command{
 		}
 		//don't write file with stdout
 		if !stdout {
+			thisUser, err := user.Current()


Something like userLvl or owner etc sounds better.

jdknives · 2022-04-14T20:17:58Z

cmd/skywire-cli/commands/config/gen.go

@@ -261,6 +335,32 @@ var genConfigCmd = &cobra.Command{
 		}
 		//don't write file with stdout
 		if !stdout {
+			thisUser, err := user.Current()


Something like userLvl or owner etc sounds better.

jdknives · 2022-04-14T20:18:45Z

cmd/skywire-cli/commands/config/gen.go

@@ -261,6 +335,32 @@ var genConfigCmd = &cobra.Command{
 		}
 		//don't write file with stdout
 		if !stdout {
+			thisUser, err := user.Current()
+			if err != nil {
+				panic(err)


What errors could this return? Are they clear enough in themselves? Should we exit cleanly and log something here instead?

jdknives · 2022-04-14T20:18:45Z

cmd/skywire-cli/commands/config/gen.go

@@ -261,6 +335,32 @@ var genConfigCmd = &cobra.Command{
 		}
 		//don't write file with stdout
 		if !stdout {
+			thisUser, err := user.Current()
+			if err != nil {
+				panic(err)


What errors could this return? Are they clear enough in themselves? Should we exit cleanly and log something here instead?

no need to really error here & Ive changed it to fail transparently. This is only checking for root and it will just not change the default from false to true.

jdknives · 2022-04-15T08:31:33Z

internal/gui/gui.go

-		initQuitBtn()
-
-		go handleUserInteraction(conf, doneCh)
+	if checkRoot() {


isRoot maybe clearer?

jdknives · 2022-04-15T08:32:39Z

internal/gui/gui_unix.go


 func platformExecUninstall() error {
 	return osutil.Run("/bin/bash", "-c", deinstallerPath)
 }
+
+func checkRoot() bool {
+	thisUser, err := user.Current()


Same suggestion as above. Using this sounds odd to me.

jdknives · 2022-04-15T08:34:26Z

internal/gui/gui_unix.go

+func checkRoot() bool {
+	thisUser, err := user.Current()
+	if err != nil {
+		panic(err)


I would prefer to not panic here and log a fatal err with some context.

the logging overhead is unnecessary and it doesn't really need to fail, only to return true if root is detected.

this check is for limiting the available buttons in the systray to ones that are appropriate to launch by the current user. The browser won't be allowed to run as root in the userspace by the system, typically.

jdknives · 2022-04-15T08:36:48Z

pkg/skyenv/values.go

@@ -169,3 +169,9 @@ func Version() string {
 	}
 	return v
 }
+
+// HomePath gets the current user's home folder
+func HomePath() string {


What if this returns an error? Will the visor start? If so, where will it start?

You are correct, this could cause an odd configuration to be produced if HomePath returns an empty string.

I've now implemented a check on the config gen command to not offer the -u --user flag if HomePath returns an empty string. Similarly to how the visor determines if the -u flag should be an option based on the presence of the config or not.

if HomePath returned empty before these changes it would have been obvious from the config gen help menu as it's displayed in the flag description.

jdknives · 2022-04-15T08:37:36Z

pkg/skyenv/values_darwin.go

+	var usrconfig PkgConfig
+	usrconfig.Launcher.BinPath = "/Library/Application Support/Skywire/apps"
+	usrconfig.LocalPath = HomePath() + "/.skywire/local"
+	usrconfig.Hypervisor.DbPath = HomePath() + "/.skywire/users.db" //permissions errors if the process is not run as root.


What are you trying to say with the comment? It seems it was copied from previous implementation for opt. Does it indeed fail if not run as root in HOME?

accidental inclusion, and the original comment issue is now basically resolved

jdknives · 2022-04-15T08:38:54Z

pkg/skyenv/values_windows.go

+	var usrconfig PkgConfig
+	usrconfig.Launcher.BinPath = "C:/Program Files/Skywire/apps"
+	usrconfig.LocalPath = HomePath() + "/.skywire/local"
+	usrconfig.Hypervisor.DbPath = HomePath() + "/.skywire/users.db" //permissions errors if the process is not run as root.


Same question about the validity of the comment as above.

accidental inclusion, and the original comment issue is now basically resolved

0pcom added 4 commits April 11, 2022 14:24

various small fixes

179b821

make format check

b12e954

run-systray makefile directive

0ec4869

disable systray when visor is run as root - browser cannot launch as …

998b5e0

…root

require root user to run VPN client & server

b158522

ersonp reviewed Apr 12, 2022

View reviewed changes

local/.gitignore Outdated Show resolved Hide resolved

mrpalide requested changes Apr 12, 2022

View reviewed changes

0pcom added 2 commits April 12, 2022 14:11

disable browser-launching systray buttons as root ; disable apps that…

cc62436

… are not found at path specified in config during config gen with small exceptions

disable browser-launching systray buttons as root ; disable apps that…

6fce3d7

… are not found at path specified in config during config gen with small exceptions

0pcom added 3 commits April 12, 2022 14:30

fix app autodisable in config gen

cf7778a

fail on incorrect or insufficient write permissions - prevent writing…

4d86344

… as root to non root-owned path and fail before attempted non-root write to root-owned path for skywire-cli config gen and skywire-visor

fail on incorrect or insufficient write permissions - prevent writing…

6a1a4a7

… as root to non root-owned path and fail before attempted non-root write to root-owned path for skywire-cli config gen and skywire-visor

0pcom added 6 commits April 13, 2022 13:20

implement -u --user flag for skywire-cli config gen & skywire-visor

6b5fab8

fix -u flag imlementation for skywire-visor

f83f340

implement -u --user flag for skywire-cli config gen & skywire-visor

650ccf3

implement -u --user flag for skywire-cli config gen & skywire-visor

cb14f5f

fix default visor config name when unspecified

1cb56a0

revert apps back to using pflags

eb20e12

change fail to warn about writing with elevated permissions. Make log…

20aa58f

…ging more descriptive around these errors. Add dtection of unpublised changes since commit in logging

ersonp approved these changes Apr 15, 2022

View reviewed changes

jdknives approved these changes Apr 15, 2022

View reviewed changes

0pcom added 2 commits April 15, 2022 08:51

cleanup per @jdknives suggestions

54c17d6

make format check

7894274

mrpalide approved these changes Apr 18, 2022

View reviewed changes

mrpalide merged commit 28ea17d into skycoin:develop Apr 18, 2022

0pcom deleted the x branch April 29, 2022 01:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

various small fixes #1151

various small fixes #1151

0pcom commented Apr 11, 2022

0pcom commented Apr 11, 2022

ersonp commented Apr 12, 2022 •

edited

Loading

ersonp commented Apr 12, 2022 •

edited

Loading

0pcom commented Apr 12, 2022

0pcom commented Apr 13, 2022

ersonp commented Apr 14, 2022

0pcom commented Apr 14, 2022

jdknives Apr 14, 2022

jdknives Apr 14, 2022

jdknives Apr 14, 2022

jdknives Apr 14, 2022

0pcom Apr 15, 2022

jdknives Apr 15, 2022

0pcom Apr 15, 2022

jdknives Apr 15, 2022

0pcom Apr 15, 2022

jdknives Apr 15, 2022

0pcom Apr 15, 2022

jdknives Apr 15, 2022

0pcom Apr 15, 2022

jdknives Apr 15, 2022

0pcom Apr 15, 2022

jdknives Apr 15, 2022

0pcom Apr 15, 2022

various small fixes #1151

various small fixes #1151

Conversation

0pcom commented Apr 11, 2022

0pcom commented Apr 11, 2022

ersonp commented Apr 12, 2022 • edited Loading

ersonp commented Apr 12, 2022 • edited Loading

0pcom commented Apr 12, 2022

0pcom commented Apr 13, 2022

ersonp commented Apr 14, 2022

0pcom commented Apr 14, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ersonp commented Apr 12, 2022 •

edited

Loading

ersonp commented Apr 12, 2022 •

edited

Loading