Handle cert injection properly for authn-k8s (#1789)

* Inject cert using bash script

There is [an issue](kubernetes/kubernetes#89899)
in k8s where messages that are sent via WebSockets to Kubectl Exec are
left hanging without returning a response when using STDIN.

However, if we inject the cert by running a bash script that exits explicitly
then we don't have the hanging issue, and the K8s API will close the socket
upon cert injection success/failure.

* Make exec command timeout configurable

We now enable configuration of the exec command timeout. This
can be done by setting the `KUBECTL_EXEC_COMMAND_TIMEOUT` env var.

* Return 202 instead of 200 for inject_client_cert requests

The "inject_client_cert" request now returns 202 Accepted instead of 200 OK to
indicate that the cert injection has started but not necessarily completed.

* Print cert injection logs upon error

The "inject_client_cert" request now writes the injection logs
to the client container. We can print them when we have an error to
help troubleshooting

* Verify "inject_client_cert" responds with 202

We should verify that the response code of the request
is 202 Accepted in successful cases

* Extract file copy logic to new command class

This logic was inside kubectl_exec.rb which had a lot going
on already. This commit will enhance this logic's readability

* Refactor kubectl_client to kube_client

The latter better tells its purpose as it a k8s client
and not a Kubectl client

* Remove redundant -r flag

* Add description and fix indentation

* Refactor SetFileContentInContainer to CopyTextToFileInContainer

* Add UTs for CopyTextToFileInContainer class

* Implement some PR requests

CHANGELOG.md

@@ -23,6 +23,9 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.
   if the environment variable is not present, rather than attempting to use the empty string value. [cyberark/conjur#1841](https://github.com/cyberark/conjur/issues/1841)
 
 ### Changed
+- The "inject_client_cert" request now returns 202 Accepted instead of 200 OK to
+  indicate that the cert injection has started but not necessarily completed.
+  [cyberark/conjur#1848](https://github.com/cyberark/conjur/issues/1848)
 - The Conjur server request logs now records the same IP address used by audit
   logs and network authentication filters with the `restricted_to` attribute.
   [cyberark/conjur#1719](https://github.com/cyberark/conjur/issues/1719)

app/controllers/authenticate_controller.rb

@@ -129,7 +129,7 @@ def k8s_inject_client_cert
       # and the prefix is in the header. This is done to maintain backwards-compatibility
       host_id_prefix:   request.headers["Host-Id-Prefix"]
     )
-    head :ok
+    head :accepted
   rescue => e
     handle_authentication_error(e)
   end

app/domain/authentication/authn_k8s/copy_text_to_file_in_container.rb

@@ -0,0 +1,71 @@
+# frozen_string_literal: true
+
+require 'command_class'
+
+# SetFileContentInContainer is used to set some text into a file inside a container
+module Authentication
+  module AuthnK8s
+
+    CopyTextToFileInContainer ||= CommandClass.new(
+      dependencies: {
+        kubectl_exec:      KubectlExec.new,
+        k8s_object_lookup: K8sObjectLookup,
+        logger:            Rails.logger
+      },
+      inputs: %i(webservice pod_namespace pod_name container path content mode)
+    ) do
+
+      LOG_FILE = "${TMPDIR:-/tmp}/conjur_set_file_content.log"
+
+      def call
+        copy_text_to_file_in_container
+      end
+
+      private
+
+      def copy_text_to_file_in_container
+        @kubectl_exec.call(
+          k8s_object_lookup: @k8s_object_lookup.new(@webservice),
+          pod_namespace:     @pod_namespace,
+          pod_name:          @pod_name,
+          container:         @container,
+          cmds:              %w(sh),
+          body:              set_file_content_script(@path, @content, @mode),
+          stdin:             true
+        )
+      end
+
+      # Sets the content of a file in a given path to the given content
+      # We first copy the content into a temporary file and only then move it to
+      # the desired path as the client polls on its existence and we want it to
+      # exist only when the whole content is present.
+      #
+      # We redirect the output to a log file on the authn-client container
+      # that will be written in its logs for supportability.
+      def set_file_content_script(path, content, mode)
+        tmp_cert = "#{path}.tmp"
+
+        "
+#!/bin/sh
+set -e
+
+cleanup() {
+  rm -f \"#{tmp_cert}\"
+}
+trap cleanup EXIT
+
+set_file_content() {
+  cat > \"#{tmp_cert}\" <<EOF
+#{content}
+EOF
+
+  chmod \"#{mode}\" \"#{tmp_cert}\"
+  mv \"#{tmp_cert}\" \"#{path}\"
+}
+
+set_file_content > \"#{LOG_FILE}\" 2>&1
+"
+      end
+    end
+  end
+end

app/domain/authentication/authn_k8s/inject_client_cert.rb

@@ -9,13 +9,14 @@ module AuthnK8s
 
     InjectClientCert ||= CommandClass.new(
       dependencies: {
-        logger:                 Rails.logger,
-        resource_class:         Resource,
-        conjur_ca_repo:         Repos::ConjurCA,
-        kubectl_exec:           KubectlExec,
-        validate_pod_request:   ValidatePodRequest.new,
-        extract_container_name: ExtractContainerName.new,
-        audit_log:              ::Audit.logger
+        logger:                         Rails.logger,
+        resource_class:                 Resource,
+        conjur_ca_repo:                 Repos::ConjurCA,
+        kubectl_exec:                   KubectlExec,
+        copy_text_to_file_in_container: CopyTextToFileInContainer.new,
+        validate_pod_request:           ValidatePodRequest.new,
+        extract_container_name:         ExtractContainerName.new,
+        audit_log:                      ::Audit.logger
       },
       inputs: %i(conjur_account service_id csr host_id_prefix client_ip)
     ) do
@@ -79,17 +80,18 @@ def install_signed_cert
           pod_name
         ))
 
-        resp = @kubectl_exec.new.copy(
-          k8s_object_lookup: k8s_object_lookup,
+        resp = @copy_text_to_file_in_container.call(
+          webservice: webservice,
           pod_namespace: pod_namespace,
           pod_name: pod_name,
           container: container_name,
           path: cert_file_path,
           content: cert_to_install.to_pem,
-          mode: 0o644
+          mode: "644"
         )
+
         validate_cert_installation(resp)
-        @logger.debug(LogMessages::Authentication::AuthnK8s::CopySSLToPodSuccess.new)
+        @logger.debug(LogMessages::Authentication::AuthnK8s::InitializeCopySSLToPodSuccess.new)
       end
 
       def validate_cert_installation(resp)
@@ -123,12 +125,6 @@ def spiffe_id
         @spiffe_id ||= SpiffeId.new(smart_csr.spiffe_id)
       end
 
-      def pod
-        @pod ||= k8s_object_lookup.pod_by_name(
-          spiffe_id.name, spiffe_id.namespace
-        )
-      end
-
       def host
         @host ||= @resource_class[host_id]
       end
@@ -170,10 +166,6 @@ def cert_to_install
         )
       end
 
-      def k8s_object_lookup
-        @k8s_object_lookup ||= K8sObjectLookup.new(webservice)
-      end
-
       def container_name
         @extract_container_name.call(
           service_id: @service_id,

app/domain/authentication/authn_k8s/k8s_object_lookup.rb

@@ -68,7 +68,7 @@ def api_url
       end
 
       # Gets the client object to the /api v1 endpoint.
-      def kubectl_client
+      def kube_client
         KubeClientFactory.client(host_url: api_url, options: options)
       end
 
@@ -149,7 +149,7 @@ def k8s_client_for_method method_name
       # List them in the order that you want them to be searched for methods.
       def k8s_clients
         @clients ||= [
-          kubectl_client,
+          kube_client,
           KubeClientFactory.client(
             api: 'apis/apps', version: 'v1', host_url: api_url,
             options: options

app/domain/authentication/authn_k8s/kubectl_exec.rb

@@ -1,3 +1,5 @@
+# frozen_string_literal: true
+
 require 'command_class'
 require 'uri'
 require 'websocket'
@@ -10,8 +12,10 @@ module Authentication
   module AuthnK8s
 
     KubectlExec ||= CommandClass.new(
-      dependencies: { logger: Rails.logger,
-                      timeout: 5.seconds },
+      dependencies: {
+        env:    ENV,
+        logger: Rails.logger
+      },
       inputs: %i( k8s_object_lookup
                   pod_namespace
                   pod_name
@@ -20,12 +24,18 @@ module AuthnK8s
                   body
                   stdin )
     ) do
+
+      extend Forwardable
+      def_delegators :@k8s_object_lookup, :kube_client
+
+      DEFAULT_KUBECTL_EXEC_COMMAND_TIMEOUT = 5
+
       def call
         @message_log = MessageLog.new
         @channel_closed = false
 
         url = server_url(@cmds, @stdin)
-        headers = kubeclient.headers.clone
+        headers = kube_client.headers.clone
         ws_client = WebSocket::Client::Simple.connect(url, headers: headers)
 
         add_websocket_event_handlers(ws_client, @body, @stdin)
@@ -34,6 +44,7 @@ def call
 
         unless @channel_closed
           raise Errors::Authentication::AuthnK8s::CommandTimedOut.new(
+            timeout,
             @container,
             @pod_name
           )
@@ -58,6 +69,11 @@ def on_open(ws_client, body, stdin)
           if stdin
             data = WebSocketMessage.channel_byte('stdin') + body
             ws_client.send(data)
+
+            # We close the socket and don't wait for the cert to be fully injected
+            # so that we can finish handling the request quickly and don't leave the
+            # Conjur server hanging. If an error occurred it will be written to
+            # the client container logs.
             ws_client.send(nil, type: :close)
           end
         end
@@ -109,10 +125,6 @@ def on_error(err)
 
       private
 
-      def kubeclient
-        @kubeclient ||= @k8s_object_lookup.kubectl_client
-      end
-
       def add_websocket_event_handlers(ws_client, body, stdin)
         kubectl = self
 
@@ -123,7 +135,7 @@ def add_websocket_event_handlers(ws_client, body, stdin)
       end
 
       def wait_for_close_message
-        (@timeout / 0.1).to_i.times do
+        (timeout / 0.1).to_i.times do
           break if @channel_closed
           sleep 0.1
         end
@@ -140,12 +152,22 @@ def base_query_string_parts
       end
 
       def server_url(cmds, stdin)
-        api_uri = kubeclient.api_endpoint
+        api_uri = kube_client.api_endpoint
         base_url = "wss://#{api_uri.host}:#{api_uri.port}"
         path = "/api/v1/namespaces/#{@pod_namespace}/pods/#{@pod_name}/exec"
         query = query_string(cmds, stdin)
         "#{base_url}#{path}?#{query}"
       end
+
+      def timeout
+        return @timeout if @timeout
+
+        kube_timeout = @env["KUBECTL_EXEC_COMMAND_TIMEOUT"]
+        not_provided = kube_timeout.to_s.strip.empty?
+        default = DEFAULT_KUBECTL_EXEC_COMMAND_TIMEOUT
+        # If the value of KUBECTL_EXEC_COMMAND_TIMEOUT is not an integer it will be zero
+        @timeout = not_provided ? default : kube_timeout.to_i
+      end
     end
 
     class KubectlExec
@@ -171,38 +193,6 @@ def execute(k8s_object_lookup:,
           stdin: stdin
         )
       end
-
-      def copy(k8s_object_lookup:,
-        pod_namespace:,
-        pod_name:,
-        path:,
-        content:,
-        mode:,
-        container: 'authenticator')
-        execute(
-          k8s_object_lookup: k8s_object_lookup,
-          pod_namespace: pod_namespace,
-          pod_name: pod_name,
-          container: container,
-          cmds: ['tar', 'xvf', '-', '-C', '/'],
-          body: tar_file_as_string(path, content, mode),
-          stdin: true
-        )
-      end
-
-      private
-
-      def tar_file_as_string(path, content, mode)
-        tarfile = StringIO.new("")
-
-        Gem::Package::TarWriter.new(tarfile) do |tar|
-          tar.add_file(path, mode) do |tf|
-            tf.write(content)
-          end
-        end
-
-        tarfile.string
-      end
     end
 
     # Utility class for processing WebSocket messages.

app/domain/errors.rb

@@ -263,7 +263,7 @@ module AuthnK8s
       )
 
       CommandTimedOut = ::Util::TrackableErrorClass.new(
-        msg:  "Command timed out in container '{0}' of pod '{1}'",
+        msg:  "Command timed out after {0} seconds in container '{1}' of pod '{2}'",
         code: "CONJ00033E"
       )
 

app/domain/logs.rb

@@ -139,7 +139,7 @@ module AuthnK8s
       )
 
       PodMessageData = ::Util::TrackableLogMessageClass.new(
-        msg:  "Pod: '{0-pod-name}', message: '{1-message-type}', data: '{2-message-data}'",
+        msg:  "Pod '{0-pod-name}': message: '{1-message-type}', data: '{2-message-data}'",
         code: "CONJ00013D"
       )
 
@@ -149,13 +149,13 @@ module AuthnK8s
       )
 
       CopySSLToPod = ::Util::TrackableLogMessageClass.new(
-        msg:  "Copying SSL certificate to {0-container-name}:{1-cert-file-path} " \
+        msg:  "Copying client certificate to {0-container-name}:{1-cert-file-path} " \
             "in {2-pod-namespace}/{3-pod-name}",
         code: "CONJ00015D"
       )
 
-      CopySSLToPodSuccess = ::Util::TrackableLogMessageClass.new(
-        msg:  "Copied SSL certificate successfully",
+      InitializeCopySSLToPodSuccess = ::Util::TrackableLogMessageClass.new(
+        msg:  "Started copying the client certificate successfully",
         code: "CONJ00037D"
       )
 

cucumber/kubernetes/features/step_definitions/login_steps.rb

@@ -36,7 +36,8 @@ def login_with_custom_prefix request_ip, host_id_suffix, host_id_prefix, success
 def login_with_username request_ip, username, success, headers = {}
   begin
     @pkey = OpenSSL::PKey::RSA.new 2048
-    login(username, request_ip, authn_k8s_host, @pkey, headers)
+    response = login(username, request_ip, authn_k8s_host, @pkey, headers)
+    expect(response.code).to be(202)
   rescue
     raise if success
     @error = $!

cucumber/kubernetes/features/support/hooks.rb

@@ -8,7 +8,7 @@
 
 Before do
   # Erase the certificates and keys from each container.
-  kubectl_client.get_pods(namespace: namespace).select{|p| p.metadata.namespace == namespace}.each do |pod|
+  kube_client.get_pods(namespace: namespace).select{|p| p.metadata.namespace == namespace}.each do |pod|
     next unless (ready_status = pod.status.conditions.find { |c| c.type == "Ready" })
     next unless ready_status.status == "True"
     next unless pod.metadata.name =~ /inventory\-deployment/