Merge branch 'tickets/2.7.x/5777' into 2.7.x

* tickets/2.7.x/5777:
  Deprecate RestAuthConfig#allowed? in favor of #check_authorization
  Fix #6026 - security file should support inline comments
  Fix #5010 - Allow leading whitespace in auth.conf
  Fix #5777 - rule interpolation broke auth.conf CIDR rules

8 files changed, 193 insertions, 17 deletions

diff --git a/lib/puppet/file_serving/configuration/parser.rb b/lib/puppet/file_serving/configuration/parser.rb
index 334201d37..83b75e28f 100644
--- a/lib/puppet/file_serving/configuration/parser.rb
+++ b/lib/puppet/file_serving/configuration/parser.rb
@@ -24,9 +24,10 @@ class Puppet::FileServing::Configuration::Parser < Puppet::Util::LoadedFile
         when /^\s*$/; next # skip blank lines
         when /\[([-\w]+)\]/
           mount = newmount($1)
-        when /^\s*(\w+)\s+(.+)$/
+        when /^\s*(\w+)\s+(.+?)(\s*#.*)?$/
           var = $1
           value = $2
+          value.strip!
           raise(ArgumentError, "Fileserver configuration file does not use '=' as a separator") if value =~ /^=/
           case var
           when "path"
@@ -58,12 +59,8 @@ class Puppet::FileServing::Configuration::Parser < Puppet::Util::LoadedFile
       begin
         mount.info "allowing #{val} access"
         mount.allow(val)
-      rescue AuthStoreError => detail
-
-              raise ArgumentError.new(
-        detail.to_s,
-        
-          @count, file)
+      rescue Puppet::AuthStoreError => detail
+        raise ArgumentError.new(detail.to_s, @count, file)
       end
     }
   end
@@ -75,12 +72,8 @@ class Puppet::FileServing::Configuration::Parser < Puppet::Util::LoadedFile
       begin
         mount.info "denying #{val} access"
         mount.deny(val)
-      rescue AuthStoreError => detail
-
-              raise ArgumentError.new(
-        detail.to_s,
-        
-          @count, file)
+      rescue Puppet::AuthStoreError => detail
+        raise ArgumentError.new(detail.to_s, @count, file)
       end
     }
   end
diff --git a/lib/puppet/network/authconfig.rb b/lib/puppet/network/authconfig.rb
index 4ba89fa71..1e486a2f9 100644
--- a/lib/puppet/network/authconfig.rb
+++ b/lib/puppet/network/authconfig.rb
@@ -102,7 +102,7 @@ module Puppet
               name = $3 if $2 == "path"
               name.chomp!
               right = newrights.newright(name, count, @file)
