Refactoring of library to simplify the kex/cipher handling #134

artem-sidorenko · 2016-11-09T16:42:37Z

TODOs

move entire library code to the module DevSec::SshHardening, extend Chef::Recipe only with functions used in the recipe
Create some spec tests for DevSec::SshHardening module
Simplify node object handling (avoid node object attribute)

artem-sidorenko · 2016-11-09T16:47:14Z

@chris-rock @atomic111 what do you think about the direction?

I guess we should place this to the 2.0.0 because of the chef version. What do you think?

coveralls · 2016-11-09T16:52:27Z

Coverage decreased (-5.08%) to 94.922% when pulling 21a7eaf on artem-sidorenko:refactor-libraries into 803e394 on dev-sec:master.

coveralls · 2016-11-09T19:25:00Z

Coverage remained the same at 100.0% when pulling c8f9aba on artem-sidorenko:refactor-libraries into 803e394 on dev-sec:master.

coveralls · 2016-11-09T23:43:18Z

Coverage decreased (-8.3%) to 91.739% when pulling 0cff3c9 on artem-sidorenko:refactor-libraries into 803e394 on dev-sec:master.

coveralls · 2016-11-10T17:58:13Z

Coverage decreased (-1.09%) to 98.913% when pulling c047c60 on artem-sidorenko:refactor-libraries into 803e394 on dev-sec:master.

coveralls · 2016-11-10T18:40:44Z

Coverage decreased (-0.4%) to 99.566% when pulling 18579f6 on artem-sidorenko:refactor-libraries into 803e394 on dev-sec:master.

coveralls · 2016-11-10T18:45:22Z

Coverage decreased (-0.4%) to 99.566% when pulling 18579f6 on artem-sidorenko:refactor-libraries into 803e394 on dev-sec:master.

coveralls · 2016-11-10T18:56:46Z

Coverage remained the same at 100.0% when pulling afb7f33 on artem-sidorenko:refactor-libraries into 803e394 on dev-sec:master.

artem-sidorenko · 2016-11-10T18:59:37Z

