From 2cd01b237c24132555df1f4e80868d609b8396aa Mon Sep 17 00:00:00 2001 From: "R.I.Pienaar" Date: Sun, 24 Jun 2012 15:23:41 +0100 Subject: [PATCH] 15191 - Use UUIDs for request IDs rather than our hacky way Previously we created our own request ids, these were not great multiple requests in the same milli second on the same host could cause request id clashes...not a problem in functionality with how the new message flow works but it would be a auditing problem to have dupes. We now use a more reliably random UUID implementation based on OpenSSL 128 bit random numbers and RFC compliant UUID formats The SSL#uuid function returns standard format UUIDs for either a given string or based on OpenSSL::BN.rand(). In the Message class we remove the -s in the UUID thus keeping new and old request ids the same size and format ensuring everyone who may have audit parsers wont break as a result of this code --- lib/mcollective/message.rb | 5 ++++- lib/mcollective/monkey_patches.rb | 9 +++++++++ lib/mcollective/ssl.rb | 33 +++++++++++++++++++++++++++++++ spec/unit/message_spec.rb | 7 ++----- spec/unit/ssl_spec.rb | 10 ++++++++++ website/changelog.md | 1 + 6 files changed, 59 insertions(+), 6 deletions(-) diff --git a/lib/mcollective/message.rb b/lib/mcollective/message.rb index a325fd1e..c12f4b45 100644 --- a/lib/mcollective/message.rb +++ b/lib/mcollective/message.rb @@ -233,7 +233,10 @@ def publish end def create_reqid - Digest::MD5.hexdigest("#{Config.instance.identity}-#{Time.now.to_f}-#{agent}-#{collective}") + # we gsub out the -s so that the format of the id does not + # change from previous versions, these should just be more + # unique than previous ones + SSL.uuid.gsub("-", "") end end end diff --git a/lib/mcollective/monkey_patches.rb b/lib/mcollective/monkey_patches.rb index 036b4693..fbf72e34 100644 --- a/lib/mcollective/monkey_patches.rb +++ b/lib/mcollective/monkey_patches.rb @@ -59,6 +59,15 @@ def in_groups_of(chunk_size, padded_with=nil, &block) end unless method_defined?(:in_groups_of) end +class String + def bytes(&block) + # This should not be necessary, really ... + require 'enumerator' + return to_enum(:each_byte) unless block_given? + each_byte(&block) + end unless method_defined?(:bytes) +end + class Dir def self.mktmpdir(prefix_suffix=nil, tmpdir=nil) case prefix_suffix diff --git a/lib/mcollective/ssl.rb b/lib/mcollective/ssl.rb index e8fb2f9b..72df0f39 100644 --- a/lib/mcollective/ssl.rb +++ b/lib/mcollective/ssl.rb @@ -204,6 +204,39 @@ def self.md5(string) Digest::MD5.hexdigest(string) end + # Creates a RFC 4122 version 5 UUID. If string is supplied it will produce repeatable + # UUIDs for that string else a random 128bit string will be used from OpenSSL::BN + # + # Code used with permission from: + # https://github.com/kwilczynski/puppet-functions/blob/master/lib/puppet/parser/functions/uuid.rb + # + def self.uuid(string=nil) + string ||= OpenSSL::Random.random_bytes(16).unpack('H*').shift + + uuid_name_space_dns = "\x6b\xa7\xb8\x10\x9d\xad\x11\xd1\x80\xb4\x00\xc0\x4f\xd4\x30\xc8" + + sha1 = Digest::SHA1.new + sha1.update(uuid_name_space_dns) + sha1.update(string) + + # first 16 bytes.. + bytes = sha1.digest[0, 16].bytes.to_a + + # version 5 adjustments + bytes[6] &= 0x0f + bytes[6] |= 0x50 + + # variant is DCE 1.1 + bytes[8] &= 0x3f + bytes[8] |= 0x80 + + bytes = [4, 2, 2, 2, 6].collect do |i| + bytes.slice!(0, i).pack('C*').unpack('H*') + end + + bytes.join('-') + end + # Reads either a :public or :private key from disk, uses an # optional passphrase to read the private key def read_key(type, key=nil, passphrase=nil) diff --git a/spec/unit/message_spec.rb b/spec/unit/message_spec.rb index fc2730f3..46ad7da6 100755 --- a/spec/unit/message_spec.rb +++ b/spec/unit/message_spec.rb @@ -212,7 +212,7 @@ module MCollective security.expects(:encoderequest).with("identity", 'payload', '123', Util.empty_filter, 'rspec_agent', 'mcollective', 60).twice PluginManager.expects("[]").with("security_plugin").returns(security).twice - Config.instance.expects(:identity).returns("identity").times(4) + Config.instance.expects(:identity).returns("identity").twice Message.any_instance.expects(:requestid).returns("123").twice @@ -414,10 +414,7 @@ module MCollective it "should create a valid request id" do m = Message.new("msg", "message", :agent => "rspec", :collective => "mc") - Config.instance.expects(:identity).returns("rspec") - Time.expects(:now).returns(1.1) - - Digest::MD5.expects(:hexdigest).with("rspec-1.1-rspec-mc").returns("reqid") + SSL.expects(:uuid).returns("reqid") m.create_reqid.should == "reqid" end diff --git a/spec/unit/ssl_spec.rb b/spec/unit/ssl_spec.rb index 8eb795a0..32239527 100755 --- a/spec/unit/ssl_spec.rb +++ b/spec/unit/ssl_spec.rb @@ -240,5 +240,15 @@ module MCollective }.to raise_error("Crypted data should include data") end end + + describe "#uuid" do + it "should produce repeatable uuids" do + SSL.uuid("hello world").should == SSL.uuid("hello world") + end + + it "should not always produce the same uuid" do + SSL.uuid.should_not == SSL.uuid + end + end end end diff --git a/website/changelog.md b/website/changelog.md index e21a6510..bfa86fdf 100644 --- a/website/changelog.md +++ b/website/changelog.md @@ -10,6 +10,7 @@ toc: false |----|-----------|------| |2012/06/25|Batched RPC requests will now all have the same requestid|15195| |2012/06/25|Record the request id on M::Client and in the RPC client stats|15194| +|2012/06/24|Use UUIDs for the request id rather than our own weak implementation|15191| |2012/06/18|The DDL can now define defaults for outputs and the RPC replies are pre-populated|15087| |2012/06/18|Remove unused agent help code|15084| |2012/06/18|Remove unused code from the *discovery* agent related to inventory and facts|15083|