-
Notifications
You must be signed in to change notification settings - Fork 92
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
Allowed MACs should allow for greater security #24
Conversation
Gemfile.lock
Outdated
@@ -0,0 +1,192 @@ | |||
GEM |
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.
We should not include Gemfile.lock
if sshd_config.MACs | ||
describe sshd_config.MACs.split(',').each do | ||
it { should match(/^((hmac-sha2-512-etm@openssh\.com|hmac-sha2-256-etm@openssh\.com|umac-128-etm@openssh\.com|hmac-sha2-512|hmac-sha2-256|umac-128@openssh\.com|curve25519-sha256@libssh\.org|diffie-hellman-group-exchange-sha256),)*(hmac-sha2-512-etm@openssh\.com|hmac-sha2-256-etm@openssh\.com|umac-128-etm@openssh\.com|hmac-sha2-512|hmac-sha2-256|umac-128@openssh\.com)$/) } | ||
sshd_config.MACs.split(',').each do |m| |
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.
It feels like we re-invent the wheel. We should consider depending on https://github.com/dev-sec/ssh-baseline and the defined resources to define the best macs and ciphers for the operating system.
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.
That makes sense, sorry I don't know your overall goals or layout for all of dev-sec just wanting to use the common benchmark for my purposes.
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.
@millerthomasj Oh, that was not a complaint :-) I just wanted to mention it, since we should consider harmonizing this in the future
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'll merge this for now, and I agree that we should start reusing code in the other profiles. Will have a look at the best way to approach that.
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.
Thank you for this improvement @millerthomasj
if sshd_config.MACs | ||
describe sshd_config.MACs.split(',').each do | ||
it { should match(/^((hmac-sha2-512-etm@openssh\.com|hmac-sha2-256-etm@openssh\.com|umac-128-etm@openssh\.com|hmac-sha2-512|hmac-sha2-256|umac-128@openssh\.com|curve25519-sha256@libssh\.org|diffie-hellman-group-exchange-sha256),)*(hmac-sha2-512-etm@openssh\.com|hmac-sha2-256-etm@openssh\.com|umac-128-etm@openssh\.com|hmac-sha2-512|hmac-sha2-256|umac-128@openssh\.com)$/) } | ||
sshd_config.MACs.split(',').each do |m| |
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'll merge this for now, and I agree that we should start reusing code in the other profiles. Will have a look at the best way to approach that.
The current match scheme does not allow for greater security, this check will ensure that each MAC is in the allowed list instead.