Brice has actually taken this over.

Sounds like this is almost ready, just a couple of small changes left.

In testing this, I think that it makes more sense to have something like a default configuration for authenticated connections, and then a different default configuration for unauthenticated connections.

From what I can tell of the current configuration, any unauthenticated client can save certificates to the server, which I think is a pretty significant security hole. The only things that should be possible for unauth’d clients is to save CSRs and retrieve certificates.

luke wrote:

In testing this, I think that it makes more sense to have something like a default configuration for authenticated connections, and then a different default configuration for unauthenticated connections.

From what I can tell of the current configuration, any unauthenticated client can save certificates to the server, which I think is a pretty significant security hole. The only things that should be possible for unauth’d clients is to save CSRs and retrieve certificates.

So your suggestion would be:

to be able to restrict an ACL to an authenticated or unauthenticated request in the auth.conf file

to let the unauthenticated case use the same code as the authenticated case

to have the default set of rule define rules for authenticated and unauthenticated cases

Is that right?

luke wrote:

In testing this, I think that it makes more sense to have something like a default configuration for authenticated connections, and then a different default configuration for unauthenticated connections.

From what I can tell of the current configuration, any unauthenticated client can save certificates to the server, which I think is a pretty significant security hole. The only things that should be possible for unauth’d clients is to save CSRs and retrieve certificates.

I have an implementation (and posted it for review to the puppet-dev list) in my tickets/1875 github branch: http://github.com/masterzen/puppet/tree/tickets/1875

masterzen wrote:

luke wrote:

In testing this, I think that it makes more sense to have something like a default configuration for authenticated connections, and then a different default configuration for unauthenticated connections.

From what I can tell of the current configuration, any unauthenticated client can save certificates to the server, which I think is a pretty significant security hole. The only things that should be possible for unauth’d clients is to save CSRs and retrieve certificates.

I have an implementation (and posted it for review to the puppet-dev list) in my tickets/1875 github branch: http://github.com/masterzen/puppet/tree/tickets/1875

Latest (and hopefully) final implementation is in the tickets/master/1875 branch of my gh repository: http://github.com/masterzen/puppet/tree/tickets/master/1875

Note: it is rebased on top of lak’s tickets/master/2149, so to be applied after 2149.

I can’t seem to find an easy way to test this, but my client is apparently asking for the catalog for an unqualified host, and I’m getting an exception:

Users/luke/puppet/lib/puppet/network/authstore.rb:332:in `parse'
/Users/luke/puppet/lib/puppet/network/authstore.rb:193:in `pattern='
/Users/luke/puppet/lib/puppet/network/authstore.rb:228:in `interpolate'
/Users/luke/puppet/lib/puppet/network/authstore.rb:77:in `interpolate'
/Users/luke/puppet/lib/puppet/network/authstore.rb:76:in `collect'
/Users/luke/puppet/lib/puppet/network/authstore.rb:76:in `interpolate'
/Users/luke/puppet/lib/puppet/network/rights.rb:188:in `allowed?'
/Users/luke/puppet/lib/puppet/network/rights.rb:50:in `fail_on_deny'
/System/Library/Frameworks/Ruby.framework/Versions/1.8/usr/lib/ruby/1.8/webrick/httputils.rb:236:in `find'
/Users/luke/puppet/lib/puppet/network/rights.rb:43:in `each'
/Users/luke/puppet/lib/puppet/network/rights.rb:43:in `find'
/Users/luke/puppet/lib/puppet/network/rights.rb:43:in `fail_on_deny'
/Users/luke/puppet/lib/puppet/network/rest_authconfig.rb:36:in `allowed?'
/Users/luke/puppet/lib/puppet/network/rest_authorization.rb:21:in `check_authorization'
/Users/luke/puppet/lib/puppet/network/http/handler.rb:46:in `process'
/Users/luke/puppet/lib/puppet/network/http/webrick/rest.rb:23:in `service'
/System/Library/Frameworks/Ruby.framework/Versions/1.8/usr/lib/ruby/1.8/webrick/httpserver.rb:104:in `service'
/System/Library/Frameworks/Ruby.framework/Versions/1.8/usr/lib/ruby/1.8/webrick/httpserver.rb:65:in `run'
/System/Library/Frameworks/Ruby.framework/Versions/1.8/usr/lib/ruby/1.8/webrick/server.rb:173:in `start_thread'
/System/Library/Frameworks/Ruby.framework/Versions/1.8/usr/lib/ruby/1.8/webrick/server.rb:162:in `start'
/System/Library/Frameworks/Ruby.framework/Versions/1.8/usr/lib/ruby/1.8/webrick/server.rb:162:in `start_thread'
/System/Library/Frameworks/Ruby.framework/Versions/1.8/usr/lib/ruby/1.8/webrick/server.rb:95:in `start'
/System/Library/Frameworks/Ruby.framework/Versions/1.8/usr/lib/ruby/1.8/webrick/server.rb:92:in `each'
/System/Library/Frameworks/Ruby.framework/Versions/1.8/usr/lib/ruby/1.8/webrick/server.rb:92:in `start'
/System/Library/Frameworks/Ruby.framework/Versions/1.8/usr/lib/ruby/1.8/webrick/server.rb:23:in `start'
/System/Library/Frameworks/Ruby.framework/Versions/1.8/usr/lib/ruby/1.8/webrick/server.rb:82:in `start'
/Users/luke/puppet/lib/puppet/network/http/webrick.rb:40:in `listen'
/Users/luke/puppet/lib/puppet/network/http/webrick.rb:40:in `initialize'
/Users/luke/puppet/lib/puppet/network/http/webrick.rb:40:in `new'
/Users/luke/puppet/lib/puppet/network/http/webrick.rb:40:in `listen'
/Users/luke/puppet/lib/puppet/network/http/webrick.rb:37:in `synchronize'
/Users/luke/puppet/lib/puppet/network/http/webrick.rb:37:in `listen'
/Users/luke/puppet/lib/puppet/network/server.rb:128:in `listen'
/Users/luke/puppet/lib/puppet/network/server.rb:143:in `start'
/Users/luke/puppet/lib/puppet/daemon.rb:128:in `start'
/Users/luke/puppet/lib/puppet/application/puppetmasterd.rb:87:in `main'
/Users/luke/puppet/lib/puppet/application.rb:226:in `send'
/Users/luke/puppet/lib/puppet/application.rb:226:in `run_command'
/Users/luke/puppet/lib/puppet/application.rb:217:in `run'
sbin/puppetmasterd:66
err: Could not check authorization: Invalid pattern phage

(Hit submit too soon.)

Note that I added an extra wrapper around the auth check to make this error read a bit better.

The only thing I’ve firmly established is that the problem occurs when my host (whose certname is other.madstop.com) asks for /catalog/phage (phage being its real name).

luke wrote:

(Hit submit too soon.)

Note that I added an extra wrapper around the auth check to make this error read a bit better.

The only thing I’ve firmly established is that the problem occurs when my host (whose certname is other.madstop.com) asks for /catalog/phage (phage being its real name).

That’s why I asked you a couple of days ago on #puppet why you used Facter.value(“hostname”) for catalog retrieval, and Puppet[:certname] for facts retrieval, because I was hitting the same issue. The real cause of the problem is that the underlying authstore requires fully qualified names, but the requests to catalog indirectors sends only the hostname, so in turns we’re trying to create a dynamic temporary allow(“hostname”), which is forbidden.

Now that I think about it, it looks like a bug, as hostnames can clearly not be unique among the various nodes connecting to a master (ie host1.domain1.com and host2.domain2.com would get the same request, and maybe the same catalog… after looking to the code, in fact no it works, because what matters is the request determined node which hopefully uses the certname).

So it’s just the catalog request that is wrong (the logic is OK), we should move from /catalog/hostname to /catalog/fqdn or /catalog/certname.

What do you think? If you’re OK, I’ll queue a patch to fix this issue.

masterzen wrote:

luke wrote:

(Hit submit too soon.)

Note that I added an extra wrapper around the auth check to make this error read a bit better.

The only thing I’ve firmly established is that the problem occurs when my host (whose certname is other.madstop.com) asks for /catalog/phage (phage being its real name).

That’s why I asked you a couple of days ago on #puppet why you used Facter.value(“hostname”) for catalog retrieval, and Puppet[:certname] for facts retrieval, because I was hitting the same issue. The real cause of the problem is that the underlying authstore requires fully qualified names, but the requests to catalog indirectors sends only the hostname, so in turns we’re trying to create a dynamic temporary allow(“hostname”), which is forbidden.

Now that I think about it, it looks like a bug, as hostnames can clearly not be unique among the various nodes connecting to a master (ie host1.domain1.com and host2.domain2.com would get the same request, and maybe the same catalog… after looking to the code, in fact no it works, because what matters is the request determined node which hopefully uses the certname).

So it’s just the catalog request that is wrong (the logic is OK), we should move from /catalog/hostname to /catalog/fqdn or /catalog/certname.

What do you think? If you’re OK, I’ll queue a patch to fix this issue.

I updated my tickets/master/1875 branch with this fix included.

It looks like this doesn’t allow blanket statements like ‘allow ’ or ‘deny ’.

I think that’s an existing limitation, though, so I’m comfortable merging as is and fixing that later.

I ran through a basic list of tests and everything seems to work, so I’m comfortable checking in as is.

Can you rebase tickets/master/1875 on current HEAD please.

luke wrote:

It looks like this doesn’t allow blanket statements like ‘allow ’ or ‘deny ’.

I think that’s an existing limitation, though, so I’m comfortable merging as is and fixing that later.

‘allow *’ works fine. But deny * is not supported. This is because of the way authstore is working (ie it sorts allow rules before deny rules and finishes with an implit deny all).

So basically, it has no sense to write: allow something deny *

and a lone: deny * is equivalent to nothing.

If we want something which would allow more complex scenarios, we need something ala Apache, with a directive to specify in which order ACE should be evaluated: order allow, deny allow this deny * Then deny would be applied after the allow.

I thought about this, but in the end I think your system works, except we are tempted to add some “deny ” as catch-all rules, which is forbidden. And not that an “allow ” is treated before any other rules, so that even if you have some denies after that they can’t be reached.

If you think we need to change this, then let’s open a ticket, I’ll be happy to work on this.

I ran through a basic list of tests and everything seems to work, so I’m comfortable checking in as is.

Thanks, I’ll rebase to latest HEAD tonight.

I rebased against the latest master, you can apply. Thanks,

Pushed in branch master.