Refactor rest authorization to raise exceptions deeper

The idea is to raise an AuthorizationException at the same place
we check the authorization instead of in an upper level to be
able to spot where the authorization took place in the exception
backtrace.

Moreover, this changes also makes Rights::allowed? to return
the matching acl so that the upper layer can have a chance to
report which ACL resulted in the match.

Signed-off-by: Brice Figureau <brice-puppet@daysofwonder.com>

5 files changed, 94 insertions, 56 deletions

diff --git a/lib/puppet/network/authconfig.rb b/lib/puppet/network/authconfig.rb
index 3e0807ad1..3e40c9d7c 100644
--- a/lib/puppet/network/authconfig.rb
+++ b/lib/puppet/network/authconfig.rb
@@ -32,9 +32,8 @@ module Puppet
                 return @rights[name].allowed?(request.name, request.ip)
             elsif @rights.include?(namespace)
                 return @rights[namespace].allowed?(request.name, request.ip)
-            else
-                return false
             end
+            false
         end
 
         # Does the file exist?  Puppetmasterd does not require it, but
@@ -111,7 +110,7 @@ module Puppet
                                 name = $3
                             end
                             name.chomp!
-                            right = newrights.newright(name, count)
+                            right = newrights.newright(name, count, @file)
                         when /^\s*(allow|deny|method|environment)\s+(.+)$/
                             parse_right_directive(right, $1, $2, count)
                         else
diff --git a/lib/puppet/network/http/handler.rb b/lib/puppet/network/http/handler.rb
index 20234b2da..c6d34fe43 100644
--- a/lib/puppet/network/http/handler.rb
+++ b/lib/puppet/network/http/handler.rb
@@ -3,6 +3,7 @@ end
 
 require 'puppet/network/http/api/v1'
 require 'puppet/network/rest_authorization'
+require 'puppet/network/rights'
 
 module Puppet::Network::HTTP::Handler
     include Puppet::Network::HTTP::API::V1
@@ -40,11 +41,9 @@ module Puppet::Network::HTTP::Handler
     def process(request, response)
         indirection_request = uri2indirection(http_method(request), path(request), params(request))
 
-        if authorized?(indirection_request)
-            send("do_%s" % indirection_request.method, indirection_request, request, response)
-        else
-            return do_exception(response, "Request forbidden by configuration %s %s" % [indirection_request.indirection_name, indirection_request.key], 403)
-        end
+        check_authorization(indirection_request)
+
+        send("do_%s" % indirection_request.method, indirection_request, request, response)
     rescue Exception => e
         return do_exception(response, e)
     end
@@ -60,6 +59,11 @@ module Puppet::Network::HTTP::Handler
     end
 
     def do_exception(response, exception, status=400)
+        if exception.is_a?(Puppet::Network::AuthorizationError)
+            # make sure we return the correct status code
+            # for authorization issues
+            status = 403 if status == 400
+        end
         if exception.is_a?(Exception)
             puts exception.backtrace if Puppet[:trace]
             Puppet.err(exception)
diff --git a/lib/puppet/network/rest_authconfig.rb b/lib/puppet/network/rest_authconfig.rb
index e3fd51753..22be8b007 100644
--- a/lib/puppet/network/rest_authconfig.rb
+++ b/lib/puppet/network/rest_authconfig.rb
@@ -23,9 +23,16 @@ module Puppet
         end
 
         # check wether this request is allowed in our ACL
+        # raise an Puppet::Network::AuthorizedError if the request
+        # is denied.
         def allowed?(request)
             read()
-            return @rights.allowed?(build_uri(request), request.node, request.ip, request.method, request.environment)
+
+            @rights.fail_on_deny(build_uri(request),
+                                    :node => request.node,
+                                    :ip => request.ip,
+                                    :method => request.method,
+                                    :environment => request.environment)
         end
 
         def initialize(file = nil, parsenow = true)
diff --git a/lib/puppet/network/rest_authorization.rb b/lib/puppet/network/rest_authorization.rb
index 3278640fe..e6f62d914 100644
--- a/lib/puppet/network/rest_authorization.rb
+++ b/lib/puppet/network/rest_authorization.rb
@@ -2,12 +2,10 @@ require 'puppet/network/client_request'
 require 'puppet/network/rest_authconfig'
 
 module Puppet::Network
-    # Most of our subclassing is just so that we can get
-    # access to information from the request object, like
-    # the client name and IP address.
-    class InvalidClientRequest < Puppet::Error; end
+
     module RestAuthorization
 
+
         # Create our config object if necessary. If there's no configuration file
         # we install our defaults
         def authconfig
@@ -20,28 +18,25 @@ module Puppet::Network
 
         # Verify that our client has access.  We allow untrusted access to
         # certificates terminus but no others.
-        def authorized?(request)
-            msg = "%s client %s access to %s [%s]" %
-                   [ request.authenticated? ? "authenticated" : "unauthenticated",
-                    (request.node.nil? ? request.ip : "#{request.node}(#{request.ip})"),
-                    request.indirection_name, request.method ]
-
+        def check_authorization(request)
             if request.authenticated?
-                res = authenticated_authorized?(request, msg )
+                authenticated_authorized?(request)
             else
-                res = unauthenticated_authorized?(request, msg)
+                unless unauthenticated_authorized?(request)
+                    msg = "%s access to %s [%s]" % [ (request.node.nil? ? request.ip : "#{request.node}(#{request.ip})"), request.indirection_name, request.method ]
+                    Puppet.warning("Denying access: " + msg)
+                    raise AuthorizationError.new( "Forbidden request:" + msg )
+                end
             end
