Skip to content
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

PasswordAuthentication yes in the upgrade script #21

Closed
wants to merge 1 commit into from

Conversation

anmol26s
Copy link

@anmol26s anmol26s commented Nov 9, 2017

If authentication through is disabled in ssh, sftp fails. Adding PasswordAuthentication yes for the webapp user makes it work only for the weapp.


Problem

  • Description of why you made this PR

Solution

  • And how you fix that

PR Status

Work finished. Package_check, basic tests and upgrade from last version OK.
Could be reviewed and tested.

Validation

Minor decision

  • Upgrade previous version :
  • Code review :
  • Approval (LGTM) :
  • Approval (LGTM) :
  • CI succeeded : Build Status
    When the PR is mark as ready to merge, you have to wait for 3 days before really merge it.

@anmol26s
Copy link
Author

anmol26s commented Apr 1, 2018

Any update on it? In my case I have to add this line every time the app is installed because I have disabled password login in the ssh.

@maniackcrudelis
Copy link
Contributor

Password login isn't disable by default on YunoHost. I don't think we should add this in the config.
I've also some issues with my own ssh config, I handle it myself because I've changed my config.

But, I don't use that app myself. Any other point of view ?

@anmol26s
Copy link
Author

anmol26s commented Apr 1, 2018

If there is no issue regarding this to anyone else, I am ok with it.

@zamentur
Copy link
Contributor

PasswordAuthentication is disable by default on several yunohost setup, currently it depends of your way to install yunohost. A PR, try to standardize the SSHD config on yunohost, but you can't consider passwordauthentication is disable or authorize.

So your proposition to specifically authorize this user seems to be the proper way

@zamentur
Copy link
Contributor

Note this PR conflict with YunoHost/yunohost#518

Precisely: here: https://github.com/YunoHost/yunohost/pull/518/files#diff-eae476b86ee57954ecbc550786e69a27R57

With this PR, it will be possible to use a sshd_config.d directory, like that change are manage in a proper way : https://github.com/YunoHost/yunohost/pull/518/files#diff-ca404b72f582b63a6e02136bd1726d6aR53

@alexAubin
Copy link
Member

With this PR, it will be possible to use a sshd_config.d directory, like that change are manage in a proper way : https://github.com/YunoHost/yunohost/pull/518/files#diff-ca404b72f582b63a6e02136bd1726d6aR53

Update about this : this doesn't work :s

So not sure what to do exactly about this ... But there are some movement on the whole ssh config (to be standardized by the migration included in 3.4). Haven't checked the whole SFTP thing though :/

Naively I would refrain from doing brutal seds to the sshd_config. If we really want to extend the ssh conf, then there might be some trick with the conf_regen. I did this kind of thing with mailman because I wanted to extend the postfix conf without breaking the regenconf.

I added this hook : https://github.com/YunoHost-Apps/mailman_ynh/blob/master/sources/hooks/conf_regen/98-postfix_mailman

Which is called postfix_something and therefore is to be ran each time a postfix regenconf is triggered. You should be able to achieve the same with ssh.

The alternative can also be to add an "AllowGroup sftpusers" or something like this .. This is related to Josue's work on group permissions.

@maniackcrudelis
Copy link
Contributor

Looks like we should have a sshd_config.d, https://github.com/YunoHost/yunohost/pull/518/files#diff-843b7c9e9160b9d943fc5cb50163d9c9R49 now.
This PR should be reworked to use it, at least. Still don't know about the whole idea if it's good or bad.

@alexAubin
Copy link
Member

Looks like we should have a sshd_config.d

N.B. : There is no ssh_config.d, and in fact no mechanism in sshd to create such a thing. I dunno how ljf came up with this thing but my understanding is that it simply does not exist...

For an alternative clean solution, c.f. my previous comment though it's a bit technical to create and manipulate those hooks, but at least it won't break the regenconf like manually editing the file does :s

@maniackcrudelis
Copy link
Contributor

Should I understand that the migration do create the .d directory, but sshd doesn't use it ?

@alexAubin
Copy link
Member

Hmmm nope I don't think the migration creates any .d ... There is just simply no possibility to include files in a sshd_config file ... This is apparently possible for client configuration but not server (sshd) configuration

@alexAubin
Copy link
Member

@maniackcrudelis
Copy link
Contributor

maniackcrudelis commented Mar 10, 2019

Ok. But this line says "Create sshd_config.d dir".

Anyway, I trust you, I was just saying that because of the comment and the mkdir after, mkdir(SSHD_CONF + '.d'

@alexAubin
Copy link
Member

Meh, yes indeed ... Didn't saw that line, should also have removed it in this commit : YunoHost/yunohost@25efab7

I'll do it naow

@maniackcrudelis
Copy link
Contributor

By the way, that's really too bad that sshd doesn't use a .d
That would be better that way...

@alexAubin
Copy link
Member

Not relevant anymore, was done in the app at some point, now managed by the core

@alexAubin alexAubin closed this May 10, 2021
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.

4 participants