Merge pull request #474 from Shopify/secret_censoring

Additional safeguards against printing Secret content

README.md

@@ -114,6 +114,8 @@ Refer to `kubernetes-deploy --help` for the authoritative set of options.
 resource to deploy.
 - `--selector`: Instructs kubernetes-deploy to only prune resources which match the specified label selector, such as `environment=staging`. If you use this option, all resource templates must specify matching labels. See [Sharing a namespace](#sharing-a-namespace) below.
 
+> **NOTICE**: Deploy Secret resources at your own risk. Although we will fix any reported leak vectors with urgency, we cannot guarantee that sensitive information will never be logged.
+
 ### Sharing a namespace
 
 By default, kubernetes-deploy will prune any resources in the target namespace which have the `kubectl.kubernetes.io/last-applied-configuration` annotation and are not a result of the current deployment process, on the assumption that there is a one-to-one relationship between application deployment and namespace, and that a deployment provisions all relevant resources in the namespace.

lib/kubernetes-deploy/deploy_task.rb

@@ -252,7 +252,7 @@ def validate_resources(resources)
       return unless failed_resources.present?
 
       failed_resources.each do |r|
-        content = File.read(r.file_path) if File.file?(r.file_path)
+        content = File.read(r.file_path) if File.file?(r.file_path) && !r.sensitive_template_content?
         record_invalid_template(err: r.validation_error_msg, filename: File.basename(r.file_path), content: content)
       end
       raise FatalDeploymentError, "Template validation failed"
@@ -319,7 +319,13 @@ def split_templates(filename)
     def record_invalid_template(err:, filename:, content: nil)
       debug_msg = ColorizedString.new("Invalid template: #{filename}\n").red
       debug_msg += "> Error message:\n#{FormattedLogger.indent_four(err)}"
-      debug_msg += "\n> Template content:\n#{FormattedLogger.indent_four(content)}" if content
+      if content
+        debug_msg += if content =~ /kind:\s*Secret/
+          "\n> Template content: Suppressed because it may contain a Secret"
+        else
+          "\n> Template content:\n#{FormattedLogger.indent_four(content)}"
+        end
+      end
       @logger.summary.add_paragraph(debug_msg)
     end
 
@@ -445,7 +451,7 @@ def apply_all(resources, prune)
           prune_whitelist.each { |type| command.push("--prune-whitelist=#{type}") }
         end
 
-        output_is_sensitive = resources.any?(&:kubectl_output_is_sensitive?)
+        output_is_sensitive = resources.any?(&:sensitive_template_content?)
         out, err, st = kubectl.run(*command, log_failure: false, output_is_sensitive: output_is_sensitive)
 
         if st.success?
@@ -473,7 +479,7 @@ def record_apply_failure(err, resources: [])
 
       unidentified_errors = []
       filenames_with_sensitive_content = resources
-        .select(&:kubectl_output_is_sensitive?)
+        .select(&:sensitive_template_content?)
         .map { |r| File.basename(r.file_path) }
 
       err.each_line do |line|

lib/kubernetes-deploy/ejson_secret_provisioner.rb

@@ -62,7 +62,11 @@ def build_secrets
 
         secrets.map do |secret_name, secret_spec|
           validate_secret_spec(secret_name, secret_spec)
-          generate_secret_resource(secret_name, secret_spec["_type"], secret_spec["data"])
+          resource = generate_secret_resource(secret_name, secret_spec["_type"], secret_spec["data"])
+          unless resource.validate_definition(@kubectl)
+            raise EjsonSecretError, "Resulting resource Secret/#{secret_name} failed validation"
+          end
+          resource
         end
       end
     end

lib/kubernetes-deploy/kubernetes_resource.rb

@@ -30,15 +30,13 @@ class KubernetesResource
 
     TIMEOUT_OVERRIDE_ANNOTATION = "kubernetes-deploy.shopify.io/timeout-override"
     LAST_APPLIED_ANNOTATION = "kubectl.kubernetes.io/last-applied-configuration"
-    KUBECTL_OUTPUT_IS_SENSITIVE = false
+    SENSITIVE_TEMPLATE_CONTENT = false
 
     class << self
       def build(namespace:, context:, definition:, logger:, statsd_tags:, crd: nil)
+        validate_definition_essentials(definition)
         opts = { namespace: namespace, context: context, definition: definition, logger: logger,
                  statsd_tags: statsd_tags }
-        if definition["kind"].blank?
-          raise InvalidTemplateError.new("Template missing 'Kind'", content: definition.to_yaml)
-        end
         if (klass = class_for_kind(definition["kind"]))
           return klass.new(**opts)
         end
@@ -66,6 +64,24 @@ def timeout
       def kind
         name.demodulize
       end
+
+      private
+
+      def validate_definition_essentials(definition)
+        debug_content = <<~STRING
+          apiVersion: #{definition.fetch('apiVersion', '<missing>')}
+          kind: #{definition.fetch('kind', '<missing>')}
+          metadata: #{definition.fetch('metadata', {})}
+          <Template body suppressed because content sensitivity could not be determined.>
+        STRING
+        if definition["kind"].blank?
+          raise InvalidTemplateError.new("Template is missing required field 'kind'", content: debug_content)
+        end
+
+        if definition.dig('metadata', 'name').blank?
+          raise InvalidTemplateError.new("Template is missing required field 'metadata.name'", content: debug_content)
+        end
+      end
     end
 
     def timeout
@@ -86,12 +102,7 @@ def pretty_timeout_type
 
     def initialize(namespace:, context:, definition:, logger:, statsd_tags: [])
       # subclasses must also set these if they define their own initializer
-      @name = definition.dig("metadata", "name")
-      unless @name.present?
-        logger.summary.add_paragraph("Rendered template content:\n#{definition.to_yaml}")
-        raise FatalDeploymentError, "Template is missing required field metadata.name"
-      end
-
+      @name = definition.dig("metadata", "name").to_s
       @optional_statsd_tags = statsd_tags
       @namespace = namespace
       @context = context
@@ -113,9 +124,9 @@ def validate_definition(kubectl, selector: nil)
       validate_timeout_annotation
 
       command = ["create", "-f", file_path, "--dry-run", "--output=name"]
-      _, err, st = kubectl.run(*command, log_failure: false, output_is_sensitive: kubectl_output_is_sensitive?)
+      _, err, st = kubectl.run(*command, log_failure: false, output_is_sensitive: sensitive_template_content?)
       return true if st.success?
-      if kubectl_output_is_sensitive?
+      if sensitive_template_content?
         @validation_errors << <<-EOS
           Validation for #{id} failed. Detailed information is unavailable as the raw error may contain sensitive data.
         EOS
@@ -322,8 +333,8 @@ def report_status_to_statsd(watch_time)
       end
     end
 
-    def kubectl_output_is_sensitive?
-      self.class::KUBECTL_OUTPUT_IS_SENSITIVE
+    def sensitive_template_content?
+      self.class::SENSITIVE_TEMPLATE_CONTENT
     end
 
     class Event

lib/kubernetes-deploy/kubernetes_resource/secret.rb

@@ -2,7 +2,7 @@
 module KubernetesDeploy
   class Secret < KubernetesResource
     TIMEOUT = 30.seconds
-    KUBECTL_OUTPUT_IS_SENSITIVE = true
+    SENSITIVE_TEMPLATE_CONTENT = true
 
     def status
       exists? ? "Available" : "Not Found"

lib/kubernetes-deploy/resource_cache.rb

@@ -51,7 +51,7 @@ def use_or_populate_cache(kind)
 
     def fetch_by_kind(kind)
       resource_class = KubernetesResource.class_for_kind(kind)
-      output_is_sensitive = resource_class.nil? ? false : resource_class::KUBECTL_OUTPUT_IS_SENSITIVE
+      output_is_sensitive = resource_class.nil? ? false : resource_class::SENSITIVE_TEMPLATE_CONTENT
       raw_json, _, st = @kubectl.run("get", kind, "--chunk-size=0", attempts: 5, output: "json",
          output_is_sensitive: output_is_sensitive)
       raise KubectlError unless st.success?

test/fixtures/invalid-resources/bad_binding_secret.yml.erb

@@ -0,0 +1,8 @@
+apiVersion: v1
+kind: Secret
+metadata:
+  name: hello-secret
+type: Opaque
+data:
+  username: <%= some_value_i_forgot %>
+  password: cGFzc3dvcmQ=

test/integration-serial/serial_deploy_test.rb

@@ -364,7 +364,7 @@ def test_deploying_crs_with_invalid_crd_conditions_fails
   # to recreate such a condition
   def test_apply_failure_with_sensitive_resources_hides_template_content
     logger.level = 0
-    KubernetesDeploy::Deployment.any_instance.expects(:kubectl_output_is_sensitive?).returns(true).at_least_once
+    KubernetesDeploy::Deployment.any_instance.expects(:sensitive_template_content?).returns(true).at_least_once
     result = deploy_fixtures("hello-cloud", subset: ["web.yml.erb"]) do |fixtures|
       bad_port_name = "http_test_is_really_long_and_invalid_chars"
       svc = fixtures["web.yml.erb"]["Service"].first

test/integration/kubernetes_deploy_test.rb

@@ -825,9 +825,10 @@ def test_deploy_aborts_immediately_if_metadata_name_missing
 
     assert_logs_match_all([
       "Result: FAILURE",
-      "Template is missing required field metadata.name",
-      "Rendered template content:",
+      "Template is missing required field 'metadata.name'",
+      "Template content:",
       "kind: ConfigMap",
+      'metadata: {"labels"=>{"name"=>"hello-cloud-configmap-data", "app"=>"hello-cloud"}}',
     ], in_order: true)
   end
 
@@ -1172,14 +1173,11 @@ def test_raise_on_yaml_missing_kind
     assert_logs_match_all([
       "Invalid template: missing_kind.yml",
       "> Error message:",
-      "Template missing 'Kind'",
+      "Template is missing required field 'kind'",
       "> Template content:",
-      "---",
       "apiVersion: v1",
-      "metadata:",
-      "  name: test",
-      "data:",
-      "  datapoint: value1",
+      "kind: <missing>",
+      'metadata: {"name"=>"test"}',
     ], in_order: true)
   end
 
@@ -1225,6 +1223,70 @@ def test_apply_failure_with_sensitive_resources_hides_raw_output
     ])
   end
 
