Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding support for OpenID-Connect/OAuth2 in the API #737

Merged
merged 2 commits into from
Feb 20, 2020

Conversation

abellotti
Copy link
Member

@abellotti abellotti commented Feb 6, 2020

This Enhancement enables OpenID-Connect authentication (OAuth2) on the API. Current support for OpenID-Connect is only for the web interface and the API is not enabled as such. The only enablement needed is to configure the ManageIQ Appliance for OpenID-Connect as per:

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

This enhancement leverages the appliance configuration and enables the following:

First Use Case:

  • Authenticating with the API using a user access token (JWT) as obtained from the Identity provider. The token is simply passed to the API via the Authorization header as follows:
  curl -H "Authorization: Bearer <JWT>" https://<appliance>/api

The API performs the necessary Token introspection to validate the token and obtain the user details from the OpenID Client Claim definitions.

The API then performs the proper mapping of the OpenID Claim data to the currently supported headers as per the OIDC Assertions in the document referenced above for external authentication to work as it does today.

Second Use Case:

  • Authenticating via Basic Auth.

    Here the standard basic authentication username and password is passed along to the API. However, when the appliance is configured for OpenID-Connect authentication, the API will request a token generation for the user from the IDP. If successful, It then follows the same authentication logic as the Bearer/JWT authentication mentioned above.

NOTE:

While this approach works fine and leverages the appliance configuration as is with no change, an alternate approach where the Apache configuration could be configured to protect the /api endpoint behind OpenID-Connect may be preferred.

Similar to how external authentication (Kerberos) is defined and wired for the API, see https://github.com/ManageIQ/manageiq-appliance/blob/236641d25b8b5d8a864893191955c8c19af1f275/TEMPLATE/etc/httpd/conf.d/manageiq-external-auth.conf.erb#L37 for the details there.

Though for OpenID-Connect/OAuth2, we would need to modify the https://github.com/ManageIQ/manageiq-appliance/blob/master/TEMPLATE/etc/httpd/conf.d/manageiq-external-auth-openidc.conf.erb file and define/protect the /api as documented here: https://github.com/zmartzone/mod_auth_openidc

This would also allow us to have the single Apache container communicating with the IDP and having the API/WS containers only handle the trusted Apache headers coming in internally, as was done with https://github.com/ManageIQ/httpd_configmap_generator for the ManageIQ httpd container for the podified ManageIQ.

BZ: https://bugzilla.redhat.com/show_bug.cgi?id=1805279

Leverages the current OpenIDC configuration of the applicance
for fetching client id, secret and OpenID Metadata URL and info
for getting to the token authentication and token introspection
endpoints of the IDP.

Supports:
- Authenticating with the API using a user Access Token coming
  in as the Authorization Bearer JWT.
- Authenticating with Basic Auth where the username and password
  specified is authenticated against the OpenID-Connect IDP
  and a JWT is fetched for the user.
the user from the Authenticated JWT token info (all the
user claim data coming back from KeyCloak).

We define the external auth headers as if they were coming
in from Apache so we leverage the current httpd auth type
for user management.
@miq-bot
Copy link
Member

miq-bot commented Feb 6, 2020

Checked commits abellotti/manageiq-api@7061eb1~...eb2017a with ruby 2.5.5, rubocop 0.69.0, haml-lint 0.20.0, and yamllint 1.10.0
1 file checked, 0 offenses detected
Everything looks fine. 🏆

@abellotti
Copy link
Member Author

abellotti commented Feb 6, 2020

/cc @gtanzillo @Fryguy @jvlcek @chessbyte - as promised.

/cc @clearnight79

@abellotti
Copy link
Member Author

Marking as [WIP] - Actual implementation would move a lot of this code to https://github.com/ManageIQ/manageiq/blob/master/app/models/authenticator/httpd.rb

@clearnight79
Copy link

@abellotti "Actual implementation" mean the alternative Apache configuration solution?

@clearnight79
Copy link

@abellotti if I want to patch my current CF 5.0, is this the file I need to replace?
/opt/rh/cfme-gemset/bundler/gems/cfme-api-006d816cf2a0/app/controllers/api/base_controller/authentication.rb

@abellotti
Copy link
Member Author

abellotti commented Feb 6, 2020 via email

@abellotti
Copy link
Member Author

abellotti commented Feb 6, 2020

@clearnight79 that looks about right.

running bundle show manageiq-api to get the gem's location, plus the path app/controllers/api/base_controller/authentication.rb

@jvlcek
Copy link
Member

jvlcek commented Feb 6, 2020

Running this on the appliance will return the path where the API code is installed:

bundle info manageiq-api --path

@clearnight79
Copy link

clearnight79 commented Feb 7, 2020

I tried to enable UI OIDC following https://www.manageiq.org/docs/reference/latest/auth/openid_connect
but no success, keep getting 500 internal error after click the corporate login.
I know Redhat may have only tested with KeyCloak and the authentication module https://github.com/zmartzone/mod_auth_openidc did say it support many others like Azure, Gitlib, etc, but not MCM Pak IAM. Do you have any clue? @abellotti

@abellotti abellotti changed the title [WIP] Adding support for OpenID-Connect/OAuth2 in the API Adding support for OpenID-Connect/OAuth2 in the API Feb 20, 2020
@abellotti abellotti removed the wip label Feb 20, 2020
@Fryguy
Copy link
Member

Fryguy commented Feb 20, 2020

Seems @clearnight79 had some success. So, I'm going to merge this as is, and we can iterate on cleanup, as I'd like to get this into a potential build for further testing. I think moving forward we should...

a) Make a PR to "clean up" the code by moving this into the httpd authenticator (@jvlcek Can you take this on?)
b) Research moving this out of Ruby code and into mod_auth_openidc configuration (@jvlcek Can you also look into this after the clean up)
c) Discuss the scope attribute and implement as necessary (@abellotti Can you open an issue to track this?)

@Fryguy Fryguy merged commit 95e86ab into ManageIQ:master Feb 20, 2020
@Fryguy Fryguy added this to the Sprint 131 Ending Mar 2, 2020 milestone Feb 20, 2020
@abellotti
Copy link
Member Author

@Fryguy ManageIQ/manageiq#19858 for the optional scope parameter. /cc @jvlcek

@abellotti
Copy link
Member Author

@clearnight79 did this PR work as is for you or did you need to modify any of the OOTB configuration files

  • /etc/httpd/conf.d/manageiq-external-auth-openidc.conf
  • /etc/httpd/conf.d/manageiq-remote-user-openidc.conf

And the related code here ?

simaishi pushed a commit that referenced this pull request Feb 21, 2020
Adding support for OpenID-Connect/OAuth2 in the API

(cherry picked from commit 95e86ab)

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1805914
@simaishi
Copy link
Contributor

Ivanchuk backport details:

$ git log -1
commit 5d482984334e606d74c72182fb07b5db33a7040b
Author: Jason Frey <jfrey@redhat.com>
Date:   Thu Feb 20 10:22:56 2020 -0500

    Merge pull request #737 from abellotti/support_oidc

    Adding support for OpenID-Connect/OAuth2 in the API

    (cherry picked from commit 95e86abb70c2a601f424e18f3d8d0f3d119c9d01)

    Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1805914

@jvlcek
Copy link
Member

jvlcek commented Feb 24, 2020

FYI: I've created the following issues to track the above mentioned follow on work to this PR

@chessbyte
Copy link
Member

Part of ManageIQ/manageiq#19867

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants