From 289f66c00d24cdaea380246f432509caf7e2a23d Mon Sep 17 00:00:00 2001 From: Jason Pickens Date: Sun, 18 Aug 2019 19:58:05 +1200 Subject: [PATCH 1/8] Store signatures as git blobs --- lib/overcommit.rb | 1 + lib/overcommit/configuration.rb | 79 +++++++++++------------- lib/overcommit/configuration_loader.rb | 2 +- lib/overcommit/exceptions.rb | 6 ++ lib/overcommit/git_repo.rb | 70 +++++++++++++++++++++ lib/overcommit/signature.rb | 85 ++++++++++++++++++++++++++ 6 files changed, 199 insertions(+), 44 deletions(-) create mode 100644 lib/overcommit/signature.rb diff --git a/lib/overcommit.rb b/lib/overcommit.rb index 15fef94f..0dda5dab 100644 --- a/lib/overcommit.rb +++ b/lib/overcommit.rb @@ -24,3 +24,4 @@ require 'overcommit/installer' require 'overcommit/logger' require 'overcommit/version' +require 'overcommit/signature' diff --git a/lib/overcommit/configuration.rb b/lib/overcommit/configuration.rb index f5151031..cb8b08c3 100644 --- a/lib/overcommit/configuration.rb +++ b/lib/overcommit/configuration.rb @@ -5,7 +5,7 @@ module Overcommit # Stores configuration for Overcommit and the hooks it runs. - class Configuration # rubocop:disable Metrics/ClassLength + class Configuration # rubocop:disable ClassLength # Creates a configuration from the given hash. # # @param hash [Hash] loaded YAML config file as a hash @@ -198,6 +198,13 @@ def signature_changed? signature != stored_signature end + # Return whether the signature for this configuration has been verified. + # + # @return [true,false] + def signature_verified? + !signature_changed? || Overcommit::Signature.verified?(object_signature) + end + # Return whether a previous signature has been recorded for this # configuration. # @@ -214,39 +221,22 @@ def verify_signatures? return false if ENV['OVERCOMMIT_NO_VERIFY'] return true if @hash['verify_signatures'] != false - scope = Overcommit::GitConfig.signature_scope - result = Overcommit::Utils.execute( - %W[git config #{scope} --get #{verify_signature_config_key}] - ) - - if result.status == 1 # Key doesn't exist - return true - elsif result.status != 0 - raise Overcommit::Exceptions::GitConfigError, - "Unable to read from local repo git config: #{result.stderr}" + verify = Overcommit::GitRepo.get_local_config(verify_signature_config_key) + if verify.nil? + true + else + # We don't cast since we want to allow anything to count as "true" except + # a literal zero + result.stdout.strip != '0' end - - # We don't cast since we want to allow anything to count as "true" except - # a literal zero - result.stdout.strip != '0' end # Update the currently stored signature for this hook. def update_signature! - scope = Overcommit::GitConfig.signature_scope - result = Overcommit::Utils.execute( - %W[git config #{scope}] + [signature_config_key, signature] - ) - + Overcommit::Signature.verify(object_signature) + Overcommit::GitRepo.set_local_config(signature_config_key, signature) verify_signature_value = @hash['verify_signatures'] ? 1 : 0 - result &&= Overcommit::Utils.execute( - %W[git config #{scope} #{verify_signature_config_key} #{verify_signature_value}] - ) - - unless result.success? - raise Overcommit::Exceptions::GitConfigError, - "Unable to write to local repo git config: #{result.stderr}" - end + Overcommit::GitRepo.set_local_config(verify_signature_config_key, verify_signature_value) end protected @@ -317,29 +307,32 @@ def smart_merge(parent, child) # # @return [String] def signature - Digest::SHA256.hexdigest(@hash.to_json) + Overcommit::Signature.sign(@hash.to_json) end - # Returns the stored signature of this repo's Overcommit configuration. + # Returns the git object ID of this configuration. + # + # @return [String] + def git_object_id + Overcommit::GitRepo.blob_id(@hash.to_json) + end + + # Returns the object signature of this configuration. + # + # @return [Hash] + def object_signature + { git_object_id => signature } + end + + # Returns the last stored signature of this repo's Overcommit configuration. # # This is intended to be compared against the current signature of this # configuration object. # # @return [String] def stored_signature - scope = Overcommit::GitConfig.signature_scope - result = Overcommit::Utils.execute( - %W[git config #{scope} --get] + [signature_config_key] - ) - - if result.status == 1 # Key doesn't exist - return '' - elsif result.status != 0 - raise Overcommit::Exceptions::GitConfigError, - "Unable to read from local repo git config: #{result.stderr}" - end - - result.stdout.chomp + signature = Overcommit::GitRepo.get_local_config(signature_config_key) + signature || '' end def signature_config_key diff --git a/lib/overcommit/configuration_loader.rb b/lib/overcommit/configuration_loader.rb index 8fd68a01..5473e8ce 100644 --- a/lib/overcommit/configuration_loader.rb +++ b/lib/overcommit/configuration_loader.rb @@ -95,7 +95,7 @@ def verify_signatures(config) "No previously recorded signature for configuration file.\n" \ 'Run `overcommit --sign` if you trust the hooks in this repository.' - elsif config.signature_changed? + elsif !config.signature_verified? raise Overcommit::Exceptions::ConfigurationSignatureChanged, "Signature of configuration file has changed!\n" \ "Run `overcommit --sign` once you've verified the configuration changes." diff --git a/lib/overcommit/exceptions.rb b/lib/overcommit/exceptions.rb index 050982fb..2feebf0f 100644 --- a/lib/overcommit/exceptions.rb +++ b/lib/overcommit/exceptions.rb @@ -19,6 +19,12 @@ class GitSubmoduleError < Error; end # Raised when there was a problem reading git revision information with `rev-list`. class GitRevListError < Error; end + # Raised when there was a problem computing the ID of git repository objects. + class GitHashObjectError < StandardError; end + + # Raised when there was a problem retrieving information from git repository objects. + class GitCatFileError < StandardError; end + # Raised when a {HookContext} is unable to setup the environment before a run. class HookSetupFailed < Error; end diff --git a/lib/overcommit/git_repo.rb b/lib/overcommit/git_repo.rb index 737b9b90..1591a1ec 100644 --- a/lib/overcommit/git_repo.rb +++ b/lib/overcommit/git_repo.rb @@ -303,5 +303,75 @@ def branches_containing_commit(commit_ref) def current_branch `git symbolic-ref --short -q HEAD`.chomp end + + # Get the value of a local configuration option. + # + # @param name [String] the name of the option + # @return [String, nil] the value of the option or _nil_ if the option does not exist + def get_local_config(name) + result = Overcommit::Utils.execute( + %w[git config --local --get] << name + ) + if result.status == 1 # Key doesn't exist + return nil + elsif result.status != 0 + raise Overcommit::Exceptions::GitConfigError, + "Unable to read from local repo git config: #{result.stderr}" + end + + result.stdout.chomp + end + + # Sets the value of a local configuration option. + # + # @param name [String] the name of the option + # @param value [String] the value to set + def set_local_config(name, value) + result = Overcommit::Utils.execute( + %w[git config --local] + [name, value] + ) + unless result.success? + raise Overcommit::Exceptions::GitConfigError, + "Unable to write to local repo git config: #{result.stderr}" + end + end + + # Computes the git object ID for the given blob contents and optionally writes that blob to the + # git repository. + # + # @param blob [String] the blob contents + # @param write [Boolean] true if this blob should be written to the git repository + # @return [String] the git object ID + def blob_id(blob, write = false) + write_args = write ? %w[-w] : [] + result = Overcommit::Utils.execute( + %w[git hash-object] + write_args + %w[--stdin], + input: blob + ) + if result.success? + result.stdout.chomp + else + raise Overcommit::Exceptions::GitHashObjectError, + "Failed to compute blob ID: #{result.stderr}" + end + end + + # Retrieves the contents of a blob from the git repository. + # + # @param blob_id [String] the blob ID + # @return [String, nil] the contents of the blob or _nil_ if it does not exist + def blob_contents(blob_id) + result = Overcommit::Utils.execute( + %w[git cat-file -p] << "#{blob_id}^{blob}" + ) + if result.success? + result.stdout + elsif result.status == 128 # 128 is returned when the blob does not exist. + nil + else + raise Overcommit::Exceptions::GitCatFileError, + "Failed to get blob contents: #{result.stderr}" + end + end end end diff --git a/lib/overcommit/signature.rb b/lib/overcommit/signature.rb new file mode 100644 index 00000000..80349495 --- /dev/null +++ b/lib/overcommit/signature.rb @@ -0,0 +1,85 @@ +# frozen_string_literal: true + +require 'digest' + +module Overcommit + # Calculates, stores, and retrieves stored signatures of git objects. + module Signature + # Calculates the signature of some _string_. + # + # @param arg [String] + # @return [String] + def self.sign(arg) + Digest::SHA256.hexdigest(arg) + end + + # Stores the object signatures as verified. + # + # @param object_signatures [Hash] a hash of git object hashes to their signatures + def verify(object_signatures) + blob = to_blob(object_signatures) + Overcommit::GitRepo.blob_id(blob, write: true) + end + + # Have the given object signatures been verified? + # + # @param object_signatures [Hash] a hash of git object hashes to their signatures + # @return [true,false] + def verified?(object_signatures) + blob = to_blob(object_signatures) + object_signatures_id = Overcommit::GitRepo.blob_id(blob) + existing_blob = Overcommit::GitRepo.blob_contents(object_signatures_id) + # Compare the contents in case of a SHA1 collision. + existing_blob == blob + end + + private + + # Converts a Hash of git object hashes and their signatures to a signature blob. + # + # @example + # rubocop:disable Metrics/LineLength + # Converts from: + # { + # "333EC19DD1707BA9D10EB18913C7214D701691BB" => "2938858697BE5C7552CD9463DA010E01F01AC3B8AD65FE584975CE4F2FFC1E7E", + # "426637B5CCDA600060C504CCB133D1976576E594" => "9526B9B463BDC1E856C12518998C704D070C0C217F2896959DB22DBEE7345684" + # } + # to: + # <<~SIGNATURES.chomp + # 333EC19DD1707BA9D10EB18913C7214D701691BB 2938858697BE5C7552CD9463DA010E01F01AC3B8AD65FE584975CE4F2FFC1E7E + # 426637B5CCDA600060C504CCB133D1976576E594 9526B9B463BDC1E856C12518998C704D070C0C217F2896959DB22DBEE7345684 + # SIGNATURES + # rubocop:enable Metrics/LineLength + # + # @param object_signatures [Hash] + # @return [String] + def to_blob(object_signatures) + lines = object_signatures.map do |git_object_hash, signature| + "#{git_object_hash} #{signature}" + end + lines.join("\n") + end + + # Converts a signature blob to a Hash of git object hashes and their signatures. + # + # @example + # rubocop:disable Metrics/LineLength + # Converts from: + # <<~SIGNATURES.chomp + # 333EC19DD1707BA9D10EB18913C7214D701691BB 2938858697BE5C7552CD9463DA010E01F01AC3B8AD65FE584975CE4F2FFC1E7E + # 426637B5CCDA600060C504CCB133D1976576E594 9526B9B463BDC1E856C12518998C704D070C0C217F2896959DB22DBEE7345684 + # SIGNATURES + # to: + # { + # "333EC19DD1707BA9D10EB18913C7214D701691BB" => "2938858697BE5C7552CD9463DA010E01F01AC3B8AD65FE584975CE4F2FFC1E7E", + # "426637B5CCDA600060C504CCB133D1976576E594" => "9526B9B463BDC1E856C12518998C704D070C0C217F2896959DB22DBEE7345684" + # } + # rubocop:enable Metrics/LineLength + # + # @param blob [String] + # @return [Hash] + def to_hash(blob) + Hash[blob.split('\n').each_slice.map { |line| line.split(' ') }] + end + end +end From 7f520dda4ae7a8223c68953beb803d2e0502d24f Mon Sep 17 00:00:00 2001 From: Jason Pickens Date: Sun, 18 Aug 2019 22:25:43 +1200 Subject: [PATCH 2/8] Store hook signatures as git blobs --- lib/overcommit/configuration.rb | 13 +--- .../hook_loader/plugin_hook_loader.rb | 16 ++--- lib/overcommit/hook_signer.rb | 67 +++++++++---------- lib/overcommit/signature.rb | 25 +++++-- spec/overcommit/hook_signer_spec.rb | 2 +- 5 files changed, 64 insertions(+), 59 deletions(-) diff --git a/lib/overcommit/configuration.rb b/lib/overcommit/configuration.rb index cb8b08c3..1a235a3a 100644 --- a/lib/overcommit/configuration.rb +++ b/lib/overcommit/configuration.rb @@ -190,19 +190,12 @@ def plugin_hook?(hook_context_or_type, hook_name) File.exist?(File.join(plugin_directory, hook_type_name, "#{hook_name}.rb")) end - # Return whether the signature for this configuration has changed since it - # was last calculated. - # - # @return [true,false] - def signature_changed? - signature != stored_signature - end - # Return whether the signature for this configuration has been verified. # # @return [true,false] def signature_verified? - !signature_changed? || Overcommit::Signature.verified?(object_signature) + signature_unchanged = signature == stored_signature + signature_unchanged || Overcommit::Signature.verified?(object_signature) end # Return whether a previous signature has been recorded for this @@ -321,7 +314,7 @@ def git_object_id # # @return [Hash] def object_signature - { git_object_id => signature } + Overcommit::Signature.object_signature(@hash.to_json) end # Returns the last stored signature of this repo's Overcommit configuration. diff --git a/lib/overcommit/hook_loader/plugin_hook_loader.rb b/lib/overcommit/hook_loader/plugin_hook_loader.rb index d168a0de..12d4fe45 100644 --- a/lib/overcommit/hook_loader/plugin_hook_loader.rb +++ b/lib/overcommit/hook_loader/plugin_hook_loader.rb @@ -7,7 +7,7 @@ module Overcommit::HookLoader # is running in. class PluginHookLoader < Base def load_hooks - check_for_modified_plugins if @config.verify_signatures? + check_for_unverified_plugins if @config.verify_signatures? hooks = plugin_paths.map do |plugin_path| require plugin_path @@ -22,9 +22,9 @@ def load_hooks end def update_signatures - log.success('No plugin signatures have changed') if modified_plugins.empty? + log.success('All plugin signatures have been verified') if unverified_plugins.empty? - modified_plugins.each do |plugin| + unverified_plugins.each do |plugin| plugin.update_signature! log.warning "Updated signature of plugin #{plugin.hook_name}" end @@ -47,20 +47,20 @@ def ad_hoc_hook_names @config.enabled_ad_hoc_hooks(@context) end - def modified_plugins + def unverified_plugins (plugin_hook_names + ad_hoc_hook_names). map { |hook_name| Overcommit::HookSigner.new(hook_name, @config, @context) }. - select(&:signature_changed?) + reject(&:signature_verified?) end - def check_for_modified_plugins - return if modified_plugins.empty? + def check_for_unverified_plugins + return if unverified_plugins.empty? log.bold_warning "The following #{@context.hook_script_name} plugins " \ 'have been added, changed, or had their configuration modified:' log.newline - modified_plugins.each do |signer| + unverified_plugins.each do |signer| log.warning " * #{signer.hook_name} in #{signer.hook_path}" end diff --git a/lib/overcommit/hook_signer.rb b/lib/overcommit/hook_signer.rb index 67870488..ebf408d0 100644 --- a/lib/overcommit/hook_signer.rb +++ b/lib/overcommit/hook_signer.rb @@ -52,31 +52,23 @@ def hook_path def signable_file?(file) return unless file - sep = Overcommit::OS.windows? ? '\\' : File::SEPARATOR file.start_with?(".#{sep}") || file.start_with?(Overcommit::Utils.repo_root) end - # Return whether the signature for this hook has changed since it was last - # calculated. + # Return whether the signature for this hook has been verified. # # @return [true,false] - def signature_changed? - signature != stored_signature + def signature_verified? + signature_unchanged = signature == stored_signature + signature_unchanged || Overcommit::Signature.verified?(object_signatures) end # Update the current stored signature for this hook. def update_signature! - scope = Overcommit::GitConfig.signature_scope - result = Overcommit::Utils.execute( - %W[git config #{scope}] + [signature_config_key, signature] - ) - - unless result.success? - raise Overcommit::Exceptions::GitConfigError, - "Unable to write to local repo git config: #{result.stderr}" - end + Overcommit::Signature.verify(object_signatures) + Overcommit::GitRepo.set_local_config(signature_config_key, signature) end private @@ -87,16 +79,32 @@ def update_signature! # This way, if either the plugin code changes or its configuration changes, # the hash will change and we can alert the user to this change. def signature - hook_config = @config.for_hook(@hook_name, @context.hook_class_name). - dup. - tap { |config| IGNORED_CONFIG_KEYS.each { |k| config.delete(k) } } + Overcommit::Signature.sign_signatures(object_signatures) + end - content_to_sign = - if signable_file?(hook_path) && Overcommit::GitRepo.tracked?(hook_path) - hook_contents - end + # Returns the object signatures of this configuration. + # + # @return [Hash] + def object_signatures + sig = Overcommit::Signature.object_signature(hook_config.to_json) + content = content_to_sign + if content.nil? + sig + else + sig.merge(Overcommit::Signature.object_signature(content)) + end + end + + def hook_config + @config.for_hook(@hook_name, @context.hook_class_name). + dup. + tap { |config| IGNORED_CONFIG_KEYS.each { |k| config.delete(k) } } + end - Digest::SHA256.hexdigest(content_to_sign.to_s + hook_config.to_s) + def content_to_sign + if signable_file?(hook_path) && Overcommit::GitRepo.tracked?(hook_path) + hook_contents + end end def hook_contents @@ -104,19 +112,8 @@ def hook_contents end def stored_signature - scope = Overcommit::GitConfig.signature_scope - result = Overcommit::Utils.execute( - %W[git config #{scope} --get] + [signature_config_key] - ) - - if result.status == 1 # Key doesn't exist - return '' - elsif result.status != 0 - raise Overcommit::Exceptions::GitConfigError, - "Unable to read from local repo git config: #{result.stderr}" - end - - result.stdout.chomp + signature = Overcommit::GitRepo.get_local_config(signature_config_key) + signature || '' end def signature_config_key diff --git a/lib/overcommit/signature.rb b/lib/overcommit/signature.rb index 80349495..7a665a04 100644 --- a/lib/overcommit/signature.rb +++ b/lib/overcommit/signature.rb @@ -5,7 +5,7 @@ module Overcommit # Calculates, stores, and retrieves stored signatures of git objects. module Signature - # Calculates the signature of some _string_. + # Calculates the signature of some _String_. # # @param arg [String] # @return [String] @@ -13,10 +13,25 @@ def self.sign(arg) Digest::SHA256.hexdigest(arg) end + # Calculates the signature of some _Hash_. + # + # @param arg [Hash] + # @return [String] + def self.sign_signatures(arg) + sign(to_blob(arg)) + end + + # Returns the object signature for some blob. + # + # @return [Hash] + def self.object_signature(blob) + { Overcommit::GitRepo.blob_id(blob) => sign(blob) } + end + # Stores the object signatures as verified. # # @param object_signatures [Hash] a hash of git object hashes to their signatures - def verify(object_signatures) + def self.verify(object_signatures) blob = to_blob(object_signatures) Overcommit::GitRepo.blob_id(blob, write: true) end @@ -25,7 +40,7 @@ def verify(object_signatures) # # @param object_signatures [Hash] a hash of git object hashes to their signatures # @return [true,false] - def verified?(object_signatures) + def self.verified?(object_signatures) blob = to_blob(object_signatures) object_signatures_id = Overcommit::GitRepo.blob_id(blob) existing_blob = Overcommit::GitRepo.blob_contents(object_signatures_id) @@ -53,7 +68,7 @@ def verified?(object_signatures) # # @param object_signatures [Hash] # @return [String] - def to_blob(object_signatures) + def self.to_blob(object_signatures) lines = object_signatures.map do |git_object_hash, signature| "#{git_object_hash} #{signature}" end @@ -78,7 +93,7 @@ def to_blob(object_signatures) # # @param blob [String] # @return [Hash] - def to_hash(blob) + def self.to_hash(blob) Hash[blob.split('\n').each_slice.map { |line| line.split(' ') }] end end diff --git a/spec/overcommit/hook_signer_spec.rb b/spec/overcommit/hook_signer_spec.rb index 98862fd0..3d737982 100644 --- a/spec/overcommit/hook_signer_spec.rb +++ b/spec/overcommit/hook_signer_spec.rb @@ -26,7 +26,7 @@ def run let(:modified_hook_contents) { hook_contents } - subject { signer.signature_changed? } + subject { signer.signature_verified? } around do |example| repo do From 672d13cd821fafae757f3fec413cca996c2cead4 Mon Sep 17 00:00:00 2001 From: Jason Pickens Date: Sun, 18 Aug 2019 23:09:45 +1200 Subject: [PATCH 3/8] Store hook signatures as git blobs --- lib/overcommit/signature.rb | 166 ++++++++++++++++++------------------ 1 file changed, 84 insertions(+), 82 deletions(-) diff --git a/lib/overcommit/signature.rb b/lib/overcommit/signature.rb index 7a665a04..a3e40dd8 100644 --- a/lib/overcommit/signature.rb +++ b/lib/overcommit/signature.rb @@ -5,96 +5,98 @@ module Overcommit # Calculates, stores, and retrieves stored signatures of git objects. module Signature - # Calculates the signature of some _String_. - # - # @param arg [String] - # @return [String] - def self.sign(arg) - Digest::SHA256.hexdigest(arg) - end + class << self + # Calculates the signature of some _String_. + # + # @param arg [String] + # @return [String] + def sign(arg) + Digest::SHA256.hexdigest(arg) + end - # Calculates the signature of some _Hash_. - # - # @param arg [Hash] - # @return [String] - def self.sign_signatures(arg) - sign(to_blob(arg)) - end + # Calculates the signature of some _Hash_. + # + # @param arg [Hash] + # @return [String] + def sign_signatures(arg) + sign(to_blob(arg)) + end - # Returns the object signature for some blob. - # - # @return [Hash] - def self.object_signature(blob) - { Overcommit::GitRepo.blob_id(blob) => sign(blob) } - end + # Returns the object signature for some blob. + # + # @return [Hash] + def object_signature(blob) + { Overcommit::GitRepo.blob_id(blob) => sign(blob) } + end - # Stores the object signatures as verified. - # - # @param object_signatures [Hash] a hash of git object hashes to their signatures - def self.verify(object_signatures) - blob = to_blob(object_signatures) - Overcommit::GitRepo.blob_id(blob, write: true) - end + # Stores the object signatures as verified. + # + # @param object_signatures [Hash] a hash of git object hashes to their signatures + def verify(object_signatures) + blob = to_blob(object_signatures) + Overcommit::GitRepo.blob_id(blob, write: true) + end - # Have the given object signatures been verified? - # - # @param object_signatures [Hash] a hash of git object hashes to their signatures - # @return [true,false] - def self.verified?(object_signatures) - blob = to_blob(object_signatures) - object_signatures_id = Overcommit::GitRepo.blob_id(blob) - existing_blob = Overcommit::GitRepo.blob_contents(object_signatures_id) - # Compare the contents in case of a SHA1 collision. - existing_blob == blob - end + # Have the given object signatures been verified? + # + # @param object_signatures [Hash] a hash of git object hashes to their signatures + # @return [true,false] + def verified?(object_signatures) + blob = to_blob(object_signatures) + object_signatures_id = Overcommit::GitRepo.blob_id(blob) + existing_blob = Overcommit::GitRepo.blob_contents(object_signatures_id) + # Compare the contents in case of a SHA1 collision. + existing_blob == blob + end - private + private - # Converts a Hash of git object hashes and their signatures to a signature blob. - # - # @example - # rubocop:disable Metrics/LineLength - # Converts from: - # { - # "333EC19DD1707BA9D10EB18913C7214D701691BB" => "2938858697BE5C7552CD9463DA010E01F01AC3B8AD65FE584975CE4F2FFC1E7E", - # "426637B5CCDA600060C504CCB133D1976576E594" => "9526B9B463BDC1E856C12518998C704D070C0C217F2896959DB22DBEE7345684" - # } - # to: - # <<~SIGNATURES.chomp - # 333EC19DD1707BA9D10EB18913C7214D701691BB 2938858697BE5C7552CD9463DA010E01F01AC3B8AD65FE584975CE4F2FFC1E7E - # 426637B5CCDA600060C504CCB133D1976576E594 9526B9B463BDC1E856C12518998C704D070C0C217F2896959DB22DBEE7345684 - # SIGNATURES - # rubocop:enable Metrics/LineLength - # - # @param object_signatures [Hash] - # @return [String] - def self.to_blob(object_signatures) - lines = object_signatures.map do |git_object_hash, signature| - "#{git_object_hash} #{signature}" + # Converts a Hash of git object hashes and their signatures to a signature blob. + # + # @example + # rubocop:disable Metrics/LineLength + # Converts from: + # { + # "333EC19DD1707BA9D10EB18913C7214D701691BB" => "2938858697BE5C7552CD9463DA010E01F01AC3B8AD65FE584975CE4F2FFC1E7E", + # "426637B5CCDA600060C504CCB133D1976576E594" => "9526B9B463BDC1E856C12518998C704D070C0C217F2896959DB22DBEE7345684" + # } + # to: + # <<~SIGNATURES.chomp + # 333EC19DD1707BA9D10EB18913C7214D701691BB 2938858697BE5C7552CD9463DA010E01F01AC3B8AD65FE584975CE4F2FFC1E7E + # 426637B5CCDA600060C504CCB133D1976576E594 9526B9B463BDC1E856C12518998C704D070C0C217F2896959DB22DBEE7345684 + # SIGNATURES + # rubocop:enable Metrics/LineLength + # + # @param object_signatures [Hash] + # @return [String] + def to_blob(object_signatures) + lines = object_signatures.map do |git_object_hash, signature| + "#{git_object_hash} #{signature}" + end + lines.join("\n") end - lines.join("\n") - end - # Converts a signature blob to a Hash of git object hashes and their signatures. - # - # @example - # rubocop:disable Metrics/LineLength - # Converts from: - # <<~SIGNATURES.chomp - # 333EC19DD1707BA9D10EB18913C7214D701691BB 2938858697BE5C7552CD9463DA010E01F01AC3B8AD65FE584975CE4F2FFC1E7E - # 426637B5CCDA600060C504CCB133D1976576E594 9526B9B463BDC1E856C12518998C704D070C0C217F2896959DB22DBEE7345684 - # SIGNATURES - # to: - # { - # "333EC19DD1707BA9D10EB18913C7214D701691BB" => "2938858697BE5C7552CD9463DA010E01F01AC3B8AD65FE584975CE4F2FFC1E7E", - # "426637B5CCDA600060C504CCB133D1976576E594" => "9526B9B463BDC1E856C12518998C704D070C0C217F2896959DB22DBEE7345684" - # } - # rubocop:enable Metrics/LineLength - # - # @param blob [String] - # @return [Hash] - def self.to_hash(blob) - Hash[blob.split('\n').each_slice.map { |line| line.split(' ') }] + # Converts a signature blob to a Hash of git object hashes and their signatures. + # + # @example + # rubocop:disable Metrics/LineLength + # Converts from: + # <<~SIGNATURES.chomp + # 333EC19DD1707BA9D10EB18913C7214D701691BB 2938858697BE5C7552CD9463DA010E01F01AC3B8AD65FE584975CE4F2FFC1E7E + # 426637B5CCDA600060C504CCB133D1976576E594 9526B9B463BDC1E856C12518998C704D070C0C217F2896959DB22DBEE7345684 + # SIGNATURES + # to: + # { + # "333EC19DD1707BA9D10EB18913C7214D701691BB" => "2938858697BE5C7552CD9463DA010E01F01AC3B8AD65FE584975CE4F2FFC1E7E", + # "426637B5CCDA600060C504CCB133D1976576E594" => "9526B9B463BDC1E856C12518998C704D070C0C217F2896959DB22DBEE7345684" + # } + # rubocop:enable Metrics/LineLength + # + # @param blob [String] + # @return [Hash] + def to_hash(blob) + Hash[blob.split('\n').each_slice.map { |line| line.split(' ') }] + end end end end From 67e8b53ab18958e2af8850b41fab272b30092024 Mon Sep 17 00:00:00 2001 From: Jason Pickens Date: Sun, 18 Aug 2019 23:16:23 +1200 Subject: [PATCH 4/8] Fix git config arguments --- lib/overcommit/git_repo.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/overcommit/git_repo.rb b/lib/overcommit/git_repo.rb index 1591a1ec..6e40bd74 100644 --- a/lib/overcommit/git_repo.rb +++ b/lib/overcommit/git_repo.rb @@ -328,7 +328,7 @@ def get_local_config(name) # @param value [String] the value to set def set_local_config(name, value) result = Overcommit::Utils.execute( - %w[git config --local] + [name, value] + %W[git config --local #{name} #{value}] ) unless result.success? raise Overcommit::Exceptions::GitConfigError, @@ -362,7 +362,7 @@ def blob_id(blob, write = false) # @return [String, nil] the contents of the blob or _nil_ if it does not exist def blob_contents(blob_id) result = Overcommit::Utils.execute( - %w[git cat-file -p] << "#{blob_id}^{blob}" + %W[git cat-file -p #{blob_id}^{blob}] ) if result.success? result.stdout From b2a8623d023b17c495a79c9ab84c2981f2cb0f90 Mon Sep 17 00:00:00 2001 From: Jason Pickens Date: Sun, 18 Aug 2019 23:23:15 +1200 Subject: [PATCH 5/8] Fix verify signatures result --- lib/overcommit/configuration.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/overcommit/configuration.rb b/lib/overcommit/configuration.rb index 1a235a3a..a90d9231 100644 --- a/lib/overcommit/configuration.rb +++ b/lib/overcommit/configuration.rb @@ -220,7 +220,7 @@ def verify_signatures? else # We don't cast since we want to allow anything to count as "true" except # a literal zero - result.stdout.strip != '0' + verify.strip != '0' end end From 2d67a62c44acc5a74f5349c7c3fa35146b266a1f Mon Sep 17 00:00:00 2001 From: Jason Pickens Date: Sat, 31 Aug 2019 14:10:15 +1200 Subject: [PATCH 6/8] Configure RSpec example status persistence file --- .gitignore | 1 + spec/spec_helper.rb | 2 ++ 2 files changed, 3 insertions(+) diff --git a/.gitignore b/.gitignore index 7ea8f1f7..8f7444a0 100644 --- a/.gitignore +++ b/.gitignore @@ -5,3 +5,4 @@ pkg/ .idea .history/ .vscode/ +/spec_example_status diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index e1416265..74f580bc 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -64,4 +64,6 @@ Overcommit::Utils.instance_variable_set(:"@#{var}", nil) end end + + config.example_status_persistence_file_path = 'spec_example_status' end From 724ea09ca5035d8eacdc2614f276b6983361ad06 Mon Sep 17 00:00:00 2001 From: Jason Pickens Date: Sun, 1 Sep 2019 16:08:38 +1200 Subject: [PATCH 7/8] Fix hook signer tests --- lib/overcommit/git_repo.rb | 2 +- spec/overcommit/hook_signer_spec.rb | 81 +++++++++++++++++++++++------ 2 files changed, 65 insertions(+), 18 deletions(-) diff --git a/lib/overcommit/git_repo.rb b/lib/overcommit/git_repo.rb index 6e40bd74..0e4ad363 100644 --- a/lib/overcommit/git_repo.rb +++ b/lib/overcommit/git_repo.rb @@ -365,7 +365,7 @@ def blob_contents(blob_id) %W[git cat-file -p #{blob_id}^{blob}] ) if result.success? - result.stdout + result.stdout.chomp elsif result.status == 128 # 128 is returned when the blob does not exist. nil else diff --git a/spec/overcommit/hook_signer_spec.rb b/spec/overcommit/hook_signer_spec.rb index 3d737982..8bef1090 100644 --- a/spec/overcommit/hook_signer_spec.rb +++ b/spec/overcommit/hook_signer_spec.rb @@ -3,7 +3,7 @@ require 'spec_helper' describe Overcommit::HookSigner do - describe '#signature_changed?' do + describe '#signature_verified?' do let(:config) { double('config') } let(:context) { double('context') } @@ -11,10 +11,11 @@ described_class.new('.git-hooks/pre_commit/some_path.rb', config, context) end - let(:hook_config) { { 'enabled' => false } } + let(:original_hook_config) { { 'enabled' => false } } + let(:hook_config) { original_hook_config } let(:modified_hook_config) { hook_config } - let(:hook_contents) { <<-RUBY } + let(:original_hook_contents) { <<-RUBY } module Overcommit::Hook::PreCommit class SomeHook def run @@ -23,7 +24,7 @@ def run end end RUBY - + let(:hook_contents) { original_hook_contents } let(:modified_hook_contents) { hook_contents } subject { signer.signature_verified? } @@ -49,17 +50,7 @@ def run signer.stub(:hook_contents).and_return(modified_hook_contents) end - context 'when the hook code and config are the same' do - it { should == false } - - context 'and the user has specified they wish to skip the hook' do - let(:modified_hook_config) { hook_config.merge('skip' => true) } - - it { should == false } - end - end - - context 'when the hook code has changed' do + shared_context 'hook code has changed' do let(:modified_hook_contents) { <<-RUBY } module Overcommit::Hook::PreCommit class SomeHook @@ -69,14 +60,70 @@ def run end end RUBY + end - it { should == true } + shared_context 'hook code has changed back' do + include_context 'hook code has changed' + before do + signer.update_signature! + config.stub(:for_hook).and_return(modified_hook_config) + signer.stub(:hook_contents).and_return(original_hook_contents) + end end - context 'when the hook config has changed' do + shared_context 'hook config has changed' do let(:modified_hook_config) { { 'enabled' => true } } + end + shared_context 'hook config has changed back' do + include_context 'hook config has changed' + before do + signer.update_signature! + config.stub(:for_hook).and_return(original_hook_config) + signer.stub(:hook_contents).and_return(modified_hook_contents) + end + end + + shared_examples 'signature has been verified' do it { should == true } end + + shared_examples 'signature has not been verified' do + it { should == false } + end + + context 'when the hook code and config are the same' do + include_examples 'signature has been verified' + + context 'and the user has specified they wish to skip the hook' do + let(:modified_hook_config) { hook_config.merge('skip' => true) } + + include_examples 'signature has been verified' + end + end + + context 'when the hook code has changed' do + include_context 'hook code has changed' + + include_examples 'signature has not been verified' + end + + context 'when the hook code has changed back to a previously verified signature' do + include_context 'hook code has changed back' + + include_examples 'signature has been verified' + end + + context 'when the hook config has changed' do + include_context 'hook config has changed' + + include_examples 'signature has not been verified' + end + + context 'when the hook config has changed back' do + include_context 'hook config has changed back' + + include_examples 'signature has been verified' + end end end From acb75aade766851aeaa8dabb09547359796272b1 Mon Sep 17 00:00:00 2001 From: Shane da Silva Date: Mon, 9 Nov 2026 22:18:36 -0800 Subject: [PATCH 8/8] Remember local configuration and hook approvals securely Replace Git object storage with SHA256-indexed local approval files. Migrate legacy signatures, preserve worktree scoping, and support simple revocation without growing Git config. Cover checkout and rebase regressions, forged remote objects, migration, garbage collection, concurrency, and error handling. --- .gitignore | 1 - CHANGELOG.md | 2 + README.md | 47 +++-- lib/overcommit/configuration.rb | 70 +++---- lib/overcommit/exceptions.rb | 9 +- lib/overcommit/git_repo.rb | 70 ------- lib/overcommit/hook_signer.rb | 49 ++--- lib/overcommit/signature.rb | 143 ++++++-------- spec/integration/signature_history_spec.rb | 154 +++++++++++++++ spec/integration/worktree_signing_spec.rb | 29 ++- spec/overcommit/signature_spec.rb | 212 +++++++++++++++++++++ spec/spec_helper.rb | 2 - 12 files changed, 530 insertions(+), 258 deletions(-) create mode 100644 spec/integration/signature_history_spec.rb create mode 100644 spec/overcommit/signature_spec.rb diff --git a/.gitignore b/.gitignore index 8f7444a0..7ea8f1f7 100644 --- a/.gitignore +++ b/.gitignore @@ -5,4 +5,3 @@ pkg/ .idea .history/ .vscode/ -/spec_example_status diff --git a/CHANGELOG.md b/CHANGELOG.md index c620806d..dc4ced45 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,8 @@ ## 0.75.0 +* Remember all locally approved configuration and hook signatures across rebases + * Gracefully handle hook loading on systems with non-UTF8 locale * Re-enable running test suite on Windows environments diff --git a/README.md b/README.md index 37abc7c7..59bc5ff2 100644 --- a/README.md +++ b/README.md @@ -721,22 +721,37 @@ that executes some malicious code. When you fetch and checkout this pull request, the `post-checkout` hook will be run on your machine, along with the malicious code that you just checked out. -Overcommit attempts to address this problem by storing a signature of your -configuration and all hook plugin code since the last time it ran. When the -signature changes, a warning is displayed alerting you to which plugins have -changed. It is then up to you to manually verify that the changes are not -malicious, and then continue running the hooks. - -The signature is derived from the contents of the plugin's source code itself -and any configuration for the plugin. Thus a change to the plugin's source -code or your local repo's `.overcommit.yml` file could result in a signature -change. - -Signatures are stored in your local repo's git configuration. If you use -multiple [worktrees](https://git-scm.com/docs/git-worktree) whose -configurations differ, enable `extensions.worktreeConfig` (`git config -extensions.worktreeConfig true`) so that Overcommit stores signatures per -worktree and signing in one worktree does not invalidate the others. +Overcommit stores signatures of configurations and hook plugin code you have +explicitly approved with `overcommit --sign` and `overcommit --sign `. +Unrecognized signatures trigger a warning so you can verify the changes before +running hooks. All previously approved signatures are remembered, so checking +out an older revision or rebasing through approved configurations does not +require signing them again. + +A plugin's signature covers its source code together with its configuration. +Approving two versions does not approve other combinations of their code and +configuration. + +Approvals are stored under `overcommit-signatures` in your repository's Git +directory, outside Git's object database. They cannot be committed, pushed, or +fetched, and Git garbage collection does not remove them. Each approval is a +small file at `/`, so lookups +do not scan the history. The file contains the original config key (such as +`overcommit.configuration.signature` or `overcommit.PreCommit.MyHook.signature`) +and the approved signature, one per line. There is no history size limit. + +To revoke one approval, delete its file. To revoke all approvals, delete the +`overcommit-signatures` directory. Run `overcommit --sign` and +`overcommit --sign ` to approve the current versions again. Existing +signatures in local Git config migrate automatically on first use; their old +keys are removed so deleting approval files also revokes migrated signatures. + +By default, linked [worktrees](https://git-scm.com/docs/git-worktree) share +history in their common Git directory. Enable `extensions.worktreeConfig` +(`git config extensions.worktreeConfig true`) to keep approvals and the +`verify_signatures` setting separate in each worktree's Git directory. Use +`git rev-parse --git-common-dir` to find shared storage, or +`git rev-parse --git-path overcommit-signatures` to find per-worktree storage. ### Disabling Signature Checking diff --git a/lib/overcommit/configuration.rb b/lib/overcommit/configuration.rb index a90d9231..8d955c86 100644 --- a/lib/overcommit/configuration.rb +++ b/lib/overcommit/configuration.rb @@ -5,7 +5,7 @@ module Overcommit # Stores configuration for Overcommit and the hooks it runs. - class Configuration # rubocop:disable ClassLength + class Configuration # rubocop:disable Metrics/ClassLength # Creates a configuration from the given hash. # # @param hash [Hash] loaded YAML config file as a hash @@ -190,12 +190,11 @@ def plugin_hook?(hook_context_or_type, hook_name) File.exist?(File.join(plugin_directory, hook_type_name, "#{hook_name}.rb")) end - # Return whether the signature for this configuration has been verified. + # Return whether this configuration has been explicitly trusted before. # # @return [true,false] def signature_verified? - signature_unchanged = signature == stored_signature - signature_unchanged || Overcommit::Signature.verified?(object_signature) + signature_store.include?(signature) end # Return whether a previous signature has been recorded for this @@ -203,7 +202,7 @@ def signature_verified? # # @return [true,false] def previous_signature? - !stored_signature.empty? + signature_store.recorded? end # Returns whether this configuration should verify itself by checking the @@ -214,22 +213,36 @@ def verify_signatures? return false if ENV['OVERCOMMIT_NO_VERIFY'] return true if @hash['verify_signatures'] != false - verify = Overcommit::GitRepo.get_local_config(verify_signature_config_key) - if verify.nil? - true - else - # We don't cast since we want to allow anything to count as "true" except - # a literal zero - verify.strip != '0' + scope = Overcommit::GitConfig.signature_scope + result = Overcommit::Utils.execute( + %W[git config #{scope} --get #{verify_signature_config_key}] + ) + + if result.status == 1 # Key doesn't exist + return true + elsif result.status != 0 + raise Overcommit::Exceptions::GitConfigError, + "Unable to read from local repo git config: #{result.stderr}" end + + # We don't cast since we want to allow anything to count as "true" except + # a literal zero + result.stdout.strip != '0' end # Update the currently stored signature for this hook. def update_signature! - Overcommit::Signature.verify(object_signature) - Overcommit::GitRepo.set_local_config(signature_config_key, signature) + signature_store.add(signature) + scope = Overcommit::GitConfig.signature_scope verify_signature_value = @hash['verify_signatures'] ? 1 : 0 - Overcommit::GitRepo.set_local_config(verify_signature_config_key, verify_signature_value) + result = Overcommit::Utils.execute( + %W[git config #{scope} #{verify_signature_config_key} #{verify_signature_value}] + ) + + unless result.success? + raise Overcommit::Exceptions::GitConfigError, + "Unable to write to local repo git config: #{result.stderr}" + end end protected @@ -300,32 +313,11 @@ def smart_merge(parent, child) # # @return [String] def signature - Overcommit::Signature.sign(@hash.to_json) - end - - # Returns the git object ID of this configuration. - # - # @return [String] - def git_object_id - Overcommit::GitRepo.blob_id(@hash.to_json) + Digest::SHA256.hexdigest(@hash.to_json) end - # Returns the object signature of this configuration. - # - # @return [Hash] - def object_signature - Overcommit::Signature.object_signature(@hash.to_json) - end - - # Returns the last stored signature of this repo's Overcommit configuration. - # - # This is intended to be compared against the current signature of this - # configuration object. - # - # @return [String] - def stored_signature - signature = Overcommit::GitRepo.get_local_config(signature_config_key) - signature || '' + def signature_store + @signature_store ||= Overcommit::Signature.new(signature_config_key) end def signature_config_key diff --git a/lib/overcommit/exceptions.rb b/lib/overcommit/exceptions.rb index 2feebf0f..87011023 100644 --- a/lib/overcommit/exceptions.rb +++ b/lib/overcommit/exceptions.rb @@ -13,18 +13,15 @@ class ConfigurationSignatureChanged < Error; end # Raised when trying to read/write to/from the local repo git config fails. class GitConfigError < Error; end + # Raised when local signature history cannot be read or written. + class SignatureError < Error; end + # Raised when there was a problem reading submodule information for a repo. class GitSubmoduleError < Error; end # Raised when there was a problem reading git revision information with `rev-list`. class GitRevListError < Error; end - # Raised when there was a problem computing the ID of git repository objects. - class GitHashObjectError < StandardError; end - - # Raised when there was a problem retrieving information from git repository objects. - class GitCatFileError < StandardError; end - # Raised when a {HookContext} is unable to setup the environment before a run. class HookSetupFailed < Error; end diff --git a/lib/overcommit/git_repo.rb b/lib/overcommit/git_repo.rb index 0e4ad363..737b9b90 100644 --- a/lib/overcommit/git_repo.rb +++ b/lib/overcommit/git_repo.rb @@ -303,75 +303,5 @@ def branches_containing_commit(commit_ref) def current_branch `git symbolic-ref --short -q HEAD`.chomp end - - # Get the value of a local configuration option. - # - # @param name [String] the name of the option - # @return [String, nil] the value of the option or _nil_ if the option does not exist - def get_local_config(name) - result = Overcommit::Utils.execute( - %w[git config --local --get] << name - ) - if result.status == 1 # Key doesn't exist - return nil - elsif result.status != 0 - raise Overcommit::Exceptions::GitConfigError, - "Unable to read from local repo git config: #{result.stderr}" - end - - result.stdout.chomp - end - - # Sets the value of a local configuration option. - # - # @param name [String] the name of the option - # @param value [String] the value to set - def set_local_config(name, value) - result = Overcommit::Utils.execute( - %W[git config --local #{name} #{value}] - ) - unless result.success? - raise Overcommit::Exceptions::GitConfigError, - "Unable to write to local repo git config: #{result.stderr}" - end - end - - # Computes the git object ID for the given blob contents and optionally writes that blob to the - # git repository. - # - # @param blob [String] the blob contents - # @param write [Boolean] true if this blob should be written to the git repository - # @return [String] the git object ID - def blob_id(blob, write = false) - write_args = write ? %w[-w] : [] - result = Overcommit::Utils.execute( - %w[git hash-object] + write_args + %w[--stdin], - input: blob - ) - if result.success? - result.stdout.chomp - else - raise Overcommit::Exceptions::GitHashObjectError, - "Failed to compute blob ID: #{result.stderr}" - end - end - - # Retrieves the contents of a blob from the git repository. - # - # @param blob_id [String] the blob ID - # @return [String, nil] the contents of the blob or _nil_ if it does not exist - def blob_contents(blob_id) - result = Overcommit::Utils.execute( - %W[git cat-file -p #{blob_id}^{blob}] - ) - if result.success? - result.stdout.chomp - elsif result.status == 128 # 128 is returned when the blob does not exist. - nil - else - raise Overcommit::Exceptions::GitCatFileError, - "Failed to get blob contents: #{result.stderr}" - end - end end end diff --git a/lib/overcommit/hook_signer.rb b/lib/overcommit/hook_signer.rb index ebf408d0..3bbf3c6e 100644 --- a/lib/overcommit/hook_signer.rb +++ b/lib/overcommit/hook_signer.rb @@ -52,23 +52,23 @@ def hook_path def signable_file?(file) return unless file + sep = Overcommit::OS.windows? ? '\\' : File::SEPARATOR file.start_with?(".#{sep}") || file.start_with?(Overcommit::Utils.repo_root) end - # Return whether the signature for this hook has been verified. + # Return whether this exact combination of hook code and configuration + # has been explicitly trusted before. # # @return [true,false] def signature_verified? - signature_unchanged = signature == stored_signature - signature_unchanged || Overcommit::Signature.verified?(object_signatures) + signature_store.include?(signature) end - # Update the current stored signature for this hook. + # Remember the current signature without forgetting older approvals. def update_signature! - Overcommit::Signature.verify(object_signatures) - Overcommit::GitRepo.set_local_config(signature_config_key, signature) + signature_store.add(signature) end private @@ -79,41 +79,24 @@ def update_signature! # This way, if either the plugin code changes or its configuration changes, # the hash will change and we can alert the user to this change. def signature - Overcommit::Signature.sign_signatures(object_signatures) - end + hook_config = @config.for_hook(@hook_name, @context.hook_class_name). + dup. + tap { |config| IGNORED_CONFIG_KEYS.each { |k| config.delete(k) } } - # Returns the object signatures of this configuration. - # - # @return [Hash] - def object_signatures - sig = Overcommit::Signature.object_signature(hook_config.to_json) - content = content_to_sign - if content.nil? - sig - else - sig.merge(Overcommit::Signature.object_signature(content)) - end - end - - def hook_config - @config.for_hook(@hook_name, @context.hook_class_name). - dup. - tap { |config| IGNORED_CONFIG_KEYS.each { |k| config.delete(k) } } - end + content_to_sign = + if signable_file?(hook_path) && Overcommit::GitRepo.tracked?(hook_path) + hook_contents + end - def content_to_sign - if signable_file?(hook_path) && Overcommit::GitRepo.tracked?(hook_path) - hook_contents - end + Digest::SHA256.hexdigest(content_to_sign.to_s + hook_config.to_s) end def hook_contents File.read(hook_path) end - def stored_signature - signature = Overcommit::GitRepo.get_local_config(signature_config_key) - signature || '' + def signature_store + @signature_store ||= Overcommit::Signature.new(signature_config_key) end def signature_config_key diff --git a/lib/overcommit/signature.rb b/lib/overcommit/signature.rb index a3e40dd8..a61af9ce 100644 --- a/lib/overcommit/signature.rb +++ b/lib/overcommit/signature.rb @@ -1,102 +1,71 @@ # frozen_string_literal: true require 'digest' +require 'fileutils' module Overcommit - # Calculates, stores, and retrieves stored signatures of git objects. - module Signature - class << self - # Calculates the signature of some _String_. - # - # @param arg [String] - # @return [String] - def sign(arg) - Digest::SHA256.hexdigest(arg) - end + # Repository-local approvals, indexed by hook identity and SHA256 signature. + # These files are outside Git's object database so fetching cannot add trust. + class Signature + def initialize(config_key) + @config_key = config_key + scope = Overcommit::GitConfig.signature_scope + root = if scope == '--worktree' + Overcommit::Utils.git_path('overcommit-signatures') + else + File.join(Overcommit::Utils.git_dir, 'overcommit-signatures') + end + @directory = File.join(root, Digest::SHA256.hexdigest(config_key)) + migrate_legacy_signature(scope) + end + + def include?(signature) + File.read(path(signature)) == contents(signature) + rescue Errno::ENOENT + false + rescue SystemCallError => e + raise Overcommit::Exceptions::SignatureError, "Unable to read signature history: #{e}" + end + + def recorded? + File.directory?(@directory) + end - # Calculates the signature of some _Hash_. - # - # @param arg [Hash] - # @return [String] - def sign_signatures(arg) - sign(to_blob(arg)) - end + def add(signature) + FileUtils.mkdir_p(@directory) + File.write(path(signature), contents(signature), mode: 'w', perm: 0o600) + rescue SystemCallError => e + raise Overcommit::Exceptions::SignatureError, "Unable to write signature history: #{e}" + end - # Returns the object signature for some blob. - # - # @return [Hash] - def object_signature(blob) - { Overcommit::GitRepo.blob_id(blob) => sign(blob) } - end + private - # Stores the object signatures as verified. - # - # @param object_signatures [Hash] a hash of git object hashes to their signatures - def verify(object_signatures) - blob = to_blob(object_signatures) - Overcommit::GitRepo.blob_id(blob, write: true) - end + def path(signature) + File.join(@directory, signature) + end - # Have the given object signatures been verified? - # - # @param object_signatures [Hash] a hash of git object hashes to their signatures - # @return [true,false] - def verified?(object_signatures) - blob = to_blob(object_signatures) - object_signatures_id = Overcommit::GitRepo.blob_id(blob) - existing_blob = Overcommit::GitRepo.blob_contents(object_signatures_id) - # Compare the contents in case of a SHA1 collision. - existing_blob == blob - end + def contents(signature) + "#{@config_key}\n#{signature}\n" + end - private + # Preserve an existing approval before removing its legacy Git config key. + # Removing the key means deleting history files actually revokes trust. + def migrate_legacy_signature(scope) + result = Overcommit::Utils.execute(%W[git config #{scope} --get] + [@config_key]) + return if result.status == 1 + + check_config_result(result) + signature = result.stdout.chomp + add(signature) if signature.match?(/\A[0-9a-f]{64}\z/) + result = Overcommit::Utils.execute(%W[git config #{scope} --unset-all] + [@config_key]) + check_config_result(result) unless result.status == 5 # Already removed by another process. + end - # Converts a Hash of git object hashes and their signatures to a signature blob. - # - # @example - # rubocop:disable Metrics/LineLength - # Converts from: - # { - # "333EC19DD1707BA9D10EB18913C7214D701691BB" => "2938858697BE5C7552CD9463DA010E01F01AC3B8AD65FE584975CE4F2FFC1E7E", - # "426637B5CCDA600060C504CCB133D1976576E594" => "9526B9B463BDC1E856C12518998C704D070C0C217F2896959DB22DBEE7345684" - # } - # to: - # <<~SIGNATURES.chomp - # 333EC19DD1707BA9D10EB18913C7214D701691BB 2938858697BE5C7552CD9463DA010E01F01AC3B8AD65FE584975CE4F2FFC1E7E - # 426637B5CCDA600060C504CCB133D1976576E594 9526B9B463BDC1E856C12518998C704D070C0C217F2896959DB22DBEE7345684 - # SIGNATURES - # rubocop:enable Metrics/LineLength - # - # @param object_signatures [Hash] - # @return [String] - def to_blob(object_signatures) - lines = object_signatures.map do |git_object_hash, signature| - "#{git_object_hash} #{signature}" - end - lines.join("\n") - end + def check_config_result(result) + return if result.success? - # Converts a signature blob to a Hash of git object hashes and their signatures. - # - # @example - # rubocop:disable Metrics/LineLength - # Converts from: - # <<~SIGNATURES.chomp - # 333EC19DD1707BA9D10EB18913C7214D701691BB 2938858697BE5C7552CD9463DA010E01F01AC3B8AD65FE584975CE4F2FFC1E7E - # 426637B5CCDA600060C504CCB133D1976576E594 9526B9B463BDC1E856C12518998C704D070C0C217F2896959DB22DBEE7345684 - # SIGNATURES - # to: - # { - # "333EC19DD1707BA9D10EB18913C7214D701691BB" => "2938858697BE5C7552CD9463DA010E01F01AC3B8AD65FE584975CE4F2FFC1E7E", - # "426637B5CCDA600060C504CCB133D1976576E594" => "9526B9B463BDC1E856C12518998C704D070C0C217F2896959DB22DBEE7345684" - # } - # rubocop:enable Metrics/LineLength - # - # @param blob [String] - # @return [Hash] - def to_hash(blob) - Hash[blob.split('\n').each_slice.map { |line| line.split(' ') }] - end + raise Overcommit::Exceptions::GitConfigError, + "Unable to migrate local repo git config signature: #{result.stderr}" end end end diff --git a/spec/integration/signature_history_spec.rb b/spec/integration/signature_history_spec.rb new file mode 100644 index 00000000..0d4eca07 --- /dev/null +++ b/spec/integration/signature_history_spec.rb @@ -0,0 +1,154 @@ +# frozen_string_literal: true + +require 'spec_helper' +require 'yaml' + +describe 'remembering previously approved configurations and hooks' do + let(:enable_verification) { true } + let(:plugin_path) { '.git-hooks/pre_commit/history_hook.rb' } + let(:original_config) do + { + 'verify_signatures' => true, + 'CommitMsg' => { 'ALL' => { 'enabled' => false } }, + 'PreCommit' => { + 'ALL' => { 'enabled' => false }, + 'HistoryHook' => { 'enabled' => true, 'requires_files' => false }, + }, + } + end + let(:original_plugin) do + <<~RUBY_CODE + module Overcommit::Hook::PreCommit + class HistoryHook < Base + def run + :pass + end + end + end + RUBY_CODE + end + + def run_command(*command) + result = shell(command) + expect(result.status).to eq(0), "#{command.inspect}: #{result.stdout}\n#{result.stderr}" + result + end + + def sign + run_command('overcommit', '--sign') + run_command('overcommit', '--sign', 'pre-commit') + end + + def commit + shell(%w[git commit --allow-empty -m Test]) + end + + around do |example| + repo do + echo(original_config.to_yaml, '.overcommit.yml') + FileUtils.mkdir_p(File.dirname(plugin_path)) + echo(original_plugin, plugin_path) + install_overcommit_hooks + run_command('git', 'add', '.') + sign + run_command('git', 'commit', '-m', 'Original') + @original = get_sha1('HEAD') + + changed = original_config.merge('HistoryOption' => 'changed') + echo(changed.to_yaml, '.overcommit.yml') + echo("#{original_plugin}\n# Changed plugin", plugin_path) + run_command('git', 'add', '.') + sign + run_command('git', 'commit', '-m', 'Changed') + @changed = get_sha1('HEAD') + example.run + end + end + + it 'allows checkout between both signed configurations without signing again' do + [@original, @changed, @original].each do |revision| + run_command('git', 'checkout', '--detach', revision) + commit.status.should == 0 + end + end + + it 'rebases onto an older approved configuration and stays on the branch' do + run_command('git', 'checkout', '-b', 'feature') + echo('feature', 'feature.txt') + run_command('git', 'add', 'feature.txt') + run_command('git', 'commit', '-m', 'Feature') + + result = run_command('git', 'rebase', '--onto', @original, @changed) + result.stderr.should_not include('Signature of configuration file has changed') + run_command('git', 'symbolic-ref', '--short', 'HEAD').stdout.chomp.should == 'feature' + File.read('.overcommit.yml').should == original_config.to_yaml + File.read(plugin_path).should == original_plugin + File.read('feature.txt').should == "feature\n" + commit.status.should == 0 + end + + it 'still rejects a configuration that has never been signed' do + echo(original_config.merge('HistoryOption' => 'never approved').to_yaml, '.overcommit.yml') + result = commit + result.status.should_not == 0 + result.stderr.should include('Signature of configuration file has changed') + end + + it 'still rejects plugin code that has never been signed' do + echo("#{original_plugin}\n# Never approved", plugin_path) + run_command('git', 'add', plugin_path) + result = commit + result.status.should_not == 0 + result.stderr.should include('HistoryHook') + end + + it 'requires approval for a new combination of previously signed code and configuration' do + # The plugin configuration and its code are signed together. Approval for + # two combinations must not allow arbitrary recombinations of their parts. + changed = original_config + changed['PreCommit']['HistoryHook']['new_option'] = true + echo(changed.to_yaml, '.overcommit.yml') + sign + echo(original_plugin, plugin_path) + run_command('git', 'add', '.') + result = commit + result.status.should_not == 0 + result.stderr.should include('HistoryHook') + end + + it 'allows a signed old plugin with the same configuration after reverting its code' do + echo(original_plugin, plugin_path) + run_command('git', 'add', plugin_path) + commit.status.should == 0 + end + + it 'migrates configuration and plugin approvals created by older releases' do + logger = Overcommit::Logger.new(StringIO.new) + config = Overcommit::ConfigurationLoader.new(logger, verify: false).load_repo_config + # These are the pre-history signature formats, independent of the new store. + config_signature = Digest::SHA256.hexdigest(config.instance_variable_get(:@hash).to_json) + hook_config = config.for_hook('HistoryHook', 'PreCommit').dup + hook_config.delete('skip') + hook_signature = Digest::SHA256.hexdigest(File.read(plugin_path) + hook_config.to_s) + FileUtils.rm_rf(File.join(Overcommit::Utils.git_dir, 'overcommit-signatures')) + run_command('git', 'config', '--local', 'overcommit.configuration.signature', config_signature) + run_command('git', 'config', '--local', + 'overcommit.PreCommit.HistoryHook.signature', hook_signature) + + commit.status.should == 0 + %w[overcommit.configuration.signature overcommit.PreCommit.HistoryHook.signature].each do |key| + shell(%W[git config --local --get #{key}]).status.should == 1 + end + FileUtils.rm_rf(File.join(Overcommit::Utils.git_dir, 'overcommit-signatures')) + commit.status.should_not == 0 + end + + it 'requires signing again after all local history is deleted' do + FileUtils.rm_rf(File.join(Overcommit::Utils.git_dir, 'overcommit-signatures')) + result = commit + result.status.should_not == 0 + result.stderr.should include('No previously recorded signature') + sign + commit.status.should == 0 + end +end diff --git a/spec/integration/worktree_signing_spec.rb b/spec/integration/worktree_signing_spec.rb index c992829b..514652fb 100644 --- a/spec/integration/worktree_signing_spec.rb +++ b/spec/integration/worktree_signing_spec.rb @@ -62,10 +62,9 @@ def commit_in_linked_worktree context 'and extensions.worktreeConfig is not enabled' do let(:worktree_config) { false } - it 'reports a signature error in the worktree signed first' do - result = commit_in_main_worktree - result.status.should_not == 0 - result.stderr.should include 'Signature of configuration file has changed!' + it 'remembers both approved configurations in the shared history' do + commit_in_main_worktree.status.should == 0 + commit_in_linked_worktree.status.should == 0 end end @@ -101,4 +100,26 @@ def commit_in_linked_worktree end end end + + context 'when verification is enabled and only the main worktree is signed' do + let(:sign_linked_worktree) { false } + + context 'and extensions.worktreeConfig is not enabled' do + let(:worktree_config) { false } + + it 'shares approval for an identical configuration' do + commit_in_linked_worktree.status.should == 0 + end + end + + context 'and extensions.worktreeConfig is enabled' do + let(:worktree_config) { true } + + it 'does not share approval with the unsigned worktree' do + result = commit_in_linked_worktree + result.status.should_not == 0 + result.stderr.should include 'No previously recorded signature for configuration file.' + end + end + end end diff --git a/spec/overcommit/signature_spec.rb b/spec/overcommit/signature_spec.rb new file mode 100644 index 00000000..d051fd2a --- /dev/null +++ b/spec/overcommit/signature_spec.rb @@ -0,0 +1,212 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Overcommit::Signature do + let(:key) { 'overcommit.configuration.signature' } + let(:signature) { Digest::SHA256.hexdigest('original configuration') } + let(:other_signature) { Digest::SHA256.hexdigest('changed configuration') } + let(:store) { described_class.new(key) } + let(:root) { File.join(Overcommit::Utils.git_dir, 'overcommit-signatures') } + let(:signature_file) { File.join(root, Digest::SHA256.hexdigest(key), signature) } + + around do |example| + repo { example.run } + end + + it 'does not approve a signature that was never signed' do + store.include?(signature).should == false + store.recorded?.should == false + File.exist?(root).should == false + end + + it 'remembers all approvals across instances without growing Git config' do + store.add(signature) + store.add(other_signature) + restored = described_class.new(key) + restored.include?(signature).should == true + restored.include?(other_signature).should == true + restored.recorded?.should == true + shell(%W[git config --local --get #{key}]).status.should == 1 + end + + it 'signs the same content idempotently' do + 2.times { store.add(signature) } + Dir[File.join(root, '*', '*')].size.should == 1 + store.include?(signature).should == true + end + + it 'indexes approvals independently of the number of historical signatures' do + 1000.times { |i| store.add(Digest::SHA256.hexdigest(i.to_s)) } + store.add(signature) + File.should_receive(:read).once.with(signature_file).and_call_original + Dir.should_not_receive(:glob) + store.include?(signature).should == true + end + + it 'keeps approvals separate for each hook and hook type' do + store.add(signature) + %w[overcommit.PreCommit.Custom.signature + overcommit.PostCheckout.Custom.signature].each do |name| + described_class.new(name).include?(signature).should == false + end + end + + it 'does not derive filesystem paths from hook names' do + described_class.new('overcommit.PreCommit.../../Custom.signature').add(signature) + Dir[File.join(root, '*', '*')].size.should == 1 + end + + it 'revokes one approval by removing its file while preserving others' do + store.add(signature) + store.add(other_signature) + File.delete(signature_file) + restored = described_class.new(key) + restored.include?(signature).should == false + restored.include?(other_signature).should == true + end + + it 'revokes all approvals by removing the history directory' do + store.add(signature) + FileUtils.rm_rf(root) + restored = described_class.new(key) + restored.include?(signature).should == false + restored.recorded?.should == false + end + + it 'rejects a corrupted or truncated approval file' do + store.add(signature) + File.write(signature_file, '') + store.include?(signature).should == false + end + + it 'rejects an approval file copied from another hook' do + store.add(signature) + File.write(signature_file, "overcommit.PreCommit.Custom.signature\n#{signature}\n") + store.include?(signature).should == false + end + + it 'does not lose approvals when Git garbage collects unreachable objects' do + store.add(signature) + shell(%w[git gc --prune=now]).status.should == 0 + described_class.new(key).include?(signature).should == true + end + + it 'does not trust signature blobs received through a remote tag' do + # Reproduce the original implementation's trust-injection vulnerability. + store.add(signature) + File.write('untrusted-configuration', 'changed configuration') + config_id = shell(%w[git hash-object untrusted-configuration]).stdout.chomp + # The old implementation would accept this blob's presence as approval. + File.write('injected-signature', "#{config_id} #{other_signature}") + object_id = shell(%w[git hash-object -w injected-signature]).stdout.chomp + shell(%W[git update-ref refs/tags/forged-approval #{object_id}]).status.should == 0 + destination = File.join(File.dirname(Dir.pwd), 'clone') + shell(['git', 'clone', '--no-local', Dir.pwd, destination]).status.should == 0 + Dir.chdir(destination) do + %w[git_dir repo_root].each { |name| Overcommit::Utils.instance_variable_set("@#{name}", nil) } + shell(%W[git cat-file -e #{object_id}]).status.should == 0 + cloned = described_class.new(key) + cloned.include?(signature).should == false + cloned.include?(other_signature).should == false + end + end + + context 'with a legacy approval in Git config' do + before do + shell(%W[git config --local #{key} #{signature}]).status.should == 0 + end + + it 'migrates the old approval and removes the old key' do + store.include?(signature).should == true + shell(%W[git config --local --get #{key}]).status.should == 1 + store.include?(other_signature).should == false + end + + it 'preserves the old approval when signing a new configuration first' do + store.add(other_signature) + store.include?(signature).should == true + store.include?(other_signature).should == true + end + + it 'does not resurrect a revoked legacy approval' do + store.include?(signature).should == true + FileUtils.rm_rf(root) + described_class.new(key).include?(signature).should == false + end + + it 'leaves the legacy approval intact if writing history fails' do + FileUtils.should_receive(:mkdir_p).and_raise(Errno::EACCES) + expect do + store + end.to raise_error(Overcommit::Exceptions::SignatureError, /write signature history/) + shell(%W[git config --local --get #{key}]).stdout.chomp.should == signature + end + + it 'does not accept an invalid legacy digest as a path' do + shell(%W[git config --local #{key} ../../untrusted]).status.should == 0 + store.include?(signature).should == false + File.exist?(root).should == false + end + end + + context 'with worktree configuration enabled' do + before { shell(%w[git config --local extensions.worktreeConfig true]) } + + it 'migrates only the approval in worktree config' do + shell(%W[git config --local #{key} #{other_signature}]).status.should == 0 + shell(%W[git config --worktree #{key} #{signature}]).status.should == 0 + store.include?(signature).should == true + store.include?(other_signature).should == false + shell(%W[git config --worktree --get #{key}]).status.should == 1 + shell(%W[git config --local --get #{key}]).stdout.chomp.should == other_signature + end + end + + it 'supports repositories whose Git directory is stored separately' do + destination = File.join(File.dirname(Dir.pwd), 'repository metadata') + shell(['git', 'init', '--separate-git-dir', destination]).status.should == 0 + Overcommit::Utils.instance_variable_set(:@git_dir, nil) + store.add(signature) + File.file?('.git').should == true + Dir[File.join(destination, 'overcommit-signatures', '*', '*')].size.should == 1 + described_class.new(key).include?(signature).should == true + end + + it 'handles concurrent approval writes without forgetting any signatures' do + # Separate instances have no shared Ruby state, as with concurrent hook runs. + stores = Array.new(4) { described_class.new(key) } + signatures = Array.new(40) { |i| Digest::SHA256.hexdigest(i.to_s) } + threads = stores.each_with_index.map do |history, index| + Thread.new { signatures.rotate(index * 10).each { |sig| history.add(sig) } } + end + threads.each(&:value) + restored = described_class.new(key) + signatures.each { |sig| restored.include?(sig).should == true } + end + + it 'fails closed if reading an approval fails' do + store.add(signature) + File.should_receive(:read).with(signature_file).and_raise(Errno::EACCES) + expect { store.include?(signature) }. + to raise_error(Overcommit::Exceptions::SignatureError, /read signature history/) + end + + it 'reports Git config read errors' do + Overcommit::Utils.stub(:execute).and_call_original + result = double(status: 2, success?: false, stderr: 'cannot read config') + Overcommit::Utils.should_receive(:execute). + with(%W[git config --local --get #{key}]).and_return(result) + expect { store }.to raise_error(Overcommit::Exceptions::GitConfigError, /cannot read config/) + end + + it 'reports Git config removal errors without losing the migrated approval' do + shell(%W[git config --local #{key} #{signature}]).status.should == 0 + Overcommit::Utils.stub(:execute).and_call_original + result = double(status: 2, success?: false, stderr: 'cannot lock config') + Overcommit::Utils.should_receive(:execute). + with(%W[git config --local --unset-all #{key}]).and_return(result) + expect { store }.to raise_error(Overcommit::Exceptions::GitConfigError, /cannot lock config/) + File.read(signature_file).should include(signature) + end +end diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index 74f580bc..e1416265 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -64,6 +64,4 @@ Overcommit::Utils.instance_variable_set(:"@#{var}", nil) end end - - config.example_status_persistence_file_path = 'spec_example_status' end