8000 make SFTP extensions configurable by drakkan · Pull Request #315 · pkg/sftp · GitHub
[go: up one dir, main page]

Skip to content

make SFTP extensions configurable #315

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Nov 11, 2019
Merged

make SFTP extensions configurable #315

merged 1 commit into from
Nov 11, 2019

Conversation

drakkan
Copy link
Collaborator
@drakkan drakkan commented Oct 26, 2019

This patch allow to configure the supported extensions

@drakkan drakkan force-pushed the extensions branch 2 times, most recently from c66a36f to c9868fd Compare October 26, 2019 19:10
@eikenb
Copy link
Member
eikenb commented Nov 2, 2019

@drakkan Thanks for the submission and I think it is a good idea. I'll be happy to merge it though there is one thing... Could you add a test or 2 covering the new functions?

@drakkan
Copy link
Collaborator Author
drakkan commented Nov 8, 2019

@eikenb, sorry for the delay. I added some basic test cases, please let me know if they are enough.

@eikenb
Copy link
Member
eikenb commented Nov 9, 2019

@drakkan You don't have to apologize to me for delays... I'm more guilty than anyone on that front around here.

@puellanivis Thanks for reviewing that. Very much appreciated.

8000

@eikenb
Copy link
Member
eikenb commented Nov 10, 2019

@drakkan Thanks for the tests and I just had one more thought (sorry)...

I was looking it over again and wondered if it might not be better if we kept SSHExtensionPair private and made SetSFTPExtensions take a list of the names they wanted. Changing the signature to...

func SetSFTPExtensions(extensions ...string) error {
...

And calling it...

SetSFTPExtensions("hardlink@openssh.com", "posix-rename@openssh.com")

Seems like this would both keep the API as is and seems like it be easier to call.

What do you think?

@eikenb eikenb added this to the v1.11.0 milestone Nov 10, 2019
@puellanivis
Copy link
Collaborator

I considered that as well, I was not sure how important the “Data” field of the extension pair was. If it’s just ”always the value 1” then it makes sense.

@drakkan
Copy link
Collaborator Author
drakkan commented Nov 11, 2019

@eikenb @puellanivis please let me know if there are any remaining fixes/improvements, thanks!

@eikenb
Copy link
Member
eikenb commented Nov 11, 2019

I considered that as well, I was not sure how important the “Data” field of the extension pair was. If it’s just ”always the value 1” then it makes sense.

It may not always be "1", but it will always be fixed... at least for Openssh extensions. The spec is very vague about what it is used for, only specifying that both fields (name, data) must be present though the data one may be blank. Openssh uses the data field as an incremental version number, and the later versions supersede the former.

So it might be something other than "1", but it will still be a fixed setting. In other words, I don't have any plans on supporting multiple versions of a specific extension protocol.

@eikenb eikenb merged commit 8488d36 into pkg:master Nov 11, 2019
drakkan added a commit to drakkan/sftpgo that referenced this pull request Nov 12, 2019
update pkg/sftp to a git revision that includes the needed patch

pkg/sftp#315
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants
0