+  def test_validation_failure_on_sensitive_resources_does_not_print_template
+    selector = KubernetesDeploy::LabelSelector.parse("branch=master")
+    assert_deploy_failure(deploy_fixtures("hello-cloud", subset: %w(secret.yml), selector: selector))
+    assert_logs_match_all([
+      "Template validation failed",
+      "Invalid template: Secret-hello-secret",
+      "selector branch=master passed in, but no labels were defined",
+    ], in_order: true)
+    refute_logs_match("password")
+    refute_logs_match("YWRtaW4=")
+  end
+
+  def test_render_failure_on_sensitive_resource_does_not_print_template
+    assert_deploy_failure(deploy_fixtures("invalid-resources", subset: %w(bad_binding_secret.yml.erb)))
+    assert_logs_match_all([
+      "Failed to render and parse template",
+      "Invalid template: bad_binding_secret.yml.erb",
+      "undefined local variable or method",
+      "Template content: Suppressed because it may contain a Secret",
+    ], in_order: true)
+    refute_logs_match("password")
+    refute_logs_match("YWRtaW4=")
+  end
+
+  def test_missing_name_on_secret_does_not_print_template_at_all
+    result = deploy_fixtures("hello-cloud", subset: %w(secret.yml)) do |fixtures|
+      secret = fixtures["secret.yml"]["Secret"].first
+      secret["metadata"].delete("name")
+    end
+    assert_deploy_failure(result)
+
+    assert_logs_match_all([
+      "Invalid template: secret.yml",
+      "Template is missing required field 'metadata.name'",
+      "Template content: Suppressed because it may contain a Secret",
+    ], in_order: true)
+
+    refute_logs_match("apiVersion:")
+    refute_logs_match("password")
+    refute_logs_match("YWRtaW4=")
+  end
+
+  def test_missing_name_on_unknown_resource_prints_metadata_but_not_body
+    result = deploy_fixtures("hello-cloud", subset: %w(secret.yml)) do |fixtures|
+      secret = fixtures["secret.yml"]["Secret"].first
+      secret["metadata"].delete("name")
+      secret["kind"] = "SpecialSecret"
+      secret["metadata"]["labels"] = { "should_appear" => true }
+    end
+    assert_deploy_failure(result)
+
+    assert_logs_match_all([
+      "Invalid template: secret.yml",
+      "Template is missing required field 'metadata.name'",
+      "apiVersion: v1",
+      "kind: SpecialSecret",
+      'metadata: {"labels"=>{"should_appear"=>true}}',
+      "<Template body suppressed because content sensitivity could not be determined.>",
+    ], in_order: true)
+
+    refute_logs_match("password")
+    refute_logs_match("YWRtaW4=")
+  end
+
   private
 
   def expected_daemonset_pod_count

