On Apr 20, 2009, at 2:02 PM, Brice Figureau wrote:
>
> On 20/04/09 17:21, Luke Kanies wrote:
>> 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.
>
> In fact I'm not so sure about this.
>
> If we make the default as 'apply to authenticated request only',
> then we
> potentially introduce a new hole, which is people might forget to
> apply
> the ACL to both auth and unauth and leave the unauth side completely
> unprotected.
But isn't the default behaviour to deny? In which case, specifying
behaviour would only allow exactly what you specified, and everything
else would be denied, just like we would expect.
>
> This is especially true for deny all ACL like:
>
> path /
>
> which deny everything else.
>
> Right now, if you don't specify the default, the ACL applies to both
> auth and unauth kind of requests. This has the merit I think to be
> crystal clear: either specify the kind of path you want to protect
> compared to the kind of request.
>
> On the other hand, if we move toward this direction, then we need a
> new
> token to express that the ACL must not be restricted to any kind of
> authentication state, something ala:
> auth any
>
> Comments?
I'm comfortable leaving the default for 'auth' out for now and just
leaving pretty strong warnings about the implications (e.g., in an
example default file we publish). We can always add it later.
>
>>>>> # 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'.
>
> So, you mean we don't need find. I'll remove it to see if it chokes.
Right.
>
>> I should be merging today the code that makes 'save' unnecessary,
>> too,
>> since Catalog.find will include the facts in the request.
>
> About, this. I don't know the size of the facts in general, but I'm
> pretty sure every web server (or even clients) has a limit for the
> query
> size, which might produce lots of strange things if the master receive
> truncated facts. Is there a way we can alert the admin if those are
> truncated?
At first I figured we'd be fine because we're doing a similar
operation in xmlrpc, but it uses a Post while our REST support uses a
Get. However, I've not had problems so far, and I expect this to come
out in the wash, as it were, while we're testing.
If there is a limit... well, we're in some kind of trouble then,
because we're pretty limited in our options at that point.
>
>>> 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.
>
> That was my idea too.
>
>> 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.
>
> Note, that's real easy to implement. I'll try to do it later tonight.
>
>>>>> # 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
>>>>>
>>>>>
>>>
>>>
>>
>>
>
>
> --
> Brice Figureau
> Days of Wonder
> http://www.daysofwonder.com
>
> >
--
A citizen of America will cross the ocean to fight for democracy, but
won't cross the street to vote in a national election.
--Bill Vaughan
---------------------------------------------------------------------
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
-~----------~----~----~----~------~----~------~--~---
