use rootDir as chroot

[mcluseau]

Hi,

I may haven't understood the intent of "rootDir", but I understood it as a chroot-like parameter. I changed the packet handling in the server to take this idea in account.

If I'm wrong, please comment here so I can add a "chroot" argument somewhere and make another pull request introducing it.

Thanks!

[davecheney]

@MikaelCluseau thanks for your PR. Can you please raise an issue describing the problem you are having, what you tried, and what prevented you from achieving your goal.

It may be that this PR fixes the issue, in which case, that's very simple, but I prefer to discuss the issue first, then start coding.

Thanks

Dave

[marksheahan]

The original intention of that parameter (although it is not implemented) was just a default working directory to use if the incoming path was not an absolute path. Something like:
sftp to server.com as user 'mark', put local.file remote.file. I'd expect the file to on server.com to be in /home/mark/remote.file, not in /. rootDir is a vague name for this, defaultWorkingDirectory would be better.

I would prefer to see this renamed serverRoot or virtualRoot; the word 'chroot' to me suggests using the chroot syscall achieve such behavior, whereas this is using code wrappers around the paths.

chroot and default working directory are orthogonal concepts, so a distinct parameter would be nice, but I also don't want to add a parameter to NewServer() because that will break existing code.

What does everyone think?

[mcluseau]

Hi there. OK I understand.
I think rootDir is a good name for the feature, and since you seem to agree that it should be renamed, I'd like to suggest using rootDir for this intent and rename the current rootDir to something like you suggested (initialDir?). But I will following the naming you wish :)
About the NewServer, I do agree that the current interface must not change. I appears to me that it has a lot of parameters and this suggest that it would probably better to introduce a constructor with a ServerConfig object. The name could be NewServerWithConfig(config ServerConfig).

Envoyé depuis un mobile.

-------- Message d'origine --------
De : Mark Sheahan notifications@github.com
Date : 14/12/2015 03:26 (GMT+01:00)
À : pkg/sftp sftp@noreply.github.com
Cc : Mikaël Cluseau mcluseau@isi.nc
Objet : Re: [sftp] use rootDir as chroot (#56)

The original intention of that parameter (although it is not implemented) was just a default working directory to use if the incoming path was not an absolute path. Something like:

sftp to server.com as user 'mark', put local.file remote.file. I'd expect the file to on server.com to be in /home/mark/remote.file, not in /. rootDir is a vague name for this, defaultWorkingDirectory would be better.

I would prefer to see this renamed serverRoot or virtualRoot; the word 'chroot' to me suggests using the chroot syscall achieve such behavior, whereas this is using code wrappers around the paths.

chroot and default working directory are orthogonal concepts, so a distinct parameter would be nice, but I also don't want to add a parameter to NewServer() because that will break existing code.

What does everyone think?

—
Reply to this email directly or view it on GitHub.

[davecheney]

If you want to start adding configuration to the serve object the
functional option pattern may be a better solution.

On Mon, 14 Dec 2015, 15:45 Mikaël Cluseau notifications@github.com wrote:

Hi there. OK I understand.
I think rootDir is a good name for the feature, and since you seem to
agree that it should be renamed, I'd like to suggest using rootDir for this
intent and rename the current rootDir to something like you suggested
(initialDir?). But I will following the naming you wish :)
About the NewServer, I do agree that the current interface must not
change. I appears to me that it has a lot of parameters and this suggest
that it would probably better to introduce a constructor with a
ServerConfig object. The name could be NewServerWithConfig(config
ServerConfig).

Envoyé depuis un mobile.

