This should be the commit that brings us to Beta 1. All tests pass, although I get some (gracefully handled) failures in tc_metrics.rb, and there is now a config file for the fileserver module, including authorization specification for it. I have also reworked error handling in the xmlrpc client and server so errors should propagate more correctly.

git-svn-id: https://reductivelabs.com/svn/puppet/trunk@594 980ebf18-57e1-0310-9a29-db15c13687c0

11 files changed, 125 insertions, 47 deletions

diff --git a/lib/puppet/client.rb b/lib/puppet/client.rb
index 67307e621..868c86f58 100644
--- a/lib/puppet/client.rb
+++ b/lib/puppet/client.rb
@@ -47,14 +47,14 @@ module Puppet
                         begin
                             call("%s.%s" % [namespace, method.to_s],*args)
                         rescue XMLRPC::FaultException => detail
-                            Puppet.err "XML Could not call %s.%s: %s" %
+                            Puppet.err "Could not call %s.%s: %s" %
                                 [namespace, method, detail.faultString]
                             raise NetworkClientError,
                                 "XMLRPC Error: %s" % detail.faultString
-                        rescue => detail
-                            Puppet.err "Could not call %s.%s: %s" %
-                                [namespace, method, detail.inspect]
-                            raise NetworkClientError.new(detail.to_s)
+                        #rescue => detail
+                        #    Puppet.err "Could not call %s.%s: %s" %
+                        #        [namespace, method, detail.inspect]
+                        #    raise NetworkClientError.new(detail.to_s)
                         end
                     }
                 }
diff --git a/lib/puppet/daemon.rb b/lib/puppet/daemon.rb
index 837debd79..040292b36 100755
--- a/lib/puppet/daemon.rb
+++ b/lib/puppet/daemon.rb
@@ -6,8 +6,10 @@ module Puppet
     module Daemon
         def daemonize
             unless Puppet[:logdest] == :file
-                Puppet.err "You must reset log destination before daemonizing"
+                raise Puppet::DevError,
+                    "You must reset log destination before daemonizing"
             end
+
             if pid = fork()
                 Process.detach(pid)
                 exit(0)
diff --git a/lib/puppet/server.rb b/lib/puppet/server.rb
index 47b53a27b..d3259e60d 100644
--- a/lib/puppet/server.rb
+++ b/lib/puppet/server.rb
@@ -32,6 +32,8 @@ module Puppet
             include Puppet::Daemon
 
             def initialize(hash = {})