-            Puppet.notice((res ? "Allowing " : "Denying ") + msg)
-            return res
         end
 
         # delegate to our authorization file
-        def authenticated_authorized?(request, msg)
+        def authenticated_authorized?(request)
             authconfig.allowed?(request)
         end
 
         # allow only certificate requests when not authenticated
-        def unauthenticated_authorized?(request, msg)
+        def unauthenticated_authorized?(request)
             request.indirection_name == :certificate or request.indirection_name == :certificate_request
         end
     end
diff --git a/lib/puppet/network/rights.rb b/lib/puppet/network/rights.rb
index 7f4bed7f6..c98b84e8d 100755
--- a/lib/puppet/network/rights.rb
+++ b/lib/puppet/network/rights.rb
@@ -1,10 +1,16 @@
 require 'puppet/network/authstore'
+require 'puppet/error'
+
+module Puppet::Network
+
+# this exception is thrown when a request is not authenticated
+class AuthorizationError < Puppet::Error; end
 
 # Define a set of rights and who has access to them.
 # There are two types of rights:
 #  * named rights (ie a common string)
 #  * path based rights (which are matched on a longest prefix basis)
-class Puppet::Network::Rights
+class Rights
 
     # We basically just proxy directly to our rights.  Each Right stores
     # its own auth abilities.
@@ -18,31 +24,57 @@ class Puppet::Network::Rights
         end
     end
 
+    # Check that name is allowed or not
     def allowed?(name, *args)
+        begin
+            fail_on_deny(name, *args)
+        rescue AuthorizationError
+            return false
+        rescue ArgumentError
+            # the namespace contract says we should raise this error
+            # if we didn't find the right acl
+            raise
+        end
+        return true
+    end
+
+    def fail_on_deny(name, args = {})
         res = :nomatch
         right = @rights.find do |acl|
+            found = false
             # an acl can return :dunno, which means "I'm not qualified to answer your question, 
             # please ask someone else". This is used when for instance an acl matches, but not for the
             # current rest method, where we might think some other acl might be more specific.
             if match = acl.match?(name)
-                args << match
-                if (res = acl.allowed?(*args)) != :dunno
-                    return res
+                args[:match] = match
+                if (res = acl.allowed?(args[:node], args[:ip], args)) != :dunno
+                    # return early if we're allowed
+                    return if res
+                    # we matched, select this acl
+                    found = true
                 end
             end
-            false
+            found
         end
 
-        # if allowed or denied, tell it to the world
-        return res unless res == :nomatch
-
-        # there were no rights allowing/denying name
-        # if name is not a path, let's throw
-        raise ArgumentError, "Unknown namespace right '%s'" % name unless name =~ /^\//
+        # if we end here, then that means we either didn't match
+        # or failed, in any case will throw an error to the outside world
+        if name =~ /^\//
+            # we're a patch ACL, let's fail
+            msg = "%s access to %s [%s]" % [ (args[:node].nil? ? args[:ip] : "#{args[:node]}(#{args[:ip]})"), name, args[:method] ]
 
-        # but if this was a path, we implement a deny all policy by default
-        # on unknown rights.
-        return false
+            error = AuthorizationError.new("Forbidden request: " + msg)
+            if right
+                error.file = right.file
+                error.line = right.line
+            end
+            Puppet.warning("Denying access: " + error.to_s)
+        else
+            # there were no rights allowing/denying name
+            # if name is not a path, let's throw
+            error = ArgumentError.new "Unknown namespace right '%s'" % name
+        end
+        raise error
     end
 
     def initialize()
@@ -62,8 +94,8 @@ class Puppet::Network::Rights
     end
 
     # Define a new right to which access can be provided.
-    def newright(name, line=nil)
-        add_right( Right.new(name, line) )
+    def newright(name, line=nil, file=nil)
+        add_right( Right.new(name, line, file) )
     end
 
     private
@@ -88,18 +120,21 @@ class Puppet::Network::Rights
 
     # A right.
     class Right < Puppet::Network::AuthStore
-        attr_accessor :name, :key, :acl_type, :line
+        include Puppet::FileCollection::Lookup
+
+        attr_accessor :name, :key, :acl_type
         attr_accessor :methods, :environment
 
         ALL = [:save, :destroy, :find, :search]
 
         Puppet::Util.logmethods(self, true)
 
-        def initialize(name, line)
+        def initialize(name, line, file)
             @methods = []
             @environment = []
             @name = name
             @line = line || 0
+            @file = file
 
             case name
             when Symbol
@@ -140,18 +175,16 @@ class Puppet::Network::Rights
         # if this right is too restrictive (ie we don't match this access method)
         # then return :dunno so that upper layers have a chance to try another right
         # tailored to the given method
-        def allowed?(name, ip, method = nil, environment = nil, match = nil)
-            return :dunno if acl_type == :regex and not @methods.include?(method)
-            return :dunno if acl_type == :regex and @environment.size > 0 and not @environment.include?(environment)
-
-            if acl_type == :regex and match # make sure any capture are replaced
-                interpolate(match)
-            end
-
-            res = super(name,ip)
-
-            if acl_type == :regex
-                reset_interpolation
+        def allowed?(name, ip, args)
+            return :dunno if acl_type == :regex and not @methods.include?(args[:method])
+            return :dunno if acl_type == :regex and @environment.size > 0 and not @environment.include?(args[:environment])
+
+            begin
+                # make sure any capture are replaced if needed
+                interpolate(args[:match]) if acl_type == :regex and args[:match]
+                res = super(name,ip)
+            ensure
+                reset_interpolation if acl_type == :regex
             end
             res
         end
@@ -222,4 +255,4 @@ class Puppet::Network::Rights
     end
 
 end
-
+end