On 13/04/09 22:25, Luke Kanies wrote: > Hi Brice, > > This is the only commit in this series I've got comments remaining on, > and I want to stress: These comments are optional, and the goal with > them is that the code be clearer and that less of the code knows about > other parts of the code. If you think my recommendations don't result > in cleaner, more maintainable code, then please ignore them. You're > the one writing the code, so you're the in best position to judge what > the most maintainable form is.
No, I think you're right. I'll see what's possible. > On Apr 11, 2009, at 12:39 PM, Brice Figureau wrote: > >> 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) > > I think it's a bit weird that you're passing what is otherwise a > private instance (the acl) up the chain. I was sure you would argue that wasn't a good idea :-) > I feel bad that I keeping > pushing you on this, since it's a relatively small thing, but I think > something like this might be more appropriate: > > if right = @rights[name] || @rights[namespace] > right.fail_unless_allowed(request.name, request.ip) > else > raise AuthorizationError, "Access to %s denied by default" % [....] > end Yes, this works for this part of the code which only checks for the so-called namespace rights, because there we can find easily the matching right and it is unique. For the REST authorization, we have to cycle through all the sub-rights before finding the only one that matches. What you propose can't be done without breaking completely the current allowed? API. In fact I tried to keep the allowed? API entry we already have. Now if we are willing to break this API (and raise on deny as deeply as in the rights instance), then that'd really simpler. I can push the check code even deeper than it is now. > That way the ACL isn't exposed anywhere it shouldn't be, and the ACL's > failure method can take into account any info it does or doesn't have > available: > > def fail_unless_allowed(name, ip) > return if allowed?(name, ip) > msg = "Access to %s denied to %s(%s)" % [path, name, ip] > msg += " in file %s" % file if file > msg += " at line %s" % line if line > > raise AuthorizationError, msg > end I think I can move fail_unless_allowed in Rights (not Rights::Right), without breaking allowed? too much. Hmm, let me try this. I'll repost (only) this patch when done to see if that's better for you. > Note that Puppet::Error actually has line/file attributes, and it will > add them automatically, so if your AuthorizationError inherits from > it, you could do: > > ... > error = Puppet::Error.new("Access....") shouldn't it be: error = AuthorizationError.new(...) ? > error.line = self.line > error.file = self.file > raise error > > Then... (below)... > >> 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] > > Keeping in mind that you've got to inform both sides of the connection > (server and client), I'd prefer if each side got exactly the same > information. And shouldn't the server side should get the log as a > 'warning', rather than notice? Fair enough. Note that it is a notice in the namespaceauth code, so I kept it. Warning makes more sense I think. > Basically, I would do something like: > > Puppet.warning "Denying " + msg > raise AuthorizationError, "Request forbidden: %s" % msg > > I expect this will entail a slight separation between the server and > client messages, but they should be able to be almost the same. OK. > I just know that, for some reason, people tend not to look at server > logs when they see exceptions on the client, so we have to both log > useful info on the server *and* propagate that same info to the client. OK. > If you follow my advice from above (failing locally), then you could > just do something like this: > > begin > ....perform authorization checks.... > rescue AuthorizationError => error > # Log on the server side > Puppet.warning "Client %s(%s) denied: %s" % [request.name, > request.ip, error] > raise # send the error back to the client > end > >> 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 >> >> > > -- Brice Figureau Days of Wonder http://www.daysofwonder.com --~--~---------~--~----~------------~-------~--~----~ 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 -~----------~----~----~----~------~----~------~--~---
