Merge pull request #7694 from mgugino-upstream-stage/sess-auth-refactor

Refactor session authentication secrets

playbooks/openshift-master/private/config.yml

@@ -77,90 +77,16 @@
   hosts: oo_first_master
   roles:
   - role: openshift_facts
-  post_tasks:
-  - openshift_facts:
-      role: master
-      local_facts:
-        session_auth_secrets: "{{ openshift_master_session_auth_secrets | default(openshift.master.session_auth_secrets | default(None)) }}"
-        session_encryption_secrets: "{{ openshift_master_session_encryption_secrets | default(openshift.master.session_encryption_secrets | default(None)) }}"
-  - name: Check for existing configuration
-    stat:
-      path: /etc/origin/master/master-config.yaml
-    register: master_config_stat
-
-  - name: Set clean install fact
-    set_fact:
-      l_clean_install: "{{ not master_config_stat.stat.exists | bool }}"
-
-  - name: Determine if etcd3 storage is in use
-    command: grep  -Pzo  "storage-backend:\n.*etcd3" /etc/origin/master/master-config.yaml -q
-    register: etcd3_grep
-    failed_when: false
-    changed_when: false
-
-  - name: Set etcd3 fact
-    set_fact:
-      l_etcd3_enabled: "{{ etcd3_grep.rc == 0 | bool }}"
-
-  - name: Check if atomic-openshift-master sysconfig exists yet
-    stat:
-      path: /etc/sysconfig/atomic-openshift-master
-    register: l_aom_exists
-
-  - name: Preserve OPENSHIFT_DEFAULT_REGISTRY master parameter if present
-    command: awk '/^OPENSHIFT_DEFAULT_REGISTRY/' /etc/sysconfig/atomic-openshift-master
-    register: l_default_registry_defined
-    when: l_aom_exists.stat.exists | bool
-
-  - name: Check if atomic-openshift-master-api sysconfig exists yet
-    stat:
-      path: /etc/sysconfig/atomic-openshift-master-api
-    register: l_aom_api_exists
-
-  - name: Preserve OPENSHIFT_DEFAULT_REGISTRY master-api parameter if present
-    command: awk '/^OPENSHIFT_DEFAULT_REGISTRY/' /etc/sysconfig/atomic-openshift-master-api
-    register: l_default_registry_defined_api
-    when: l_aom_api_exists.stat.exists | bool
-
-  - name: Check if atomic-openshift-master-controllers sysconfig exists yet
-    stat:
-      path: /etc/sysconfig/atomic-openshift-master-controllers
-    register: l_aom_controllers_exists
-
-  - name: Preserve OPENSHIFT_DEFAULT_REGISTRY master-controllers parameter if present
-    command: awk '/^OPENSHIFT_DEFAULT_REGISTRY/' /etc/sysconfig/atomic-openshift-master-controllers
-    register: l_default_registry_defined_controllers
-    when: l_aom_controllers_exists.stat.exists | bool
-
-  - name: Update facts with OPENSHIFT_DEFAULT_REGISTRY value
-    set_fact:
-      l_default_registry_value: "{{ l_default_registry_defined.stdout | default('') }}"
-      l_default_registry_value_api: "{{ l_default_registry_defined_api.stdout | default('') }}"
-      l_default_registry_value_controllers: "{{ l_default_registry_defined_controllers.stdout | default('') }}"
-
-- name: Generate master session secrets
-  hosts: oo_first_master
-  vars:
-    g_session_secrets_present: "{{ (openshift.master.session_auth_secrets | default([])) | length > 0 and (openshift.master.session_encryption_secrets | default([])) | length > 0 }}"
-    g_session_auth_secrets: "{{ [ 24 | lib_utils_oo_generate_secret ] }}"
-    g_session_encryption_secrets: "{{ [ 24 | lib_utils_oo_generate_secret ] }}"
-  roles:
-  - role: openshift_facts
   tasks:
-  - openshift_facts:
-      role: master
-      local_facts:
-        session_auth_secrets: "{{ g_session_auth_secrets }}"
-        session_encryption_secrets: "{{ g_session_encryption_secrets }}"
-    when: not g_session_secrets_present | bool
+  - import_role:
+      name: openshift_control_plane
+      tasks_from: check_existing_config.yml
 
 - name: Configure masters
   hosts: oo_masters_to_config
   any_errors_fatal: true
   vars:
     openshift_master_count: "{{ openshift.master.master_count }}"
-    openshift_master_session_auth_secrets: "{{ hostvars[groups.oo_first_master.0].openshift.master.session_auth_secrets }}"
-    openshift_master_session_encryption_secrets: "{{ hostvars[groups.oo_first_master.0].openshift.master.session_encryption_secrets }}"
     openshift_ca_host: "{{ groups.oo_first_master.0 }}"
   pre_tasks:
   - name: Prepare the bootstrap node config on masters for self-hosting

roles/lib_utils/action_plugins/sanity_checks.py

@@ -160,6 +160,36 @@ class ActionModule(ActionBase):
                 raise errors.AnsibleModuleError(error_msg)
         return None
 
+    def check_session_auth_secrets(self, hostvars, host):
+        """Checks session_auth_secrets is correctly formatted"""
+        sas = self.template_var(hostvars, host,
+                                'openshift_master_session_auth_secrets')
+        ses = self.template_var(hostvars, host,
+                                'openshift_master_session_encryption_secrets')
+        # This variable isn't mandatory, only check if set.
+        if sas is None and ses is None:
+            return None
+
+        if not (
+                issubclass(type(sas), list) and issubclass(type(ses), list)
+        ) or len(sas) != len(ses):
+            raise errors.AnsibleModuleError(
+                'Expects openshift_master_session_auth_secrets and '
+                'openshift_master_session_encryption_secrets are equal length lists')
+
+        for secret in sas:
+            if len(secret) < 32:
+                raise errors.AnsibleModuleError(
+                    'Invalid secret in openshift_master_session_auth_secrets. '
+                    'Secrets must be at least 32 characters in length.')
+
+        for secret in ses:
+            if len(secret) not in [16, 24, 32]:
+                raise errors.AnsibleModuleError(
+                    'Invalid secret in openshift_master_session_encryption_secrets. '
+                    'Secrets must be 16, 24, or 32 characters in length.')
+        return None
+
     def run_checks(self, hostvars, host):
         """Execute the hostvars validations against host"""
         distro = self.template_var(hostvars, host, 'ansible_distribution')
@@ -170,6 +200,7 @@ class ActionModule(ActionBase):
         self.network_plugin_check(hostvars, host)
         self.check_hostname_vars(hostvars, host)
         self.check_supported_ocp_version(hostvars, host, odt)
+        self.check_session_auth_secrets(hostvars, host)
 
     def run(self, tmp=None, task_vars=None):
         result = super(ActionModule, self).run(tmp, task_vars)

roles/openshift_control_plane/defaults/main.yml

@@ -145,3 +145,9 @@ openshift_master_csr_namespace: openshift-infra
 
 openshift_master_config_file: "{{ openshift_master_config_dir }}/master-config.yaml"
 openshift_master_scheduler_conf: "{{ openshift_master_config_dir }}/scheduler.json"
+
+l_osm_sess_auth_def: "{{ hostvars[groups.oo_first_master.0]['l_osm_session_auth_secrets'] }}"
+l_osm_session_auth_secrets: "{{ openshift_master_session_auth_secrets | default(l_osm_sess_secret_def) }}"
+
+l_osm_sess_encrypt_def: "{{ hostvars[groups.oo_first_master.0]['l_osm_session_encryption_secrets'] }}"
+l_osm_session_encryption_secrets: "{{ openshift_master_session_encryption_secrets | default(l_osm_sess_encrypt_def) }}"

roles/openshift_control_plane/tasks/check_existing_config.yml

@@ -0,0 +1,57 @@
+---
+- name: Check for existing configuration
+  stat:
+    path: /etc/origin/master/master-config.yaml
+  register: master_config_stat
+
+- name: Set clean install fact
+  set_fact:
+    l_clean_install: "{{ not master_config_stat.stat.exists | bool }}"
+
+- name: Determine if etcd3 storage is in use
+  command: grep  -Pzo  "storage-backend:\n.*etcd3" /etc/origin/master/master-config.yaml -q
+  register: etcd3_grep
+  failed_when: false
+  changed_when: false
+
+- name: Set etcd3 fact
+  set_fact:
+    l_etcd3_enabled: "{{ etcd3_grep.rc == 0 | bool }}"
+
+- name: Check if atomic-openshift-master sysconfig exists yet
+  stat:
+    path: /etc/sysconfig/atomic-openshift-master
+  register: l_aom_exists
+
+- name: Preserve OPENSHIFT_DEFAULT_REGISTRY master parameter if present
+  command: awk '/^OPENSHIFT_DEFAULT_REGISTRY/' /etc/sysconfig/atomic-openshift-master
+  register: l_default_registry_defined
+  when: l_aom_exists.stat.exists | bool
+
+- name: Check if atomic-openshift-master-api sysconfig exists yet
+  stat:
+    path: /etc/sysconfig/atomic-openshift-master-api
+  register: l_aom_api_exists
+
+- name: Preserve OPENSHIFT_DEFAULT_REGISTRY master-api parameter if present
+  command: awk '/^OPENSHIFT_DEFAULT_REGISTRY/' /etc/sysconfig/atomic-openshift-master-api
+  register: l_default_registry_defined_api
+  when: l_aom_api_exists.stat.exists | bool
+
+- name: Check if atomic-openshift-master-controllers sysconfig exists yet
+  stat:
+    path: /etc/sysconfig/atomic-openshift-master-controllers
+  register: l_aom_controllers_exists
+
+- name: Preserve OPENSHIFT_DEFAULT_REGISTRY master-controllers parameter if present
+  command: awk '/^OPENSHIFT_DEFAULT_REGISTRY/' /etc/sysconfig/atomic-openshift-master-controllers
+  register: l_default_registry_defined_controllers
+  when: l_aom_controllers_exists.stat.exists | bool
+
+- name: Update facts with OPENSHIFT_DEFAULT_REGISTRY value
+  set_fact:
+    l_default_registry_value: "{{ l_default_registry_defined.stdout | default('') }}"
+    l_default_registry_value_api: "{{ l_default_registry_defined_api.stdout | default('') }}"
+    l_default_registry_value_controllers: "{{ l_default_registry_defined_controllers.stdout | default('') }}"
+
+- import_tasks: generate_session_secrets.yml

roles/openshift_control_plane/tasks/generate_session_secrets.yml

@@ -0,0 +1,28 @@
+---
+# This should be run on the first master so we can set_fact some items
+# to ensure values are consistent across cluster
+
+- name: Determine if sessions secrets already in place
+  stat:
+    path: "{{ openshift.master.session_secrets_file }}"
+  register: l_osm_session_secrets_stat
+
+- name: setup session secrets if not defined
+  set_fact:
+    l_osm_session_auth_secrets: "{{ [ 24 | lib_utils_oo_generate_secret ] }}"
+    l_osm_session_encryption_secrets: "{{ [ 24 | lib_utils_oo_generate_secret ] }}"
+  when: not l_osm_session_secrets_stat.stat.exists
+
+# lib_utils_oo_collect is a custom filter in
+# roles/lib_utils/filter_plugins/oo_filters.py
+- name: Gather existing session secrets from first master
+  set_fact:
+    l_osm_session_auth_secrets: "{{ l_existing_osm_session.secrets | lib_utils_oo_collect('authentication') }}"
+    l_osm_session_encryption_secrets: "{{ l_existing_osm_session.secrets | lib_utils_oo_collect('encryption') }}"
+  vars:
+    l_existing_osm_session: "{{ (osm_session_secrets_stat.content | b64decode | from_yaml) }}"
+  when:
+  - l_osm_session_secrets_stat.stat.exists
+  - l_existing_osm_session.secrets is defined
+  - l_existing_osm_session.secrets != ''
+  - l_existing_osm_session.secrets != []

roles/openshift_control_plane/tasks/main.yml

@@ -129,9 +129,6 @@
     owner: root
     group: root
     mode: 0600
-  when:
-  - openshift.master.session_auth_secrets is defined
-  - openshift.master.session_encryption_secrets is defined
 
 - set_fact:
     # translate_idps is a custom filter in role lib_utils

roles/openshift_control_plane/templates/master.yaml.v1.j2

@@ -185,9 +185,7 @@ oauthConfig:
   sessionConfig:
     sessionMaxAgeSeconds: {{ openshift.master.session_max_seconds }}
     sessionName: {{ openshift.master.session_name }}
-{% if openshift.master.session_auth_secrets is defined and openshift.master.session_encryption_secrets is defined %}
     sessionSecretsFile: {{ openshift.master.session_secrets_file }}
-{% endif %}
   tokenConfig:
     accessTokenMaxAgeSeconds: {{ openshift.master.access_token_max_seconds }}
     authorizeTokenMaxAgeSeconds: {{ openshift.master.auth_token_max_seconds }}

roles/openshift_control_plane/templates/sessionSecretsFile.yaml.v1.j2

@@ -1,7 +1,7 @@
 apiVersion: v1
 kind: SessionSecrets
 secrets:
-{% for secret in openshift.master.session_auth_secrets %}
-- authentication: "{{ openshift.master.session_auth_secrets[loop.index0] }}"
-  encryption: "{{ openshift.master.session_encryption_secrets[loop.index0] }}"
+{% for secret in l_osm_session_auth_secrets %}
+- authentication: "{{ l_osm_session_auth_secrets[loop.index0] }}"
+  encryption: "{{ l_osm_session_encryption_secrets[loop.index0] }}"
 {% endfor %}

roles/openshift_facts/library/openshift_facts.py

@@ -1334,7 +1334,6 @@ class OpenShiftFacts(object):
         pop_obsolete_local_facts(new_local_facts)
 
         if new_local_facts != local_facts:
-            self.validate_local_facts(new_local_facts)
             changed = True
             if not module.check_mode:  # noqa: F405
                 save_local_facts(self.filename, new_local_facts)
@@ -1359,71 +1358,6 @@ class OpenShiftFacts(object):
             del facts[fact]
         return facts
 
-    def validate_local_facts(self, facts=None):
-        """ Validate local facts
-
-            Args:
-                facts (dict): local facts to validate
-        """
-        invalid_facts = dict()
-        invalid_facts = self.validate_master_facts(facts, invalid_facts)
-        if invalid_facts:
-            msg = 'Invalid facts detected:\n'
-            # pylint: disable=consider-iterating-dictionary
-            for key in invalid_facts.keys():
-                msg += '{0}: {1}\n'.format(key, invalid_facts[key])
-            module.fail_json(msg=msg, changed=self.changed)  # noqa: F405
-
-    # disabling pylint errors for line-too-long since we're dealing
-    # with best effort reduction of error messages here.
-    # disabling errors for too-many-branches since we require checking
-    # many conditions.
-    # pylint: disable=line-too-long, too-many-branches
-    @staticmethod
-    def validate_master_facts(facts, invalid_facts):
-        """ Validate master facts
-
-            Args:
-                facts (dict): local facts to validate
-                invalid_facts (dict): collected invalid_facts
-
-            Returns:
-                dict: Invalid facts
-        """
-        if 'master' in facts:
-            # openshift.master.session_auth_secrets
-            if 'session_auth_secrets' in facts['master']:
-                session_auth_secrets = facts['master']['session_auth_secrets']
-                if not issubclass(type(session_auth_secrets), list):
-                    invalid_facts['session_auth_secrets'] = 'Expects session_auth_secrets is a list.'
-                elif 'session_encryption_secrets' not in facts['master']:
-                    invalid_facts['session_auth_secrets'] = ('openshift_master_session_encryption secrets must be set '
-                                                             'if openshift_master_session_auth_secrets is provided.')
-                elif len(session_auth_secrets) != len(facts['master']['session_encryption_secrets']):
-                    invalid_facts['session_auth_secrets'] = ('openshift_master_session_auth_secrets and '
-                                                             'openshift_master_session_encryption_secrets must be '
-                                                             'equal length.')
-                else:
-                    for secret in session_auth_secrets:
-                        if len(secret) < 32:
-                            invalid_facts['session_auth_secrets'] = ('Invalid secret in session_auth_secrets. '
-                                                                     'Secrets must be at least 32 characters in length.')
-            # openshift.master.session_encryption_secrets
-            if 'session_encryption_secrets' in facts['master']:
-                session_encryption_secrets = facts['master']['session_encryption_secrets']
-                if not issubclass(type(session_encryption_secrets), list):
-                    invalid_facts['session_encryption_secrets'] = 'Expects session_encryption_secrets is a list.'
-                elif 'session_auth_secrets' not in facts['master']:
-                    invalid_facts['session_encryption_secrets'] = ('openshift_master_session_auth_secrets must be '
-                                                                   'set if openshift_master_session_encryption_secrets '
-                                                                   'is provided.')
-                else:
-                    for secret in session_encryption_secrets:
-                        if len(secret) not in [16, 24, 32]:
-                            invalid_facts['session_encryption_secrets'] = ('Invalid secret in session_encryption_secrets. '
-                                                                           'Secrets must be 16, 24, or 32 characters in length.')
-        return invalid_facts
-
 
 def main():
     """ main """

roles/openshift_master_facts/tasks/main.yml

@@ -48,8 +48,6 @@
       session_max_seconds: "{{ openshift_master_session_max_seconds | default(None) }}"
       session_name: "{{ openshift_master_session_name | default(None) }}"
       session_secrets_file: "{{ openshift_master_session_secrets_file | default(None) }}"
-      session_auth_secrets: "{{ openshift_master_session_auth_secrets | default(None) }}"
-      session_encryption_secrets: "{{ openshift_master_session_encryption_secrets | default(None) }}"
       access_token_max_seconds: "{{ openshift_master_access_token_max_seconds | default(None) }}"
       auth_token_max_seconds: "{{ openshift_master_auth_token_max_seconds | default(None) }}"
       # oo_htpasswd_users_from_file is a custom filter in role lib_utils