Prefer certname or URI when compiling?

[Brice Figureau]

Hi,

When a node wants a catalog it sends the following REST uri:

GET /environment/catalog/<node name>

But the catalog compiler terminus (see
lib/puppet/indirector/catalog/compiler.rb) prefers to trust the given
node certname over the one in the URI.

This means a given node can only gets its own catalog as given in the
certificate.
This is good for security, even though the default shipped auth.conf
already does about the same with:

path ~ ^/catalog/([^/]+)$
method find
allow $1

which only allows the sending node to ask for its own catalog.

The issue is that this forces puppet-load to request only one catalog
for all its simulated clients, which is too bad.

I plan to add multi-node clients to puppet-load, but for this I need
puppet to compile the catalog for the node given in the URI and not the
certname. This would allow to have only one cert for puppet-load that
allows to compile every node:

path ~ ^/catalog/([^/]+)$
method find
allow $1
allow puppet-load.domain.com

Of course, this is a security issue, but I'm sure puppet-load users are
well aware of this and would do the necessary to never run this on
production masters.

So, I guess this is an Request for Comment about changing this behavior.
Thanks,
--
Brice Figureau
My Blog: http://www.masterzen.fr/

[Eric Sorenson]

Brice +1 on this idea as it helps with another use case I have: i'd like my monitoring server to be able to request catalogs for clients other than itself. This would help make sure that we're not failing catalog compilation for classes that might not be included in the monitoring server's node definition.

> --
> You received this message because you are subscribed to the Google Groups "Puppet Developers" group.
> To post to this group, send email to puppe...@googlegroups.com.
> To unsubscribe from this group, send email to puppet-dev+...@googlegroups.com.
> For more options, visit this group at http://groups.google.com/group/puppet-dev?hl=en.
>

[Trevor Vaughan]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Could you make this behavior a configurable option?

Safe by default and unsafe when explicitly told to be.

Trevor

- --
Trevor Vaughan
Vice President, Onyx Point, Inc.
email: tvau...@onyxpoint.com
phone: 410-541-ONYX (6699)
pgp: 0x6C701E94

- -- This account not approved for unencrypted sensitive information --
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (GNU/Linux)

iQEcBAEBAgAGBQJMqZxGAAoJECNCGV1OLcyp7ZwH/Rv9tI7AZttCmEzEd/xiZLs7
qQZpybJfT8F0w3l3f+lkIDxYqkjsfVBe5Aa+MPuy+gb38+N8DTa/D4UYv5YgldgR
hFod0d8SThBtrpUcJIYaBBoLbKtR8Ztd0Ft31vuR6Bk9A7W+TwJtNfdB05tBojTo
KENX5uQ59FgCenkrf67Jmt36sVvM2by+HOzN+9R4IwjXg/DZxqmbu3OaZeEVP1YW
+cjC04jd6xpSSycxwAfNIVY9znuZtlHQDztYE3bfp8tTvciWllFS9qWcFwViNqxE
PjgK8ampzVM7iIPK7sl7mYwwJH6Af1VtQLyNJUJiMMBhYH6j32bW8p3ECL5B6AA=
=IebS
-----END PGP SIGNATURE-----

[David Schmitt]

On 10/4/2010 11:20 AM, Trevor Vaughan wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> Could you make this behavior a configurable option?
>
> Safe by default and unsafe when explicitly told to be.

IIUIC the code does only replicate the default settings in auth. Thus
removing the extra check in the code would still leave the default
protection in the auth.conf, but would enable Brice to remove it for his
load testing.

Brice, please correct me if I misunderstood the situation.

Best Regards, David

--
dasz.at OG Tel: +43 (0)664 2602670 Web: http://dasz.at
Klosterneuburg UID: ATU64260999

FB-Nr.: FN 309285 g FB-Gericht: LG Korneuburg

[Luke Kanies]

I think this is a good idea. As you say, it should be an authorization issue, not a code or configuration issue, and auth.conf already handles that effectively.

Before you submit a patch for it, can you confirm that the default auth.conf configuration does not allow a host to request someone else's catalog? I know it's not supposed to, but it'd be nice to get some manaul validation as part of the patch.