@chris-rock @arlimus @atomic111 its ready. Can you please review? Please do not merge, we need the 1.3.0 first (#135).

Followups of this PR will be addressed by issues #136 and #137

artem-sidorenko · 2016-11-17T11:08:53Z

@chris-rock @arlimus @atomic111 any remarks? I would really appreciate the review of this one

coveralls · 2016-11-23T19:16:55Z

Coverage remained the same at 100.0% when pulling 994436e on artem-sidorenko:refactor-libraries into 10953dc on dev-sec:master.

coveralls · 2016-11-23T19:22:08Z

Coverage remained the same at 100.0% when pulling 994436e on artem-sidorenko:refactor-libraries into 10953dc on dev-sec:master.

artem-sidorenko · 2016-11-30T07:35:51Z

@chris-rock @arlimus @atomic111 anybody? :-(

chris-rock · 2016-11-30T09:59:52Z

@artem-sidorenko Great work. Let me have a look at this later today.

chris-rock

@artem-sidorenko Thank you for the improvement. I apologize for the late review. I like your clean-up and and the improved version detection. I am not sure about the meta programing. Could you explain why we need it?

chris-rock · 2016-11-11T19:03:08Z

libraries/devsec_ssh.rb

+
+        type, call = parse_method(method)
+
+        Chef::Log.debug("Called #{method}")


Do we need the whitespace between the lines?

chris-rock · 2016-12-04T22:24:47Z

libraries/devsec_ssh.rb

@@ -0,0 +1,220 @@
+# encoding: utf-8


any reason why you prefix the file with devsec

Module/class name is DevSec::SSH, as there is no similar path in place, I called the file devsec_ssh to allow a similar matching between file name and class/module name

chris-rock · 2016-12-04T22:25:42Z

libraries/devsec_ssh.rb

+
+        ssh_version = send("get_ssh_#{type}_version", node)
+
+        Chef::Log.debug("Detected ssh version #{ssh_version}")


I like the additional logging

chris-rock · 2016-12-04T22:30:33Z

libraries/devsec_ssh.rb

+      private
+
+      def method_missing(method, node, enable_weak = false)
+        super unless respond_to_missing?(method)


Instead of using method missing, I would prefer to use get_client_macs directly and use the abstracting then. I suggest, we use method_missing only if required.

Do you mean something like this?

def get_data(type, args) end def get_client_macs(args) get_data('client_macs', args) end def get_server_macs(args) get_data('server_macs', args) end

My basic idea was to avoid the same code 5 times...

@artem-sidorenko Yes, I think that would make it easier to read and understand the code. We would also be more explicit. I also believe, its easier for Ruby beginners to add contributions if we try to avoid Ruby meta programming

A good read from my perspective: https://www.codeschool.com/blog/2015/04/24/7-deadly-sins-of-ruby-metaprogramming/

@chris-rock I agree with you on the disadvantages (esp. the point about ruby beginners and contributions), I also thought about define_method like suggested by you in some other comment, in this case the usage of method_missing was the most elegant way from my perspective.

I'll have a look how to rewrite it, maybe a combination of abstraction and simple loop with define_method will be a good tradeoff and understandable for Ruby beginners too

I really like your abstractions and we should keep those, even it it means we define 3 method and they all call the same internal method. Personally I am not sure if we should even do define_method. Maybe we try to be explicit and grow from there?

@chris-rock I was thinking about something like

[:macs, :ciphers, :kexs].each do |crypto_type| define_method("get_client_#{crypto_type}") do |node, enable_weak = false| get_crypto_data(crypto_type, :client, node, enable_weak) end define_method("get_server_#{crypto_type}") do |node, enable_weak = false| get_crypto_data(crypto_type, :server, node, enable_weak) end end

What do you think?

What do you mean with grow from there?

'grow' - to see if we need further abstractions. I am not 100% sure if we should use define_method. I would like to get a perspective from @arlimus or @atomic111

@chris-rock I pushed the ef4e38a without method_missing and define_method

chris-rock · 2016-12-04T22:32:00Z

libraries/devsec_ssh.rb

+      class << self
+        KLASS ||= ::DevSec::Ssh
+
+        def method_missing(method, *args)


I would try to avoid method_missing here. I am not sure why we need that.

We can also get rid of this proxy connector and integrate the entire thing as Chef::Recipe::DevSec::SSH?

This particular method_missing is a transparent proxy, if you want to extend / change ::DevSec::Ssh - you do not have to change something here

I removed this proxy completely and we use DevSec::Ssh now

chris-rock · 2016-12-04T22:33:23Z

libraries/devsec_ssh.rb

+      end
+
+      def get_ssh_version(node, package)
+        version = node['packages'][package]['version']


👍 I like the version detection based on the real package

chris-rock · 2016-12-04T22:34:35Z

recipes/client.rb

-    mac: SshMac.get_macs(node, node['ssh']['client']['weak_hmac']),
-    kex: SshKex.get_kexs(node, node['ssh']['client']['weak_kex']),
-    cipher: SshCipher.get_ciphers(node, node['ssh']['client']['cbc_required']),
+    mac: DevSec.get_client_macs(node, node['ssh']['client']['weak_hmac']),


Should we use DevSec::SSH as namespace? In order to avoid name conflicts with other cookbooks?

artem-sidorenko · 2016-12-05T07:04:19Z

@chris-rock

I am not sure about the meta programing. Could you explain why we need it?

My first iteration was without it, I ended up with almost a same code for all functions get_[client|server]_[kexs|macs|ciphers]. So I decided to go that way in order to avoid the duplication 5 times

coveralls · 2016-12-05T07:24:33Z

Coverage remained the same at 100.0% when pulling 271dbab on artem-sidorenko:refactor-libraries into 10953dc on dev-sec:master.

coveralls · 2016-12-05T14:05:31Z

Coverage remained the same at 100.0% when pulling 3751bf2 on artem-sidorenko:refactor-libraries into 10953dc on dev-sec:master.

coveralls · 2016-12-05T14:20:08Z

Coverage remained the same at 100.0% when pulling ef4e38a on artem-sidorenko:refactor-libraries into 10953dc on dev-sec:master.

artem-sidorenko · 2016-12-05T16:47:39Z

The node object should not be passed anymore as parameter from recipe.

Please do not merge this PR after you think the review is done, I'll squash the fixup commits prior to the merge to master

coveralls · 2016-12-05T17:27:32Z

Coverage decreased (-0.2%) to 99.771% when pulling 8817a44 on artem-sidorenko:refactor-libraries into 10953dc on dev-sec:master.

coveralls · 2016-12-05T18:11:41Z

Coverage remained the same at 100.0% when pulling d50ba56 on artem-sidorenko:refactor-libraries into 10953dc on dev-sec:master.

artem-sidorenko · 2016-12-09T15:18:46Z

@chris-rock I already removed the metaprogramming implementation, so there is no open discussion point anymore.

Please let us proceed here, this PR is already open for 1 month and it blocks me on further progress in this cookbook.

Do you see any other points? Otherwise please tell me "LGTM" and I will squash&merge it and start to work on other tasks.

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: dev-sec#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

atomic111 · 2016-12-13T22:08:41Z

@artem-sidorenko great job!!! i will discuss this with @chris-rock. it looks pretty complete

chris-rock

This looks good to me @artem-sidorenko Once you remove the WIP from the title we should merge it

chris-rock · 2016-12-13T22:57:35Z

libraries/devsec_ssh.rb

+      end
+
+      def get_client_macs(enable_weak = false)
+        get_crypto_data(:macs, :client, enable_weak)


👍 cool abstraction. much easier to read!

Module `DevSec::Ssh` delivers the crypto parameters. There is autodetection of ssh version with fallback to 5.9

coveralls · 2016-12-14T07:10:33Z

Coverage remained the same at 100.0% when pulling 33caca8 on artem-sidorenko:refactor-libraries into 10953dc on dev-sec:master.

artem-sidorenko · 2016-12-14T07:14:20Z

@chris-rock @atomic111 thank you! I bumped the master to 2.0.0

artem-sidorenko force-pushed the refactor-libraries branch from 0cff3c9 to c047c60 Compare November 10, 2016 17:43

artem-sidorenko force-pushed the refactor-libraries branch 2 times, most recently from b4bef9a to 18579f6 Compare November 10, 2016 18:30

artem-sidorenko force-pushed the refactor-libraries branch from 18579f6 to afb7f33 Compare November 10, 2016 18:45

This was referenced Nov 10, 2016

Move UsePrivilegeSeparation.get to the new library #136

Closed

Move entire crypto parameter configuration in tests to the centralized place #137

Closed

artem-sidorenko force-pushed the refactor-libraries branch from afb7f33 to 5493a4c Compare November 23, 2016 19:04

artem-sidorenko changed the title ~~WIP: Refactor the library to simplify the kex/cipher handling~~ Refactor the library to simplify the kex/cipher handling Nov 23, 2016

artem-sidorenko added this to the v2.0.0 milestone Nov 23, 2016

chris-rock reviewed Dec 4, 2016

View reviewed changes

artem-sidorenko changed the title ~~Refactor the library to simplify the kex/cipher handling~~ WIP: Refactor the library to simplify the kex/cipher handling Dec 5, 2016

bazbremner mentioned this pull request Dec 13, 2016

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

Merged

chris-rock approved these changes Dec 13, 2016

View reviewed changes

artem-sidorenko added 2 commits December 14, 2016 07:59

Refactoring: simplify handling of crypto parameters

a419535

Module `DevSec::Ssh` delivers the crypto parameters. There is autodetection of ssh version with fallback to 5.9

Bumping master to 2.0.0 as next release

33caca8

artem-sidorenko force-pushed the refactor-libraries branch from d50ba56 to 33caca8 Compare December 14, 2016 06:59

artem-sidorenko changed the title ~~WIP: Refactor the library to simplify the kex/cipher handling~~ Refactoring of library to simplify the kex/cipher handling Dec 14, 2016

artem-sidorenko merged commit 0fa0082 into dev-sec:master Dec 14, 2016

artem-sidorenko deleted the refactor-libraries branch December 14, 2016 07:13


		type, call = parse_method(method)

		Chef::Log.debug("Called #{method}")


		ssh_version = send("get_ssh_#{type}_version", node)

		Chef::Log.debug("Detected ssh version #{ssh_version}")

Refactoring of library to simplify the kex/cipher handling #134

Refactoring of library to simplify the kex/cipher handling #134

Conversation

artem-sidorenko commented Nov 9, 2016 • edited Loading

artem-sidorenko commented Nov 9, 2016

coveralls commented Nov 9, 2016

coveralls commented Nov 9, 2016

coveralls commented Nov 9, 2016

coveralls commented Nov 10, 2016

coveralls commented Nov 10, 2016

coveralls commented Nov 10, 2016

coveralls commented Nov 10, 2016

artem-sidorenko commented Nov 10, 2016

artem-sidorenko commented Nov 17, 2016

coveralls commented Nov 23, 2016

coveralls commented Nov 23, 2016

artem-sidorenko commented Nov 30, 2016

chris-rock commented Nov 30, 2016

chris-rock left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

artem-sidorenko Dec 5, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

artem-sidorenko commented Dec 5, 2016

coveralls commented Dec 5, 2016

coveralls commented Dec 5, 2016

coveralls commented Dec 5, 2016

artem-sidorenko commented Dec 5, 2016

coveralls commented Dec 5, 2016

coveralls commented Dec 5, 2016

artem-sidorenko commented Dec 9, 2016

atomic111 commented Dec 13, 2016

chris-rock left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Dec 14, 2016

artem-sidorenko commented Dec 14, 2016

artem-sidorenko commented Nov 9, 2016 •

edited

Loading

artem-sidorenko Dec 5, 2016 •

edited

Loading