From b33ffa6d0e1b4cfaed76ebe36ab810f18e2eef2a Mon Sep 17 00:00:00 2001
From: Andrew Janssen <m.andrew.janssen@gmail.com>
Date: Fri, 15 Nov 2024 09:12:03 -0800
Subject: [PATCH] Raise exception on blank registry credentials, warn on blank
 secrets

---
 lib/kamal/configuration/registry.rb | 11 +++++++++-
 lib/kamal/secrets.rb                | 33 ++++++++++++++++++++++++++++-
 test/commands/registry_test.rb      | 12 +++++++++++
 3 files changed, 54 insertions(+), 2 deletions(-)

diff --git a/lib/kamal/configuration/registry.rb b/lib/kamal/configuration/registry.rb
index 763cf976a..e56c954bc 100644
--- a/lib/kamal/configuration/registry.rb
+++ b/lib/kamal/configuration/registry.rb
@@ -24,7 +24,16 @@ def password
   private
     def lookup(key)
       if registry_config[key].is_a?(Array)
-        secrets[registry_config[key].first]
+        secret = secrets[registry_config[key].first]
+        # Although the key is present, its value may be empty due to environment
+        # variable substitution. I.e. if it refers to an empty or unset env var.
+        unless secret.present?
+          raise Kamal::ConfigurationError, "The required secret " \
+            "'registry.#{key}' does not have a value. Did you forget to set " \
+            "its value with an environment variable or a secret helper?"
+        end
+
+        secret
       else
         registry_config[key]
       end
diff --git a/lib/kamal/secrets.rb b/lib/kamal/secrets.rb
index 7a382f988..09c734518 100644
--- a/lib/kamal/secrets.rb
+++ b/lib/kamal/secrets.rb
@@ -10,9 +10,13 @@ def initialize(destination: nil)
 
   def [](key)
     # Fetching secrets may ask the user for input, so ensure only one thread does that
-    @mutex.synchronize do
+    value = @mutex.synchronize do
       secrets.fetch(key)
     end
+
+    blank_key_warning(key) unless value.present?
+
+    value
   rescue KeyError
     if secrets_files.present?
       raise Kamal::ConfigurationError, "Secret '#{key}' not found in #{secrets_files.join(", ")}"
@@ -30,6 +34,25 @@ def secrets_files
   end
 
   private
+    def blank_key_warning(key)
+      warn "Warning: Kamal secret #{key} is blank."
+      secrets_files.each do |secrets_file|
+        next unless File.exist?(secrets_file)
+
+        File.foreach(secrets_file).with_index do |line, line_num|
+          next unless line.match?(/^\s*#{key}=/)
+
+          warn "Tip: see #{secrets_file}:#{line_num + 1}: #{line.strip}"
+          if line.match?(/^\s*#{key}=\$\w+/)
+            warn "Tip: the environment variable #{key} is #{ENV[key].nil? ?
+              "not set" : "blank"}. Did you forget to set it?"
+          elsif (matches = line.match(/^\s*#{key}=\$\(([^)]+)\)/)).present?
+            warn "Tip: the shell command \`#{matches[1]}\` returned an empty value."
+          end
+        end
+      end
+    end
+
     def secrets
       @secrets ||= secrets_files.inject({}) do |secrets, secrets_file|
         secrets.merge!(::Dotenv.parse(secrets_file))
@@ -39,4 +62,12 @@ def secrets
     def secrets_filenames
       [ ".kamal/secrets-common", ".kamal/secrets#{(".#{@destination}" if @destination)}" ]
     end
+
+    # Suppress warnings if launched from bin/test
+    alias_method :kernel_warn, :warn
+    def warn(message)
+      return if ENV["RAILS_ENV"] == "test"
+
+      kernel_warn message
+    end
 end
diff --git a/test/commands/registry_test.rb b/test/commands/registry_test.rb
index cf2734b72..250e3b159 100755
--- a/test/commands/registry_test.rb
+++ b/test/commands/registry_test.rb
@@ -49,6 +49,18 @@ class CommandsRegistryTest < ActiveSupport::TestCase
     end
   end
 
+  test "registry login with blank ENV password" do
+    with_test_secrets("secrets" => "KAMAL_REGISTRY_PASSWORD=") do
+      @config[:registry]["password"] = [ "KAMAL_REGISTRY_PASSWORD" ]
+
+      assert_raises Kamal::ConfigurationError do
+        assert_equal \
+          "docker login hub.docker.com -u \"dhh\" -p \"\"",
+          registry.login.join(" ")
+      end
+    end
+  end
+
   test "registry logout" do
     assert_equal \
       "docker logout hub.docker.com",