Thanks,
Luke

--
Those who speak most of progress measure it by quantity and not
by quality. --George Santayana
---------------------------------------------------------------------
Luke Kanies -|- http://puppetlabs.com -|- +1(615)594-8199

[Jeff McCune]

On Sun, Oct 3, 2010 at 2:02 AM, Brice Figureau
<brice-...@daysofwonder.com> wrote:
> This means a given node can only gets its own catalog as given in the
> certificate.

Brice,

Have you tried to set the puppet master's node_name config setting to
"facter" and then have puppet-load.rb "lie" about it's fqdn fact when
it requests a catalog?

node_name defaults to "cert" which sounds like the cause of the
trouble you're running into.

This will allow one certificate to be used to request arbitrary node
catalogs and shouldn't require any modifications to puppet.

Perhaps I misunderstand the problem though.

Hope this helps,
--
Jeff McCune
http://www.puppetlabs.com/

[Jeff McCune]

On Tue, Oct 5, 2010 at 8:24 PM, Jeff McCune <je...@puppetlabs.com> wrote:
> On Sun, Oct 3, 2010 at 2:02 AM, Brice Figureau
> <brice-...@daysofwonder.com> wrote:
>> This means a given node can only gets its own catalog as given in the
>> certificate.
>
> Brice,
>
> Have you tried to set the puppet master's node_name config setting to
> "facter" and then have puppet-load.rb "lie" about it's fqdn fact when
> it requests a catalog?

Eh, nevermind... node_name isn't the right place to solve this.

I think you're totally right, the catalog terminus should prefer the
name given in the URI and the authorization system should allow or
deny the request ahead of time.

[Brice Figureau]

Yes, both the default shipped auth.conf and the default rule we insert
if there is none are correctly secured.

[Brice Figureau]

On 06/10/10 05:27, Jeff McCune wrote:
> On Tue, Oct 5, 2010 at 8:24 PM, Jeff McCune <je...@puppetlabs.com> wrote:
>> On Sun, Oct 3, 2010 at 2:02 AM, Brice Figureau
>> <brice-...@daysofwonder.com> wrote:
>>> This means a given node can only gets its own catalog as given in the
>>> certificate.
>>
>> Brice,
>>
>> Have you tried to set the puppet master's node_name config setting to
>> "facter" and then have puppet-load.rb "lie" about it's fqdn fact when
>> it requests a catalog?
>
> Eh, nevermind... node_name isn't the right place to solve this.

Correct.

> I think you're totally right, the catalog terminus should prefer the
> name given in the URI and the authorization system should allow or
> deny the request ahead of time.

I'll send a patch next week.

[Brice Figureau]

This is a behavior change. Before this patch, we always used the currently
connected node's certname to compile the catalog, despite the value of
the catalog URI REST request.
With this patch we now use the URI as the compiled node name.

This is safe because the default auth.conf (and default inserted rules
when no auth.conf is present) only allow the given connected node to
compile its own catalog.

But this also allows for greater flexibility with auth.conf. For instance
it can be used by a monitoring system to check multiple nodes catalogs
with only one certificate:

path ~ ^/catalog/([^/]+)$
method find
allow $1

allow monitoring-station.domain.com

Signed-off-by: Brice Figureau <brice-...@daysofwonder.com>
---
lib/puppet/indirector/catalog/compiler.rb | 12 ++++++++----
spec/unit/indirector/catalog/compiler_spec.rb | 17 +++++++++--------
2 files changed, 17 insertions(+), 12 deletions(-)

diff --git a/lib/puppet/indirector/catalog/compiler.rb b/lib/puppet/indirector/catalog/compiler.rb
index c50022f..6375e80 100644
--- a/lib/puppet/indirector/catalog/compiler.rb
+++ b/lib/puppet/indirector/catalog/compiler.rb
@@ -107,10 +107,14 @@ class Puppet::Resource::Catalog::Compiler < Puppet::Indirector::Code
return node
end