+                # FIXME we should have some kind of access control here, using
+                # :RequestHandler
                 hash[:Port] ||= Puppet[:masterport]
                 hash[:Logger] ||= self.httplog
                 hash[:AccessLog] ||= [
@@ -139,7 +141,7 @@ module Puppet
 
             @name = :Status
 
-            def status(status = nil, request = nil)
+            def status(status = nil, client = nil, clientip = nil)
                 Puppet.warning "Returning status"
                 return 1
             end
@@ -150,6 +152,7 @@ module Puppet
     #---------------------------------------------------------------
 end
 
+require 'puppet/server/authstore'
 require 'puppet/server/servlet'
 require 'puppet/server/master'
 require 'puppet/server/ca'
diff --git a/lib/puppet/server/ca.rb b/lib/puppet/server/ca.rb
index 20caee9bb..669fe1290 100644
--- a/lib/puppet/server/ca.rb
+++ b/lib/puppet/server/ca.rb
@@ -67,7 +67,7 @@ class Server
 
         # our client sends us a csr, and we either store it for later signing,
         # or we sign it right away
-        def getcert(csrtext, request = nil)
+        def getcert(csrtext, client = nil, clientip = nil)
             # okay, i need to retrieve the hostname from the csr, and then
             # verify that i get the same hostname through reverse lookup or
             # something
diff --git a/lib/puppet/server/filebucket.rb b/lib/puppet/server/filebucket.rb
index fa02d967a..9c02e0794 100755
--- a/lib/puppet/server/filebucket.rb
+++ b/lib/puppet/server/filebucket.rb
@@ -60,7 +60,7 @@ class Server
         end
 
         # accept a file from a client
-        def addfile(string,path, request = nil)
+        def addfile(string,path, client = nil, clientip = nil)
             #puts "entering addfile"
             contents = Base64.decode64(string)
             #puts "string is decoded"
@@ -129,7 +129,7 @@ class Server
             return md5
         end
 
-        def getfile(md5, request = nil)
+        def getfile(md5, client = nil, clientip = nil)
             bpath, bfile, bpaths = FileBucket.paths(@bucket,md5)
 
             unless FileTest.exists?(bfile)
diff --git a/lib/puppet/server/fileserver.rb b/lib/puppet/server/fileserver.rb
index 179459f42..6eec3503d 100755
--- a/lib/puppet/server/fileserver.rb
+++ b/lib/puppet/server/fileserver.rb
@@ -1,4 +1,5 @@
 require 'puppet'
+require 'webrick/httpstatus'
 require 'cgi'
 
 module Puppet
@@ -40,18 +41,22 @@ class Server
             return obj
         end
 
-        def describe(file, request = nil)
+        def describe(file, client = nil, clientip = nil)
             mount, path = splitpath(file)
 
-            subdir = nil
-            unless subdir = subdir(mount, path)
+            unless @mounts[mount].allowed?(client, clientip)
+                raise Puppet::Server::AuthorizationError, "Cannot access %s" % mount
+            end
+
+            sdir = nil
+            unless sdir = subdir(mount, path)
                 Puppet.notice "Could not find subdirectory %s" %
                     "//%s/%s" % [mount, path]
                 return ""
             end
 
             obj = nil
-            unless obj = self.check(subdir)
+            unless obj = self.check(sdir)
                 return ""
             end
 
@@ -112,9 +117,13 @@ class Server
             end
         end
 
-        def list(dir, recurse = false, sum = "md5", request = nil)
+        def list(dir, recurse = false, client = nil, clientip = nil)
             mount, path = splitpath(dir)
 
+            unless @mounts[mount].allowed?(client, clientip)
+                raise Puppet::Server::AuthorizationError, "Cannot access %s" % mount
+            end
+
             subdir = nil
             unless subdir = subdir(mount, path)
                 Puppet.notice "Could not find subdirectory %s" %
@@ -194,11 +203,25 @@ class Server
                                 mount.path = value
                             when "allow":
                                 value.split(/\s*,\s*/).each { |val|
-                                    mount.allow(val)
+                                    begin
+                                        Puppet.info "Allowing %s access to %s" %
+                                            [val, mount.name]
+                                        mount.allow(val)
+                                    rescue AuthStoreError => detail
+                                        raise Puppet::Error, "%s at line %s of %s" %
+                                            [detail.to_s, count, @config]
+                                    end
                                 }
                             when "deny":
                                 value.split(/\s*,\s*/).each { |val|
-                                    mount.deny(val)
+                                    begin
+                                        Puppet.info "Denying %s access to %s" %
+                                            [val, mount.name]
+                                        mount.deny(val)
+                                    rescue AuthStoreError => detail
+                                        raise Puppet::Error, "%s at line %s of %s" %
+                                            [detail.to_s, count, @config]
+                                    end
                                 }
                             else
                                 raise Puppet::Error,
@@ -218,12 +241,15 @@ class Server
             end
         end
 
-        def retrieve(file, request = nil)
+        def retrieve(file, client = nil, clientip = nil)
             mount, path = splitpath(file)
 
             unless (@mounts.include?(mount))
-                # FIXME I really need some better way to pass and handle xmlrpc errors
-                raise FileServerError, "%s not mounted" % mount
+                raise Puppet::Server::FileServerError, "%s not mounted" % mount
+            end
+
+            unless @mounts[mount].allowed?(client, clientip)
+                raise Puppet::Server::AuthorizationError, "Cannot access %s" % mount
             end
 
             fpath = nil
@@ -327,30 +353,20 @@ class Server
             return dirname
         end
 
-        class Mount
+        class Mount < AuthStore
             attr_reader :path, :name
 
-            def allow(pattern)
-            end
-
-            def allowed?(host)
-            end
-
-            def deny(pattern)
-            end
-
             def initialize(name, path = nil)
                 unless name =~ %r{^\w+$}
                     raise FileServerError, "Invalid name format '%s'" % name
                 end
                 @name = name
 
-                @allow = []
-                @deny = []
-
                 if path
                     self.path = path
                 end
+
+                super()
             end
 
             def path=(path)
diff --git a/lib/puppet/server/master.rb b/lib/puppet/server/master.rb
index 00d62eb5b..b45f91210 100644
--- a/lib/puppet/server/master.rb
+++ b/lib/puppet/server/master.rb
@@ -17,6 +17,7 @@ class Server
 
         def initialize(hash = {})
 
+            # FIXME this should all be s/:File/:Manifest/g or something
             # build our AST
             @file = hash[:File] || Puppet[:manifest]
             @parser = Puppet::Parser::Parser.new()
@@ -37,8 +38,8 @@ class Server
             end
         end
 
-        def getconfig(facts, request = nil)
-            if request
+        def getconfig(facts, client = nil, clientip = nil)
+            if client
                 #Puppet.warning request.inspect
             end
             if @local
diff --git a/lib/puppet/server/servlet.rb b/lib/puppet/server/servlet.rb
index b14efe645..4c45ebc62 100644
--- a/lib/puppet/server/servlet.rb
+++ b/lib/puppet/server/servlet.rb
@@ -4,6 +4,8 @@ module Puppet
 class Server
     class ServletError < RuntimeError; end
     class Servlet < XMLRPC::WEBrickServlet
+        ERR_UNAUTHORIZED = 30
+
         attr_accessor :request
 
         # this is just a duplicate of the normal method; it's here for
@@ -12,6 +14,10 @@ class Server
             self.new(server, *options)
         end
 
+        def authorize(request, method)
+            true
+        end
+
         def initialize(server, handlers)
             #Puppet.info server.inspect
             
@@ -28,28 +34,55 @@ class Server
             }
 
             @request = nil
+            @client = nil
+            @clientip = nil
             self.set_service_hook { |obj, *args|
                 #raise "crap!"
-                if @request
-                    args.push @request
+                if @client and @clientip
+                    args.push(@client, @clientip)
                     #obj.call(args, @request)
                 end
                 begin
                     obj.call(*args)
+                rescue Puppet::Server::AuthorizationError => detail
+                    Puppet.warning obj.inspect
+                    Puppet.warning args.inspect
+                    Puppet.err "Permission denied: %s" % detail.to_s
+                    raise XMLRPC::FaultException.new(
+                        1, detail.to_s
+                    )
+                rescue Puppet::Error => detail
+                    Puppet.warning obj.inspect
+                    Puppet.warning args.inspect
+                    Puppet.err "Puppet error: %s" % detail.to_s
+                    raise XMLRPC::FaultException.new(
+                        1, detail.to_s
+                    )
                 rescue => detail
                     Puppet.warning obj.inspect
                     Puppet.warning args.inspect
                     Puppet.err "Could not call: %s" % detail.to_s
+                    raise error
                 end
             }
         end
 
         def service(request, response)
             @request = request
-            if @request.client_cert
-                Puppet.info "client cert is %s" % @request.client_cert
+            if peer = request.peeraddr
+                @client = peer[2]
+                @clientip = peer[3]
+            else
+                raise XMLRPC::FaultException.new(
+                    ERR_UNCAUGHT_EXCEPTION,
+                    "Could not retrieve client information"
+                )
+            end
+
+            if request.client_cert
+                Puppet.info "client cert is %s" % request.client_cert
             end
