Fixing multiple issues #22

elmobp · 2016-12-01T05:18:52Z

Support for Ruby 2.0+ (Xenial!!)
Add support for array's in protocols
Check protocols to ensure they are valid
Adjust tests to suit
Bump to 0.8.8

jamesmollison · 2016-12-01T05:30:24Z

Have this gem on Trusty and Xenial boxes which are running ruby 2.3.0 and 1.9.3 respectively. Once the protocol issue was corrected the iptables rule output was identical on both hosts.

auxesis · 2016-12-01T05:42:33Z

lib/ript/dsl/primitives/filter.rb

@@ -80,7 +79,6 @@ def build_rule(name, block, opts={})
              to_address    = @labels[to][:address]

              attributes = {
-                             "table"       => "filter",


What's the reasoning behind omitting the table attribute?

The original reason it was included was to be explicit about what rules are being generated, and thus future proof Ript. If the future behaviour of iptables changes, and the filter table is no longer the default table, then Ript will silently break.

Bonus: if you add this attribute back back, your PR will be way smaller because you won't have to modify all the tests.

filter is not understood on xenial, jump on a xenial box and try do iptables with --filter fail

Are you sure?

root@ubuntu-512mb-sfo1-01:~# iptables --table filter --new-chain basic-a root@ubuntu-512mb-sfo1-01:~# iptables --table filter -L Chain INPUT (policy ACCEPT) target prot opt source destination Chain FORWARD (policy ACCEPT) target prot opt source destination Chain OUTPUT (policy ACCEPT) target prot opt source destination Chain basic-a (0 references) target prot opt source destination root@ubuntu-512mb-sfo1-01:~# cat /etc/lsb-release DISTRIB_ID=Ubuntu DISTRIB_RELEASE=16.04 DISTRIB_CODENAME=xenial DISTRIB_DESCRIPTION="Ubuntu 16.04.1 LTS"

What kernel?

root@ubuntu-512mb-sfo1-01:~# uname -a Linux ubuntu-512mb-sfo1-01 4.4.0-47-generic #68-Ubuntu SMP Wed Oct 26 19:39:52 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux

Do you have the filter module loaded into your kernel?

root@ubuntu-512mb-sfo1-01:~# lsmod |grep filter iptable_filter 16384 1 ip_tables 24576 1 iptable_filter x_tables 36864 2 ip_tables,iptable_filter

Yes sir, I think we have hit a weird bug in iptables somewhere Ill add the filter back in and post the results..

Adding it back we just realised what it was :| the template manifest had an invalid protocol in it so it was crapping out isiss != isis :(

Will fix the test in the morning

auxesis · 2016-12-01T05:44:55Z

ript.gemspec

@@ -7,8 +7,8 @@ Gem::Specification.new do |s|
  s.name        = "ript"
  s.version     = Ript::VERSION
  s.platform    = Gem::Platform::RUBY
-  s.authors     = [ "Lindsay Holmwood" ]
-  s.email       = [ "lindsay@bulletproof.net" ]


I would recommend updating AUTHORS.md and explaining what foundation@bulletproof.net is.

Thanks for adding the bit to AUTHORS.md!

auxesis · 2016-12-01T06:09:50Z

AUTHORS.md


 Inspiration given by:

 Matt Moor (@mattm0)

-Ript is copyright Bulletproof Networks 2011-2012, all rights reserved.
+Contact:
+foundation@bulletproof.net will send an email to the development team wihtibn Bulletproof Networks, any issues should be raised via Github.


s/wihtibn/within/

s/Github/GitHub/

ghost

LGTM 👍

Not a show stopper but would recommend running this through a linter as there are a few stylistic things that wouldn't pass our style guide.

Minor things like the use of double quotes etc.

ghost · 2016-12-01T20:21:13Z

README.md


 It is also recommended that you use [rbenv](http://rbenv.org/).

 ``` bash
-rbenv install 1.9.3-p484
+rbenv install 2.3.0


if the ruby-version is set to 1.9.3-p125, why install 2.3.1?

@mfsgr I fix that now

ghost · 2016-12-01T20:28:50Z

lib/ript/patches.rb

-    after  = self.dup.drop_while {|k, v| k != key }
-    before << opts.to_a.flatten
-    self.clear.merge!(Hash[before]).merge!(Hash[after])
+  def insert_before(key, kvpair)


elmobp · 2016-12-02T00:17:55Z

Tests are passing again!

elmobp · 2016-12-02T00:18:58Z

As for the styling, I will do this on 0.8.9

Support for 0.8.8 and Ruby 2 Documentation updates Ensure protocol actually exists Change author to Bulletproof Fix tests Add some authors I need to fix the tests again Update filter.rb Fix tests Change ruby version Cleanup Update filter.rb Final release

gergnz · 2016-12-04T22:50:47Z

AUTHORS.md

@@ -8,9 +8,14 @@ Patches have been merged from:
 Arthur Barton (@arthurbarton)
 John Ferlito (@johnf)
 Jesse Reynolds (@jessereynolds)
+Michael Baker (@elmobp)
+Greg Cockburn (gergnz)


gergnz · 2016-12-04T22:51:33Z

CHANGELOG.md

+# 0.8.8 - 2016/12/02
+ - Bug: Support for an array of protocols (@elmobp)
+ - Bug: Support Ruby 2.0+ (@elmobp)
+ - Feature: Protocol validation using /etc/protocols, by adding this support in the validation ensures many ohter parts of the software performed correctly (@elmobp)


s/ohter/other/

gergnz · 2016-12-04T22:51:58Z

Gemfile.lock


 GEM
-  remote: https://rubygems.org/
+  remote: http://rubygems.org/


why did we go t F438 o http instead of https?

gergnz · 2016-12-04T22:53:03Z

LICENCE

@@ -1,4 +1,4 @@
-Copyright 2011-2012 Bulletproof Networks. All rights reserved.
+Copyright 2011-2012 Bulletproof Networks. 


Are we actually Bulletproof Networks?, We should probably use "Bulletproof Group Ltd"

gergnz · 2016-12-04T22:54:01Z

lib/ript/dsl/primitives/filter.rb

@@ -121,26 +119,44 @@ def build_rule(name, block, opts={})
              end

              # If we have arguments, iterate through them
-              if arguments.size > 0
+              if arguments.size > 0 


white space at the end.

gergnz · 2016-12-04T22:54:33Z

lib/ript/dsl/primitives/nat.rb

@@ -57,7 +57,7 @@ def rewrite(name, opts={}, &block)
                            "jump"        => "ACCEPT" }

            @froms.map {|from| @labels[from][:address]}.each do |address|
-              attributes.insert_before("destination", "source" => address)
+              attributes.insert_before("destination", [ "source", address ])


extra space in array.

gergnz · 2016-12-04T22:54:39Z

lib/ript/dsl/primitives/nat.rb

@@ -113,7 +113,7 @@ def rewrite(name, opts={}, &block)
                                      "dport"       => destination_port,
                                      "jump"        => "ACCEPT" }

-                      attributes.insert_before("destination", "source" => from_address) unless from_address == "0.0.0.0/0"
+                      attributes.insert_before("destination", [ "source", from_address ]) unless from_address == "0.0.0.0/0"


extra space in array.

gergnz · 2016-12-04T22:54:44Z

lib/ript/dsl/primitives/nat.rb

@@ -136,7 +136,7 @@ def rewrite(name, opts={}, &block)
                                    "dport"       => port,
                                    "jump"        => "ACCEPT" }

-                    attributes.insert_before("destination", "source" => from_address) unless from_address == "0.0.0.0/0"
+                    attributes.insert_before("destination", [ "source" , from_address ]) unless from_address == "0.0.0.0/0"


extra space in array.

gergnz · 2016-12-04T22:55:14Z

ript.gemspec

@@ -7,8 +7,8 @@ Gem::Specification.new do |s|
  s.name        = "ript"
  s.version     = Ript::VERSION
  s.platform    = Gem::Platform::RUBY
-  s.authors     = [ "Lindsay Holmwood" ]
-  s.email       = [ "lindsay@bulletproof.net" ]
+  s.authors     = [ "Bulletproof Networks" ]


Bulletproof Group Ltd

elmobp assigned johnf, jamesmollison, gergnz and ghost Dec 1, 2016

elmobp added the in progress label Dec 1, 2016

auxesis reviewed Dec 1, 2016

View reviewed changes

ghost reviewed Dec 1, 2016

View reviewed changes

ghost assigned elmobp and unassigned ghost Dec 1, 2016

Support ruby v2

6da621d

Support for 0.8.8 and Ruby 2 Documentation updates Ensure protocol actually exists Change author to Bulletproof Fix tests Add some authors I need to fix the tests again Update filter.rb Fix tests Change ruby version Cleanup Update filter.rb Final release

elmobp force-pushed the master branch from 85077af to 6da621d Compare December 2, 2016 05:51

Change licence

255f740

gergnz suggested changes Dec 4, 2016

View reviewed changes

Requested changes

a91f258

gergnz approved these changes Dec 4, 2016

View reviewed changes

Rename

4b3f90d

gergnz approved these changes Dec 4, 2016

View reviewed changes

elmobp added 2 commits December 5, 2016 08:19

RM

e4112a7

Resolve conflict

25e33af

elmobp merged commit 834ccef into ac3cloud:master Dec 5, 2016

elmobp removed the in progress label Dec 5, 2016

jamesmollison mentioned this pull request Dec 5, 2016

LICENCE is contradictory #23

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fixing multiple issues #22

Fixing multiple issues #22

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

		@@ -1,4 +1,4 @@
		Copyright 2011-2012 Bulletproof Networks. All rights reserved.
		Copyright 2011-2012 Bulletproof Networks.

Fixing multiple issues #22

Fixing multiple issues #22

Uh oh!

Conversation

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!