Dissalow custom CA paths for identity providers #9731

vrutkovs · 2018-08-23T14:39:11Z

This commit would ensure ldap auth provider won't rewrite any files
in the static pod and gets placed in /etc/origin/master/ldap_ca.crt.

Previously, before static pod deployment,
openshift_master_identity_providers
param for LDAP auth could specify the file, which was copied on the
host and was used by API server, running on the host.

In 3.10+ this file needs to be mounted in the API server container
(along with the rest of configuration), so a custom path in ca section
could be left on the node. This file was copied unconditionally and
could rewrite any CA / config file.

This commit would always place contents from openshift_master_ldap_ca
(or file from openshift_master_ldap_ca_file) in
/etc/origin/master/ldap_ca.crt, which would be mounted in a static pod.

Related to https://bugzilla.redhat.com/show_bug.cgi?id=1614414, https://bugzilla.redhat.com/show_bug.cgi?id=1616262 and #9397

Fixes #6191

TODO:

make sure LDAPPasswordIdentityProvider has ca autofilled with ldap_ca if openshift_master_ldap_ca_file is set
Fix other providers using CA

michaelgugino

Either need to hack the ldap ca stuff out of openshift_facts entirely or leave that condition as-is.

michaelgugino · 2018-08-23T16:55:37Z

roles/openshift_master_facts/tasks/main.yml

@@ -41,7 +41,7 @@
      bind_addr: "{{ openshift_master_bind_addr | default(None) }}"
      session_max_seconds: "{{ openshift_master_session_max_seconds | default(None) }}"
      session_name: "{{ openshift_master_session_name | default(None) }}"
-      ldap_ca: "{{ openshift_master_ldap_ca | default(lookup('file', openshift_master_ldap_ca_file) if openshift_master_ldap_ca_file is defined else None) }}"
+      ldap_ca: "{{ openshift_master_ldap_ca | default(lookup('file', openshift_master_ldap_ca_file)) }}"


I don't think this change is desired.

Right, no need to touch that

michaelgugino · 2018-08-24T14:32:56Z

roles/openshift_control_plane/tasks/main.yml

@@ -99,7 +99,7 @@

 - name: Create the request header ca file if needed
  copy:
-    dest: "{{ item.clientCA if 'clientCA' in item and '/' in item.clientCA else '/etc/origin/master/' ~ item.clientCA | default('request_header_ca.crt') }}"
+    dest: "/etc/origin/master/request_header_ca.crt"


We need to evaluate the boolean cases in this PR as well.

It seems that this file is only created of the clientCA key is defined in an item in openshift_master_identity_providers. Since we've updated the docs, that field is no longer there, thus this field might not get defined.

