On Sun, 2009-04-19 at 22:24 -0500, Luke Kanies wrote:
> 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.
Yes, you're right it might make more sense. I'll change the default.
> >
> > # 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.
You mean find?
You need save if you want to send the facts from the client to the
server, isn't it?
Note: I got those entries by trial an error. Starting with a fresh
master and a fresh client (without certificate), allowing all what was
denied until I could complete a full run. It is possible I missed a few
things or broaden the perm too much, though.
> >
> > + :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?
It's trivial, although the way the default entries are installed doesn't
allow it for the moment, but it's still a trivial change.
In terms of auth.conf, it would be as easy as:
path ~ ^\/catalog\/([^/]+)
auth yes
method find
allow $1
Thus only 'host.domain.com' can access to /catalog/host.domain.com and
so on.
Note: this can work only for authenticated requests, because the reverse
lookup of the ip address might not match the node name, so
unauthenticated ACL of this type might not work.
> 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.
Yes, but at the same time, if they override the rule in the auth.conf,
this ACL might not be installed and the security of the system would be
really weak.
I think I'll have to rework the default ACL system to be smarter to
detect such weakened situation (note that I don't really see how, but
you might have some ideas). At the moment, the system is pretty dumb.
> 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.
We can merge the next revision of this patch, and if I didn't implement
this, let's open a new ticket.
> >
> > # 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
> >
> >
> > >
>
--~--~---------~--~----~------------~-------~--~----~
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
-~----------~----~----~----~------~----~------~--~---
