Add node attributes to override KEX, MAC and cipher values #141

Merged
merged 1 commit into from Dec 14, 2016

Conversation

Projects
None yet
4 participants
@bazbremner
Contributor

bazbremner commented Dec 13, 2016

Comments and feedback on this approach, as well as around the default Kex, MAC and cipher choices are welcome - I note that there's a refactor of Kex and Cipher selections going on, plus there may be further discussions to be had on the default lists, so this is a quick hack to allow a complete override of several values.

Original commit message follows:

There's advice available on preferred choices of key exchange, message
authentication and ciphers from a number of sources [1][2][3], all of
which don't entirely agree with each other, and then there's the
hardcoded selection of Kex, MAC and ciphers encoded in this cookbook.

At the time of committing, there is a refactor going on to simplify kex
and cipher handling:
#134

Even in that refactor, hmac-ripemd160 MACs, which have been removed in
OpenSSH 6.7 (and hence flagged by ssh-audit[1] and are absent from
Mozilla's recommendations[2] for modern sshd, yet are still recommended
by secure secure shell[3]) are included in the default MAC list.

Likewise hmac-sha2-256 and hmac-sha2-512 are flagged by ssh-audit[1] as
they are encrypt-and-MAC, which has a number of issues, discussed in
secure secure shell[3].

There is likely to be more complexity and balancing of features/security
to consider plus the future changes of refactors in this cookbook, so
initially, I'd just like a way of overriding the generated defaults.

[1] https://github.com/arthepsy/ssh-audit
[2] https://wiki.mozilla.org/Security/Guidelines/OpenSSH
[3] https://stribika.github.io/2015/01/04/secure-secure-shell.html

@bazbremner bazbremner changed the title from Add node attributes to override KEX, MAC and cipher values to WIP: Add node attributes to override KEX, MAC and cipher values Dec 13, 2016

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Dec 13, 2016

Coverage Status

Coverage remained the same at 100.0% when pulling 9948c24 on bazbremner:override_kex_mac_and_ciphers into 10953dc on dev-sec:master.

Coverage Status

Coverage remained the same at 100.0% when pulling 9948c24 on bazbremner:override_kex_mac_and_ciphers into 10953dc on dev-sec:master.

@chris-rock

This comment has been minimized.

Show comment
Hide comment
@chris-rock

chris-rock Dec 13, 2016

Member

@bazbremner Awesome! Thank you for bringing up the discussion. We are very close to merge #134 We should build this PR into two iterations: enable customization as you showed in this PR and updating the ciphers for newer versions. For that, lets also double-check https://bettercrypto.org, since we follow their recommendation

Member

chris-rock commented Dec 13, 2016

@bazbremner Awesome! Thank you for bringing up the discussion. We are very close to merge #134 We should build this PR into two iterations: enable customization as you showed in this PR and updating the ciphers for newer versions. For that, lets also double-check https://bettercrypto.org, since we follow their recommendation

@bazbremner

This comment has been minimized.

Show comment
Hide comment
@bazbremner

bazbremner Dec 13, 2016

Contributor

@chris-rock thanks for the comment. Yes, I agree that updating the generated ciphers should be a separate PR, but I thought the additional complexity and discussion that entails is useful background and justification for allowing a blanket override, as provided by my changes.

Assuming the basic premise is OK, what other changes would you like me to make to bring this PR into a mergable state?

Contributor

bazbremner commented Dec 13, 2016

@chris-rock thanks for the comment. Yes, I agree that updating the generated ciphers should be a separate PR, but I thought the additional complexity and discussion that entails is useful background and justification for allowing a blanket override, as provided by my changes.

Assuming the basic premise is OK, what other changes would you like me to make to bring this PR into a mergable state?

@artem-sidorenko

This comment has been minimized.

Show comment
Hide comment
@artem-sidorenko

artem-sidorenko Dec 14, 2016

Member

@bazbremner I like this approach

Regarding cipher update, I see it like @chris-rock: it should be done in a separate PR

Regarding this PR, the #134 is merged, can you please rebase?

Member

artem-sidorenko commented Dec 14, 2016

@bazbremner I like this approach

Regarding cipher update, I see it like @chris-rock: it should be done in a separate PR

Regarding this PR, the #134 is merged, can you please rebase?

Add node attributes to override KEX, MAC and cipher values
There's advice available on preferred choices of key exchange, message
authentication and ciphers from a number of sources [1][2][3], all of
which don't _entirely_ agree with each other, and then there's the
hardcoded selection of Kex, MAC and ciphers encoded in this cookbook.

After initial discussions around this change with @chris-rock and
@artem-sideorenko, there may be follow-on changes to the hardcoded
selections this cookbook generates, however that's a topic for future
discussion and PRs.

There is likely to be more complexity and balancing of features/security
to consider plus the future changes of refactors in this cookbook, so
initially, I'd just like a way of overriding the generated defaults.

Note that `node['ssh'][{'client', 'server'}][{'weak_hmac', 'weak_kex',
'cbc_required'}}` are all ignored if these overrides are used, as the
user is supplying their preferred choices, rather than relying on the
cookbook's generated strings.

[1] https://github.com/arthepsy/ssh-audit
[2] https://wiki.mozilla.org/Security/Guidelines/OpenSSH
[3] https://stribika.github.io/2015/01/04/secure-secure-shell.html
@bazbremner

This comment has been minimized.

Show comment
Hide comment
@bazbremner

bazbremner Dec 14, 2016

Contributor

@chris-rock @artem-sidorenko rebased following the merge of #134. I've also reworded the commit message following our discussions. 👍

Contributor

bazbremner commented Dec 14, 2016

@chris-rock @artem-sidorenko rebased following the merge of #134. I've also reworded the commit message following our discussions. 👍

@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Dec 14, 2016

Coverage Status

Coverage remained the same at 100.0% when pulling 35c2be2 on bazbremner:override_kex_mac_and_ciphers into 0fa0082 on dev-sec:master.

Coverage Status

Coverage remained the same at 100.0% when pulling 35c2be2 on bazbremner:override_kex_mac_and_ciphers into 0fa0082 on dev-sec:master.

@artem-sidorenko

This comment has been minimized.

Show comment
Hide comment
@artem-sidorenko

artem-sidorenko Dec 14, 2016

Member

@bazbremner please also remove the WIP from PR title (as gets pulled by changelog generator)

LGTM,
@chris-rock ?

Member

artem-sidorenko commented Dec 14, 2016

@bazbremner please also remove the WIP from PR title (as gets pulled by changelog generator)

LGTM,
@chris-rock ?

@bazbremner bazbremner changed the title from WIP: Add node attributes to override KEX, MAC and cipher values to Add node attributes to override KEX, MAC and cipher values Dec 14, 2016

@chris-rock

This looks great to me. Thank you @bazbremner @artem-sidorenko

@artem-sidorenko artem-sidorenko merged commit d1f1bf1 into dev-sec:master Dec 14, 2016

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 100.0%
Details
@artem-sidorenko

This comment has been minimized.

Show comment
Hide comment
Member

artem-sidorenko commented Dec 14, 2016

@bazbremner thank you!

@bazbremner

This comment has been minimized.

Show comment
Hide comment
@bazbremner

bazbremner Dec 14, 2016

Contributor

No problems, thanks for the speedy turnaround and feedback.

Contributor

bazbremner commented Dec 14, 2016

No problems, thanks for the speedy turnaround and feedback.

@bazbremner bazbremner deleted the bazbremner:override_kex_mac_and_ciphers branch Dec 14, 2016

artem-sidorenko added a commit to artem-forks/chef-ssh-hardening that referenced this pull request Dec 22, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment