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

Provide granular noop for ssh configuration #789

Merged
merged 2 commits into from
Oct 13, 2024

Conversation

seven-beep
Copy link
Contributor

Hello,

We would like to have more fine grained options on applying or not specific configurations.

This commit let the user choose to noop some configuration by setting their value to false.

Another option would have been to create more variables but I was reticent to do so.

Motivation for theses options are we may configure ourselves some (ssh host key regeneration in a templating system) or we are not ready for others (ssh_kex will break dist-upgrades, letting the operator without ssh).

@seven-beep seven-beep force-pushed the master branch 2 times, most recently from a5d7464 to c55f95b Compare September 6, 2024 11:05
@seven-beep
Copy link
Contributor Author

The pgp signing should be ok now.

@seven-beep seven-beep force-pushed the master branch 2 times, most recently from 776189a to ea9cdd7 Compare September 6, 2024 16:06
@seven-beep
Copy link
Contributor Author

Well I guess it is more valid to use new variables to implement this behavior.

We would like to have more fine grained options on applying or not specific configurations.

This commit let the user choose to noop some configuration with a few new
boolean variables.

Motivation for theses options are we may configure ourselves some (ssh host key
regeneration in a templating system) or we are not ready for others (ssh_kex
will break dist-upgrades, letting the operator without ssh).

Signed-off-by: seven beep <ebn@entreparentheses.xyz>
@seven-beep
Copy link
Contributor Author

I corrected a few mispellings that was caught by codespell.

@@ -73,14 +73,14 @@ LogLevel {{ sshd_log_level }}
# -- see: (http://net-ssh.github.com/net-ssh/classes/Net/SSH/Transport/CipherFactory.html)
#
{# This outputs 'Ciphers <list-of-ciphers>' if ssh_ciphers is defined or '#Ciphers' if ssh_ciphers is undefined -#}
{{ 'Ciphers ' ~ ssh_ciphers|join(',') if ssh_ciphers else 'Ciphers'|comment }}
{{ 'Ciphers ' ~ ssh_ciphers|join(',') if ssh_ciphers and ssh_ciphers_config else 'Ciphers'|comment }}
Copy link
Member

Choose a reason for hiding this comment

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

This and the changes for MACs and Kex shouldn't be needed since ssh_ciphers can only be set in two ways:

  • define it as a variable, then ansible.builtin.include_tasks: crypto_ciphers.yml won't be run.
  • by the tasks in ansible.builtin.include_tasks: crypto_ciphers.yml

Or is there something I am missing where we want to define Ciphers but not actually configure them?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually on main we have :

  • If I don't define them as a variable, the tasks crypto_ciphers.yml will define them for me and the template will use the defaults and hardcode them in the configuration file.
  • If I define them as a non true variable, the same apply as I wasn't defining them.
  • If I define them as a variable, the template will use this variable and hardcode them in the configuration file.

So what about if I doesn't want to define Ciphers and does not want to configure them ?

@@ -34,7 +34,7 @@ ListenAddress {{ address }}
{% endfor %}

# HostKeys are listed here.
{% for key in ssh_host_key_files %}
{% for key in ssh_host_key_files if ssh_host_key_config%}
Copy link
Member

Choose a reason for hiding this comment

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

Can we do something like {{ 'Ciphers ' ~ ssh_ciphers|join(',') if ssh_ciphers and ssh_ciphers_config else 'Ciphers'|comment }} here, too?

@seven-beep seven-beep requested a review from rndmh3ro October 2, 2024 21:22
@schurzi
Copy link
Contributor

schurzi commented Oct 4, 2024

@seven-beep I understand what you are trying to implement here and I see value in it, especially for system updates. What I don't want is introduce even more variables as switches for these values.

@rndmh3ro what do you think about removing the cipher variables (ssh_macs,ssh_ciphers,ssh_kex) from defaults/main.yml and change the check in our role from when: not ssh_macs to when: ssh_macs is undefined. That way a user can define the variable as false and the template will create a comment instead of a configuration.

@rndmh3ro
Copy link
Member

rndmh3ro commented Oct 7, 2024

@rndmh3ro what do you think about removing the cipher variables (ssh_macs,ssh_ciphers,ssh_kex) from defaults/main.yml and change the check in our role from when: not ssh_macs to when: ssh_macs is undefined. That way a user can define the variable as false and the template will create a comment instead of a configuration.

Good idea! @seven-beep what do you think?

@schurzi schurzi changed the title Provide granular noop for shh configuration Provide granular noop for ssh configuration Oct 7, 2024
We would like to have more fine grained options on applying or not specific configurations.

This commit let the user choose to disable configurations for `ssh_host_key_config`,
`ssh_ciphers_config`, `ssh_host_key_config`, `ssh_macs_config` by setting them
to False.

Motivation for theses options are we may configure ourselves some (ssh host key
regeneration in a templating system) or we are not ready for others (ssh_kex
will break dist-upgrades, letting the operator without ssh).

Signed-off-by: seven beep <ebn@entreparentheses.xyz>
@seven-beep
Copy link
Contributor Author

Hello @schurzi, @rndmh3ro,

I agree that less variables is desirable, and it was my original intent to not add any new variables to get this feature. However I think I got caught by the meta/argument-specs.yml of the role because we might lose proper typing for the variables by allowing them to be a list or a boolean.

I implemented your recommendation in the last iteration, if this fails because of the meta/argument-specs.yml, I might need advice on this particular point.

@rndmh3ro
Copy link
Member

LGTM! Will test it some more.

@rndmh3ro rndmh3ro merged commit a97a54d into dev-sec:master Oct 13, 2024
34 of 37 checks passed
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