Enhance HMAC authentication: improve timestamp validation and add tests for various timestamp formats

Author: GrantBirki

lib/hooks/app/auth/auth.rb

@@ -37,12 +37,8 @@ def validate_auth!(payload, headers, endpoint_config, global_config = {})
         end
 
         log.debug("validating auth for request with auth_class: #{auth_class.name}")
-
-        unless auth_class.valid?(
-          payload:,
-          headers:,
-          config: endpoint_config
-        )
+        unless auth_class.valid?(payload:, headers:, config: endpoint_config)
+          log.warn("authentication failed for request with auth_class: #{auth_class.name}")
           error!("authentication failed", 401)
         end
       end

lib/hooks/plugins/auth/hmac.rb

@@ -117,7 +117,10 @@ def self.valid?(payload:, headers:, config:)
 
           # Validate timestamp if required (for services that include timestamp validation)
           if validator_config[:timestamp_header]
-            return false unless valid_timestamp?(normalized_headers, validator_config)
+            unless valid_timestamp?(normalized_headers, validator_config)
+              log.warn("Auth::HMAC validation failed: Invalid timestamp")
+              return false
+            end
           end
 
           # Compute expected signature
@@ -180,34 +183,99 @@ def self.normalize_headers(headers)
         #
         # Checks if the provided timestamp is within the configured tolerance
         # of the current time. This prevents replay attacks using old requests.
+        # Supports both ISO 8601 UTC timestamps and Unix timestamps.
         #
         # @param headers [Hash<String, String>] Normalized HTTP headers
         # @param config [Hash<Symbol, Object>] Validator configuration
         # @return [Boolean] true if timestamp is valid or not required, false otherwise
         # @note Returns false if timestamp header is missing when required
         # @note Tolerance is applied as absolute difference (past or future)
+        # @note Tries ISO 8601 UTC format first, then falls back to Unix timestamp
         # @api private
         def self.valid_timestamp?(headers, config)
           timestamp_header = config[:timestamp_header]
+          tolerance = config[:timestamp_tolerance] || 300
           return false unless timestamp_header
 
-          timestamp_header = timestamp_header.downcase
-          timestamp_value = headers[timestamp_header]
-
+          timestamp_value = headers[timestamp_header.downcase]
           return false unless timestamp_value
+          return false if timestamp_value.strip.empty?
 
-          # Security: Strict timestamp validation - must be only digits with no leading zeros
-          return false unless timestamp_value.match?(/\A[1-9]\d*\z/) || timestamp_value == "0"
+          parsed_timestamp = parse_timestamp(timestamp_value.strip)
+          return false unless parsed_timestamp
 
-          timestamp = timestamp_value.to_i
+          # parsed_timestamp is a Time object
+          now = Time.now.utc
+          (now - parsed_timestamp).abs <= tolerance
+        end
 
-          # Ensure timestamp is a positive integer (reject zero and negative)
-          return false unless timestamp > 0
+        # Parse timestamp value supporting both ISO 8601 UTC and Unix formats
+        #
+        # Attempts to parse the timestamp in the following order:
+        # 1. ISO 8601 UTC format (e.g., "2025-06-12T10:30:00Z")
+        # 2. Unix timestamp (e.g., "1609459200")
+        #
+        # @param timestamp_value [String] The timestamp string to parse
+        # @return [Time, nil] Time object if parsing succeeds, nil otherwise
+        # @note Security: Strict validation prevents various injection attacks
+        # @api private
+        def self.parse_timestamp(timestamp_value)
+          # Try ISO 8601 first, then Unix
+          ts = parse_iso8601_timestamp(timestamp_value)
+          return ts if ts
+          ts = parse_unix_timestamp(timestamp_value)
+          return ts if ts
+          nil
+        end
 
-          current_time = Time.now.to_i
-          tolerance = config[:timestamp_tolerance]
+        # Check if timestamp string looks like ISO 8601 UTC format
+        #
+        # @param timestamp_value [String] The timestamp string to check
+        # @return [Boolean] true if it appears to be ISO 8601 format
+        # @api private
+        def self.iso8601_timestamp?(timestamp_value)
+          # Accepts Z, +00:00, or +0000, and T or space as separator
+          !!(timestamp_value =~ /\A\d{4}-\d{2}-\d{2}[T ]\d{2}:\d{2}:\d{2}(?:\.\d+)?(Z|\+00:00|\+0000)\z/)
+        end
 