-            when /^\s*(allow|deny|method|environment|auth(?:enticated)?)\s+(.+)$/
+            when /^\s*(allow|deny|method|environment|auth(?:enticated)?)\s+(.+?)(\s*#.*)?$/
               parse_right_directive(right, $1, $2, count)
             else
               raise ConfigurationError, "Invalid line #{count}: #{line}"
@@ -130,6 +130,7 @@ module Puppet
     end
 
     def parse_right_directive(right, var, value, count)
+      value.strip!
       case var
       when "allow"
         modify_right(right, :allow, value, "allowing %s access", count)
@@ -159,6 +160,7 @@ module Puppet
     def modify_right(right, method, value, msg, count)
       value.split(/\s*,\s*/).each do |val|
         begin
+          val.strip!
           right.info msg % val
           right.send(method, val)
         rescue AuthStoreError => detail
diff --git a/lib/puppet/network/rest_authconfig.rb b/lib/puppet/network/rest_authconfig.rb
index dfe8f85c4..7dcc81ef4 100644
--- a/lib/puppet/network/rest_authconfig.rb
+++ b/lib/puppet/network/rest_authconfig.rb
@@ -29,10 +29,15 @@ module Puppet
       @main
     end
 
+    def allowed?(request)
+      Puppet.deprecation_warning "allowed? should not be called for REST authorization - use check_authorization instead"
+      check_authorization(request)
+    end
+
     # check wether this request is allowed in our ACL
     # raise an Puppet::Network::AuthorizedError if the request
     # is denied.
-    def allowed?(indirection, method, key, params)
+    def check_authorization(indirection, method, key, params)
       read
 
       # we're splitting the request in part because
diff --git a/lib/puppet/network/rest_authorization.rb b/lib/puppet/network/rest_authorization.rb
index 50f094e3e..d636d486a 100644
--- a/lib/puppet/network/rest_authorization.rb
+++ b/lib/puppet/network/rest_authorization.rb
@@ -16,7 +16,7 @@ module Puppet::Network
 
     # Verify that our client has access.
     def check_authorization(indirection, method, key, params)
-      authconfig.allowed?(indirection, method, key, params)
+      authconfig.check_authorization(indirection, method, key, params)
     end
   end
 end
diff --git a/spec/integration/network/rest_authconfig_spec.rb b/spec/integration/network/rest_authconfig_spec.rb
new file mode 100644
index 000000000..d2f539cd4
--- /dev/null
+++ b/spec/integration/network/rest_authconfig_spec.rb
@@ -0,0 +1,145 @@
+require 'spec_helper'
+
+require 'puppet/network/rest_authconfig'
+
+RSpec::Matchers.define :allow do |params|
+
+  match do |auth|
+    begin
+      auth.check_authorization(params[0], params[1], params[2], params[3])
+      true
+    rescue Puppet::Network::AuthorizationError
+      false
+    end
+  end
+
+  failure_message_for_should do |instance|
+    "expected #{params[3][:node]}/#{params[3][:ip]} to be allowed"
+  end
+
+  failure_message_for_should_not do |instance|
+    "expected #{params[3][:node]}/#{params[3][:ip]} to be forbidden"
+  end
+end
+
+describe Puppet::Network::RestAuthConfig do
+  include PuppetSpec::Files
+
+  before(:each) do
+    Puppet[:rest_authconfig] = tmpfile('auth.conf')
+  end
+
+  def add_rule(rule)
+    File.open(Puppet[:rest_authconfig],"w+") do |f|
+      f.print "path /test\n#{rule}\n"
+    end
+    @auth = Puppet::Network::RestAuthConfig.new(Puppet[:rest_authconfig], true)
+  end
+
+  def add_regex_rule(regex, rule)
+    File.open(Puppet[:rest_authconfig],"w+") do |f|
+      f.print "path ~ #{regex}\n#{rule}\n"
+    end
+    @auth = Puppet::Network::RestAuthConfig.new(Puppet[:rest_authconfig], true)
+  end
+
+  def request(args = {})
+    { :ip => '10.1.1.1', :node => 'host.domain.com', :key => 'key', :authenticated => true }.each do |k,v|
+      args[k] ||= v
+    end
+    ['test', :find, args[:key], args]
+  end
+
+  it "should support IPv4 address" do
+    add_rule("allow 10.1.1.1")
+
+    @auth.should allow(request)
+  end
+
+  it "should support CIDR IPv4 address" do
+    add_rule("allow 10.0.0.0/8")
+
+    @auth.should allow(request)
+  end
+
+  it "should support wildcard IPv4 address" do
+    add_rule("allow 10.1.1.*")
+
+    @auth.should allow(request)
+  end
+
+  it "should support IPv6 address" do
+    add_rule("allow 2001:DB8::8:800:200C:417A")
+
+    @auth.should allow(request(:ip => '2001:DB8::8:800:200C:417A'))
+  end
+
+  it "should support hostname" do
+    add_rule("allow host.domain.com")
+
+    @auth.should allow(request)
+  end
+
+  it "should support wildcard host" do
+    add_rule("allow *.domain.com")
+
+    @auth.should allow(request)
+  end
+
+  it "should support hostname backreferences" do
+    add_regex_rule('^/test/([^/]+)$', "allow $1.domain.com")
+
+    @auth.should allow(request(:key => 'host'))
+  end
+
+  it "should support opaque strings" do
+    add_rule("allow this-is-opaque@or-not")
+
+    @auth.should allow(request(:node => 'this-is-opaque@or-not'))
+  end
+
+  it "should support opaque strings and backreferences" do
+    add_regex_rule('^/test/([^/]+)$', "allow $1")
+
+    @auth.should allow(request(:key => 'this-is-opaque@or-not', :node => 'this-is-opaque@or-not'))
+  end
+
+  it "should support hostname ending with '.'" do
+    pending('bug #7589')
+    add_rule("allow host.domain.com.")
+
+    @auth.should allow(request(:node => 'host.domain.com.'))
+  end
+
+  it "should support hostname ending with '.' and backreferences" do
+    pending('bug #7589')
+    add_regex_rule('^/test/([^/]+)$',"allow $1")
+
+    @auth.should allow(request(:node => 'host.domain.com.'))
+  end
+
+  it "should support trailing whitespace" do
+    add_rule('allow host.domain.com    ')
+
+    @auth.should allow(request)
+  end
+
+  it "should support inlined comments" do
+    add_rule('allow host.domain.com # will it work?')
+
+    @auth.should allow(request)
+  end
+
+  it "should deny non-matching host" do
+    add_rule("allow inexistant")
+
+    @auth.should_not allow(request)
+  end
+
+  it "should deny denied hosts" do
+    add_rule("deny host.domain.com")
+
+    @auth.should_not allow(request)
+  end
+
+end
\ No newline at end of file
diff --git a/spec/unit/file_serving/configuration/parser_spec.rb b/spec/unit/file_serving/configuration/parser_spec.rb
index 3d6b3e234..5ccfc5075 100755
--- a/spec/unit/file_serving/configuration/parser_spec.rb
+++ b/spec/unit/file_serving/configuration/parser_spec.rb
@@ -118,6 +118,14 @@ describe Puppet::FileServing::Configuration::Parser do
       @parser.parse
     end
 
+    it "should support inline comments" do
+      mock_file_content "[one]\nallow something \# will it work?\n"
+
+      @mount.expects(:info)
+      @mount.expects(:allow).with("something")
+      @parser.parse
+    end
+
     it "should tell the mount to deny any deny values from the section" do
       mock_file_content "[one]\ndeny something\n"
 
diff --git a/spec/unit/network/authconfig_spec.rb b/spec/unit/network/authconfig_spec.rb
index c47b2e0c5..ca94cc1ab 100755
--- a/spec/unit/network/authconfig_spec.rb
+++ b/spec/unit/network/authconfig_spec.rb
@@ -184,6 +184,29 @@ describe Puppet::Network::AuthConfig do
       @authconfig.read
     end
 
+    it "should strip whitespace around ACE" do
+      acl = stub 'acl', :info
+
+      @fd.stubs(:each).multiple_yields('[puppetca]', ' allow 127.0.0.1 , 172.16.10.0  ')
+      @rights.stubs(:newright).with("[puppetca]", 1, 'dummy').returns(acl)
+
+      acl.expects(:allow).with('127.0.0.1')
+      acl.expects(:allow).with('172.16.10.0')
+
+      @authconfig.read
+    end
+
+    it "should allow ACE inline comments" do
+      acl = stub 'acl', :info
+
+      @fd.stubs(:each).multiple_yields('[puppetca]', ' allow 127.0.0.1 # will it work?')
+      @rights.stubs(:newright).with("[puppetca]", 1, 'dummy').returns(acl)
+
+      acl.expects(:allow).with('127.0.0.1')
+
+      @authconfig.read
+    end
+
     it "should create an allow ACE on each subsequent allow" do
       acl = stub 'acl', :info
 
diff --git a/spec/unit/network/rest_authconfig_spec.rb b/spec/unit/network/rest_authconfig_spec.rb
index e1403997f..bebbb874f 100755
--- a/spec/unit/network/rest_authconfig_spec.rb
+++ b/spec/unit/network/rest_authconfig_spec.rb
@@ -29,7 +29,7 @@ describe Puppet::Network::RestAuthConfig do
     params = {:ip => "127.0.0.1", :node => "me", :environment => :env, :authenticated => true}
     @acl.expects(:is_request_forbidden_and_why?).with("path", :save, "to/resource", params).returns(nil)
 
-    @authconfig.allowed?("path", :save, "to/resource", params)
+    @authconfig.check_authorization("path", :save, "to/resource", params)
   end
 
   describe "when defining an acl with mk_acl" do