Very nice. A couple of comments below. On Apr 19, 2009, at 11:38 AM, Brice Figureau wrote:
> > Before this change, unauthenticated REST requests where > inconditionnaly > allowed, as long as they were to the certificate terminus. > This could be a security hole, so now the REST requests, authenticated > or unauthenticated are all submitted to the REST authorization > layer. > The default authorizations now contains directives to allow > unauthenticated > requests to the various certificate terminus to allow new hosts. > The conf/auth.conf file has been modified to match such defaults. > > > Signed-off-by: Brice Figureau <[email protected]> > --- > conf/auth.conf | 33 +++++++++++++++++++++ > lib/puppet/network/rest_authconfig.rb | 21 ++++++++----- > lib/puppet/network/rest_authorization.rb | 21 +------------- > spec/unit/network/rest_authconfig.rb | 25 +++++++++++----- > spec/unit/network/rest_authorization.rb | 46 +++++ > +----------------------- > 5 files changed, 74 insertions(+), 72 deletions(-) > > diff --git a/conf/auth.conf b/conf/auth.conf > index 784acc9..d42a347 100644 > --- a/conf/auth.conf > +++ b/conf/auth.conf > @@ -11,6 +11,7 @@ > # path /path/to/resource > # [environment envlist] > # [method methodlist] > +# [auth[enthicated] {yes|no|on|off}] > # allow [host|ip|*] > # deny [host|ip] > # > @@ -24,6 +25,7 @@ > # path ~ regex > # [environment envlist] > # [method methodlist] > +# [auth[enthicated] {yes|no|on|off}] > # allow [host|ip|*] > # deny [host|ip] > # > @@ -36,24 +38,35 @@ > # path ~ ^/path/to/resource > # is essentially equivalent to path /path/to/resource > # > +# environment:: restrict an ACL to a specific set of environments > +# method:: restrict an ACL to a specific set of methods > +# auth:: restrict an ACL to an authenticated or unauthenticated > request > +# > > +### Authenticated ACL - those applies only when the client > +### has a valid certificate and is thus authenticated > + > # allow nodes to drop and find their facts > path /facts > +auth yes > method save,find > allow * > > # allow all nodes to get their catalogs (ie their configuration) > path /catalog > +auth yes > method find > allow * > > # allow all nodes to access the certificates services > path /certificate > +auth yes > method find > allow * It *looks* like there's no default value for 'auth', but it might be a good idea for it to default to 'yes'. I expect most people will assume that a given rule will only apply to authenticated users, so it's probably safest to default to that, and then have this switch that allows them to say that a given rule applies to unauthenticated users. > > # allow all nodes to store their reports > path /report > +auth yes > method save > allow * > > @@ -61,6 +74,26 @@ allow * > # which means in practice that fileserver.conf will > # still be used > path /file > +auth yes > +allow * > + > +### Unauthenticated ACL, for clients for which the current master > doesn't > +### have a valid certificate > + > +# allow access to the master CA > +path /certificate/ca > +auth no > +method find > +allow * > + > +path /certificate/ > +auth no > +method find > +allow * > + > +path /certificate_request > +auth no > +method find, save > allow * > > # this one is not stricly necessary, but it has the merit > diff --git a/lib/puppet/network/rest_authconfig.rb b/lib/puppet/ > network/rest_authconfig.rb > index 22be8b0..2b15b96 100644 > --- a/lib/puppet/network/rest_authconfig.rb > +++ b/lib/puppet/network/rest_authconfig.rb > @@ -6,13 +6,16 @@ module Puppet > attr_accessor :rights > > DEFAULT_ACL = { > - :facts => { :acl => "/facts", :method => > [:save, :find] }, > - :catalog => { :acl => "/catalog", :method => :find }, > + :facts => { :acl => "/facts", :method => > [:save, :find], :authenticated => true }, I'm not sure 'save' should be in here by default; this basically allows any host to get any other host's facts, and that's probably not what people want. > > + :catalog => { :acl => "/catalog", :method > => :find, :authenticated => true }, This is an interesting one - we've got code internally that does this for us, but basically, hosts can't actually get any catalog other than their own, even if they're theoretically authorized. The compiler terminus just ignores the request key, replacing it with the authentication name. How hard would it be for this ACE to specify that hosts can only retrieve their own catalogs? Then we could ditch the internal code that uses the authentication name instead of the request, and people would get more useful errors if they tried to do something that isn't currently allowed. Man, it's *much* nicer having a real auth system. :) FTR, this is fine as is, but if we leave it, we should file a bug for 0.26 to implement this. > > # this one will allow all file access, and thus delegate > # to fileserver.conf > :file => { :acl => "/file" }, > - :cert => { :acl => "/certificate", :method => :find }, > - :reports => { :acl => "/report", :method => :save } > + :cert => { :acl => "/certificate", :method > => :find, :authenticated => true }, > + :reports => { :acl => "/report", :method > => :save, :authenticated => true }, > + :cert_ca => { :acl => "/certificate/ca", :method > => :find, :authenticated => false }, Why is this extra entry necessary? > > + :get_cert =>{ :acl => "/certificate/", :method > => :find, :authenticated => false }, > + :cert_request =>{ :acl => "/ > certificate_request", :method => [:find, :save], :authenticated => > false }, > } > > def self.main > @@ -32,7 +35,8 @@ module Puppet > :node => request.node, > :ip => request.ip, > :method => request.method, > - :environment => > request.environment) > + :environment => > request.environment, > + :authenticated => > request.authenticated) > end > > def initialize(file = nil, parsenow = true) > @@ -52,8 +56,8 @@ module Puppet > def insert_default_acl > DEFAULT_ACL.each do |name, acl| > unless rights[acl[:acl]] > - Puppet.warning "Inserting default > '#{acl[:acl]}' acl because none were found in '%s'" % ( @file || "no > file configured") > - mk_acl(acl[:acl], acl[:method]) > + Puppet.warning "Inserting default > '#{acl[:acl]}'(%s) acl because none were found in '%s'" % > [acl[:authenticated] ? "auth" : "non-auth" , ( @file || "no file > configured")] > + mk_acl(acl[:acl], acl[:method], > acl[:authenticated]) > end > end > # queue an empty (ie deny all) right for every other path > @@ -62,7 +66,7 @@ module Puppet > rights.newright("/") unless rights["/"] > end > > - def mk_acl(path, method = nil) > + def mk_acl(path, method = nil, authenticated = nil) > @rights.newright(path) > @rights.allow(path, "*") > > @@ -70,6 +74,7 @@ module Puppet > method = [method] unless method.is_a?(Array) > method.each { |m| @rights.restrict_method(path, m) } > end > + @rights.restrict_authenticated(path, authenticated) > unless authenticated.nil? > end > > def build_uri(request) > diff --git a/lib/puppet/network/rest_authorization.rb b/lib/puppet/ > network/rest_authorization.rb > index e6f62d9..1294a3d 100644 > --- a/lib/puppet/network/rest_authorization.rb > +++ b/lib/puppet/network/rest_authorization.rb > @@ -16,29 +16,10 @@ module Puppet::Network > @authconfig > end > > - # Verify that our client has access. We allow untrusted > access to > - # certificates terminus but no others. > + # Verify that our client has access. > def check_authorization(request) > - if request.authenticated? > - authenticated_authorized?(request) > - else > - 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 > - end > - > - # delegate to our authorization file > - def authenticated_authorized?(request) > authconfig.allowed?(request) > end > - > - # allow only certificate requests when not authenticated > - def unauthenticated_authorized?(request) > - request.indirection_name == :certificate or > request.indirection_name == :certificate_request > - end > end > end > > diff --git a/spec/unit/network/rest_authconfig.rb b/spec/unit/ > network/rest_authconfig.rb > index 7dc9738..7f6817a 100644 > --- a/spec/unit/network/rest_authconfig.rb > +++ b/spec/unit/network/rest_authconfig.rb > @@ -16,8 +16,8 @@ describe Puppet::Network::RestAuthConfig do > @acl = stub_everything 'rights' > @authconfig.rights = @acl > > - @request = stub 'request', :indirection_name => > "path", :key => "to/resource", :ip => "127.0.0.1", > - :node => "me", :method > => :save, :environment => :env > + @request = stub 'request', :indirection_name => > "path", :key => "to/resource", :ip => "127.0.0.1", > + :node => "me", :method > => :save, :environment => :env, :authenticated => true > end > > it "should use the puppet default rest authorization file" do > @@ -33,7 +33,7 @@ describe Puppet::Network::RestAuthConfig do > end > > it "should ask for authorization to the ACL subsystem" do > - @acl.expects(:fail_on_deny).with("/path/to/resource", :node > => "me", :ip => "127.0.0.1", :method => :save, :environment => :env) > + @acl.expects(:fail_on_deny).with("/path/to/resource", :node > => "me", :ip => "127.0.0.1", :method => :save, :environment > => :env, :authenticated => true) > > @authconfig.allowed?(@request) > end > @@ -53,6 +53,11 @@ describe Puppet::Network::RestAuthConfig do > @acl.expects(:restrict_method).with(:path, :method) > @authconfig.mk_acl(:path, :method) > end > + > + it "should restrict the ACL to a specific authentication > state" do > + > @acl.expects(:restrict_authenticated).with(:path, :authentication) > + @authconfig.mk_acl(:path, nil, :authentication) > + end > end > > describe "when parsing the configuration file" do > @@ -70,7 +75,7 @@ describe Puppet::Network::RestAuthConfig do > @authconfig.rights.stubs(:[]).returns(true) > @authconfig.rights.stubs(:[]).with(acl).returns(nil) > > - @authconfig.expects(:mk_acl).with { |a,m| a == acl } > + @authconfig.expects(:mk_acl).with { |a,m,auth| a == acl } > > @authconfig.insert_default_acl > end > @@ -96,14 +101,18 @@ describe Puppet::Network::RestAuthConfig do > describe "when adding default ACLs" do > > [ > - { :acl => "/facts", :method => [:save, :find] }, > - { :acl => "/catalog", :method => :find }, > - { :acl => "/report", :method => :save }, > + { :acl => "/facts", :method => > [:save, :find], :authenticated => true }, > + { :acl => "/catalog", :method => :find, :authenticated > => true }, > { :acl => "/file" }, > + { :acl => "/certificate", :method > => :find, :authenticated => true }, > + { :acl => "/report", :method => :save, :authenticated > => true }, > + { :acl => "/certificate/ca", :method > => :find, :authenticated => false }, > + { :acl => "/certificate/", :method > => :find, :authenticated => false }, > + { :acl => "/certificate_request", :method => > [:find, :save], :authenticated => false } > ].each do |acl| > it "should create a default right for #{acl[:acl]}" do > @authconfig.stubs(:mk_acl) > - @authconfig.expects(:mk_acl).with(acl[:acl], > acl[:method]) > + @authconfig.expects(:mk_acl).with(acl[:acl], > acl[:method], acl[:authenticated]) > @authconfig.insert_default_acl > end > end > diff --git a/spec/unit/network/rest_authorization.rb b/spec/unit/ > network/rest_authorization.rb > index 3acd23f..db42dd7 100644 > --- a/spec/unit/network/rest_authorization.rb > +++ b/spec/unit/network/rest_authorization.rb > @@ -22,48 +22,22 @@ describe Puppet::Network::RestAuthorization do > end > > describe "when testing request authorization" do > - describe "when the client is not authenticated" do > - before :each do > - @request.stubs(:authenticated?).returns(false) > - end > + it "should delegate to the current rest authconfig" do > + > @authconfig.expects(:allowed?).with(@request).returns(true) > > - [ :certificate, :certificate_request].each do | > indirection| > - it "should allow #{indirection}" do > - > @request.stubs(:indirection_name).returns(indirection) > - 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) > - lambda > { @auth.check_authorization(@request) }.should raise_error > Puppet::Network::AuthorizationError > - end > - end > + @auth.check_authorization(@request) > end > > - describe "when the client is authenticated" do > - before :each do > - @request.stubs(:authenticated?).returns(true) > - end > - > - it "should delegate to the current rest authconfig" do > - > @authconfig.expects(:allowed?).with(@request).returns(true) > - > - @auth.check_authorization(@request) > - end > + it "should raise an AuthorizationError if authconfig raises > an AuthorizationError" do > + > @authconfig > .expects > (:allowed > ?).with > (@request > ).raises(Puppet::Network::AuthorizationError.new("forbidden")) > > - it "should raise an AuthorizationError if authconfig > raises an AuthorizationError" do > - > @authconfig > .expects > (:allowed > ?).with > (@request > ).raises(Puppet::Network::AuthorizationError.new("forbidden")) > - > - lambda > { @auth.check_authorization(@request) }.should raise_error > Puppet::Network::AuthorizationError > - end > + 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) > + 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 > + lambda > { @auth.check_authorization(@request) }.should_not raise_error > Puppet::Network::AuthorizationError > end > end > end > \ No newline at end of file > -- > 1.6.0.2 > > > > -- Don't throw away the old bucket until you know whether the new one holds water. -- Swedish Proverb --------------------------------------------------------------------- Luke Kanies | http://reductivelabs.com | http://madstop.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 -~----------~----~----~----~------~----~------~--~---