-          (current_time - timestamp).abs <= tolerance
+        # Parse ISO 8601 UTC timestamp string
+        #
+        # @param timestamp_value [String] ISO 8601 timestamp string
+        # @return [Time, nil] Time object if parsing succeeds, nil otherwise
+        # @note Only accepts UTC timestamps (ending with 'Z' or explicit UTC)
+        # @api private
+        def self.parse_iso8601_timestamp(timestamp_value)
+          # Normalize 'YYYY-MM-DD HH:MM:SS(.fraction)? +0000' to 'YYYY-MM-DDTHH:MM:SS(.fraction)?+00:00'
+          if timestamp_value =~ /\A(\d{4}-\d{2}-\d{2}) (\d{2}:\d{2}:\d{2}(?:\.\d+)?)(?: )\+0000\z/
+            timestamp_value = "#{$1}T#{$2}+00:00"
+          end
+          return nil unless iso8601_timestamp?(timestamp_value)
+          t = Time.iso8601(timestamp_value) rescue nil
+          return nil unless t
+          # Only accept UTC (Z, +00:00, or +0000)
+          return t if t.utc? || t.utc_offset == 0
+          nil
+        end
+
+        # Parse Unix timestamp string
+        #
+        # @param timestamp_value [String] Unix timestamp string
+        # @return [Time, nil] Time object if parsing succeeds, nil otherwise
+        # @api private
+        def self.parse_unix_timestamp(timestamp_value)
+          return nil unless unix_timestamp?(timestamp_value)
+          ts = timestamp_value.to_i
+          return nil if ts <= 0
+          Time.at(ts).utc
+        end
+
+        # Check if timestamp string looks like Unix timestamp format
+        #
+        # @param timestamp_value [String] The timestamp string to check
+        # @return [Boolean] true if it appears to be Unix timestamp format
+        # @api private
+        def self.unix_timestamp?(timestamp_value)
+          !!(timestamp_value =~ /\A\d+\z/) || timestamp_value == "0"
         end
 
         # Compute HMAC signature based on configuration requirements

spec/acceptance/acceptance_tests.rb

@@ -117,12 +117,149 @@
     end
 
     describe "slack" do
-      it "receives a POST request but contains an invalid HMAC signature" do
+      it "successfully processes a valid POST request with HMAC signature and timestamp" do
         payload = { text: "Hello, Slack!" }
-        digest = OpenSSL::HMAC.hexdigest(OpenSSL::Digest.new("sha256"), FAKE_ALT_HMAC_SECRET, payload.to_json)
-        headers = { "Content-Type" => "application/json", "Signature-256" => "sha256=#{digest}" }
+        timestamp = Time.now.to_i.to_s
+        body = payload.to_json
+        signing_payload = "v0:#{timestamp}:#{body}"
+        digest = OpenSSL::HMAC.hexdigest(OpenSSL::Digest.new("sha256"), FAKE_ALT_HMAC_SECRET, signing_payload)
+        headers = {
+          "Content-Type" => "application/json",
+          "Signature-256" => "v0=#{digest}",
+          "X-Timestamp" => timestamp
+        }
+        response = http.post("/webhooks/slack", body, headers)
+        expect(response).to be_a(Net::HTTPSuccess)
+        body = JSON.parse(response.body)
+        expect(body["status"]).to eq("success")
+      end
+
+      it "rejects request with expired timestamp" do
+        payload = { text: "Hello, Slack!" }
+        # Use timestamp that's 10 minutes old (beyond the 5 minute tolerance)
+        expired_timestamp = (Time.now.to_i - 600).to_s
+
+        signing_payload = "v0:#{expired_timestamp}:#{payload.to_json}"
+        digest = OpenSSL::HMAC.hexdigest(OpenSSL::Digest.new("sha256"), FAKE_ALT_HMAC_SECRET, signing_payload)
+
+        headers = {
+          "Content-Type" => "application/json",
+          "Signature-256" => "v0=#{digest}",
+          "X-Timestamp" => expired_timestamp
+        }
+
         response = http.post("/webhooks/slack", payload.to_json, headers)
+        expect(response).to be_a(Net::HTTPUnauthorized)
+        expect(response.body).to include("authentication failed")
+      end
+
+      it "rejects request with missing timestamp header" do
+        payload = { text: "Hello, Slack!" }
+        timestamp = Time.now.to_i.to_s
 