- # If the request is authenticated, then the 'node' info will
- # be available; if not, then we use the passed-in key. We rely
- # on our authorization system to determine whether this is allowed.
- name = request.node || request.key
+ # We rely on our authorization system to determine whether the connected
+ # node is allowed to compile the catalog's node referenced by key.
+ # By default the REST authorization system makes sure only the connected node
+ # can compile his catalog.
+ # This allows for instance monitoring systems or puppet-load to check several
+ # node's catalog with only one certificate and a modification to auth.conf
+ # If no key is provided we can only compile the currently connected node.
+ name = request.key || request.node
if node = find_node(name)
return node
end
diff --git a/spec/unit/indirector/catalog/compiler_spec.rb b/spec/unit/indirector/catalog/compiler_spec.rb
index 2ae5f6f..6c950b6 100755
--- a/spec/unit/indirector/catalog/compiler_spec.rb
+++ b/spec/unit/indirector/catalog/compiler_spec.rb
@@ -6,6 +6,7 @@
require File.dirname(__FILE__) + '/../../../spec_helper'

require 'puppet/indirector/catalog/compiler'
+require 'puppet/rails'

describe Puppet::Resource::Catalog::Compiler do
before do
@@ -33,8 +34,8 @@ describe Puppet::Resource::Catalog::Compiler do
Puppet::Node.stubs(:find).with('node1').returns(node1)
Puppet::Node.stubs(:find).with('node2').returns(node2)

- compiler.find(stub('request', :node => 'node1', :options => {}))
- compiler.find(stub('node2request', :node => 'node2', :options => {}))
+ compiler.find(stub('request', :key => 'node1', :node => 'node1', :options => {}))
+ compiler.find(stub('node2request', :key => 'node2', :node => 'node2', :options => {}))
end

it "should provide a method for determining if the catalog is networked" do
@@ -70,7 +71,7 @@ describe Puppet::Resource::Catalog::Compiler do
@node = Puppet::Node.new @name
@node.stubs(:merge)
Puppet::Node.stubs(:find).returns @node
- @request = stub 'request', :key => "does not matter", :node => @name, :options => {}
+ @request = stub 'request', :key => @name, :node => @name, :options => {}
end

it "should directly use provided nodes" do
@@ -80,14 +81,14 @@ describe Puppet::Resource::Catalog::Compiler do
@compiler.find(@request)
end

- it "should use the request's node name if no explicit node is provided" do
+ it "should use the authenticated node name if no request key is provided" do
+ @request.stubs(:key).returns(nil)
Puppet::Node.expects(:find).with(@name).returns(@node)
@compiler.expects(:compile).with(@node)
@compiler.find(@request)
end

- it "should use the provided node name if no explicit node is provided and no authenticated node information is available" do
- @request.expects(:node).returns nil
+ it "should use the provided node name by default" do
@request.expects(:key).returns "my_node"

Puppet::Node.expects(:find).with("my_node").returns @node
@@ -198,7 +199,7 @@ describe Puppet::Resource::Catalog::Compiler do
@compiler = Puppet::Resource::Catalog::Compiler.new
@name = "me"
@node = mock 'node'
- @request = stub 'request', :node => @name, :options => {}
+ @request = stub 'request', :key => @name, :options => {}
@compiler.stubs(:compile)
end

@@ -217,7 +218,7 @@ describe Puppet::Resource::Catalog::Compiler do
@compiler = Puppet::Resource::Catalog::Compiler.new
@name = "me"
@node = mock 'node'
- @request = stub 'request', :node => @name, :options => {}
+ @request = stub 'request', :key => @name, :options => {}
@compiler.stubs(:compile)
Puppet::Node.stubs(:find).with(@name).returns(@node)
end
--
1.7.2.1

[Matt Robinson]

Brice, this does allow you to do what you want, but for some reason it
breaks security. You say:

This is safe because the default auth.conf (and default inserted rules
when no auth.conf is present) only allow the given connected node to
compile its own catalog.

However, when I tested this with the default auth.conf I was able to
get catalogs other than the one for the connecting node. Not sure
why. Without the patch:

err: Forbidden request: localhost(127.0.0.1) access to
/catalog/othernodename [find] at line 93

[Brice Figureau]

Hi Matt,

