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 <[email protected]> --- lib/puppet/network/authconfig.rb | 8 ++-- lib/puppet/network/http/handler.rb | 13 ++++-- lib/puppet/network/rest_authorization.rb | 43 +++++++++++++------ lib/puppet/network/rights.rb | 22 +++++----- spec/integration/indirector/certificate/rest.rb | 2 +- .../indirector/certificate_request/rest.rb | 2 +- .../indirector/certificate_revocation_list/rest.rb | 2 +- spec/integration/indirector/report/rest.rb | 2 +- spec/integration/indirector/rest.rb | 4 +- spec/unit/network/http/handler.rb | 8 ++-- spec/unit/network/rest_authorization.rb | 20 +++++++-- spec/unit/network/rights.rb | 41 +++++++++++++++--- test/network/rights.rb | 6 ++- 13 files changed, 117 insertions(+), 56 deletions(-) diff --git a/lib/puppet/network/authconfig.rb b/lib/puppet/network/authconfig.rb index 3e0807a..3aaab9a 100644 --- a/lib/puppet/network/authconfig.rb +++ b/lib/puppet/network/authconfig.rb @@ -28,13 +28,13 @@ module Puppet read() + res = false if @rights.include?(name) - return @rights[name].allowed?(request.name, request.ip) + res, acl = @rights[name].allowed?(request.name, request.ip) elsif @rights.include?(namespace) - return @rights[namespace].allowed?(request.name, request.ip) - else - return false + res, acl = @rights[namespace].allowed?(request.name, request.ip) end + return res end # Does the file exist? Puppetmasterd does not require it, but diff --git a/lib/puppet/network/http/handler.rb b/lib/puppet/network/http/handler.rb index 20234b2..12cdf9b 100644 --- a/lib/puppet/network/http/handler.rb +++ b/lib/puppet/network/http/handler.rb @@ -40,11 +40,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 +58,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_authorization.rb b/lib/puppet/network/rest_authorization.rb index 3278640..3d84cdd 100644 --- a/lib/puppet/network/rest_authorization.rb +++ b/lib/puppet/network/rest_authorization.rb @@ -2,12 +2,13 @@ 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 + + # this exception is thrown when a request is not authenticated + class AuthorizationError < RuntimeError; end + module RestAuthorization + # Create our config object if necessary. If there's no configuration file # we install our defaults def authconfig @@ -18,30 +19,44 @@ module Puppet::Network @authconfig end + def authenticated_to_s(request) + request.authenticated? ? "authenticated" : "unauthenticated" + end + # Verify that our client has access. We allow untrusted access to # certificates terminus but no others. - def authorized?(request) + def check_authorization(request) msg = "%s client %s access to %s [%s]" % - [ request.authenticated? ? "authenticated" : "unauthenticated", + [ authenticated_to_s(request), (request.node.nil? ? request.ip : "#{request.node}(#{request.ip})"), request.indirection_name, request.method ] - if request.authenticated? - res = authenticated_authorized?(request, msg ) - else - res = unauthenticated_authorized?(request, msg) + method = authenticated_to_s(request) + "_authorized?" + + res, acl = send(method.to_sym, request) + unless res + unless acl.nil? + msg += " matching rule: '#{acl.name}'" + if acl.line != 0 and authconfig.exists? + msg += " of #{authconfig} line: #{acl.line}" + else + msg += " from default ACLs" + end + end + Puppet.notice("Denying "+ msg) + raise AuthorizationError, "Request forbidden by configuration %s %s" % [request.indirection_name, request.key] end - Puppet.notice((res ? "Allowing " : "Denying ") + msg) - return res + + Puppet.notice("Allowing "+ msg) 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 7f4bed7..959b004 100755 --- a/lib/puppet/network/rights.rb +++ b/lib/puppet/network/rights.rb @@ -18,6 +18,7 @@ class Puppet::Network::Rights end end + # Check that name is allowed or not def allowed?(name, *args) res = :nomatch right = @rights.find do |acl| @@ -27,14 +28,15 @@ class Puppet::Network::Rights if match = acl.match?(name) args << match if (res = acl.allowed?(*args)) != :dunno - return res + return res, acl end + args.pop end false end # if allowed or denied, tell it to the world - return res unless res == :nomatch + return res, nil unless (res == :nomatch or res == :dunno) # there were no rights allowing/denying name # if name is not a path, let's throw @@ -42,7 +44,7 @@ class Puppet::Network::Rights # but if this was a path, we implement a deny all policy by default # on unknown rights. - return false + return false, nil end def initialize() @@ -144,14 +146,12 @@ class Puppet::Network::Rights 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 + begin + # make sure any capture are replaced if needed + interpolate(match) if acl_type == :regex and match + res = super(name,ip) + ensure + reset_interpolation if acl_type == :regex end res end diff --git a/spec/integration/indirector/certificate/rest.rb b/spec/integration/indirector/certificate/rest.rb index c42bafb..8512fa9 100755 --- a/spec/integration/indirector/certificate/rest.rb +++ b/spec/integration/indirector/certificate/rest.rb @@ -48,7 +48,7 @@ describe "Certificate REST Terminus" do @mock_model = stub('faked model', :name => "certificate") Puppet::Indirector::Request.any_instance.stubs(:model).returns(@mock_model) - Puppet::Network::HTTP::WEBrickREST.any_instance.stubs(:authorized?).returns(true) + Puppet::Network::HTTP::WEBrickREST.any_instance.stubs(:check_authorization).returns(true) end after do diff --git a/spec/integration/indirector/certificate_request/rest.rb b/spec/integration/indirector/certificate_request/rest.rb index 1381876..a1dc5c0 100755 --- a/spec/integration/indirector/certificate_request/rest.rb +++ b/spec/integration/indirector/certificate_request/rest.rb @@ -53,7 +53,7 @@ describe "Certificate Request REST Terminus" do @mock_model = stub('faked model', :name => "certificate request") Puppet::Indirector::Request.any_instance.stubs(:model).returns(@mock_model) - Puppet::Network::HTTP::WEBrickREST.any_instance.stubs(:authorized?).returns(true) + Puppet::Network::HTTP::WEBrickREST.any_instance.stubs(:check_authorization).returns(true) end after do diff --git a/spec/integration/indirector/certificate_revocation_list/rest.rb b/spec/integration/indirector/certificate_revocation_list/rest.rb index a1ba4f9..dce0cf0 100755 --- a/spec/integration/indirector/certificate_revocation_list/rest.rb +++ b/spec/integration/indirector/certificate_revocation_list/rest.rb @@ -57,7 +57,7 @@ describe "Certificate REST Terminus" do @mock_model = stub('faked model', :name => "certificate") Puppet::Indirector::Request.any_instance.stubs(:model).returns(@mock_model) - Puppet::Network::HTTP::WEBrickREST.any_instance.stubs(:authorized?).returns(true) + Puppet::Network::HTTP::WEBrickREST.any_instance.stubs(:check_authorization).returns(true) end after do diff --git a/spec/integration/indirector/report/rest.rb b/spec/integration/indirector/report/rest.rb index 1150075..7903ac9 100644 --- a/spec/integration/indirector/report/rest.rb +++ b/spec/integration/indirector/report/rest.rb @@ -50,7 +50,7 @@ describe "Report REST Terminus" do @mock_model = stub_everything 'faked model', :name => "report", :convert_from => @report Puppet::Indirector::Request.any_instance.stubs(:model).returns(@mock_model) - Puppet::Network::HTTP::WEBrickREST.any_instance.stubs(:authorized?).returns(true) + Puppet::Network::HTTP::WEBrickREST.any_instance.stubs(:check_authorization) end after do diff --git a/spec/integration/indirector/rest.rb b/spec/integration/indirector/rest.rb index 34619c4..ede073d 100755 --- a/spec/integration/indirector/rest.rb +++ b/spec/integration/indirector/rest.rb @@ -78,7 +78,7 @@ describe Puppet::Indirector::REST do Puppet::Indirector::Request.any_instance.stubs(:model).returns(@mock_model) # do not trigger the authorization layer - Puppet::Network::HTTP::WEBrickREST.any_instance.stubs(:authorized?).returns(true) + Puppet::Network::HTTP::WEBrickREST.any_instance.stubs(:check_authorization).returns(true) end describe "when finding a model instance over REST" do @@ -311,7 +311,7 @@ describe Puppet::Indirector::REST do Puppet::Indirector::Request.any_instance.stubs(:model).returns(@mock_model) # do not trigger the authorization layer - Puppet::Network::HTTP::MongrelREST.any_instance.stubs(:authorized?).returns(true) + Puppet::Network::HTTP::MongrelREST.any_instance.stubs(:check_authorization).returns(true) end after do diff --git a/spec/unit/network/http/handler.rb b/spec/unit/network/http/handler.rb index 7b7ef47..743830a 100755 --- a/spec/unit/network/http/handler.rb +++ b/spec/unit/network/http/handler.rb @@ -49,7 +49,7 @@ describe Puppet::Network::HTTP::Handler do @result = stub 'result', :render => "mytext" - @handler.stubs(:authorized?).returns(true) + @handler.stubs(:check_authorization) stub_server_interface end @@ -97,7 +97,7 @@ describe Puppet::Network::HTTP::Handler do @handler.expects(:do_mymethod).with(request, @request, @response) - @handler.expects(:authorized?).with(request).returns(true) + @handler.expects(:check_authorization).with(request) @handler.process(@request, @response) end @@ -108,9 +108,9 @@ describe Puppet::Network::HTTP::Handler do @handler.expects(:do_mymethod).never - @handler.expects(:authorized?).with(request).returns(false) + @handler.expects(:check_authorization).with(request).raises(Puppet::Network::AuthorizationError) - @handler.expects(:set_response)#.with { |response, body, status| status == 403 } + @handler.expects(:set_response).with { |response, body, status| status == 403 } @handler.process(@request, @response) end diff --git a/spec/unit/network/rest_authorization.rb b/spec/unit/network/rest_authorization.rb index 15351b1..46274bb 100644 --- a/spec/unit/network/rest_authorization.rb +++ b/spec/unit/network/rest_authorization.rb @@ -29,14 +29,14 @@ describe Puppet::Network::RestAuthorization do [ :certificate, :certificate_request].each do |indirection| it "should allow #{indirection}" do @request.stubs(:indirection_name).returns(indirection) - @auth.authorized?(@request).should be_true + lambda { @auth.check_authorization(@request) }.should_not raise_error Puppet::Network::AuthorizationError end end [ :facts, :file_metadata, :file_content, :catalog, :report, :checksum, :runner ].each do |indirection| it "should not allow #{indirection}" do @request.stubs(:indirection_name).returns(indirection) - @auth.authorized?(@request).should be_false + lambda { @auth.check_authorization(@request) }.should raise_error Puppet::Network::AuthorizationError end end end @@ -47,9 +47,21 @@ describe Puppet::Network::RestAuthorization do end it "should delegate to the current rest authconfig" do - @authconfig.expects(:allowed?).with(@request) + @authconfig.expects(:allowed?).with(@request).returns(true) - @auth.authorized?(@request) + @auth.check_authorization(@request) + end + + it "should raise an AuthorizationError if request is not allowed" do + @authconfig.expects(:allowed?).with(@request).returns(false) + + lambda { @auth.check_authorization(@request) }.should raise_error Puppet::Network::AuthorizationError + end + + it "should not raise an AuthorizationError if request is allowed" do + @authconfig.expects(:allowed?).with(@request).returns(true) + + lambda { @auth.check_authorization(@request) }.should_not raise_error Puppet::Network::AuthorizationError end end end diff --git a/spec/unit/network/rights.rb b/spec/unit/network/rights.rb index 97094f8..6d7577e 100644 --- a/spec/unit/network/rights.rb +++ b/spec/unit/network/rights.rb @@ -244,7 +244,18 @@ describe Puppet::Network::Rights do @long_acl.stubs(:match?).returns(true) @long_acl.stubs(:allowed?).returns(:returned) - @right.allowed?("/path/to/there/and/there", :args).should == :returned + res, acl = @right.allowed?("/path/to/there/and/there", :args) + res.should == :returned + end + + it "should return the matched acl" do + @right.newright("/path/to/there", 0) + + @long_acl.stubs(:match?).returns(true) + @long_acl.stubs(:allowed?).returns(:returned) + + res, acl = @right.allowed?("/path/to/there/and/there", :args) + acl.should == @long_acl end it "should not raise an error if this path acl doesn't exist" do @@ -252,7 +263,8 @@ describe Puppet::Network::Rights do end it "should return false if no path match" do - @right.allowed?("/path", :args).should be_false + res, acl = @right.allowed?("/path", :args) + res.should be_false end end @@ -313,7 +325,18 @@ describe Puppet::Network::Rights do @regex_acl1.stubs(:match?).returns(true) @regex_acl1.stubs(:allowed?).returns(:returned) - @right.allowed?("/files/repository/myfile/other", :args).should == :returned + res, acl = @right.allowed?("/files/repository/myfile/other", :args) + res.should == :returned + end + + it "should return the matched acl" do + @right.newright("~ /files/(.*)/myfile", 0) + + @regex_acl1.stubs(:match?).returns(true) + @regex_acl1.stubs(:allowed?).returns(:returned) + + res, acl = @right.allowed?("/files/repository/myfile/other", :args) + acl.should == @regex_acl1 end it "should not raise an error if no regex acl match" do @@ -321,7 +344,8 @@ describe Puppet::Network::Rights do end it "should return false if no regex match" do - @right.allowed?("/path", :args).should be_false + res, acl = @right.allowed?("/path", :args) + res.should be_false end end @@ -403,14 +427,16 @@ describe Puppet::Network::Rights do it "should return :dunno if this right is not restricted to the given method" do @acl.restrict_method(:destroy) - @acl.allowed?("me","127.0.0.1", :save).should == :dunno + res,acl = @acl.allowed?("me","127.0.0.1", :save) + res.should == :dunno end it "should return allow/deny if this right is restricted to the given method" do @acl.restrict_method(:save) @acl.allow("127.0.0.1") - @acl.allowed?("me","127.0.0.1", :save).should be_true + res, acl = @acl.allowed?("me","127.0.0.1", :save) + res.should be_true end it "should return :dunno if this right is not restricted to the given environment" do @@ -418,7 +444,8 @@ describe Puppet::Network::Rights do @acl.restrict_environment(:production) - @acl.allowed?("me","127.0.0.1", :save, :development).should == :dunno + res, acl = @acl.allowed?("me","127.0.0.1", :save, :development) + res.should == :dunno end it "should interpolate allow/deny patterns with the given match" do diff --git a/test/network/rights.rb b/test/network/rights.rb index e1d9f8a..89c6797 100755 --- a/test/network/rights.rb +++ b/test/network/rights.rb @@ -23,9 +23,13 @@ class TestRights < Test::Unit::TestCase @store.newright(:write) } - assert(! @store.allowed?(:write, "host.madstop.com", "0.0.0.0"), + res, acl = @store.allowed?(:write, "host.madstop.com", "0.0.0.0") + assert(! res, "Defaulted to allowing access") + assert(! acl.nil?, + "returned matching rights is not nil") + assert_nothing_raised { @store[:write].info "This is a log message" } -- 1.6.0.2 --~--~---------~--~----~------------~-------~--~----~ You received this message because you are subscribed to the Google Groups "Puppet Developers" group. To post to this group, send email to [email protected] To unsubscribe from this group, send email to [email protected] For more options, visit this group at http://groups.google.com/group/puppet-dev?hl=en -~----------~----~----~----~------~----~------~--~---
