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

Don't manage SSH ciphers on bullseye #151

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

mweinelt
Copy link
Contributor

@mweinelt mweinelt commented Aug 7, 2021

The OpenSSH daemon comes with reasonable defaults these days, so don't
try to manage that, by copying the same cipher list from release to
release.

Between Debian releases there is usually a large overlap over compatible
ciphers, so there shouldn't be any fear of breaking compatibility
between Proxmox VE releases either.


It might be reasonable to do the same on buster, but since I'm starting
a 7.0 cluster right now and I don't know the entire reasoning behind this
setting, I'm erring on the conservative side.

@lae
Copy link
Owner

lae commented Aug 10, 2021

pvecm add (when you go to manually stand up a cluster) iirc does this to disable weak ciphers, so this task emulates that behaviour.

Is it erring out somehow?

@mweinelt
Copy link
Contributor Author

No, it is not erroring out, but it is also just pinning the current (openssh 7.9) default cipher list and therefore provides no added value.

From the man 5 sshd_config

The default is:

[email protected],
aes128-ctr,aes192-ctr,aes256-ctr,
[email protected],[email protected]

Ideally one would manage acceptable ciphers, as well as MACs and KexAlgorithms in an sshd_config using Mozillas recommendations, but that is probably out of scope for this role.

https://infosec.mozilla.org/guidelines/openssh

@lae
Copy link
Owner

lae commented Aug 10, 2021

I guess there was another reason - from: https://git.proxmox.com/?p=pve-cluster.git;a=blob;f=data/PVE/Cluster/Setup.pm;h=1f064775af3cc4b2ac12a59d7d8abb7283f4b998;hb=HEAD#l162

# changed order to put AES before Chacha20 (most hardware has AESNI)

I'm not sure if making something that's hardcoded in PVE itself optional has any effect on support (this role is meant to mimic PVE tooling wherever it can't use them directly), but this seems like a relatively harmless thing to change in the first place. I'll wait to see if anyone else has any thoughts.

@mweinelt
Copy link
Contributor Author

Pretty sure that we should prefer chacha20-poly1305 these days, in spite of AES-NI support in hardware, I trust the OpenSSH upstream alot more to make good decisions.

I feel we as sysadmins are in a good position to make better choices that don't break any compatibility.

@lae lae changed the base branch from feature/pve7 to develop October 19, 2021 05:47
@lae
Copy link
Owner

lae commented Oct 19, 2021

Some further research:

https://lists.proxmox.com/pipermail/pve-devel/2017-August/028157.html Discussion behind the change to the ordering of ciphers
https://old.reddit.com/r/sysadmin/comments/c62owo/10_gbits_connection_but_only_1_gbits_via_scp/ Real world example of performance degradation with chacha20 on 10gbit vs AES

I vote that we keep the behaviour PVE imposes by default on both distributions. I'm fine with converting the pve_ssh_ciphers variable into a role variable so that administrators can override it, however. If you could, can you rebase this on the develop branch/force push a new commit that does this?

@mweinelt
Copy link
Contributor Author

Putting it on my todo list.

The OpenSSH daemon comes with reasonable defaults these days, so don't
try to manage that, by copying the same cipher list from release to
release.

Between Debian releases there is usually a large overlap over compatible
ciphers, so there shouldn't be any fear of breaking compatibility
between Proxmox VE releases either.
@lae lae force-pushed the pve7-no-managed-ciphers branch from 0610942 to 290e00c Compare July 10, 2024 01:58
@lae
Copy link
Owner

lae commented Jul 10, 2024

Rebased against develop to at least cleanup the unrelated/unmerged commits from the old pve7 branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants