Move API OIDC/OAuth2 support from API to ManageIQ core #19936

jvlcek · 2020-03-06T16:35:47Z

Fixes #19865

This PR moves the OpenID-Connect support for the API, as implemented with PR 737 and PR 747, from being implemented directly in the ManageIQ API code base to the ManageIQ core authenticator code base.

This PR must be merged along with the associated ManageIQ/manageiq-api
PR 772

Steps for Testing/QA

Configure an appliance for OpenID-connect authentication [1]
Log in to the appliance using the UI
Request the users Access Token
Accessing ManageIQ API with the ACCESS_TOKEN
Accessing ManageIQ API with USER and PASSWORD

The below shell commands outline how to perform steps 3 through 5:

# Request the users Access Token
TOKEN_ENDPOINT="http://${KEYCLOAK_SERVER}:8080/auth/realms/${KEYCLOAK_REALM}/protocol/openid-connect/token"
KEYCLOAK_CLIENT_ID=<the value of OIDCClientSecret from  /etc/httpd/conf.d/manageiq-external-auth-openidc.conf>
OID_CLIENT_SECRET=<the value of OIDCCLientID from  /etc/httpd/conf.d/manageiq-external-auth-openidc.conf>
OID_CLIENT_HOST=<Full DNS hostname for ManageIQ appliance>

RES=`curl -L --user ${USER}:${PASSWORD} -X POST \
  -H "Content-Type: application/x-www-form-urlencoded" \
  -d "grant_type=password" \
  -d "client_id=${KEYCLOAK_CLIENT_ID}" \
  -d "client_secret=${OID_CLIENT_SECRET}" \
  -d "username=$USER" \
  -d "password=$PASSWORD" \
  $TOKEN_ENDPOINT`
ACCESS_TOKEN=`echo $RES | jq -r '.access_token'`

# Accessing ManageIQ API with the ACCESS_TOKEN
curl -L -vvv -k -X GET -H "Authorization: Bearer ${ACCESS_TOKEN}" https://${OID_CLIENT_HOST}/api/users  | jq

# Accessing ManageIQ API with USER and PASSWORD
curl -L -vvv -k --user ${USER}:${PASSWORD} -X GET -H "Accept: application/json" https://${OID_CLIENT_HOST}/api/users | jq

[1] https://www.manageiq.org/docs/reference/latest/auth/openid_connect

jvlcek · 2020-03-06T16:41:02Z

@miq-bot assign @abellotti
@miq-bot add_label core/authentication

jvlcek · 2020-03-06T18:08:24Z

@miq-bot add_label refactoring

…ator. Fixes ManageIQ#19865

jvlcek · 2020-03-06T18:53:44Z

Note: It might be possible to simplify this solution, and eliminate most if not all of the code introduced in this PR, by leveraging OpenID-Connect/OAuth2 setup in the mod_auth_openidc configuration files. Issue 19866

Spec test will be added f it turns out not to be possible to leveraging OpenID-Connect/OAuth2 setup in the mod_auth_openidc configuration files.

app/models/authenticator/httpd.rb

app/models/authenticator/httpd_oauth2.rb

app/models/authenticator/httpd.rb

abellotti · 2020-03-09T16:14:47Z

app/models/authenticator/httpd.rb

-        request.headers['X-REMOTE-USER'].present?
+    def _authenticate(username, password, request)
+      if !user_data_collected?(request) && request.present? && oidc_configured?
+        if password.present?


don't we need to check that username is present instead ?

Will do! Thank you.

Based on review feedback.

jvlcek · 2020-03-09T18:19:56Z

@bdunne, @Fryguy and @abellotti

I believe I've addressed all of your concerns. I would find it inconceivable for you to uncover any other issues with this PR.

Thank you for your help and feedback.

jvlcek · 2020-03-09T18:22:36Z

I apologies for the full file difference for httpd/oauth2.rb The namespace changes identified by @Fryguy caused the file indentation to change.

app/models/authenticator/httpd/oauth2.rb

bdunne · 2020-03-10T15:56:22Z

Can you add some tests?

jvlcek · 2020-03-10T17:01:15Z

@bdunne Thank you for the review input.

As noted in comment 2 I plan to add tests if it turns out this code can not easily be replaced by OIDC configuration. I don't want to spend time adding tests now for code that just might be removed and the priority is to try to remove it.

