Merge branch 'terminal-max-session-time' into 'master'

Introduce maximum session time for terminal websocket connection

Closes #26263

See merge request !8413

app/controllers/admin/application_settings_controller.rb

@@ -137,6 +137,7 @@ class Admin::ApplicationSettingsController < Admin::ApplicationController
       :user_default_external,
       :user_oauth_applications,
       :version_check_enabled,
+      :terminal_max_session_time,
 
       disabled_oauth_sign_in_sources: [],
       import_sources: [],

app/models/application_setting.rb

@@ -111,6 +111,10 @@ class ApplicationSetting < ActiveRecord::Base
             presence: true,
             numericality: { only_integer: true, greater_than: :housekeeping_full_repack_period }
 
+  validates :terminal_max_session_time,
+            presence: true,
+            numericality: { only_integer: true, greater_than_or_equal_to: 0 }
+
   validates_each :restricted_visibility_levels do |record, attr, value|
     unless value.nil?
       value.each do |level|
@@ -204,7 +208,8 @@ class ApplicationSetting < ActiveRecord::Base
       signin_enabled: Settings.gitlab['signin_enabled'],
       signup_enabled: Settings.gitlab['signup_enabled'],
       two_factor_grace_period: 48,
-      user_default_external: false
+      user_default_external: false,
+      terminal_max_session_time: 0
     }
   end
 

app/models/project_services/kubernetes_service.rb

 class KubernetesService < DeploymentService
+  include Gitlab::CurrentSettings
   include Gitlab::Kubernetes
   include ReactiveCaching
 
@@ -110,7 +111,7 @@ class KubernetesService < DeploymentService
       pods = data.fetch(:pods, nil)
       filter_pods(pods, app: environment.slug).
         flat_map { |pod| terminals_for_pod(api_url, namespace, pod) }.
-        map { |terminal| add_terminal_auth(terminal, token, ca_pem) }
+        each { |terminal| add_terminal_auth(terminal, terminal_auth) }
     end
   end
 
@@ -170,4 +171,12 @@ class KubernetesService < DeploymentService
 
     url.to_s
   end
+
+  def terminal_auth
+    {
+      token: token,
+      ca_pem: ca_pem,
+      max_session_time: current_application_settings.terminal_max_session_time
+    }
+  end
 end

app/views/admin/application_settings/_form.html.haml

@@ -509,5 +509,15 @@
         .help-block
           Number of Git pushes after which 'git gc' is run.
 
+  %fieldset
+    %legend Web terminal
+    .form-group
+      = f.label :terminal_max_session_time, 'Max session time', class: 'control-label col-sm-2'
+      .col-sm-10
+        = f.number_field :terminal_max_session_time, class: 'form-control'
+        .help-block
+          Maximum time for web terminal websocket connection (in seconds).
+          Set to 0 for unlimited time.
+
   .form-actions
     = f.submit 'Save', class: 'btn btn-save'

changelogs/unreleased/terminal-max-session-time.yml

+---
+title: Introduce maximum session time for terminal websocket connection
+merge_request: 8413
+author:

db/migrate/20170126174819_add_terminal_max_session_time_to_application_settings.rb

+# See http://doc.gitlab.com/ce/development/migration_style_guide.html
+# for more information on how to write migrations for GitLab.
+
+class AddTerminalMaxSessionTimeToApplicationSettings < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  # Set this constant to true if this migration requires downtime.
+  DOWNTIME = false
+
+  # When a migration requires downtime you **must** uncomment the following
+  # constant and define a short and easy to understand explanation as to why the
+  # migration requires downtime.
+  # DOWNTIME_REASON = ''
+
+  # When using the methods "add_concurrent_index" or "add_column_with_default"
+  # you must disable the use of transactions as these methods can not run in an
+  # existing transaction. When using "add_concurrent_index" make sure that this
+  # method is the _only_ method called in the migration, any other changes
+  # should go in a separate migration. This ensures that upon failure _only_ the
+  # index creation fails and can be retried or reverted easily.
+  #
+  # To disable transactions uncomment the following line and remove these
+  # comments:
+  disable_ddl_transaction!
+
+  def up
+    add_column_with_default :application_settings, :terminal_max_session_time, :integer, default: 0, allow_null: false
+  end
+
+  def down
+    remove_column :application_settings, :terminal_max_session_time
+  end
+end