+        # Create signature with timestamp but don't include timestamp header
+        signing_payload = "v0:#{timestamp}:#{payload.to_json}"
+        digest = OpenSSL::HMAC.hexdigest(OpenSSL::Digest.new("sha256"), FAKE_ALT_HMAC_SECRET, signing_payload)
+
+        headers = {
+          "Content-Type" => "application/json",
+          "Signature-256" => "v0=#{digest}"
+          # Missing X-Timestamp header
+        }
+
+        response = http.post("/webhooks/slack", payload.to_json, headers)
+        expect(response).to be_a(Net::HTTPUnauthorized)
+        expect(response.body).to include("authentication failed")
+      end
+
+      it "rejects request with invalid timestamp format" do
+        payload = { text: "Hello, Slack!" }
+        invalid_timestamp = "not-a-timestamp"
+
+        signing_payload = "v0:#{invalid_timestamp}:#{payload.to_json}"
+        digest = OpenSSL::HMAC.hexdigest(OpenSSL::Digest.new("sha256"), FAKE_ALT_HMAC_SECRET, signing_payload)
+
+        headers = {
+          "Content-Type" => "application/json",
+          "Signature-256" => "v0=#{digest}",
+          "X-Timestamp" => invalid_timestamp
+        }
+
+        response = http.post("/webhooks/slack", payload.to_json, headers)
+        expect(response).to be_a(Net::HTTPUnauthorized)
+        expect(response.body).to include("authentication failed")
+      end
+
+      it "successfully processes request with ISO 8601 UTC timestamp" do
+        payload = { text: "Hello, Slack!" }
+        iso_timestamp = Time.now.utc.strftime("%Y-%m-%dT%H:%M:%SZ")
+        body = payload.to_json
+        signing_payload = "v0:#{iso_timestamp}:#{body}"
+        digest = OpenSSL::HMAC.hexdigest(OpenSSL::Digest.new("sha256"), FAKE_ALT_HMAC_SECRET, signing_payload)
+        headers = {
+          "Content-Type" => "application/json",
+          "Signature-256" => "v0=#{digest}",
+          "X-Timestamp" => iso_timestamp
+        }
+        response = http.post("/webhooks/slack", body, headers)
+        expect(response).to be_a(Net::HTTPSuccess)
+        body = JSON.parse(response.body)
+        expect(body["status"]).to eq("success")
+      end
+
+      it "successfully processes request with ISO 8601 UTC timestamp using +00:00 format" do
+        payload = { text: "Hello, Slack!" }
+        iso_timestamp = Time.now.utc.strftime("%Y-%m-%dT%H:%M:%S+00:00")
+        body = payload.to_json
+        signing_payload = "v0:#{iso_timestamp}:#{body}"
+        digest = OpenSSL::HMAC.hexdigest(OpenSSL::Digest.new("sha256"), FAKE_ALT_HMAC_SECRET, signing_payload)
+        headers = {
+          "Content-Type" => "application/json",
+          "Signature-256" => "v0=#{digest}",
+          "X-Timestamp" => iso_timestamp
+        }
+        response = http.post("/webhooks/slack", body, headers)
+        expect(response).to be_a(Net::HTTPSuccess)
+        body = JSON.parse(response.body)
+        expect(body["status"]).to eq("success")
+      end
+
+      it "rejects request with non-UTC ISO 8601 timestamp" do
+        payload = { text: "Hello, Slack!" }
+        # Use EST timezone (non-UTC)
+        non_utc_timestamp = Time.now.strftime("%Y-%m-%dT%H:%M:%S-05:00")
+
+        signing_payload = "v0:#{non_utc_timestamp}:#{payload.to_json}"
+        digest = OpenSSL::HMAC.hexdigest(OpenSSL::Digest.new("sha256"), FAKE_ALT_HMAC_SECRET, signing_payload)
+
+        headers = {
+          "Content-Type" => "application/json",
+          "Signature-256" => "v0=#{digest}",
+          "X-Timestamp" => non_utc_timestamp
+        }
+
+        response = http.post("/webhooks/slack", payload.to_json, headers)
+        expect(response).to be_a(Net::HTTPUnauthorized)
+        expect(response.body).to include("authentication failed")
+      end
+
+      it "rejects request with timestamp manipulation attack" do
+        payload = { text: "Hello, Slack!" }
+        original_timestamp = Time.now.to_i.to_s
+        manipulated_timestamp = (Time.now.to_i + 100).to_s  # Future timestamp
+
+        # Create signature with original timestamp
+        signing_payload = "v0:#{original_timestamp}:#{payload.to_json}"
+        digest = OpenSSL::HMAC.hexdigest(OpenSSL::Digest.new("sha256"), FAKE_ALT_HMAC_SECRET, signing_payload)
+
+        # But send manipulated timestamp in header
+        headers = {
+          "Content-Type" => "application/json",
+          "Signature-256" => "v0=#{digest}",
+          "X-Timestamp" => manipulated_timestamp
+        }
+
+        response = http.post("/webhooks/slack", payload.to_json, headers)
         expect(response).to be_a(Net::HTTPUnauthorized)
         expect(response.body).to include("authentication failed")
       end

spec/acceptance/docker-compose.yml

@@ -9,7 +9,7 @@ services:
     environment:
       LOG_LEVEL: DEBUG
       GITHUB_WEBHOOK_SECRET: "octoawesome-secret"
-      ALT_WEBHOOK_SECRET: "octoawesome-too-secret"
+      ALT_WEBHOOK_SECRET: "octoawesome-2-secret"
       SHARED_SECRET: "octoawesome-shared-secret"
       DEFAULT_RETRY_SLEEP: 0
       RETRY_LOG_RETRIES: "false"