Bug 1488505 - OpenID extra parameters not being added to the authorization token request when openshift_master_identity_providers ansible variable is set
Summary: OpenID extra parameters not being added to the authorization token request wh...
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: OpenShift Container Platform
Classification: Red Hat
Component: Installer
Version: 3.5.0
Hardware: Unspecified
OS: Unspecified
medium
medium
Target Milestone: ---
: 3.7.0
Assignee: Steve Milner
QA Contact: Gaoyun Pei
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2017-09-05 14:34 UTC by David Caldwell
Modified: 2017-12-19 07:00 UTC (History)
8 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Cause: Input for include_granted_scopes, which was expected to become a single quoted bool string, was instead being interpreted and written to file incorrectly. Consequence: The resulting configuration file could have the wrong value for include_granted_scopes. Fix: Removal of a code block which attempted to interpret the input for include_granted_scopes. Result: Input that is expected to land via include_granted_scopes passes to the master-config.yml as expected.
Clone Of:
Environment:
Last Closed: 2017-11-28 22:09:17 UTC
Target Upstream Version:
Embargoed:


Attachments (Terms of Use)
Patch for include_granted_scopes (56 bytes, text/plain)
2017-09-05 20:41 UTC, Bruno Oliveira
no flags Details


Links
System ID Private Priority Status Summary Last Updated
Red Hat Product Errata RHSA-2017:3188 0 normal SHIPPED_LIVE Moderate: Red Hat OpenShift Container Platform 3.7 security, bug, and enhancement update 2017-11-29 02:34:54 UTC

Description David Caldwell 2017-09-05 14:34:03 UTC
Description of problem:

When setting OpenID with extra parameters (include_granted_scopes: "true") within the ansible inventory file (using variable openshift_master_identity_providers), the "true" value is taken as true (without quotes) as outcome.

This causes Openshift does not retrieve all the sub fields for the users.

As a test, when you set it manually within the master-config.yaml with the proper format (include_granted_scopes: "true", enclosed between double quotes), it works:


###############################################
  identityProviders:
  - challenge: false
    login: true
    mappingMethod: claim
    name: companyokta
    provider:
      apiVersion: v1
      claims:
        email:
        - email
        id:
        - sub
        name:
        - name
        preferredUsername:
        - preferred_username
      clientID: xxx
      clientSecret: xxx
      extraAuthorizeParameters:
        include_granted_scopes: "true"
      kind: OpenIDIdentityProvider
      urls:
        authorize: https://<url>.com/oauth2/v1/authorize
        token: https://<url>.com/oauth2/v1/token
        userInfo: https://<url>.com/oauth2/v1/userinfo
###############################################
 

Version-Release number of the following components:
rpm -q openshift-ansible
rpm -q ansible
ansible --version

How reproducible:
100%

Steps to Reproduce:
1. Set openshift_master_identity_providers as follows:

###############################################
openshift_master_identity_providers=[{"name": "companyokta", "login": "true", "challenge": "false", "mappingMethod": "claim", "kind": "OpenIDIdentityProvider", "client_id": "xxx", "client_secret": "xxx", "extraAuthorizeParameters" : {"include_granted_scopes": "true"}, "claims": {"id": ["sub"], "preferredUsername": ["preferred_username"], "name": ["name"], "email": ["email"]}, "urls": {"authorize": "https://<url>.com/oauth2/v1/authorize", "token": "https://<url>.com/oauth2/v1/token", "userInfo": "https://<url>.com/oauth2/v1/userinfo"} }]
###############################################


2. Run the installer


Actual results:

➜  ~ oc get identities
NAME                                IDP NAME       IDP USER NAME          USER NAME                     USER UID
companyokta:00u11e13mwxxxxxx   companyokta   00u11e13mwNM1xxxxxx   myemailaddress.uk         4380c342-7603-11e7-bb8b-xxxxxx
companyokta:00u14da2xxxxxxxx   companyokta   00u14da2dbitr4xxxxxx   00u14da2xxxxxxx43TR0i7          8999013b-8e30-11e7-a0e2-xxxxx

On the first line, under USER NAME, you see the email of the user authenticated for the first time while openshift had the right configuration ("true" with quotes).
On the second line you see instead another user authenticated using the configuration provisioned by ansible (true without quotes).

Expected results:

OCP to recognize true (without double quotes) as a proper value for the include_granted_scopes variable.

Additional info:

https://github.com/openshift/openshift-ansible/blob/master/roles/openshift_master_facts/filter_plugins/openshift_master.py#L383-L386

Comment 2 Bruno Oliveira 2017-09-05 20:27:07 UTC
I believe this was already fixed here https://github.com/openshift/openshift-ansible/pull/2896

Comment 3 Bruno Oliveira 2017-09-05 20:41:54 UTC
Created attachment 1322402 [details]
Patch for include_granted_scopes

Comment 4 Mo 2017-09-08 21:04:25 UTC
> OCP to recognize true (without double quotes) as a proper value for the include_granted_scopes variable

This is not possible as it would require a breaking API change, and in general is incorrect (it is a map of string to string with no meaning to OpenShift, and thus OpenShift should not try to interpret it in any way and must simply pass it along).  I am surprised that it does not raise an error such as "unrecognized type: string" though (this is what you get if you try to put a bool in a string field).



https://github.com/openshift/openshift-ansible/pull/2896 needs to be updated to fix the ansible error.

Comment 5 Jan Pazdziora (Red Hat) 2017-09-11 10:37:07 UTC
(In reply to Mo from comment #4)
> 
> https://github.com/openshift/openshift-ansible/pull/2896 needs to be updated
> to fix the ansible error.

Not sure which ansible error you have in mind but I gave it a try with https://github.com/openshift/openshift-ansible/pull/5350 now.

Comment 6 Mo 2017-09-11 14:19:19 UTC
I do not understand why ansible needs to do anything special here at all. The value is a string, so why are we parsing it and then turning it back into a string? Why not simply pass it through?

Comment 7 Steve Milner 2017-09-11 20:24:04 UTC
The patch doesn't apply to master. I'll rebase it and open a new PR.

Comment 8 Steve Milner 2017-09-11 20:40:58 UTC
PR: https://github.com/openshift/openshift-ansible/pull/5364

PTAL

Comment 9 Steve Milner 2017-09-11 20:50:24 UTC
There is an alternate PR at https://github.com/openshift/openshift-ansible/pull/5350

Comment 10 Steve Milner 2017-09-12 14:11:08 UTC
We're running with https://github.com/openshift/openshift-ansible/pull/5350 as the PR of choice.

Comment 11 Jan Pazdziora (Red Hat) 2017-09-13 09:03:04 UTC
The PR was merged but I'm not happy with the result -- please see https://github.com/openshift/openshift-ansible/issues/2454#issuecomment-329101226 and subsequent comments.

Comment 12 Steve Milner 2017-09-15 13:47:16 UTC
Third PR option opened: https://github.com/openshift/openshift-ansible/pull/5427

Comment 13 Gaoyun Pei 2017-09-28 06:35:00 UTC
Verify this bug with openshift-ansible-3.7.0-0.128.0.git.0.89dcad2.el7.noarch.rpm


With the following option set in ansible inventory file:
openshift_master_identity_providers=[{"name": "companyokta", "login": "true", "challenge": "false", "mappingMethod": "claim", "kind": "OpenIDIdentityProvider", "client_id": "xxx", "client_secret": "xxx", "extraAuthorizeParameters" : {"include_granted_scopes": "true"}, "claims": {"id": ["sub"], "preferredUsername": ["preferred_username"], "name": ["name"], "email": ["email"]}, "urls": {"authorize": "https://test.com/oauth2/v1/authorize", "token": "https://test.com/oauth2/v1/token", "userInfo": "https://test.com/oauth2/v1/userinfo"} }]

After installation, check master config file:
oauthConfig:
...
      extraAuthorizeParameters:
        include_granted_scopes: 'true'



When installing env with an early version - openshift-ansible-3.7.0-0.126.0.git.0.33d254a.el7.noarch.rpm, it's configured as:
oauthConfig:
...
         extraAuthorizeParameters:
        include_granted_scopes: true

And master service failed to start due to "Invalid MasterConfig /etc/origin/master/master-config.yaml".

So move this bug to verified.

Comment 14 Steve Milner 2017-09-29 13:46:52 UTC
Added doc text

Comment 18 errata-xmlrpc 2017-11-28 22:09:17 UTC
Since the problem described in this bug report should be
resolved in a recent advisory, it has been closed with a
resolution of ERRATA.

For information on the advisory, and where to find the updated
files, follow the link below.

If the solution does not work for you, open a new bug report.

https://access.redhat.com/errata/RHSA-2017:3188


Note You need to log in before you can comment on or make changes to this bug.