Create methods on the base class that create the hashes and adjust all security plugins to use these new methods
Create test coverage for the base class and psk security plugin and fix some bugs in the abstract method checks that would have raised an exception instead of just log Signed-off-by: R.I.Pienaar <[email protected]> --- Local-branch: feature/master/5701 lib/mcollective/security/base.rb | 35 ++++- plugins/mcollective/security/aes_security.rb | 24 +--- plugins/mcollective/security/psk.rb | 26 +--- plugins/mcollective/security/sshkey.rb | 26 +--- plugins/mcollective/security/ssl.rb | 27 +--- spec/unit/security/base_spec.rb | 213 ++++++++++++++++++++++++++ spec/unit/security/psk_spec.rb | 131 ++++++++++++++++ website/changelog.md | 1 + 8 files changed, 399 insertions(+), 84 deletions(-) create mode 100644 spec/unit/security/base_spec.rb create mode 100644 spec/unit/security/psk_spec.rb diff --git a/lib/mcollective/security/base.rb b/lib/mcollective/security/base.rb index 2f32e0e..cebf5b5 100644 --- a/lib/mcollective/security/base.rb +++ b/lib/mcollective/security/base.rb @@ -122,6 +122,33 @@ module MCollective end end + def create_reply(reqid, agent, target, body) + Log.debug("Encoded a message for request #{reqid}") + + {:senderid => @config.identity, + :requestid => reqid, + :senderagent => agent, + :msgtarget => target, + :msgtime => Time.now.to_i, + :body => body} + end + + def create_request(reqid, target, filter, msg, initiated_by) + Log.debug("Encoding a request for '#{target}' with request id #{reqid}") + + req = {:body => msg, + :senderid => @config.identity, + :requestid => reqid, + :msgtarget => target, + :filter => filter, + :msgtime => Time.now.to_i} + + # if we're in use by a client add the callerid to the main client hashes + req[:callerid] = callerid if initiated_by == :client + + return req + end + # Returns a unique id for the caller, by default we just use the unix # user id, security plugins can provide their own means of doing ids. def callerid @@ -130,22 +157,22 @@ module MCollective # Security providers should provide this, see MCollective::Security::Psk def validrequest?(req) - Log.error("validrequest? is not implimented in #{this.class}") + Log.error("validrequest? is not implimented in #{self.class}") end # Security providers should provide this, see MCollective::Security::Psk def encoderequest(sender, target, msg, filter={}) - Log.error("encoderequest is not implimented in #{this.class}") + Log.error("encoderequest is not implimented in #{self.class}") end # Security providers should provide this, see MCollective::Security::Psk def encodereply(sender, target, msg, requestcallerid=nil) - Log.error("encodereply is not implimented in #{this.class}") + Log.error("encodereply is not implimented in #{self.class}") end # Security providers should provide this, see MCollective::Security::Psk def decodemsg(msg) - Log.error("decodemsg is not implimented in #{this.class}") + Log.error("decodemsg is not implimented in #{self.class}") end end end diff --git a/plugins/mcollective/security/aes_security.rb b/plugins/mcollective/security/aes_security.rb index a59e21a..736fa03 100644 --- a/plugins/mcollective/security/aes_security.rb +++ b/plugins/mcollective/security/aes_security.rb @@ -95,15 +95,8 @@ module MCollective def encodereply(sender, target, msg, requestid, requestcallerid) crypted = encrypt(serialize(msg), requestcallerid) - Log.debug("Encoded a reply for request #{requestid} for #{requestcallerid}") - - req = {:senderid => @config.identity, - :requestid => requestid, - :senderagent => sender, - :msgtarget => target, - :msgtime => Time.now.to_i, - :sslkey => crypted[:key], - :body => crypted[:data]} + req = create_reply(requestid, sender, target, crypted[:data]) + req[:sslkey] = crypted[:key] serialize(req) end @@ -112,17 +105,8 @@ module MCollective def encoderequest(sender, target, msg, requestid, filter={}) crypted = encrypt(serialize(msg), callerid) - Log.debug("Encoding a request for '#{target}' with request id #{requestid}") - - req = {:senderid => @config.identity, - :requestid => requestid, - :msgtarget => target, - :msgtime => Time.now.to_i, - :body => crypted, - :filter => filter, - :callerid => callerid, - :sslkey => crypted[:key], - :body => crypted[:data]} + req = create_request(requestid, target, filter, crypted[:data], @initiated_by) + req[:sslkey] = crypted[:key] if @config.pluginconf.include?("aes.send_pubkey") && @config.pluginconf["aes.send_pubkey"] == "1" if @initiated_by == :client diff --git a/plugins/mcollective/security/psk.rb b/plugins/mcollective/security/psk.rb index 0c1b02d..093af0a 100644 --- a/plugins/mcollective/security/psk.rb +++ b/plugins/mcollective/security/psk.rb @@ -34,15 +34,10 @@ module MCollective serialized = Marshal.dump(msg) digest = makehash(serialized) - Log.debug("Encoded a message with hash #{digest} for request #{requestid}") - - Marshal.dump({:senderid => @config.identity, - :requestid => requestid, - :senderagent => sender, - :msgtarget => target, - :msgtime => Time.now.to_i, - :hash => digest, - :body => serialized}) + req = create_reply(requestid, sender, target, serialized) + req[:hash] = digest + + Marshal.dump(req) end # Encodes a request msg @@ -50,19 +45,10 @@ module MCollective serialized = Marshal.dump(msg) digest = makehash(serialized) - Log.debug("Encoding a request for '#{target}' with request id #{requestid}") - request = {:body => serialized, - :hash => digest, - :senderid => @config.identity, - :requestid => requestid, - :msgtarget => target, - :filter => filter, - :msgtime => Time.now.to_i} - - # if we're in use by a client add the callerid to the main client hashes - request[:callerid] = callerid if @initiated_by == :client + req = create_request(requestid, target, filter, serialized, @initiated_by) + req[:hash] = digest - Marshal.dump(request) + Marshal.dump(req) end # Checks the md5 hash in the request body against our psk, the request sent for validation diff --git a/plugins/mcollective/security/sshkey.rb b/plugins/mcollective/security/sshkey.rb index c8586f2..5b64dde 100644 --- a/plugins/mcollective/security/sshkey.rb +++ b/plugins/mcollective/security/sshkey.rb @@ -80,15 +80,10 @@ module MCollective serialized = Marshal.dump(msg) digest = makehash(serialized) - Log.debug("Encoded a message with hash #{digest} for request #{requestid}") - - Marshal.dump({:senderid => @config.identity, - :requestid => requestid, - :senderagent => sender, - :msgtarget => target, - :msgtime => Time.now.to_i, - :hash => digest, - :body => serialized}) + req = create_reply(requestid, sender, target, serialized) + req[:hash] = digest + + Marshal.dump(req) end # Encodes a request msg @@ -96,19 +91,10 @@ module MCollective serialized = Marshal.dump(msg) digest = makehash(serialized) - Log.debug("Encoding a request for '#{target}' with request id #{requestid}") - request = {:body => serialized, - :hash => digest, - :senderid => @config.identity, - :requestid => requestid, - :msgtarget => target, - :filter => filter, - :msgtime => Time.now.to_i} - - # if we're in use by a client add the callerid to the main client hashes - request[:callerid] = callerid if @initiated_by == :client + req = create_request(requestid, target, filter, serialized, @initiated_by) + req[:hash] = digest - Marshal.dump(request) + Marshal.dump(req) end def callerid diff --git a/plugins/mcollective/security/ssl.rb b/plugins/mcollective/security/ssl.rb index b356241..c4d8ff5 100644 --- a/plugins/mcollective/security/ssl.rb +++ b/plugins/mcollective/security/ssl.rb @@ -100,15 +100,11 @@ module MCollective serialized = serialize(msg) digest = makehash(serialized) - Log.debug("Encoded a message for request #{requestid}") - - serialize({:senderid => @config.identity, - :requestid => requestid, - :senderagent => sender, - :msgtarget => target, - :msgtime => Time.now.to_i, - :hash => digest, - :body => serialized}) + + req = create_reply(requestid, sender, target, serialized) + req[:hash] = digest + + serialize(req) end # Encodes a request msg @@ -116,19 +112,10 @@ module MCollective serialized = serialize(msg) digest = makehash(serialized) - Log.debug("Encoding a request for '#{target}' with request id #{requestid}") - request = {:body => serialized, - :hash => digest, - :senderid => @config.identity, - :requestid => requestid, - :msgtarget => target, - :filter => filter, - :msgtime => Time.now.to_i} - - # if we're in use by a client add the callerid to the main client hashes - request[:callerid] = callerid + req = create_request(requestid, target, filter, serialized, @initiated_by) + req[:hash] = digest - serialize(request) + serialize(req) end # Checks the SSL signature in the request body diff --git a/spec/unit/security/base_spec.rb b/spec/unit/security/base_spec.rb new file mode 100644 index 0000000..882d152 --- /dev/null +++ b/spec/unit/security/base_spec.rb @@ -0,0 +1,213 @@ +#!/usr/bin/env ruby + +require File.dirname(__FILE__) + '/../../spec_helper' + +module MCollective::Security + describe Base do + before do + @config = mock("config") + @config.stubs(:identity).returns("test") + @config.stubs(:configured).returns(true) + + @stats = mock("stats") + + @time = Time.now.to_i + ::Time.stubs(:now).returns(@time) + + MCollective::Log.stubs(:debug).returns(true) + + MCollective::PluginManager << {:type => "global_stats", :class => @stats} + MCollective::Config.stubs("instance").returns(@config) + MCollective::Util.stubs("empty_filter?").returns(false) + + @plugin = Base.new + end + + after do + MCollective::PluginManager.delete("global_stats") + end + + describe "#validate_filter?" do + it "should pass on empty filter" do + MCollective::Util.stubs("empty_filter?").returns(true) + + @stats.stubs(:passed).once + @stats.stubs(:filtered).never + @stats.stubs(:passed).never + + MCollective::Log.expects(:debug).with("Message passed the filter checks").once + + @plugin.validate_filter?({}).should == true + end + + it "should pass for known classes" do + MCollective::Util.stubs("has_cf_class?").with("foo").returns(true) + + @stats.stubs(:passed).once + @stats.stubs(:filtered).never + + MCollective::Log.expects(:debug).with("Message passed the filter checks").once + MCollective::Log.expects(:debug).with("Passing based on configuration management class foo").once + + @plugin.validate_filter?({"cf_class" => ["foo"]}).should == true + end + + it "should fail for unknown classes" do + MCollective::Util.stubs("has_cf_class?").with("foo").returns(false) + + @stats.stubs(:filtered).once + @stats.stubs(:passed).never + + MCollective::Log.expects(:debug).with("Message failed the filter checks").once + MCollective::Log.expects(:debug).with("Failing based on configuration management class foo").once + + @plugin.validate_filter?({"cf_class" => ["foo"]}).should == false + end + + it "should pass for known agents" do + MCollective::Util.stubs("has_agent?").with("foo").returns(true) + + @stats.stubs(:passed).once + @stats.stubs(:filtered).never + + MCollective::Log.expects(:debug).with("Message passed the filter checks").once + MCollective::Log.expects(:debug).with("Passing based on agent foo").once + + @plugin.validate_filter?({"agent" => ["foo"]}).should == true + end + + it "should fail for unknown agents" do + MCollective::Util.stubs("has_agent?").with("foo").returns(false) + + @stats.stubs(:filtered).once + @stats.stubs(:passed).never + + MCollective::Log.expects(:debug).with("Message failed the filter checks").once + MCollective::Log.expects(:debug).with("Failing based on agent foo").once + + @plugin.validate_filter?({"agent" => ["foo"]}).should == false + end + + it "should pass for known facts" do + MCollective::Util.stubs("has_fact?").with("fact", "value", "operator").returns(true) + + @stats.stubs(:passed).once + @stats.stubs(:filtered).never + + MCollective::Log.expects(:debug).with("Message passed the filter checks").once + MCollective::Log.expects(:debug).with("Passing based on fact fact operator value").once + + @plugin.validate_filter?({"fact" => [{:fact => "fact", :operator => "operator", :value => "value"}]}).should == true + end + + it "should fail for unknown facts" do + MCollective::Util.stubs("has_fact?").with("fact", "value", "operator").returns(false) + + @stats.stubs(:filtered).once + @stats.stubs(:passed).never + + MCollective::Log.expects(:debug).with("Message failed the filter checks").once + MCollective::Log.expects(:debug).with("Failing based on fact fact operator value").once + + @plugin.validate_filter?({"fact" => [{:fact => "fact", :operator => "operator", :value => "value"}]}).should == false + end + + it "should pass for known identity" do + MCollective::Util.stubs("has_identity?").with("test").returns(true) + + @stats.stubs(:passed).once + @stats.stubs(:filtered).never + + MCollective::Log.expects(:debug).with("Message passed the filter checks").once + MCollective::Log.expects(:debug).with("Passing based on identity = test").once + + @plugin.validate_filter?({"identity" => ["test"]}).should == true + end + + it "should fail for known identity" do + MCollective::Util.stubs("has_identity?").with("test").returns(false) + + @stats.stubs(:passed).never + @stats.stubs(:filtered).once + + MCollective::Log.expects(:debug).with("Message failed the filter checks").once + MCollective::Log.expects(:debug).with("Failed based on identity = test").once + + @plugin.validate_filter?({"identity" => ["test"]}).should == false + end + end + + describe "#create_reply" do + it "should return correct data" do + expected = {:senderid => "test", + :requestid => "reqid", + :senderagent => "agent", + :msgtarget => "target", + :msgtime => @time, + :body => "body"} + + @plugin.create_reply("reqid", "agent", "target", "body").should == expected + end + end + + describe "#create_request" do + it "should return correct data" do + expected = {:body => "body", + :senderid => "test", + :requestid => "reqid", + :msgtarget => "target", + :filter => "filter", + :msgtime => @time} + + @plugin.create_request("reqid", "target", "filter", "body", :server).should == expected + end + + it "should set the callerid when appropriate" do + expected = {:body => "body", + :senderid => "test", + :requestid => "reqid", + :msgtarget => "target", + :filter => "filter", + :callerid => "callerid", + :msgtime => @time} + + @plugin.stubs(:callerid).returns("callerid") + @plugin.create_request("reqid", "target", "filter", "body", :client).should == expected + end + end + + describe "#callerid" do + it "should return a unix UID based callerid" do + @plugin.callerid.should == "uid=#{Process.uid}" + end + end + + describe "#validrequest?" do + it "should log an error when not implimented" do + MCollective::Log.expects(:error).with("validrequest? is not implimented in MCollective::Security::Base") + @plugin.validrequest?(nil) + end + end + + describe "#encoderequest" do + it "should log an error when not implimented" do + MCollective::Log.expects(:error).with("encoderequest is not implimented in MCollective::Security::Base") + @plugin.encoderequest(nil, nil, nil, nil) + end + end + + describe "#encodereply" do + it "should log an error when not implimented" do + MCollective::Log.expects(:error).with("encodereply is not implimented in MCollective::Security::Base") + @plugin.encodereply(nil, nil, nil, nil) + end + end + + describe "#decodemsg" do + it "should log an error when not implimented" do + MCollective::Log.expects(:error).with("decodemsg is not implimented in MCollective::Security::Base") + @plugin.decodemsg(nil) + end + end + end +end diff --git a/spec/unit/security/psk_spec.rb b/spec/unit/security/psk_spec.rb new file mode 100644 index 0000000..aed713a --- /dev/null +++ b/spec/unit/security/psk_spec.rb @@ -0,0 +1,131 @@ +#!/usr/bin/env ruby + +require File.dirname(__FILE__) + '/../../spec_helper' +require File.dirname(__FILE__) + '/../../../plugins/mcollective/security/psk.rb' + +module MCollective::Security + describe Psk do + before do + @config = mock("config") + @config.stubs(:identity).returns("test") + @config.stubs(:configured).returns(true) + @config.stubs(:pluginconf).returns({"psk" => "12345"}) + + @stats = mock("stats") + + @time = Time.now.to_i + ::Time.stubs(:now).returns(@time) + + MCollective::Log.stubs(:debug).returns(true) + + MCollective::PluginManager << {:type => "global_stats", :class => @stats} + MCollective::Config.stubs("instance").returns(@config) + MCollective::Util.stubs("empty_filter?").returns(false) + + @plugin = Psk.new + end + + after do + MCollective::PluginManager.delete("global_stats") + end + + describe "#decodemsg" do + it "should correctly decode a message" do + @plugin.stubs("validrequest?").returns(true).once + + msg = mock("message") + msg.stubs(:payload).returns(Marshal.dump({:body => Marshal.dump("foo")})) + + @plugin.decodemsg(msg).should == {:body=>"foo"} + end + + it "should return nil on failure" do + @plugin.stubs("validrequest?").raises("fail").once + + msg = mock("message") + msg.stubs(:payload).returns(Marshal.dump("foo")) + + expect { @plugin.decodemsg(msg) }.to raise_error("fail") + end + end + + describe "#encodereply" do + it "should correctly Marshal encode the reply" do + @plugin.stubs("create_reply").returns({:test => "test"}) + Marshal.stubs("dump").with("test message").returns("marshal_test_message").once + Marshal.stubs("dump").with({:hash => '2dbeb0d7938a08a34eacd2c1dab25602', :test => 'test'}).returns("marshal_test_reply").once + + @plugin.encodereply("sender", "target", "test message", "requestid", "callerid").should == "marshal_test_reply" + end + end + + describe "#encoderequest" do + it "should correctly Marshal encode the request" do + @plugin.stubs("create_request").returns({:test => "test"}) + Marshal.stubs("dump").with("test message").returns("marshal_test_message").once + Marshal.stubs("dump").with({:hash => '2dbeb0d7938a08a34eacd2c1dab25602', :test => 'test'}).returns("marshal_test_request").once + + @plugin.encoderequest("sender", "target", "test message", "requestid", "filter").should == "marshal_test_request" + end + end + + describe "#validrequest?" do + it "should correctly validate requests" do + @stats.stubs(:validated).once + @stats.stubs(:unvalidated).never + @plugin.validrequest?({:body => "foo", :hash => "e83ac78027b77b659a49bccbbcfa4849"}) + end + + it "should raise an exception on failure" do + @stats.stubs(:validated).never + @stats.stubs(:unvalidated).once + expect { @plugin.validrequest?({:body => "foo", :hash => ""}) }.to raise_error("Received an invalid signature in message") + end + end + + describe "#callerid" do + it "should do uid based callerid when unconfigured" do + @plugin.callerid.should == "uid=#{Process.uid}" + end + + it "should support gid based callerids" do + @config.stubs(:pluginconf).returns({"psk.callertype" => "gid"}) + @plugin.callerid.should == "gid=#{Process.gid}" + end + + it "should support group based callerids" do + @config.stubs(:pluginconf).returns({"psk.callertype" => "group"}) + @plugin.callerid.should == "group=#{Etc.getgrgid(Process.gid).name}" + end + + it "should support user based callerids" do + @config.stubs(:pluginconf).returns({"psk.callertype" => "user"}) + @plugin.callerid.should == "user=#{Etc.getlogin}" + end + + it "should support identity based callerids" do + @config.stubs(:pluginconf).returns({"psk.callertype" => "identity"}) + @plugin.callerid.should == "identity=test" + end + end + + describe "#makehash" do + it "should return the correct md5 digest" do + @plugin.send(:makehash, "foo").should == "e83ac78027b77b659a49bccbbcfa4849" + end + + it "should fail if no PSK is configured" do + @config.stubs(:pluginconf).returns({}) + expect { @plugin.send(:makehash, "foo") }.to raise_error("No plugin.psk configuration option specified") + end + + it "should support reading the PSK from the environment" do + ENV["MCOLLECTIVE_PSK"] = "54321" + + @plugin.send(:makehash, "foo").should == "d3fb63cc6b1d47cc4b2012df926c2feb" + + ENV.delete("MCOLLECTIVE_PSK") + end + end + end +end diff --git a/website/changelog.md b/website/changelog.md index efe929e..17d85d2 100644 --- a/website/changelog.md +++ b/website/changelog.md @@ -11,6 +11,7 @@ title: Changelog |Date|Description|Ticket| |----|-----------|------| +|2011/04/19|Abstract the creation of request and reply hashes to simplify connector plugin development|5701| |2011/04/15|Improve the shellsafe validator and add a Util method to do shell escaping|7066| |2011/04/14|Update Rakefile to have a mail_patches task|6874| |2011/04/13|Update vendored systemu library for Ruby 1.9.2 compatability|7067| -- 1.7.1 -- 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.
