Skip to content
This repository has been archived by the owner on Jan 17, 2021. It is now read-only.

Add a --bind flag for local port #82

Merged
merged 18 commits into from
May 8, 2019
Merged

Add a --bind flag for local port #82

merged 18 commits into from
May 8, 2019

Conversation

teddy-codes
Copy link
Contributor

Resolves #78

Copy link
Collaborator

@sreya sreya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think instead of exposing the flag to the user, we should detect the remote environment from within sshcode and if the remote environment is ChromeOS we modify the host value.

sshcode.go Outdated Show resolved Hide resolved
@teddy-codes
Copy link
Contributor Author

I am not entirely sure that there is a way to detect if the OS is ChromeOS. I have looked at go tool dist list and the only thing that I have found is linux/*, so I am not sure about the way to do this. I thought that exposing the option to the end user was the best because it always allows for local network access (killing 2 birds with one stone). As a side note, using 0.0.0.0 does not expose your computer to the world unless you have the random port by chance open on your network and the port forwarded to your machine. 0.0.0.0 only opens your computer up to your local network.

@sreya
Copy link
Collaborator

sreya commented May 1, 2019

If you had a malicious user on your local network then it could be dangerous, though I would agree in practice that's unlikely to happen. To reiterate I'd rather not expose that option to the user, the goal of sshcode is to provide a secure, simple way to connect to your code-server instance so I'd like to avoid anything that would enable the user to do otherwise

@sreya
Copy link
Collaborator

sreya commented May 1, 2019

I'll do some research to see if we can detect if an environment is ChromeOS

@teddy-codes
Copy link
Contributor Author

what would you propose as the resolution?

@sreya
Copy link
Collaborator

sreya commented May 1, 2019

To update the host value conditionally based on whether the remote instance is ChromeOS

@teddy-codes
Copy link
Contributor Author

@sreya92 It looks like lsb_release -a returns Gentoo, but not sure how we would implement that in the flow...

- Added check for gentoo on remote instance.
@deansheather
Copy link
Member

Gentoo is an actual Linux distribution which I assume ChromeOS builds off of. This solution will also work on actual Gentoo distributions as well.

@teddy-codes
Copy link
Contributor Author

How is it possible to differentiate the two?

@sreya
Copy link
Collaborator

sreya commented May 7, 2019

@roberthmiller I looked into this a good bit too and couldn't find a good indicator...after talking with @ammario I think the best solution is to go with a flag that we expose to the user like you originally proposed, I think we call it --bind, is this something you'd still like to work on?

@teddy-codes teddy-codes changed the base branch from master to fix-display May 7, 2019 19:46
@teddy-codes teddy-codes changed the base branch from fix-display to master May 7, 2019 19:46
@teddy-codes
Copy link
Contributor Author

I did not remove localPort from the options struct because the test was using it.

Copy link
Collaborator

@sreya sreya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also need to update the test to reflect the updated changes to the options

sshcode.go Outdated Show resolved Hide resolved
sshcode.go Outdated Show resolved Hide resolved
sshcode.go Outdated Show resolved Hide resolved
sshcode.go Outdated Show resolved Hide resolved
sshcode.go Outdated Show resolved Hide resolved
main.go Outdated Show resolved Hide resolved
@Zate
Copy link

Zate commented May 7, 2019

I'm not sure the bind to localhost is the right option for ChromeOS. Running this on ChromeOS is likely to only be done through Crostini. Crostini is a Debian Stretch based container that sits inside the ChromeOS managed by a VM (Termina). It can be reached by the name "penguin.linux.test" in it's default state and anything listening in the container on 0.0.0.0 will generally be able to be hit. I don't really think there will be a huge need to ssh into the Crostini container as it's not exposed externally by default.

You can use sshcode from inside that container to an external ssh host and it opens up in the ChromeOS browser just fine using www-browser --url https://<urltocode:port. The --app doesnt work though because the garcon grpc connector through to the main ChromeOS doesn't support it (yet).

The real I would simply add either a --chromeos flag, or a --local flag, or actually have it detect the crostini container by looking for /opt/google/cros-containers folder, or /ChromeOS/ folder or a few others. The /opt/google/cros-containers contains some binaries (such as garcon) with are only present in correctly setup and mounted penguin containers under ChromeOS (so far that I have seen).

If you accurately work out its Crostini, or allow the users to specify via a flag, you basically just "skip" the shh part and pretend this is the server you ssh'd to and set everything up. Download the binary, setup the folders etc (no where really to sync the extensions etc from unless perhaps from a local vscode install). You then call the www-browser binary and pass it the http://penguin.linux.test:port url and it will pop up inside the regular ChromeOS environment and work as if it was remote.

Why would someone want this? Well, the experience of running code inside a browser is actually smoother than running it natively in the crostini container in my testing. Plus having it just in a tab vs a stand alone window is simple and easy.

I've got some code somewhere I could dig up that got this working on my pixelbook, plus some that started on porting sshcode to work under powershell/windows. (Kinda paused that to see how the new terminal on windows plays out).

@sreya
Copy link
Collaborator

sreya commented May 7, 2019

@Zate this is to fix using ChromeOS as a client not a server. Currently you cannot tunnel to localhost on a chromeOS client and access code-server from the browser

@teddy-codes teddy-codes changed the title added fix for chromeos host added fix for chromeos client May 7, 2019
sshcode.go Outdated
o.localPort, err = randomPort()
var bindHost string
var bindPort string
bindHost, bindPort, err = net.SplitHostPort(o.bindAddr)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can short assign this :=

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't because of the reassignment of err

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Try it :)

sshcode.go Outdated
if err != nil {
return xerrors.Errorf("failed to parse bind address: %w", err)
}
if bindPort == "" {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would probably pull this out into a separate function func parseBindAddr(bindAddr string) (bindAddr string, err error) that splits the address and ensures that neither host nor port is empty. If one is, it gets assigned a default value, 127.0.0.1 for host, and a random port for port and then join it back together with net.JoinHostPort

sshcode.go Outdated
@@ -93,11 +101,12 @@ func sshCode(host, dir string, o options) error {
return xerrors.Errorf("failed to find available remote port: %w", err)
}

flog.Info("Tunneling local port %v to remote port %v", o.localPort, o.remotePort)
flog.Info("Tunneling local host %v:%v to remote port %v", bindHost, bindPort, o.remotePort)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd probably reword to say Tunneling remote port %v to %v, o.remotePort, bindAddr

sshcode_test.go Outdated
@@ -37,7 +37,7 @@ func TestSSHCode(t *testing.T) {
defer wg.Done()
err := sshCode("foo@127.0.0.1", "", options{
sshFlags: testSSHArgs(sshPort),
localPort: localPort,
bindAddr: fmt.Sprintf("127.0.0.1:%v", localPort),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

net.JoinHostPort("127.0.0.1", localPort)

main.go Outdated
@@ -52,6 +54,8 @@ func (c *rootCmd) RegisterFlags(fl *flag.FlagSet) {
fl.BoolVar(&c.skipSync, "skipsync", false, "skip syncing local settings and extensions to remote host")
fl.BoolVar(&c.syncBack, "b", false, "sync extensions back on termination")
fl.BoolVar(&c.printVersion, "version", false, "print version information and exit")
fl.BoolVar(&c.noOpen, "no-open", false, "do not open web browser")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

after speaking with a colleague let's handle the no-open in a separate PR, sorry for indicating otherwise earlier

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No worries. WIll change it

@sreya sreya changed the title added fix for chromeos client Add a --bind flag for local port May 8, 2019
@sreya sreya merged commit dbc0ff5 into coder:master May 8, 2019
@Zate
Copy link

Zate commented May 8, 2019

@Zate this is to fix using ChromeOS as a client not a server. Currently you cannot tunnel to localhost on a chromeOS client and access code-server from the browser

I assume by ChromeOS you mean the crostini container inside ChromeOS, or are you talking about a device in "dev mode" where you have shell access to ChromeOS itself, or even via crouton? Sorry I assumed this was related to something I posted a while ago #28 because I got a reference to this pull request.

@teddy-codes
Copy link
Contributor Author

Yes, my bad. I thought that you meant via as a client, but did not fully understand your issue. Also, I believe that this fixes crostini, but am not completely sure as I do not have a chromebook.

@sreya
Copy link
Collaborator

sreya commented May 8, 2019 via email

@Zate
Copy link

Zate commented May 8, 2019

I'll do some testing with it but I don't think this is the right way to deal with crostini. Will let you know after I test it though.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DNS option to use: 0.0.0.0
4 participants