On Apr 20, 2009, at 2:06 AM, Brice Figureau wrote:
>
> 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?
Der, you're right, I meant 'find'.
I should be merging today the code that makes 'save' unnecessary, too,
since Catalog.find will include the facts in the request.
>
> 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.
Ah. That'd actually be great documentation - who needs exactly what
rights. :)
>
>>>
>>> + :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.
That would be awesome; it would move some code out of the Catalog
support classes to where it belongs: in the auth classes.
>
>> 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.
I've generally playd caveat programmer here; we can guarantee the
default permissions, but once you start screwing with them you better
know what you're doing.
Without this move, people are limited and *can't* allow one host to
retrieve another host's catalog, which isn't good either.
>
>> 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.
Sounds good. This can easily wait, because it's a good refactor
rather than a critical change or even a new feature.
>
>>>
>>> # 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
>>>
>>>
>>>>
>>
>
>
>
> >
--
Hollywood is a place where they'll pay you a thousand dollars for a
kiss and fifty cents for your soul. -- Marilyn Monroe
---------------------------------------------------------------------
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
-~----------~----~----~----~------~----~------~--~---