On Mon, 2010-10-25 at 16:25 -0700, Matt Robinson wrote:
> Brice, this does allow you to do what you want, but for some reason it
> breaks security. You say:
>
> This is safe because the default auth.conf (and default inserted rules
> when no auth.conf is present) only allow the given connected node to
> compile its own catalog.
>
> However, when I tested this with the default auth.conf I was able to
> get catalogs other than the one for the connecting node. Not sure
> why. Without the patch:
>
> err: Forbidden request: localhost(127.0.0.1) access to
> /catalog/othernodename [find] at line 93

This is strange because this authorization check is done way before the
catalog compilation terminus is involved (where my modification is).

Something I don't get is your above error message:
* it says your node is _not_ authenticated, why is that?
* it gives the default deny catch all as the rule that triggered

When I'm trying with the default auth.conf on my branch, here's what I
get (this is with puppet-load, but that shouldn't matter):

err: Forbidden request: puppet-load.domain.com(127.0.1.1) access
to /catalog/corp.domain.com [find] authenticated at line 52

You can see that my node was authenticated, and that the catalog acl
rule triggered.

So I did a manual test (ie openssl s_client) pretending to be
unauthenticated and was still not able to access any catalogs (with
default auth.conf or without).
Then I tried being authenticated, requesting a catalog for a different
node and wasn't able to get one either (with the default auth.conf or
without).

How did you perform your tests?
Was it with a real puppetd (in which case how did you instruct it to
require a catalog for another host)?
Can you describe your test setup?

All my tests were done with a nginx+mongrel setup, and repeated under
webrick.
--
Brice Figureau
Follow the latest Puppet Community evolutions on www.planetpuppet.org!

[Matt Robinson]

Brice,
Looks like I messed up testing the #5020 patch because the helper
script I was using to start the puppetmaster was copying an overly
permissive auth.conf in. This was a relic of when I was documenting
the REST API and wanted to not have to worry about security rules. I
was testing the 2.6.x branch without this auth.conf on a different
machine, so that's why it looked like your branch introduced the
permission problem.

So +1 to this patch.

On Tue, Oct 26, 2010 at 3:13 AM, Brice Figureau
<brice-...@daysofwonder.com> wrote:
> This is strange because this authorization check is done way before the
> catalog compilation terminus is involved (where my modification is).

This is confused me too. I should've followed code to see how it
could have gotten by the early authorization.

> How did you perform your tests?

Using curl to do the requests
Using a script to start the puppetmaster (this is where my problem was)

[Markus Roberts]

Matt --

> Looks like I messed up testing the #5020 patch because the helper

script I was using to start the puppetmaster was copying an overly
permissive auth.conf in.  This was a relic of when I was documenting
the REST API and wanted to not have to worry about security rules.  I
was testing the 2.6.x branch without this auth.conf on a different
machine, so that's why it looked like your branch introduced the
permission problem.

Thank you for that explanation.  It's been bugging me all day trying to figure out how you could have gotten the results you reported!

-- Markus

-----------------------------------------------------------
The power of accurate observation is
commonly called cynicism by those
who have not got it.  ~George Bernard Shaw
------------------------------------------------------------

[Brice Figureau]

Hi Matt,

On Tue, 2010-10-26 at 15:42 -0700, Matt Robinson wrote:
> Brice,
> Looks like I messed up testing the #5020 patch because the helper
> script I was using to start the puppetmaster was copying an overly
> permissive auth.conf in. This was a relic of when I was documenting
> the REST API and wanted to not have to worry about security rules. I
> was testing the 2.6.x branch without this auth.conf on a different
> machine, so that's why it looked like your branch introduced the
> permission problem.
>
> So +1 to this patch.

Thanks for the explanation!

> On Tue, Oct 26, 2010 at 3:13 AM, Brice Figureau
> <brice-...@daysofwonder.com> wrote:
> > This is strange because this authorization check is done way before the
> > catalog compilation terminus is involved (where my modification is).
>
> This is confused me too. I should've followed code to see how it
> could have gotten by the early authorization.
>
> > How did you perform your tests?
>
> Using curl to do the requests
> Using a script to start the puppetmaster (this is where my problem was)

OK, makes sense.