From ad5e90badb676ed9209e1b15530a51bc10549575 Mon Sep 17 00:00:00 2001 From: Sonu Saha <98935583+ahasunos@users.noreply.github.com> Date: Fri, 22 Sep 2023 20:07:05 +0530 Subject: [PATCH] CHEF-5815: Update signing of all msi packages with new cli tool - `smctl` (#1128) * PARAMS: Add keypair_alias to signing_identity hash to be received in params Signed-off-by: Sonu Saha * TOOL: Add signing capability with new tool Signed-off-by: Sonu Saha * CHORE: Add some debug statements with puts - remove this later Signed-off-by: Sonu Saha * Update signing command for smctl with --fingerprint Signed-off-by: Sonu Saha * testing patch here, trying to remove + Signed-off-by: Sean Simmons * reverting this change, it did not remove the + in the file name Signed-off-by: Sean Simmons * HACK: Temporary workaround to remove '+' from package name; causing signing to fail Signed-off-by: Sonu Saha * REVERT changes made to subsituting + with _ Signed-off-by: Sonu Saha * Update semver to include '_' instead of '+' Signed-off-by: Sonu Saha * CHORE: Add temporary logs & change + to _ Signed-off-by: Sonu Saha * changing from _ to - Signed-off-by: Sean Simmons * changing from _ to - Signed-off-by: Sean Simmons * changing from _ to - Signed-off-by: Sean Simmons * CHEF-5815: Update temporary log messages - we are retaining it Signed-off-by: Sonu Saha * CHEF-5815: Remove old method for signing Signed-off-by: Sonu Saha * LINT: Fix lint offense and missing end Signed-off-by: Sonu Saha * SPECS: Fix test as per new signing method changes Signed-off-by: Sonu Saha * removing log.info and adding log.debug Signed-off-by: Sean Simmons * updating based off Marc's Comment's Signed-off-by: Sean Simmons * CHORE: Update log messages Signed-off-by: Sonu Saha --------- Signed-off-by: Sonu Saha Signed-off-by: Sean Simmons Co-authored-by: Sean Simmons --- lib/omnibus/build_version.rb | 13 +++++- lib/omnibus/build_version_dsl.rb | 7 ++- lib/omnibus/packagers/appx.rb | 5 +++ lib/omnibus/packagers/windows_base.rb | 62 +++++++++++++++------------ spec/unit/build_version_dsl_spec.rb | 8 ++-- spec/unit/build_version_spec.rb | 16 +++---- spec/unit/packagers/appx_spec.rb | 33 +++----------- spec/unit/packagers/msi_spec.rb | 35 +++------------ 8 files changed, 81 insertions(+), 98 deletions(-) diff --git a/lib/omnibus/build_version.rb b/lib/omnibus/build_version.rb index 4ce6e97b6..923c779da 100644 --- a/lib/omnibus/build_version.rb +++ b/lib/omnibus/build_version.rb @@ -87,13 +87,18 @@ def initialize(path = Config.project_root) # and can be influenced by users. def semver build_tag = version_tag + log.debug(log_key) { "#{self.class}##{__method__} - build tag: #{build_tag}" } # PRERELEASE VERSION + log.debug(log_key) { "#{self.class}##{__method__} - prerelease_version?: #{prerelease_version?}" } + if prerelease_version? # ensure all dashes are dots per precedence rules (#12) in Semver # 2.0.0-rc.1 + log.debug(log_key) { "#{self.class}##{__method__} - prerelease_tag: #{prerelease_tag}" } prerelease = prerelease_tag.tr("-", ".") build_tag << "-" << prerelease + log.debug(log_key) { "#{self.class}##{__method__} - build_tag after prerelease: #{build_tag}" } end # BUILD VERSION @@ -106,6 +111,7 @@ def semver # # format: YYYYMMDDHHMMSS example: 20130131123345 if Config.append_timestamp + log.debug(log_key) { "#{self.class}##{__method__} - build_start_time: #{build_start_time}" } build_version_items << build_start_time end @@ -114,13 +120,18 @@ def semver # # format: git.COMMITS_SINCE_TAG.GIT_SHA example: git.207.694b062 unless commits_since_tag == 0 + log.debug(log_key) { "#{self.class}##{__method__} - commits_since_tag: #{commits_since_tag}" } + log.debug(log_key) { "#{self.class}##{__method__} - git_sha_tag: #{git_sha_tag}" } build_version_items << ["git", commits_since_tag, git_sha_tag].join(".") end unless build_version_items.empty? - build_tag << "+" << build_version_items.join(".") + log.debug(log_key) { "#{self.class}##{__method__} - build_version_items: #{build_version_items}" } + build_tag << "-" << build_version_items.join(".") end + log.debug(log_key) { "#{self.class}##{__method__} - final build_tag returned: #{build_tag}" } + build_tag end diff --git a/lib/omnibus/build_version_dsl.rb b/lib/omnibus/build_version_dsl.rb index 0c74bbb18..2d25f1e59 100644 --- a/lib/omnibus/build_version_dsl.rb +++ b/lib/omnibus/build_version_dsl.rb @@ -117,8 +117,11 @@ def build_version=(new_version) # @param [String] version # @return [String] def maybe_append_timestamp(version) + log.debug(log_key) { "#{self.class}##{__method__} - Config.append_timestamp: #{Config.append_timestamp}" } + log.debug(log_key) { "#{self.class}##{__method__} - version: #{version}" } + log.debug(log_key) { "#{self.class}##{__method__} - has_timestamp?(version): #{has_timestamp?(version)}" } if Config.append_timestamp && !has_timestamp?(version) - [version, Omnibus::BuildVersion.build_start_time].join("+") + [version, Omnibus::BuildVersion.build_start_time].join("-") else version end @@ -132,7 +135,7 @@ def maybe_append_timestamp(version) # @param [String] version # @return [Boolean] def has_timestamp?(version) - _ver, build_info = version.split("+") + _ver, build_info = version.split("-") return false if build_info.nil? build_info.split(".").any? do |part| diff --git a/lib/omnibus/packagers/appx.rb b/lib/omnibus/packagers/appx.rb index 8c43c8ee2..8e66c39f1 100644 --- a/lib/omnibus/packagers/appx.rb +++ b/lib/omnibus/packagers/appx.rb @@ -52,6 +52,11 @@ class Packager::APPX < Packager::WindowsBase # @see Base#package_name def package_name + log.debug(log_key) { "#{self.class}##{__method__} - package_name: #{project.package_name}" } + log.debug(log_key) { "#{self.class}##{__method__} - build_version: #{project.build_version}" } + log.debug(log_key) { "#{self.class}##{__method__} - build_iteration: #{project.build_iteration}" } + log.debug(log_key) { "#{self.class}##{__method__} - Config.windows_arch: #{Config.windows_arch}" } + "#{project.package_name}-#{project.build_version}-#{project.build_iteration}-#{Config.windows_arch}.appx" end diff --git a/lib/omnibus/packagers/windows_base.rb b/lib/omnibus/packagers/windows_base.rb index 7773ec522..126d12c5b 100644 --- a/lib/omnibus/packagers/windows_base.rb +++ b/lib/omnibus/packagers/windows_base.rb @@ -16,9 +16,6 @@ module Omnibus class Packager::WindowsBase < Packager::Base - DEFAULT_TIMESTAMP_SERVERS = ["http://timestamp.digicert.com", - "http://timestamp.verisign.com/scripts/timestamp.dll"].freeze - # # Set the signing certificate name # @@ -59,9 +56,18 @@ def signing_identity(thumbprint = NULL, params = NULL) raise InvalidValue.new(:params, "be a Hash") end - valid_keys = %i{store timestamp_servers machine_store algorithm} + valid_keys = %i{store machine_store algorithm keypair_alias} invalid_keys = params.keys - valid_keys unless invalid_keys.empty? + + # log a deprecated warning if timestamp_server is used + if invalid_keys.include?(:timestamp_servers) + log.deprecated(log_key) do + "The signing_identity is updated to use smctl.exe. which does not require timestamp_servers" \ + "Please remove timestamp_servers from your signing_identity" + end + end + raise InvalidValue.new(:params, "contain keys from [#{valid_keys.join(", ")}]. "\ "Found invalid keys [#{invalid_keys.join(", ")}]") end @@ -77,9 +83,8 @@ def signing_identity(thumbprint = NULL, params = NULL) @signing_identity[:store] = params[:store] || "My" @signing_identity[:algorithm] = params[:algorithm] || "SHA256" - servers = params[:timestamp_servers] || DEFAULT_TIMESTAMP_SERVERS - @signing_identity[:timestamp_servers] = [servers].flatten @signing_identity[:machine_store] = params[:machine_store] || false + @signing_identity[:keypair_alias] = params[:keypair_alias] end @signing_identity @@ -102,41 +107,41 @@ def timestamp_servers signing_identity[:timestamp_servers] end + def keypair_alias + signing_identity[:keypair_alias] + end + def machine_store? signing_identity[:machine_store] end - # - # Iterates through available timestamp servers and tries to sign - # the file with with each server, stopping after the first to succeed. - # If none succeed, an exception is raised. - # + # signs the package with the given certificate def sign_package(package_file) - success = false - timestamp_servers.each do |ts| - success = try_sign(package_file, ts) - break if success - end - raise FailedToSignWindowsPackage.new unless success + raise FailedToSignWindowsPackage.new unless is_signed?(package_file) end - def try_sign(package_file, url) + def is_signed?(package_file) cmd = [].tap do |arr| - arr << "signtool.exe" - arr << "sign /v" - arr << "/t #{url}" - arr << "/fd #{algorithm}" - arr << "/sm" if machine_store? - arr << "/s #{cert_store_name}" - arr << "/sha1 #{thumbprint}" - arr << "/d #{project.package_name}" - arr << "\"#{package_file}\"" + arr << "smctl.exe" + arr << "sign" + arr << "--fingerprint #{thumbprint}" + arr << "--input #{package_file}" end.join(" ") + status = shellout(cmd) + + log.debug(log_key) { "#{self.class}##{__method__} - package_file: #{package_file}" } + log.debug(log_key) { "#{self.class}##{__method__} - cmd: #{cmd}" } + log.debug(log_key) { "#{self.class}##{__method__} - status: #{status}" } + log.debug(log_key) { "#{self.class}##{__method__} - status.exitstatus: #{status.exitstatus}" } + log.debug(log_key) { "#{self.class}##{__method__} - status.stdout: #{status.stdout}" } + log.debug(log_key) { "#{self.class}##{__method__} - status.stderr: #{status.stderr}" } + + # log the error if the signing failed if status.exitstatus != 0 log.warn(log_key) do <<-EOH.strip - Failed to add timestamp with timeserver #{url} + Failed to verify signature of #{package_file} STDOUT ------ @@ -148,6 +153,7 @@ def try_sign(package_file, url) EOH end end + status.exitstatus == 0 end diff --git a/spec/unit/build_version_dsl_spec.rb b/spec/unit/build_version_dsl_spec.rb index c746a1a1a..71db72d4a 100644 --- a/spec/unit/build_version_dsl_spec.rb +++ b/spec/unit/build_version_dsl_spec.rb @@ -34,19 +34,19 @@ module Omnibus before { Config.append_timestamp(true) } it "appends a timestamp to a static (String) version" do - expect(subject_with_version.build_version).to eq("1.0.0+#{today_string}") + expect(subject_with_version.build_version).to eq("1.0.0-#{today_string}") end it "doesn't append timestamp to something that already looks like it has a timestamp" do - semver = "1.0.0+#{today_string}.git.222.694b062" - expect(described_class.new(semver).build_version).to eq("1.0.0+#{today_string}.git.222.694b062") + semver = "1.0.0-#{today_string}.git.222.694b062" + expect(described_class.new(semver).build_version).to eq("1.0.0-#{today_string}.git.222.694b062") end it "appends a timestamp to a DSL-built version" do allow(BuildVersion).to receive(:new).and_return(BuildVersion.new) allow(BuildVersion).to receive(:new).with("/etc/zoo").and_return(zoo_version) subject_with_description.resolve(zoo_software) - expect(subject_with_description.build_version).to eq("5.5.5+#{today_string}") + expect(subject_with_description.build_version).to eq("5.5.5-#{today_string}") end end diff --git a/spec/unit/build_version_spec.rb b/spec/unit/build_version_spec.rb index 1cf480345..933ef8d63 100644 --- a/spec/unit/build_version_spec.rb +++ b/spec/unit/build_version_spec.rb @@ -109,12 +109,12 @@ module Omnibus end it "generates a version matching format 'MAJOR.MINOR.PATCH-PRERELEASE+TIMESTAMP.git.COMMITS_SINCE.GIT_SHA'" do - expect(build_version.semver).to match(/11.0.0-alpha1\+#{today_string}[0-9]+.git.207.694b062/) + expect(build_version.semver).to match(/11.0.0-alpha1\-#{today_string}[0-9]+.git.207.694b062/) end it "uses ENV['BUILD_TIMESTAMP'] to generate timestamp if set" do stub_env("BUILD_TIMESTAMP", "2012-12-25_16-41-40") - expect(build_version.semver).to eq("11.0.0-alpha1+20121225164140.git.207.694b062") + expect(build_version.semver).to eq("11.0.0-alpha1-20121225164140.git.207.694b062") end it "fails on invalid ENV['BUILD_TIMESTAMP'] values" do @@ -124,7 +124,7 @@ module Omnibus it "uses ENV['BUILD_ID'] to generate timestamp if set and BUILD_TIMESTAMP is not set" do stub_env("BUILD_ID", "2012-12-25_16-41-40") - expect(build_version.semver).to eq("11.0.0-alpha1+20121225164140.git.207.694b062") + expect(build_version.semver).to eq("11.0.0-alpha1-20121225164140.git.207.694b062") end it "fails on invalid ENV['BUILD_ID'] values" do @@ -136,7 +136,7 @@ module Omnibus let(:git_describe) { "11.0.0-alpha-3-207-g694b062" } it "converts all dashes to dots" do - expect(build_version.semver).to match(/11.0.0-alpha.3\+#{today_string}[0-9]+.git.207.694b062/) + expect(build_version.semver).to match(/11.0.0-alpha.3\-#{today_string}[0-9]+.git.207.694b062/) end end @@ -144,7 +144,7 @@ module Omnibus let(:git_describe) { "11.0.0-alpha2" } it "appends a timestamp with no git info" do - expect(build_version.semver).to match(/11.0.0-alpha2\+#{today_string}[0-9]+/) + expect(build_version.semver).to match(/11.0.0-alpha2\-#{today_string}[0-9]+/) end end @@ -152,20 +152,20 @@ module Omnibus let(:git_describe) { "11.0.0-alpha-3-207-g694b062" } context "by default" do it "appends a timestamp" do - expect(build_version.semver).to match(/11.0.0-alpha.3\+#{today_string}[0-9]+.git.207.694b062/) + expect(build_version.semver).to match(/11.0.0-alpha.3\-#{today_string}[0-9]+.git.207.694b062/) end end context "when Config.append_timestamp is true" do it "appends a timestamp" do - expect(build_version.semver).to match(/11.0.0-alpha.3\+#{today_string}[0-9]+.git.207.694b062/) + expect(build_version.semver).to match(/11.0.0-alpha.3\-#{today_string}[0-9]+.git.207.694b062/) end end context "when Config.append_timestamp is false" do before { Config.append_timestamp(false) } it "does not append a timestamp" do - expect(build_version.semver).to match(/11.0.0-alpha.3\+git.207.694b062/) + expect(build_version.semver).to match(/11.0.0-alpha.3\-git.207.694b062/) end end end diff --git a/spec/unit/packagers/appx_spec.rb b/spec/unit/packagers/appx_spec.rb index 70e6dc897..1c7cc0e32 100644 --- a/spec/unit/packagers/appx_spec.rb +++ b/spec/unit/packagers/appx_spec.rb @@ -129,39 +129,18 @@ module Omnibus allow(subject).to receive(:shellout!) end - describe "#timestamp_servers" do - it "defaults to using ['http://timestamp.digicert.com','http://timestamp.verisign.com/scripts/timestamp.dll']" do + describe "#keypair_alias" do + it "defaults to 'Chef Software, Inc.'" do subject.signing_identity("foo") - expect(subject).to receive(:try_sign).with(appx, "http://timestamp.digicert.com").and_return(false) - expect(subject).to receive(:try_sign).with(appx, "http://timestamp.verisign.com/scripts/timestamp.dll").and_return(true) + expect(subject).to receive(:is_signed?).with(appx).and_return(true) subject.sign_package(appx) end - it "uses the timestamp server if provided through the #timestamp_server dsl" do - subject.signing_identity("foo", timestamp_servers: "http://fooserver") - expect(subject).to receive(:try_sign).with(appx, "http://fooserver").and_return(true) + it "uses the keypair alias if provided through the #keypair_alias dsl" do + subject.signing_identity("foo", keypair_alias: "bar") + expect(subject).to receive(:is_signed?).with(appx).and_return(true) subject.sign_package(appx) end - - it "tries all timestamp server if provided through the #timestamp_server dsl" do - subject.signing_identity("foo", timestamp_servers: ["http://fooserver", "http://barserver"]) - expect(subject).to receive(:try_sign).with(appx, "http://fooserver").and_return(false) - expect(subject).to receive(:try_sign).with(appx, "http://barserver").and_return(true) - subject.sign_package(appx) - end - - it "tries all timestamp server if provided through the #timestamp_servers dsl and stops at the first available" do - subject.signing_identity("foo", timestamp_servers: ["http://fooserver", "http://barserver"]) - expect(subject).to receive(:try_sign).with(appx, "http://fooserver").and_return(true) - expect(subject).not_to receive(:try_sign).with(appx, "http://barserver") - subject.sign_package(appx) - end - - it "raises an exception if there are no available timestamp servers" do - subject.signing_identity("foo", timestamp_servers: "http://fooserver") - expect(subject).to receive(:try_sign).with(appx, "http://fooserver").and_return(false) - expect { subject.sign_package(appx) }.to raise_error(FailedToSignWindowsPackage) - end end end end diff --git a/spec/unit/packagers/msi_spec.rb b/spec/unit/packagers/msi_spec.rb index c83e0cda0..989ccadea 100644 --- a/spec/unit/packagers/msi_spec.rb +++ b/spec/unit/packagers/msi_spec.rb @@ -554,37 +554,16 @@ module Omnibus allow(subject).to receive(:shellout!) end - describe "#timestamp_servers" do - it "defaults to using ['http://timestamp.digicert.com','http://timestamp.verisign.com/scripts/timestamp.dll']" do - subject.signing_identity("foo") - expect(subject).to receive(:try_sign).with(msi, "http://timestamp.digicert.com").and_return(false) - expect(subject).to receive(:try_sign).with(msi, "http://timestamp.verisign.com/scripts/timestamp.dll").and_return(true) + describe "#keypair_alias" do + it "uses the keypair alias if provided through the #keypair_alias dsl" do + subject.signing_identity("foo", keypair_alias: "bar") + expect(subject).to receive(:is_signed?).with(msi).and_return(true) subject.sign_package(msi) end - it "uses the timestamp server if provided through the #timestamp_server dsl" do - subject.signing_identity("foo", timestamp_servers: "http://fooserver") - expect(subject).to receive(:try_sign).with(msi, "http://fooserver").and_return(true) - subject.sign_package(msi) - end - - it "tries all timestamp server if provided through the #timestamp_server dsl" do - subject.signing_identity("foo", timestamp_servers: ["http://fooserver", "http://barserver"]) - expect(subject).to receive(:try_sign).with(msi, "http://fooserver").and_return(false) - expect(subject).to receive(:try_sign).with(msi, "http://barserver").and_return(true) - subject.sign_package(msi) - end - - it "tries all timestamp server if provided through the #timestamp_servers dsl and stops at the first available" do - subject.signing_identity("foo", timestamp_servers: ["http://fooserver", "http://barserver"]) - expect(subject).to receive(:try_sign).with(msi, "http://fooserver").and_return(true) - expect(subject).not_to receive(:try_sign).with(msi, "http://barserver") - subject.sign_package(msi) - end - - it "raises an exception if there are no available timestamp servers" do - subject.signing_identity("foo", timestamp_servers: "http://fooserver") - expect(subject).to receive(:try_sign).with(msi, "http://fooserver").and_return(false) + it "raises an exception if the signing fails" do + subject.signing_identity("foo", keypair_alias: "bar") + expect(subject).to receive(:is_signed?).with(msi).and_return(false) expect { subject.sign_package(msi) }.to raise_error(FailedToSignWindowsPackage) end end