Probably just need to force people to migrate these CA keys to not include a leading '/' char, and then just put the file path in /etc/origin/master/* wherever they specify.

Probably just need to force people to migrate these CA keys to not include a leading '/' char, and then just put the file path in /etc/origin/master/* wherever they specify

Not sure what would be the usecase for this, although its potentially dangerous, as user may rewrite etcd/api certs if the name is ca-bundle.crt or ca.crt or admin.crt , which are pretty common.

I don't care where we put the files or what we name them, as long as they are in /etc/origin/master they will be fine. We already have logic that will block existing users from upgrades if any paths outside of /etc/origin/master are found in master's config file.

We need to implement similar logic for the identity provider dictionary for new installs, backport to 3.10. The change as-is here would break some logic, and define variables when they might otherwise be undefined by the user.

For example, https://github.com/openshift/openshift-ansible/pull/9731/files#diff-54774cd1a74c81a803bc9a139d03f9d6R109 will never be true because we're now telling users not to define the 'clientCA' key in the example.

Seems like we should just get rid of that conditional across these set of tasks?

We already have logic that will block existing users from upgrades if any paths outside of /etc/origin/master are found in master's config file

Identity providers are skipped there, as this part of the code is expected to do that. Also, this check won't forbid rewriting existing certs.

.. will never be true because we're now telling users not to define the 'clientCA' key in the example.

Correct, removed this clause.

michaelgugino · 2018-08-27T13:51:36Z

roles/openshift_control_plane/tasks/main.yml

@@ -99,7 +99,7 @@

 - name: Create the request header ca file if needed
  copy:
-    dest: "{{ item.clientCA if 'clientCA' in item and '/' in item.clientCA else '/etc/origin/master/' ~ item.clientCA | default('request_header_ca.crt') }}"
+    dest: "/etc/origin/master/request_header_ca.crt"


I don't care where we put the files or what we name them, as long as they are in /etc/origin/master they will be fine. We already have logic that will block existing users from upgrades if any paths outside of /etc/origin/master are found in master's config file.

We need to implement similar logic for the identity provider dictionary for new installs, backport to 3.10. The change as-is here would break some logic, and define variables when they might otherwise be undefined by the user.

For example, https://github.com/openshift/openshift-ansible/pull/9731/files#diff-54774cd1a74c81a803bc9a139d03f9d6R109 will never be true because we're now telling users not to define the 'clientCA' key in the example.

This commit would ensure ldap auth provider won't rewrite any files in the static pod and gets placed in /etc/origin/master/ Previously, before static pod deployment, openshift_master_identity_providers param for LDAP auth could specify the file, which was copied on the host and was used by API server, running on the host. In 3.10+ this file needs to be mounted in the API server container (along with the rest of configuration), so a custom path in `ca` section could be left on the node. This file was copied unconditionally and could rewrite any CA / config file. This commit would always place contents from `openshift_master_ldap_ca` (or file from `openshift_master_ldap_ca_file`) in /etc/origin/master/ldap_ca.crt, which would be mounted in a static pod. This also affects OpenID auth and request header auth

sdodson

lgtm, this will fail miserably if there's more than one provider of a given kind specified and they require different CAs, but I think the current implementation would fail in the same way too.

michaelgugino · 2018-08-28T12:46:46Z

lgtm, this will fail miserably if there's more than one provider of a given kind specified and they require different CAs, but I think the current implementation would fail in the same way too.

If users could specify arbitrary paths, I don't think they would fail.

michaelgugino · 2018-08-28T12:51:42Z

roles/openshift_control_plane/tasks/main.yml

@@ -78,7 +78,7 @@

 - name: Create the ldap ca file if needed
  copy:
-    dest: "{{ item.ca if 'ca' in item and '/' in item.ca else '/etc/origin/master/' ~ item.ca | default('ldap_ca.crt') }}"
+    dest: "/etc/origin/master/ldap_ca.crt"


Perhaps we should prefix each filename with the 'name' key of the auth_provider? That would allow multiple auth providers of the same type?

Good idea, implemented

michaelgugino

/lgtm

sdodson · 2018-08-28T15:24:28Z

/lgtm
nice work!

openshift-ci-robot · 2018-08-28T15:24:50Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: michaelgugino, sdodson, vrutkovs

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [michaelgugino,sdodson,vrutkovs]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

michaelgugino · 2018-08-28T17:15:35Z

/cherrypick release-3.10

openshift-cherrypick-robot · 2018-08-28T17:15:44Z

@michaelgugino: new pull request created: #9803

In response to this:

/cherrypick release-3.10

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-ci-robot requested review from michaelgugino and sdodson August 23, 2018 14:39

michaelgugino suggested changes Aug 23, 2018

View reviewed changes

vrutkovs force-pushed the ldap_ca_in_origin_master branch 3 times, most recently from 3b5dc60 to 1e2ef3f Compare August 24, 2018 11:37

openshift-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Aug 24, 2018

vrutkovs changed the title ~~WIP Dissalow custom CA file path for openshift_master_ldap_ca~~ Dissalow custom CA file path for openshift_master_ldap_ca Aug 24, 2018

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 24, 2018

vrutkovs changed the title ~~Dissalow custom CA file path for openshift_master_ldap_ca~~ Dissalow custom CA paths for identity providers Aug 24, 2018

michaelgugino suggested changes Aug 24, 2018

View reviewed changes

michaelgugino suggested changes Aug 27, 2018

View reviewed changes

vrutkovs force-pushed the ldap_ca_in_origin_master branch from 1e2ef3f to ceac075 Compare August 28, 2018 11:24

sdodson approved these changes Aug 28, 2018

View reviewed changes

michaelgugino reviewed Aug 28, 2018

View reviewed changes

Prefix identity provider's CA files with identity provider names

3c18a58

michaelgugino approved these changes Aug 28, 2018

View reviewed changes

openshift-ci-robot assigned michaelgugino Aug 28, 2018

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 28, 2018

openshift-ci-robot assigned sdodson Aug 28, 2018

openshift-merge-robot merged commit 28484ef into openshift:master Aug 28, 2018

openshift-cherrypick-robot mentioned this pull request Aug 28, 2018

[release-3.10] Dissalow custom CA paths for identity providers #9803

Merged

This was referenced Aug 29, 2018

LDAP "ca" directive not set in master-config.yml when openshift_master_ldap_ca_file is configured #6191

Closed

Fix incorrect formatting for ca file #9849

Merged

vrutkovs mentioned this pull request Dec 7, 2018

use ca defined in the inventory instead of a fixed filename #10841

Closed

Dissalow custom CA paths for identity providers #9731

Dissalow custom CA paths for identity providers #9731

vrutkovs commented Aug 23, 2018 •

edited

michaelgugino left a comment

michaelgugino Aug 23, 2018

vrutkovs Aug 24, 2018

michaelgugino Aug 24, 2018

vrutkovs Aug 27, 2018

michaelgugino Aug 27, 2018

sdodson Aug 27, 2018

vrutkovs Aug 28, 2018

michaelgugino Aug 27, 2018

sdodson left a comment

michaelgugino commented Aug 28, 2018

michaelgugino Aug 28, 2018

vrutkovs Aug 28, 2018

michaelgugino left a comment

sdodson commented Aug 28, 2018

openshift-ci-robot commented Aug 28, 2018

michaelgugino commented Aug 28, 2018

openshift-cherrypick-robot commented Aug 28, 2018

Dissalow custom CA paths for identity providers #9731

Dissalow custom CA paths for identity providers #9731

Conversation

vrutkovs commented Aug 23, 2018 • edited

michaelgugino left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sdodson left a comment

Choose a reason for hiding this comment

michaelgugino commented Aug 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelgugino left a comment

Choose a reason for hiding this comment

sdodson commented Aug 28, 2018

openshift-ci-robot commented Aug 28, 2018

michaelgugino commented Aug 28, 2018

openshift-cherrypick-robot commented Aug 28, 2018

vrutkovs commented Aug 23, 2018 •

edited