db/schema.rb

@@ -109,6 +109,7 @@ ActiveRecord::Schema.define(version: 20170204181513) do
     t.boolean "html_emails_enabled", default: true
     t.string "plantuml_url"
     t.boolean "plantuml_enabled"
+    t.integer "terminal_max_session_time", default: 0, null: false
   end
 
   create_table "audit_events", force: :cascade do |t|

doc/administration/integration/terminal.md

@@ -71,5 +71,15 @@ When these headers are not passed through, Workhorse will return a
 `400 Bad Request` response to users attempting to use a web terminal. In turn,
 they will receive a `Connection failed` message.
 
+## Limiting WebSocket connection time
+
+> [Introduced](https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/8413)
+in GitLab 8.17.
+
+Terminal sessions use long-lived connections; by default, these may last
+forever. You can configure a maximum session time in the Admin area of your
+GitLab instance if you find this undesirable from a scalability or security
+point of view.
+
 [ce-7690]: https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/7690
 [kubservice]: ../../user/project/integrations/kubernetes.md)

doc/api/settings.md

@@ -46,7 +46,8 @@ Example response:
    "koding_enabled": false,
    "koding_url": null,
    "plantuml_enabled": false,
-   "plantuml_url": null
+   "plantuml_url": null,
+   "terminal_max_session_time": 0
 }
 ```
 
@@ -84,6 +85,7 @@ PUT /application/settings
 | `disabled_oauth_sign_in_sources` | Array of strings | no | Disabled OAuth sign-in sources |
 | `plantuml_enabled` | boolean | no | Enable PlantUML integration. Default is `false`. |
 | `plantuml_url` | string | yes (if `plantuml_enabled` is `true`) |  The PlantUML instance URL for integration. |
+| `terminal_max_session_time` | integer | no | Maximum time for web terminal websocket connection (in seconds). Set to 0 for unlimited time. |
 
 ```bash
 curl --request PUT --header "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" https://gitlab.example.com/api/v3/application/settings?signup_enabled=false&default_project_visibility=1
@@ -118,6 +120,7 @@ Example response:
   "koding_enabled": false,
   "koding_url": null,
   "plantuml_enabled": false,
-  "plantuml_url": null
+  "plantuml_url": null,
+  "terminal_max_session_time": 0
 }
 ```

lib/api/entities.rb

@@ -575,6 +575,7 @@ module API
       expose :koding_url
       expose :plantuml_enabled
       expose :plantuml_url
+      expose :terminal_max_session_time
     end
 
     class Release < Grape::Entity

lib/api/settings.rb

@@ -107,6 +107,7 @@ module API
         requires :housekeeping_full_repack_period, type: Integer, desc: "Number of Git pushes after which a full 'git repack' is run."
         requires :housekeeping_gc_period, type: Integer, desc: "Number of Git pushes after which 'git gc' is run."
       end
+      optional :terminal_max_session_time, type: Integer, desc: 'Maximum time for web terminal websocket connection (in seconds). Set to 0 for unlimited time.'
       at_least_one_of :default_branch_protection, :default_project_visibility, :default_snippet_visibility,
                       :default_group_visibility, :restricted_visibility_levels, :import_sources,
                       :enabled_git_access_protocol, :gravatar_enabled, :default_projects_limit,
@@ -120,7 +121,7 @@ module API
                       :akismet_enabled, :admin_notification_email, :sentry_enabled,
                       :repository_storage, :repository_checks_enabled, :koding_enabled, :plantuml_enabled,
                       :version_check_enabled, :email_author_in_body, :html_emails_enabled,
-                      :housekeeping_enabled
+                      :housekeeping_enabled, :terminal_max_session_time
     end
     put "application/settings" do
       if current_settings.update_attributes(declared_params(include_missing: false))

lib/gitlab/kubernetes.rb

@@ -43,10 +43,10 @@ module Gitlab
       end
     end
 
-    def add_terminal_auth(terminal, token, ca_pem = nil)
+    def add_terminal_auth(terminal, token:, max_session_time:, ca_pem: nil)
       terminal[:headers]['Authorization'] << "Bearer #{token}"
+      terminal[:max_session_time] = max_session_time
       terminal[:ca_pem] = ca_pem if ca_pem.present?
-      terminal
     end
 
     def container_exec_url(api_url, namespace, pod_name, container_name)

lib/gitlab/workhorse.rb

@@ -107,7 +107,8 @@ module Gitlab
           'Terminal' => {
             'Subprotocols' => terminal[:subprotocols],
             'Url' => terminal[:url],
-            'Header' => terminal[:headers]
+            'Header' => terminal[:headers],
+            'MaxSessionTime' => terminal[:max_session_time],
           }
         }
         details['Terminal']['CAPem'] = terminal[:ca_pem] if terminal.has_key?(:ca_pem)

spec/lib/gitlab/workhorse_spec.rb

@@ -42,7 +42,8 @@ describe Gitlab::Workhorse, lib: true do
       out = {
         subprotocols: ['foo'],
         url: 'wss://example.com/terminal.ws',
-        headers: { 'Authorization' => ['Token x'] }
+        headers: { 'Authorization' => ['Token x'] },
+        max_session_time: 600
       }
       out[:ca_pem] = ca_pem if ca_pem
       out
@@ -53,7 +54,8 @@ describe Gitlab::Workhorse, lib: true do
         'Terminal' => {
           'Subprotocols' => ['foo'],
           'Url' => 'wss://example.com/terminal.ws',
-          'Header' => { 'Authorization' => ['Token x'] }
+          'Header' => { 'Authorization' => ['Token x'] },
+          'MaxSessionTime' => 600
         }
       }
       out['Terminal']['CAPem'] = ca_pem if ca_pem

spec/models/project_services/kubernetes_service_spec.rb

@@ -181,11 +181,23 @@ describe KubernetesService, models: true, caching: true do
       let(:pod) { kube_pod(app: environment.slug) }
       let(:terminals) { kube_terminals(service, pod) }
 
-      it 'returns terminals' do
-        stub_reactive_cache(service, pods: [ pod, pod, kube_pod(app: "should-be-filtered-out") ])
+      before do
+        stub_reactive_cache(
+          service,
+          pods: [ pod, pod, kube_pod(app: "should-be-filtered-out") ]
+        )
+      end
 
+      it 'returns terminals' do
         is_expected.to eq(terminals + terminals)
       end
+
+      it 'uses max session time from settings' do
+        stub_application_setting(terminal_max_session_time: 600)
+
+        times = subject.map { |terminal| terminal[:max_session_time] }
+        expect(times).to eq [600, 600, 600, 600]
+      end
     end
   end
 

spec/support/kubernetes_helpers.rb

@@ -43,7 +43,8 @@ module KubernetesHelpers
         url:  container_exec_url(service.api_url, service.namespace, pod_name, container['name']),
         subprotocols: ['channel.k8s.io'],
         headers: { 'Authorization' => ["Bearer #{service.token}"] },
-        created_at: DateTime.parse(pod['metadata']['creationTimestamp'])
+        created_at: DateTime.parse(pod['metadata']['creationTimestamp']),
+        max_session_time: 0
       }
       terminal[:ca_pem] = service.ca_pem if service.ca_pem.present?
       terminal