-------- Message d'origine --------
De : Mark Sheahan notifications@github.com
Date : 14/12/2015 03:26 (GMT+01:00)
À : pkg/sftp sftp@noreply.github.com
Cc : Mikaël Cluseau mcluseau@isi.nc
Objet : Re: [sftp] use rootDir as chroot (#56)

The original intention of that parameter (although it is not implemented)
was just a default working directory to use if the incoming path was not an
absolute path. Something like:

sftp to server.com as user 'mark', put local.file remote.file. I'd expect
the file to on server.com to be in /home/mark/remote.file, not in /.
rootDir is a vague name for this, defaultWorkingDirectory would be better.

I would prefer to see this renamed serverRoot or virtualRoot; the word
'chroot' to me suggests using the chroot syscall achieve such behavior,
whereas this is using code wrappers around the paths.

chroot and default working directory are orthogonal concepts, so a
distinct parameter would be nice, but I also don't want to add a parameter
to NewServer() because that will break existing code.

What does everyone think?

—
Reply to this email directly or view it on GitHub.

—
Reply to this email directly or view it on GitHub
#56 (comment).

[davecheney]

And a gentle reminder, as it looks like this pr is going away, please open
a feature request to discuss this before digging into the implementation.

Thanks

Dave

On Mon, 14 Dec 2015, 15:47 Dave Cheney dave@cheney.net wrote:

If you want to start adding configuration to the serve object the
functional option pattern may be a better solution.

On Mon, 14 Dec 2015, 15:45 Mikaël Cluseau notifications@github.com
wrote:

Hi there. OK I understand.
I think rootDir is a good name for the feature, and since you seem to
agree that it should be renamed, I'd like to suggest using rootDir for this
intent and rename the current rootDir to something like you suggested
(initialDir?). But I will following the naming you wish :)
About the NewServer, I do agree that the current interface must not
change. I appears to me that it has a lot of parameters and this suggest
that it would probably better to introduce a constructor with a
ServerConfig object. The name could be NewServerWithConfig(config
ServerConfig).

Envoyé depuis un mobile.

-------- Message d'origine --------
De : Mark Sheahan notifications@github.com
Date : 14/12/2015 03:26 (GMT+01:00)
À : pkg/sftp sftp@noreply.github.com
Cc : Mikaël Cluseau mcluseau@isi.nc
Objet : Re: [sftp] use rootDir as chroot (#56)

The original intention of that parameter (although it is not implemented)
was just a default working directory to use if the incoming path was not an
absolute path. Something like:

sftp to server.com as user 'mark', put local.file remote.file. I'd
expect the file to on server.com to be in /home/mark/remote.file, not in
/. rootDir is a vague name for this, defaultWorkingDirectory would be
better.

I would prefer to see this renamed serverRoot or virtualRoot; the word
'chroot' to me suggests using the chroot syscall achieve such behavior,
whereas this is using code wrappers around the paths.

chroot and default working directory are orthogonal concepts, so a
distinct parameter would be nice, but I also don't want to add a parameter
to NewServer() because that will break existing code.

What does everyone think?

—
Reply to this email directly or view it on GitHub.

—
Reply to this email directly or view it on GitHub
#56 (comment).

[mcluseau]

Another idea for the NewServer call: we could put all the "invariants" in the config object and create a method applying to it, allowing to create a server by giving it the reader and the writer. Like:
func (c ServerConfig) NewServer(r io.Reader, w io.CloseWriter)

Envoyé depuis un mobile.

-------- Message d'origine --------
De : Mark Sheahan notifications@github.com
Date : 14/12/2015 03:26 (GMT+01:00)
À : pkg/sftp sftp@noreply.github.com
Cc : Mikaël Cluseau mcluseau@isi.nc
Objet : Re: [sftp] use rootDir as chroot (#56)

The original intention of that parameter (although it is not implemented) was just a default working directory to use if the incoming path was not an absolute path. Something like:

sftp to server.com as user 'mark', put local.file remote.file. I'd expect the file to on server.com to be in /home/mark/remote.file, not in /. rootDir is a vague name for this, defaultWorkingDirectory would be better.

I would prefer to see this renamed serverRoot or virtualRoot; the word 'chroot' to me suggests using the chroot syscall achieve such behavior, whereas this is using code wrappers around the paths.

chroot and default working directory are orthogonal concepts, so a distinct parameter would be nice, but I also don't want to add a parameter to NewServer() because that will break existing code.

What does everyone think?

—
Reply to this email directly or view it on GitHub.

[mcluseau]

Ok will do it after the night.

Envoyé depuis un mobile.

[davecheney]

@MikaelCluseau I'm going to close this PR for the moment. Please raise an issue describing the problem you are trying to solve.

[mcluseau]

Yes, sorry, been traveling a lot. My need evolved anyway so I'll try to get something done here. Something like http-handlers in fact, associating a "mount point" where to bind a handler. 2 handlers then: serve a local directory structure, or delegate to another sftp endpoint through a reader and a writer. I will need to implement something quickly anyways, so I'll have demo code to explain the need too. See you next year.