remove hmac-sha1 from sshd_config

[heartsucker]

Status

Ready for review

Description of Changes

Removes a comparatively weak MAC algorithm from the sshd_config.

Deployment

1. Upgrading existing production instances: a sed command could be added to postinst to fix existing instances
2. New installs: no change

Checklist

If you made changes to the system configuration:

• Configuration tests pass

[heartsucker]

I know I said I was in hiding, but this came up during a system-wide grep, and it was such an easy fix. Now please excuse me as I disappear again.

[ghost]

👻

[ghost]

Although wikipedia says it is HMACs are substantially less affected by collisions than their underlying hashing algorithms alone, it looks like a good idea to deprecate hmac-sah1.

Could you please add a reference or explanation about why this is not going to have a negative impact on the ability of existing ssh clients to connect ? We could trivially test that but what I don't know for sure is why and when hmac-sha1 is still in use.

[conorsch]

Could you please add a reference or explanation about why this is not going to have a negative impact on the ability of existing ssh clients to connect ?

Agreed with @dachary: some information about why this change is made would be lovely. We'll need to check that we don't break Vagrant logins, because some SSH client implementations don't support modern cipher suites. By now at the end of 2017 I suspect we're covered there, but local testing is required.

For further reading on recommended MACs in SSHD hardening, see the Message Authentication Codes section here: https://stribika.github.io/2015/01/04/secure-secure-shell.html

Additionally I'll point out that as described in #2360, we'd do better to position ourselves downstream from community-maintained hardening roles. The changes presented here are already included in e.g. https://github.com/dev-sec/ansible-ssh-hardening/blob/9bb68ef07e265bb3233b1cbf0eef92231e2e6235/templates/opensshd.conf.j2#L89-L107

[conorsch]

Using Vagrant v1.9.1 here, and no problems with staging VMs.

[heartsucker]

Could you please add a reference or explanation about why this is not going to have a negative impact on the ability of existing ssh clients to connect ? We could trivially test that but what I don't know for sure is why and when hmac-sha1 is still in use.

If someone is connecting via Tails (which they should be), then they don't need HMAC-SHA1 as they have other MAC options. If someone needs it during development, chances are they are on such an old OS that nothing else will work anyway, and it's not worth supporting.

And while it's true HMAC-SHA1 hasn't been broken yet, this is a preemptive move to stay ahead of the curve.

[redshiftzero]

I'm fine with this change, but we should have a migration plan for current instances along with this PR (in the postinst script sounds fine), no?

[heartsucker]

@redshiftzero I added the postinst script along with a note that the fix doesn't need to be there permanently.

[ghost]

If a new MAC is introduced, hmac-sha3-1024 is added in the future it will be silently removed and this may break is subtle ways.

[heartsucker]

I think this is overly pedantic. It will only alter the line if hmac-sha1 is present, and it is unlikely that we add a new MAC algorithm to Ansible before this changes makes it out. Also, anything breaks because we stop supporting some new bleeding edge MAC algorithm, then we have fucked up pretty bad as hmac-sha2-512 at the very least should be around for quite a while longer.

[ghost]

I don't think this is pedantic. And it is an easy fix.

[heartsucker]

This PR solves the problem for future instances and current instances. It's not the most perfect sed command ever, but this project has a problem with merging PRs. This one is tiny and correct, so I'm mostly just raging about how incredibly slowly everything moves.

[ghost]

This specific pull request would have been merged 5 days ago if you had made the change. I know it's sometime frustrating to comply with a reviewer request. But calling the request pedantic is confrontational and does not send the right message. I usually find it easier and faster to comply.

It would be toxic to keep discussing this in a confrontational manner. We know each other well enough to understand there is no hard feelings :-) But it will definitely look different from the point of view of a reader browsing this later on.

It would also set a bad example to just merge, as if dismissing the reviewer request with a semi-aggressive tone was an example to follow.

Instead I'll merge this pull request and I will submit another one with what you think is the pedantic change. You won't have to do the work, I will. The message I'd like to send is that if you disagree with the reviewer on minor non blocking issues, you end up adding to the reviewer TODO list.

As a conclusion, I think it is healthy to have disputes in a project. We are humans and this inevitably happens. I find it very suspicious when a project has no trace of disputes: it means they are hidden. And I find it very distressing when disputes become the dominant way of communicating. I hope this tiny dispute is eventually resolved in a way that we both find acceptable.

[redshiftzero]

I understand that the speed of merging can be slow and this is frustrating. Please remember that do we have to be careful merging as we are a small team assisting a large (and growing) number of SecureDrop instances: missteps that result in bugs, security issues, or just user/admin confusion can introduce a large support burden (see: recent security vulnerability), which would result in an even slower pace of progress as developers/maintainers are pulled away to assist particular instances. We are doing our very best with the resources and time that we have to get PRs merged in.