-
Notifications
You must be signed in to change notification settings - Fork 200
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @jbronn. This looks good! :)
I have some changes I'd like to get adressed, though. Can you take a look at these?
README.md
Outdated
@@ -36,7 +36,10 @@ Warning: This role disables root-login on the target server! Please make sure yo | |||
|`ssh_allow_tcp_forwarding` | false | false to disable TCP Forwarding. Set to true to allow TCP Forwarding.| | |||
|`ssh_gateway_ports` | `false` | `false` to disable binding forwarded ports to non-loopback addresses. Set to `true` to force binding on wildcard address. Set to `clientspecified` to allow the client to specify which address to bind to.| | |||
|`ssh_allow_agent_forwarding` | false | false to disable Agent Forwarding. Set to true to allow Agent Forwarding.| | |||
|`ssh_pam` | true | true if SSH has PAM support.| |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you call these ssh_pam_support
? Same for kerberos and gssapi.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 9bee179.
README.md
Outdated
@@ -52,6 +55,7 @@ Warning: This role disables root-login on the target server! Please make sure yo | |||
|`sftp_chroot` | true | false to disable chroot for sftp| | |||
|`sftp_chroot_dir` | /home/%u | change default sftp chroot location| | |||
|`ssh_client_roaming` | false | enable experimental client roaming| | |||
|`sshd_moduli` | '/etc/ssh/moduli' | path to the SSH moduli file | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use sshd_moduli_dir
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't it be sshd_moduli_file
, as it's not a directory?
Edit: I changed it to sshd_moduli_file
in 179287b; let me know if I need to change it.
vars/Debian.yml
Outdated
@@ -1,3 +1 @@ | |||
sshd_service_name: ssh | |||
ssh_owner: root |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not really happy with this change.
Before this, there was only one place for every OS, where the owner and group were defined - the vars-file.
Now for some OS the owner is in the defaults and group is in vars.
I'd rather have these on the same place and since this cannot be the defaults, it should be vars.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 7f13313.
Thanks! :) |
This PR adds support for OpenBSD. Along the way, new variables were added to support the platform. In particular:
ssh_pam_support
,ssh_gssapi_support
, andssh_kerberos_support
: The configuration file settings (e.g.,GSSAPIAuthentication
) are invalid on the platform because the original OpenSSH does not include PAM, GSSAPI, or Kerberos support.sshd_moduli_file
: OpenBSD uses/etc/moduli
instead of/etc/ssh/moduli
Like FreeBSD (#95), there are no Docker-based tests for this kernel but I'm open to ideas.