test/unit/kubernetes-deploy/ejson_secret_provisioner_test.rb

@@ -3,9 +3,9 @@
 
 class EjsonSecretProvisionerTest < KubernetesDeploy::TestCase
   def test_resources_based_on_ejson_file_existence
-    stub_kubectl_response("get", "secret", "ejson-keys",
-      kwargs: { raise_if_not_found: true, attempts: 3, output_is_sensitive: true, log_failure: true },
-      resp: dummy_ejson_secret)
+    stub_ejson_keys_get_request
+    stub_kubectl_response("create", "-f", anything, "--dry-run", "--output=name",
+      kwargs: { log_failure: false, output_is_sensitive: true }, resp: dummy_secret_hash, json: false).times(3)
 
     assert_empty(build_provisioner(fixture_path('hello-cloud')).resources)
     refute_empty(build_provisioner(fixture_path('ejson-cloud')).resources)
@@ -20,9 +20,9 @@ def test_run_with_secrets_file_invalid_json
   end
 
   def test_resource_is_built_correctly
-    stub_kubectl_response("get", "secret", "ejson-keys",
-      kwargs: { raise_if_not_found: true, attempts: 3, output_is_sensitive: true, log_failure: true },
-      resp: dummy_ejson_secret)
+    stub_ejson_keys_get_request
+    stub_kubectl_response("create", "-f", anything, "--dry-run", "--output=name",
+      kwargs: { log_failure: false, output_is_sensitive: true }, resp: dummy_secret_hash, json: false).times(3)
 
     resources = build_provisioner(fixture_path('ejson-cloud')).resources
     refute_empty(resources)