Note: It might be possible to simplify this solution, and eliminate most if not all of the code introduced in this PR, by leveraging OpenID-Connect/OAuth2 setup in the mod_auth_openidc configuration files. Issue 19866

Spec test will be added f it turns out not to be possible to leveraging OpenID-Connect/OAuth2 setup in the mod_auth_openidc configuration files.

kbrock

This is a moved from api to core.
There are no tests over there.
The shorter term goal is to delete this.

I say merge as is.

You game with this @bdunne ?

miq-bot · 2020-03-11T17:10:56Z

Checked commits jvlcek/manageiq@e8f495c~...a517b55 with ruby 2.5.7, rubocop 0.69.0, haml-lint 0.20.0, and yamllint
2 files checked, 0 offenses detected
Everything looks fine. 🍪

bdunne · 2020-03-11T17:46:11Z

This is no longer a simple move. It's been significantly changed in the process
Why are we moving it if the plan is to replace it with something else in the short term?
Would the high-level tests (pass these HTTP headers which validate a user is logged in) be significantly different with the replacement module?

Fryguy · 2020-03-11T18:01:25Z

This is no longer a simple move. It's been significantly changed in the process

I don't think it was characterized as a simple move. This was a cleanup effort for the original hacked in feature in order to put it in a proper location, and ideally into a dedicated concern should it be removed later. Not much of the actual code should have changed except in light of cleanup.

Why are we moving it if the plan is to replace it with something else in the short term?

We are not sure how much effort it will be to replace with the Apache based thing and would prefer to have this "clean" in the jansa release. If we can get the Apache based thing, then yeah, this will go away. It was a toss-up choice and we chose cleanup followed by Apache-based.

Would the high-level tests (pass these HTTP headers which validate a user is logged in) be significantly different with the replacement module?

I don't think so, and I agree with you that it's probably a good idea to have tests for this stuff... I'd be ok with waiting on merge for some high-level tests. Might have to use a bit of mocking because the way it works it to reach out to a remote identity system.

Fryguy · 2020-03-11T18:03:44Z

@jvlcek Can you please run cross-repo tests with your multiple PRs as well as UI?

jvlcek · 2020-03-11T20:28:48Z

@bdunne Thank you for your perspective! I'm just trying to avoid wasting time on specs now if I can figure out how to get rid of this code.

@Fryguy cross-repo tests here: ManageIQ/manageiq-cross_repo-tests#85

jvlcek · 2020-03-13T10:13:20Z

@bdunne @Fryguy @abellotti and @kbrock Thoughts on merging this? The cross-repo tests passed. Are folks OK with adding specs if the alternative approach turns out not to be possible?

bdunne · 2020-03-13T12:37:19Z

@jvlcek Does this work roughly the same way as the potential replacement (HTTPD passes us some headers and that means the user is authenticated)?

bdunne · 2020-03-13T16:05:20Z

It looks like there are tests around the other parts of the authenticate conditional. Added #19866 (comment) as a reminder to add more tests if the replacement is not viable.

Move API OIDC/OAuth2 support from API to ManageIQ core (cherry picked from commit d8914fb)

simaishi · 2020-03-20T20:15:09Z

Jansa backport details:

$ git log -1
commit 447b26fa59e35147aea78b2a8ecbe348ba77e06a
Author: Brandon Dunne <bdunne@redhat.com>
Date:   Fri Mar 13 12:05:30 2020 -0400

    Merge pull request #19936 from jvlcek/oidc_api_to_core_issue_19865

    Move API OIDC/OAuth2 support from API to ManageIQ core

    (cherry picked from commit d8914fb455965c86b26ab2da9484ab2133874e3b)

jvlcek changed the title ~~Move API OpenID-Connect/OAuth2 support from API to ManageIQ authentic…~~ Move API OIDC/OAuth2 support from API to ManageIQ core Mar 6, 2020

jvlcek mentioned this pull request Mar 6, 2020

Move API OIDC/OAuth2 support from API to ManageIQ core ManageIQ/manageiq-api#772

Merged

miq-bot assigned abellotti Mar 6, 2020

miq-bot added the core/authentication label Mar 6, 2020

miq-bot added the refactoring label Mar 6, 2020

Move API OpenID-Connect/OAuth2 support from API to ManageIQ authentic…

e8f495c

