From 3008a03a2c3a20b5bc21498ddb4c91572fb8556b Mon Sep 17 00:00:00 2001 From: Mark Yen Date: Tue, 15 Sep 2020 14:59:37 -0700 Subject: [PATCH 1/5] feat: secret rotation This adds a job to trigger secret rotation for all generated secrets. This is useful for administrators to rotate all secrets (generally as a precaution); attempting to document manual instructions is likely to be error-prone. --- Makefile | 6 +- .../helm/secret-rotation/rotate-secrets.rb | 98 +++++++++++++ .../secret-rotation/rotate-secrets_spec.rb | 131 ++++++++++++++++++ chart/config/releases.yaml | 5 + chart/templates/secret-rotation.yaml | 79 +++++++++++ scripts/rubocop.sh | 15 ++ tests/rspec.sh | 10 ++ 7 files changed, 343 insertions(+), 1 deletion(-) create mode 100755 chart/assets/scripts/helm/secret-rotation/rotate-secrets.rb create mode 100755 chart/assets/scripts/helm/secret-rotation/rotate-secrets_spec.rb create mode 100644 chart/templates/secret-rotation.yaml create mode 100755 scripts/rubocop.sh create mode 100755 tests/rspec.sh diff --git a/Makefile b/Makefile index c58029386c..334436cab1 100644 --- a/Makefile +++ b/Makefile @@ -13,11 +13,14 @@ version: update-subcharts: @./scripts/update-subcharts.sh -lint: shellcheck yamllint helmlint httplint +lint: shellcheck yamllint helmlint httplint rubocop helmlint: @./scripts/helmlint.sh +rubocop: + @./scripts/rubocop.sh + shellcheck: @./scripts/shellcheck.sh @@ -26,6 +29,7 @@ yamllint: test: ./tests/config.sh + ./tests/rspec.sh .PHONY: httplint httplint: diff --git a/chart/assets/scripts/helm/secret-rotation/rotate-secrets.rb b/chart/assets/scripts/helm/secret-rotation/rotate-secrets.rb new file mode 100755 index 0000000000..02bf1db939 --- /dev/null +++ b/chart/assets/scripts/helm/secret-rotation/rotate-secrets.rb @@ -0,0 +1,98 @@ +#!/usr/bin/env ruby +# frozen_string_literal: true + +# This script is used to generate a ConfigMap to rotate all secrets in the +# deployment. + +require 'English' +require 'kubeclient' +require 'json' +require 'open3' +require 'time' +require 'yaml' + +# SecretRotator will rotate all quarks secrets for the current deployment +class SecretRotator + # Path to the Kubernetes API server CA certificate + CA_CERT_PATH = '/var/run/secrets/kubernetes.io/serviceaccount/ca.crt' + + # Path to the Kubernetes service account token + TOKEN_PATH = '/var/run/secrets/kubernetes.io/serviceaccount/token' + + # HTTP status code when a resource already exists + HTTP_STATUS_CONFLICT = 409 + + # The namespace the QuarksSecrets is in, and the same to create the ConfigMap + def namespace + @namespace ||= ENV['NAMESPACE'] || raise('NAMESPACE not set') + end + + # The BOSHDeployment name + def deployment + @deployment ||= ENV['DEPLOYMENT'] || raise('DEPLOYMENT not set') + end + + # Authentication options to create a Kubernetes client + def auth_options + { bearer_token_file: TOKEN_PATH } + end + + # SSL options to create a Kubernetes client + def ssl_options + @ssl_options ||= {}.tap do |options| + options[:ca_file] = CA_CERT_PATH if File.exist? CA_CERT_PATH + end + end + + # Kubernetes client to access default APIs + def client + @client ||= Kubeclient::Client.new( + 'https://kubernetes.default.svc', + 'v1', + auth_options: auth_options, + ssl_options: ssl_options + ) + end + + # Kubernetes client to access Quarks APIs + def quarks_client + @quarks_client ||= Kubeclient::Client.new( + 'https://kubernetes.default.svc/apis/quarks.cloudfoundry.org', + 'v1alpha1', + auth_options: auth_options, + ssl_options: ssl_options + ) + end + + # The selector used to find interesting secrets + def secret_selector + "quarks.cloudfoundry.org/deployment-name=#{deployment}" + end + + # The names of the QuarksSecrets to rotate + def secrets + quarks_client + .get_quarks_secrets(namespace: namespace, selector: secret_selector) + .map { |secret| secret.metadata.name } + end + + # The ConfigMap resource to be created + def configmap + @configmap ||= Kubeclient::Resource.new( + metadata: { + namespace: namespace, + # Set a unique-ish name to help running this multiple times + name: "rotate.all-secrets-#{Time.now.to_i}", + labels: { 'quarks.cloudfoundry.org/secret-rotation': 'true' } + }, + data: { secrets: secrets.to_json } + ) + end + + # Trigger rotation of all QuarksSecrets + def rotate + client.create_config_map configmap + end +end + +SecretRotator.new.rotate if $PROGRAM_NAME == __FILE__ diff --git a/chart/assets/scripts/helm/secret-rotation/rotate-secrets_spec.rb b/chart/assets/scripts/helm/secret-rotation/rotate-secrets_spec.rb new file mode 100755 index 0000000000..d4470ce771 --- /dev/null +++ b/chart/assets/scripts/helm/secret-rotation/rotate-secrets_spec.rb @@ -0,0 +1,131 @@ +#!/usr/bin/env ruby +# frozen_string_literal: true + +require_relative 'rotate-secrets' + +RSpec.describe(SecretRotator) do + def instance + @instance ||= described_class.new + end + + before :each do + # Default to having a good setup + allow(ENV).to receive(:[]).with('NAMESPACE').and_return 'namespace' + allow(ENV).to receive(:[]).with('DEPLOYMENT').and_return 'deployment' + allow(File).to receive(:exist?).with(described_class::CA_CERT_PATH).and_return true + end + + describe '#namespace' do + it 'returns the namespace' do + expect(ENV).to receive(:[]).with('NAMESPACE').and_return 'ns' + expect(instance.namespace).to eq 'ns' + end + + it 'errors out if namespace is not set' do + expect(ENV).to receive(:[]).with('NAMESPACE').and_return nil + expect { instance.namespace }.to raise_error(/NAMESPACE not set/) + end + end + + describe '#deployment' do + it 'returns the deployment name' do + expect(ENV).to receive(:[]).with('DEPLOYMENT').and_return 'dp' + expect(instance.deployment).to eq 'dp' + end + + it 'errors out if deployment name is not set' do + expect(ENV).to receive(:[]).with('DEPLOYMENT').and_return nil + expect { instance.deployment }.to raise_error(/DEPLOYMENT not set/) + end + end + + describe '#ssl_options' do + it 'returns empty options if the CA cert is missing' do + allow(File).to receive(:exist?).with(described_class::CA_CERT_PATH).and_return false + expect(instance.ssl_options).to be_empty + end + + it 'returns options with the CA cert path' do + expect(instance.ssl_options).to eq(ca_file: described_class::CA_CERT_PATH) + end + end + + describe '#client' do + it 'creates a Kubernetes client' do + expected_client = {} + expect(Kubeclient::Client).to receive(:new).with( + 'https://kubernetes.default.svc', + 'v1', + auth_options: { bearer_token_file: described_class::TOKEN_PATH }, + ssl_options: { ca_file: described_class::CA_CERT_PATH } + ).once.and_return expected_client + expect(instance.client).to be expected_client + + # call it again should not ask for a new client + expect(instance.client).to be expected_client + end + end + + describe '#quarks_client' do + it 'creates a Kubernetes client' do + expected_client = {} + expect(Kubeclient::Client).to receive(:new).with( + 'https://kubernetes.default.svc/apis/quarks.cloudfoundry.org', + 'v1alpha1', + auth_options: { bearer_token_file: described_class::TOKEN_PATH }, + ssl_options: { ca_file: described_class::CA_CERT_PATH } + ).once.and_return expected_client + expect(instance.quarks_client).to be expected_client + + # call it again should not ask for a new client + expect(instance.quarks_client).to be expected_client + end + end + + describe '#secrets' do + it 'returns the QuarksSecret names' do + expected_names = %w[one two three] + secrets = expected_names.map do |name| + double('QuarksSecret').tap do |secret| + expect(secret).to receive_message_chain(:metadata, :name).and_return name + end + end + + expect(instance) + .to receive_message_chain(:quarks_client, :get_quarks_secrets) + .with( + namespace: 'namespace', + selector: 'quarks.cloudfoundry.org/deployment-name=deployment' + ) + .and_return(secrets) + + expect(instance.secrets).to eq expected_names + end + end + + describe '#configmap' do + it 'returns the desired config map' do + expect(instance).to receive(:secrets).and_return %w[one two] + allow(Time).to receive(:now).and_return 123 + expect(instance.configmap.to_h).to eq( + metadata: { + namespace: 'namespace', + name: 'rotate.all-secrets-123', + labels: { 'quarks.cloudfoundry.org/secret-rotation': 'true' } + }, + data: { secrets: %w[one two].to_json } + ) + end + end + + describe '#rotate' do + it 'attempts to create a config map' do + configmap = {} + expect(instance).to receive(:configmap).and_return configmap + expect(instance) + .to receive_message_chain(:client, :create_config_map) + .with(be(configmap)) + instance.rotate + end + end +end diff --git a/chart/config/releases.yaml b/chart/config/releases.yaml index 2bcf9b811a..b2c8bc87c4 100644 --- a/chart/config/releases.yaml +++ b/chart/config/releases.yaml @@ -48,6 +48,11 @@ releases: postgres: condition: features.autoscaler.enabled version: "39" + secret-rotation: + # secret-rotation is not a BOSH release. It requires ruby & kubectl. + image: + repository: splatform/fissile-stemcell-sle + tag: SLE_15_SP1-27.4 sync-integration-tests: # XXX SITS only makes sense when using Diego; add error check somewhere? condition: testing.sync_integration_tests.enabled diff --git a/chart/templates/secret-rotation.yaml b/chart/templates/secret-rotation.yaml new file mode 100644 index 0000000000..9283822a49 --- /dev/null +++ b/chart/templates/secret-rotation.yaml @@ -0,0 +1,79 @@ +# This creates a (manually triggered) Quarks Job to rotate all generated +# secrets. +--- +apiVersion: quarks.cloudfoundry.org/v1alpha1 +kind: QuarksJob +metadata: + name: secret-rotation + namespace: {{ .Release.Namespace | quote }} + labels: + {{- list $ "secret-rotation" | include "component.labels" | nindent 4 }} +spec: + trigger: + strategy: manual + template: + spec: + template: + spec: + containers: + - name: generate-rotation-config-map + {{- with $image := index $.Values.releases "secret-rotation" "image" }} + image: {{ printf "%s:%s" $image.repository $image.tag }} + imagePullPolicy: {{ $image.pullPolicy | quote }} + {{- end }} + command: + - /usr/bin/env + - ruby + - -e + - | + {{- .Files.Get "assets/scripts/helm/secret-rotation/rotate-secrets.rb" | nindent 14 }} + env: + - name: DEPLOYMENT + value: {{ include "kubecf.deployment-name" . | quote }} + - name: NAMESPACE + valueFrom: + fieldRef: + fieldPath: metadata.namespace + restartPolicy: Never + serviceAccountName: secret-rotation + automountServiceAccountToken: true +--- +# We require a ServiceAccount with access to listing QuarksSecrets +apiVersion: v1 +kind: ServiceAccount +metadata: + name: secret-rotation + namespace: {{ .Release.Namespace | quote }} + labels: + {{- list $ "secret-rotation" | include "component.labels" | nindent 4 }} +--- +apiVersion: rbac.authorization.k8s.io/v1 +kind: Role +metadata: + name: secret-rotation + namespace: {{ .Release.Namespace | quote }} + labels: + {{- list $ "secret-rotation" | include "component.labels" | nindent 4 }} +rules: +- apiGroups: [ quarks.cloudfoundry.org ] + resources: [ quarkssecrets ] + verbs: [ list ] +- apiGroups: [ '' ] + resources: [ configmaps ] + verbs: [ create ] +--- +apiVersion: rbac.authorization.k8s.io/v1 +kind: RoleBinding +metadata: + name: secret-rotation + namespace: {{ .Release.Namespace | quote }} + labels: + {{- list $ "secret-rotation" | include "component.labels" | nindent 4 }} +subjects: +- kind: ServiceAccount + namespace: {{ .Release.Namespace | quote}} + name: secret-rotation +roleRef: + apiGroup: rbac.authorization.k8s.io + kind: Role + name: secret-rotation diff --git a/scripts/rubocop.sh b/scripts/rubocop.sh new file mode 100755 index 0000000000..9fd60b81f4 --- /dev/null +++ b/scripts/rubocop.sh @@ -0,0 +1,15 @@ +#!/usr/bin/env bash +source scripts/include/setup.sh + +require_tools rubocop + +dirs=( + chart/assets/scripts/ +) + +disabled_cops=( + Metrics/BlockLength +) +disabled_cops_string="$(IFS=, ; echo "${disabled_cops[*]}")" + +rubocop --except "${disabled_cops_string}" "${dirs[@]}" diff --git a/tests/rspec.sh b/tests/rspec.sh new file mode 100755 index 0000000000..ee22757e68 --- /dev/null +++ b/tests/rspec.sh @@ -0,0 +1,10 @@ +#!/usr/bin/env bash +source scripts/include/setup.sh + +require_tools rspec + +dirs=( + chart/assets/scripts/ +) + +rspec "${dirs[@]}" From e0dbd3e2e2498b8eb5653340f2d53d070e88b551 Mon Sep 17 00:00:00 2001 From: Mark Yen Date: Fri, 18 Sep 2020 16:35:00 -0700 Subject: [PATCH 2/5] fix: secret rotation: Try to exclude some secrets Attempting to get things in a state where rotation will not break the deployment. --- .../helm/secret-rotation/rotate-secrets.rb | 33 +++++++++++++++++-- 1 file changed, 30 insertions(+), 3 deletions(-) diff --git a/chart/assets/scripts/helm/secret-rotation/rotate-secrets.rb b/chart/assets/scripts/helm/secret-rotation/rotate-secrets.rb index 02bf1db939..fcb47d9c46 100755 --- a/chart/assets/scripts/helm/secret-rotation/rotate-secrets.rb +++ b/chart/assets/scripts/helm/secret-rotation/rotate-secrets.rb @@ -70,28 +70,55 @@ def secret_selector end # The names of the QuarksSecrets to rotate - def secrets + def all_secrets quarks_client .get_quarks_secrets(namespace: namespace, selector: secret_selector) .map { |secret| secret.metadata.name } end + def excluded_secrets + [ + # Do not rotate the various CC DB encryption related secrets; that has + # effects on the data in the databases. These need to be rotated manually. + /^var-ccdb-key-label/, + 'var-cc-db-encryption-key', + # Do not rotate the PXC root password: we can't restart the database pod + # afterwards if we do (because the database root password isn't actually + # updated). + 'var-pxc-root-password', + # Since we don't restart the PXC container, we can't update its CA cert. + 'var-pxc-ca' + ] + end + + def secrets + all_secrets.sort.reject do |secret| + excluded_secrets.any? { |excluded| excluded === secret } # rubocop:disable Style/CaseEquality + end + end + + def configmap_name + @configmap_name ||= "rotate.all-secrets.#{Time.now.to_i}" + end + # The ConfigMap resource to be created def configmap @configmap ||= Kubeclient::Resource.new( metadata: { namespace: namespace, # Set a unique-ish name to help running this multiple times - name: "rotate.all-secrets-#{Time.now.to_i}", + name: configmap_name, labels: { 'quarks.cloudfoundry.org/secret-rotation': 'true' } }, - data: { secrets: secrets.to_json } + data: { secrets: JSON.pretty_generate(secrets) } ) end # Trigger rotation of all QuarksSecrets def rotate client.create_config_map configmap + puts "Created secret rotation configmap #{configmap_name} with #{secrets.length} secrets:" + secrets.each { |secret| puts " #{secret}" } end end From 191e1a7f5134fdb1fcda5a655ccea5ba02fb3786 Mon Sep 17 00:00:00 2001 From: Mark Yen Date: Mon, 21 Sep 2020 13:43:27 -0700 Subject: [PATCH 3/5] fix: Restart instance groups on secret update We need the instance groups to be updated when their dependent secrets are updated, so that the pods can pick up the new secrets. Otherwise the instance groups in flight will not match the secrets stored in Kubernetes, and can cause issues later. --- .../operations/quarks-restart-on-update.yaml | 54 +++++++++++++++++++ 1 file changed, 54 insertions(+) create mode 100644 chart/assets/operations/quarks-restart-on-update.yaml diff --git a/chart/assets/operations/quarks-restart-on-update.yaml b/chart/assets/operations/quarks-restart-on-update.yaml new file mode 100644 index 0000000000..6e6fb58243 --- /dev/null +++ b/chart/assets/operations/quarks-restart-on-update.yaml @@ -0,0 +1,54 @@ +{{- /* + This ops file sets Quarks-specific annotations on instance groups to ensure + they are correctly restarted when their dependent secrets change (so that we + can rotate the various generated secrets). + + See: https://github.com/cloudfoundry-incubator/quarks-operator/issues/1136 +*/ -}} +{{- include "_config.load" $ }} + +{{- $instance_groups := list }} + +{{- $instance_groups = append $instance_groups "api" }} +{{- $instance_groups = append $instance_groups "cc-worker" }} +{{- $instance_groups = append $instance_groups "diego-api" }} +{{- $instance_groups = append $instance_groups "doppler" }} +{{- $instance_groups = append $instance_groups "log-api" }} +{{- $instance_groups = append $instance_groups "log-cache" }} +{{- $instance_groups = append $instance_groups "nats" }} +{{- $instance_groups = append $instance_groups "router" }} +{{- $instance_groups = append $instance_groups "scheduler" }} +{{- $instance_groups = append $instance_groups "uaa" }} + +{{- if .Values.features.autoscaler.enabled }} + {{- $instance_groups = append $instance_groups "asactors" }} + {{- $instance_groups = append $instance_groups "asapi" }} + {{- $instance_groups = append $instance_groups "asmetrics" }} + {{- $instance_groups = append $instance_groups "asnozzle" }} +{{- end }} + +{{- if .Values.features.credhub.enabled }} + {{- $instance_groups = append $instance_groups "credhub" }} +{{- end }} + +{{- if .Values.features.routing_api.enabled }} + {{- $instance_groups = append $instance_groups "routing-api" }} + {{- $instance_groups = append $instance_groups "tcp-router" }} +{{- end }} + +{{- if not $.Values.features.eirini.enabled }} + {{- $instance_groups = append $instance_groups "auctioneer" }} + {{- if not .Values.features.multiple_cluster_mode.control_plane.enabled }} + {{- $instance_groups = append $instance_groups "diego-cell" }} + {{- end }} +{{- end }} + +{{- if eq .Values.features.blobstore.provider "singleton" }} + {{- $instance_groups = append $instance_groups "singleton-blobstore" }} +{{- end }} + +{{- range $instance_groups }} +- type: replace + path: /instance_groups/name={{ . }}/env?/bosh/agent/settings/annotations/quarks.cloudfoundry.org~1restart-on-update + value: "true" +{{- end }} From 53e96deb5b8923795699d317bf74f839bb2ca350 Mon Sep 17 00:00:00 2001 From: Mark Yen Date: Mon, 21 Sep 2020 13:45:43 -0700 Subject: [PATCH 4/5] fix: database-seeder: Show which dbs were updated Print out the names of the databases we are updating in the database-seeder job, as otherwise we have no output and it is difficult to determine if we were doing anything useful. --- chart/assets/scripts/jobs/pxc/seeder.sh | 1 + 1 file changed, 1 insertion(+) diff --git a/chart/assets/scripts/jobs/pxc/seeder.sh b/chart/assets/scripts/jobs/pxc/seeder.sh index cdfe82f608..92ec824042 100644 --- a/chart/assets/scripts/jobs/pxc/seeder.sh +++ b/chart/assets/scripts/jobs/pxc/seeder.sh @@ -42,6 +42,7 @@ mysql < <( GRANT ALL ON \`${database}\`.* TO '${database}'@'%'; " + echo " ${database}" >&2 # For status done echo "COMMIT;" ) From bbbec89ca0783562bd0c4d27b6d7e355e0328603 Mon Sep 17 00:00:00 2001 From: Mark Yen Date: Fri, 18 Sep 2020 16:34:27 -0700 Subject: [PATCH 5/5] fix: make diego-cell wait for apps-dns It requires it now and falls over if it can't contact the server. --- chart/assets/operations/sequencing.yaml | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/chart/assets/operations/sequencing.yaml b/chart/assets/operations/sequencing.yaml index e5f7c5cfc6..62ee7d8f76 100644 --- a/chart/assets/operations/sequencing.yaml +++ b/chart/assets/operations/sequencing.yaml @@ -28,6 +28,7 @@ # --> tcp-router (feature: routing-api) # --> credhub (feature: credhub) # --> diego-cell (feature: not eirini) +# apps-dns --> diego-cell (feature: not eirini) # deigo-api --> auctioneer (feature: not eirini) # asdatabase --> asactors (feature: autoscaler) # --> asapi (feature: autoscaler) @@ -50,7 +51,7 @@ {{- if not .Values.features.eirini.enabled }} {{- template "wait-for" list "auctioneer" "diego-api" }} - {{- template "wait-for" list "diego-cell" "uaa" }} + {{- template "wait-for" list "diego-cell" "uaa" "apps-dns" }} {{- end }} {{- if eq .Values.features.blobstore.provider "singleton" }}