-            if @request.server_cert
+            if request.server_cert
                 #Puppet.info "server cert is %s" % @request.server_cert
             end
             #p @request
@@ -59,6 +92,8 @@ class Server
                 Puppet.err "Could not service request: %s: %s" %
                     [detail.class, detail]
             end
+            @client = nil
+            @clientip = nil
             @request = nil
         end
 
@@ -66,7 +101,21 @@ class Server
 
         # this is pretty much just a copy of the original method but with more
         # feedback
+        # here's where we have our authorization hooks
         def dispatch(methodname, *args)
+
+            if defined? @request and @request
+                unless self.authorize(@request, methodname)
+                    raise XMLRPC::FaultException.new(
+                        ERR_UNAUTHORIZED,
+                        "Host %s not authorized to call %s" %
+                            [@request.host, methodname]
+                    )
+                end
+            else
+                raise Puppet::DevError, "Did not get request in dispatch"
+            end
+
             #Puppet.warning "dispatch on %s called with %s" %
             #    [methodname, args.inspect]
             for name, obj in @handler
diff --git a/lib/puppet/sslcertificates.rb b/lib/puppet/sslcertificates.rb
index 5b587a41b..fd26c097a 100755
--- a/lib/puppet/sslcertificates.rb
+++ b/lib/puppet/sslcertificates.rb
@@ -92,7 +92,7 @@ module SSLCertificates
         when :server:
             basic_constraint = "CA:FALSE"
             key_usage = %w{digitalSignature keyEncipherment}
-        ext_key_usage = %w{serverAuth}
+        ext_key_usage = %w{serverAuth clientAuth}
         when :ocsp:
             basic_constraint = "CA:FALSE"
             key_usage = %w{nonRepudiation digitalSignature}
@@ -106,9 +106,6 @@ module SSLCertificates
             raise Puppet::Error, "unknown cert type '%s'" % hash[:type]
         end
 
-        Puppet.debug "Key usage is %s" % key_usage.inspect
-        Puppet.debug "ExtKey usage is %s" % ext_key_usage.inspect
-
         ex << ef.create_extension("nsComment",
                                   "Puppet Ruby/OpenSSL Generated Certificate")
         ex << ef.create_extension("basicConstraints", basic_constraint, true)
diff --git a/lib/puppet/type.rb b/lib/puppet/type.rb
index 107e9e906..a15f41edb 100644
--- a/lib/puppet/type.rb
+++ b/lib/puppet/type.rb
@@ -498,10 +498,18 @@ class Type < Puppet::Element
                             :should => value
                         )
                         @states[name] = newstate
-                    rescue => detail
+                    rescue Puppet::Error => detail
                         # the state failed, so just ignore it
                         Puppet.debug "State %s failed: %s" %
                             [name, detail]
+                    rescue Puppet::DevError => detail
+                        # the state failed, so just ignore it
+                        Puppet.notice "State %s failed: %s" %
+                            [name, detail]
+                    rescue => detail
+                        # the state failed, so just ignore it
+                        Puppet.err "State %s failed: %s (%s)" %
+                            [name, detail, detail.class]
                     end
                 end
             end
diff --git a/lib/puppet/type/pfile.rb b/lib/puppet/type/pfile.rb
index cffa6a890..c39b0b51f 100644
--- a/lib/puppet/type/pfile.rb
+++ b/lib/puppet/type/pfile.rb
@@ -1283,9 +1283,11 @@ module Puppet
                 when "file":
                     unless defined? @@localfileserver
                         @@localfileserver = Puppet::Server::FileServer.new(
-                            :Local => true
+                            :Local => true,
+                            :Mount => { "/" => "localhost" },
+                            :Config => false
                         )
-                        @@localfileserver.mount("/", "localhost")
+                        #@@localfileserver.mount("/", "localhost")
                     end
                     sourceobj.server = @@localfileserver
                     path = "/localhost" + uri.path