…ator. Fixes ManageIQ#19865

jvlcek force-pushed the oidc_api_to_core_issue_19865 branch from 4652746 to e8f495c Compare March 6, 2020 18:21

bdunne reviewed Mar 6, 2020

View reviewed changes

app/models/authenticator/httpd.rb Outdated Show resolved Hide resolved

Fryguy reviewed Mar 6, 2020

View reviewed changes

app/models/authenticator/httpd_oauth2.rb Outdated Show resolved Hide resolved

bdunne reviewed Mar 6, 2020

View reviewed changes

app/models/authenticator/httpd.rb Outdated Show resolved Hide resolved

abellotti reviewed Mar 9, 2020

View reviewed changes

Restructured oauth2 module and improved readability

04c3a68

Based on review feedback.

jvlcek added 2 commits March 9, 2020 15:52

Address rubocop warnings

2ba56f8

Address rubocop warnings

d748385

Fryguy reviewed Mar 10, 2020

View reviewed changes

app/models/authenticator/httpd/oauth2.rb Show resolved Hide resolved

Fryguy approved these changes Mar 10, 2020

View reviewed changes

Fryguy added the jansa/yes? label Mar 10, 2020

kbrock approved these changes Mar 10, 2020

View reviewed changes

Blank password could be valid

a517b55

bdunne mentioned this pull request Mar 13, 2020

Research moving API OpenID-Connect/OAuth2 support into Apache configuration #19866

Closed

3 tasks

bdunne merged commit d8914fb into ManageIQ:master Mar 13, 2020

bdunne assigned bdunne and unassigned abellotti Mar 13, 2020

chessbyte added jansa/yes and removed jansa/yes? labels Mar 19, 2020

simaishi pushed a commit that referenced this pull request Mar 20, 2020

Merge pull request #19936 from jvlcek/oidc_api_to_core_issue_19865

447b26f

Move API OIDC/OAuth2 support from API to ManageIQ core (cherry picked from commit d8914fb)

simaishi added jansa/backported and removed jansa/yes labels Mar 20, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move API OIDC/OAuth2 support from API to ManageIQ core #19936

Move API OIDC/OAuth2 support from API to ManageIQ core #19936

jvlcek commented Mar 6, 2020 •

edited

Loading

jvlcek commented Mar 6, 2020

jvlcek commented Mar 6, 2020

jvlcek commented Mar 6, 2020

abellotti Mar 9, 2020

jvlcek Mar 10, 2020

jvlcek commented Mar 9, 2020

jvlcek commented Mar 9, 2020

bdunne commented Mar 10, 2020

jvlcek commented Mar 10, 2020

kbrock left a comment

miq-bot commented Mar 11, 2020

bdunne commented Mar 11, 2020

Fryguy commented Mar 11, 2020 •

edited

Loading

Fryguy commented Mar 11, 2020

jvlcek commented Mar 11, 2020

jvlcek commented Mar 13, 2020

bdunne commented Mar 13, 2020

bdunne commented Mar 13, 2020

simaishi commented Mar 20, 2020

Move API OIDC/OAuth2 support from API to ManageIQ core #19936

Move API OIDC/OAuth2 support from API to ManageIQ core #19936

Conversation

jvlcek commented Mar 6, 2020 • edited Loading

Steps for Testing/QA

jvlcek commented Mar 6, 2020

jvlcek commented Mar 6, 2020

jvlcek commented Mar 6, 2020

abellotti Mar 9, 2020

Choose a reason for hiding this comment

jvlcek Mar 10, 2020

Choose a reason for hiding this comment

jvlcek commented Mar 9, 2020

jvlcek commented Mar 9, 2020

bdunne commented Mar 10, 2020

jvlcek commented Mar 10, 2020

kbrock left a comment

Choose a reason for hiding this comment

miq-bot commented Mar 11, 2020

bdunne commented Mar 11, 2020

Fryguy commented Mar 11, 2020 • edited Loading

Fryguy commented Mar 11, 2020

jvlcek commented Mar 11, 2020

jvlcek commented Mar 13, 2020

bdunne commented Mar 13, 2020

bdunne commented Mar 13, 2020

simaishi commented Mar 20, 2020

jvlcek commented Mar 6, 2020 •

edited

Loading

Fryguy commented Mar 11, 2020 •

edited

Loading