@@ -70,9 +70,7 @@ def test_run_with_cloud_keys_secret_missing
   end
 
   def test_run_with_file_missing_section_for_ejson_secrets_logs_warning
-    stub_kubectl_response("get", "secret", "ejson-keys",
-      kwargs: { raise_if_not_found: true, attempts: 3, output_is_sensitive: true, log_failure: true },
-      resp: dummy_ejson_secret)
+    stub_ejson_keys_get_request
     new_content = { "_public_key" => fixture_public_key, "not_the_right_key" => [] }
 
     with_ejson_file(new_content.to_json) do |target_dir|
@@ -82,9 +80,7 @@ def test_run_with_file_missing_section_for_ejson_secrets_logs_warning
   end
 
   def test_run_with_incomplete_secret_spec
-    stub_kubectl_response("get", "secret", "ejson-keys",
-      kwargs: { raise_if_not_found: true, attempts: 3, output_is_sensitive: true, log_failure: true },
-      resp: dummy_ejson_secret)
+    stub_ejson_keys_get_request
     new_content = {
       "_public_key" => fixture_public_key,
       "kubernetes_secrets" => { "foobar" => {} },
@@ -98,8 +94,24 @@ def test_run_with_incomplete_secret_spec
     end
   end
 
+  def test_proactively_validates_resulting_resources_and_raises_without_logging
+    stub_ejson_keys_get_request
+    KubernetesDeploy::Secret.any_instance.expects(:validate_definition).returns(false)
+    msg = "Generation of Kubernetes secrets from ejson failed: Resulting resource Secret/catphotoscom failed validation"
+    assert_raises_message(KubernetesDeploy::EjsonSecretError, msg) do
+      build_provisioner(fixture_path('ejson-cloud')).resources
+    end
+    refute_logs_match("Secret")
+  end
+
   private
 
+  def stub_ejson_keys_get_request
+    stub_kubectl_response("get", "secret", "ejson-keys",
+      kwargs: { raise_if_not_found: true, attempts: 3, output_is_sensitive: true, log_failure: true },
+      resp: dummy_ejson_secret)
+  end
+
   def correct_ejson_key_secret_data
     {
       fixture_public_key => "fedcc95132e9b399ee1f404364fdbc81bdbe4feb5b8292b061f1022481157d5a",

test/unit/kubernetes-deploy/kubernetes_resource_test.rb

@@ -39,7 +39,7 @@ def file_path
   end
 
   class DummySensitiveResource < DummyResource
-    KUBECTL_OUTPUT_IS_SENSITIVE = true
+    SENSITIVE_TEMPLATE_CONTENT = true
   end
 
   def test_unusual_timeout_output