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.

Sign up for GitHub

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

Jump to bottom

Adding support for OpenID-Connect #251

Merged
merged 6 commits into from
Jun 26, 2018
Merged

Adding support for OpenID-Connect #251

merged 6 commits into from
Jun 26, 2018

Conversation

abellotti
Copy link
Member

Adding support for OpenID-Connect

  • auth-type: openid-connect
  • new auth config parameters:
    o HTTPD_AUTH_OIDC_PROVIDER_METADATA_URL oidc-provider-metadata-url
    o HTTPD_AUTH_OIDC_CLIENT_ID oidc-client-id
    o HTTPD_AUTH_OIDC_CLIENT_SECRET oidc-client-secret

@miq-bot miq-bot added the wip label Nov 30, 2017
@abellotti
Copy link
Member Author

abellotti commented Nov 30, 2017 •
edited
Loading

marking as WIP.

  • needs testing
  • need to also verify running with older auth-config maps without the new oidc entries
    • was an issue but resolved with optional: true
  • README.md update to reflect new oidc parameters

@abellotti
Copy link
Member Author

/cc @jvlcek

@abellotti
Copy link
Member Author

Requires: ManageIQ/container-httpd#33

abellotti
abellotti commented Dec 1, 2017
View reviewed changes
templates/miq-template-ext-db.yaml Outdated
OIDCClientID ${HTTPD_AUTH_OIDC_CLIENT_ID}
OIDCClientSecret ${HTTPD_AUTH_OIDC_CLIENT_SECRET}

OIDCRedirectURI https://%{REQUEST_HOST}/oidc_login/redirect_uri
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is problematic, REQUEST_HOST is not defined at apache config file load time. investigating alternative.

@abellotti
Copy link
Member Author

Ran test with an auth configmap without the new parameters and the Pod fails to start

Error: Couldn't find key auth-oidc-provider-metadata-url in ConfigMap ext-auth/httpd-auth-configs

Hopefully there's a way to handle this in the template and if not possible document the update/migration of auth configmaps to newer pods.

@abellotti
Copy link
Member Author

looks like adding:

optional: true

might work for us.

@abellotti abellotti force-pushed the support_openid_connect branch from 991937f to e380b76 Compare April 27, 2018 13:56
jvlcek
jvlcek reviewed Jun 5, 2018
View reviewed changes
Copy link
Member

@jvlcek jvlcek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Near line 51 we need to add the RewriteCond for openid-connect

manageiq-redirects-ui:RewriteCond %{REQUEST_URI} !^/openid-connect

abellotti added 5 commits June 5, 2018 15:41
@abellotti
Adding support for OpenID-Connect
fdd60b0 
- auth-type: openid-connect
- new auth config parameters:
  o HTTPD_AUTH_OIDC_PROVIDER_METADATA_URL   oidc-provider-metadata-url
  o HTTPD_AUTH_OIDC_CLIENT_ID               oidc-client-id
  o HTTPD_AUTH_OIDC_CLIENT_SECRET           oidc-client-secret
@abellotti
Needed to leverage the APPLICATION_DOMAIN for defining
c4b8ff8 
the RedirectURI for OpenID-Connect.
@abellotti
Updated miq-template.yaml and miq-template-ext-db.yaml
858e62d 
so that the OIDCRedirectURI is quoted, otherwise the
${APPLICATION_DOMAIN} portion is substituted when
viewing/editing the configmap.
@abellotti
Making the HTTPD_AUTH_OIDC_* environment variables
03d6944 
optional so older httpd auth-configmaps will still
work with the newer pods supporting OpenID-Connect.
@abellotti
For OpenID-Connect /openid-connect is only served by mod_auth_openidc,
85fa9de 
do not send to back-end pods.
@abellotti abellotti force-pushed the support_openid_connect branch from e380b76 to 85fa9de Compare June 5, 2018 19:48
@abellotti abellotti changed the title [WIP] Adding support for OpenID-Connect Adding support for OpenID-Connect Jun 11, 2018
@miq-bot miq-bot removed the wip label Jun 11, 2018
@jvlcek
Copy link
Member

jvlcek commented Jun 13, 2018

LGTM 👍 Thank you @abellotti

@jvlcek jvlcek mentioned this pull request Jun 15, 2018
Closed
@jvlcek
Copy link
Member

jvlcek commented Jun 18, 2018 •
edited
Loading

@abellotti The updates to the README can be pulled from my closed PR
https://github.com/ManageIQ/manageiq-pods/pull/296/files#diff-04c6e90faac2675aa89e2176d2eec7d8

@jvlcek jvlcek mentioned this pull request Jun 22, 2018
Merged
@miq-bot
Copy link
Member

miq-bot commented Jun 22, 2018

Checked commits abellotti/manageiq-pods@fdd60b0~...109fa75 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 1 offense detected

**

  • 💣 💥 🔥 🚒 - Linter/Yaml - missing config files

@carbonin
Copy link
Member

This looks good to me. @jvlcek Merging this before ManageIQ/httpd_configmap_generator#33 won't cause any issues right?

@jvlcek
Copy link
Member

jvlcek commented Jun 26, 2018

Thank you @carbonin and correct, merging this before ManageIQ/httpd_configmap_generator#33 won't cause any issues.

@carbonin carbonin self-assigned this Jun 26, 2018
@carbonin carbonin merged commit 72dfee6 into ManageIQ:master Jun 26, 2018
@carbonin carbonin added this to the Sprint 89 Ending Jul 2, 2018 milestone Jun 26, 2018
simaishi pushed a commit that referenced this pull request Jun 27, 2018
@carbonin @simaishi
Merge pull request #251 from abellotti/support_openid_connect
8ad3851 
Adding support for OpenID-Connect
(cherry picked from commit 72dfee6)
@simaishi
Copy link
Contributor

Hammer backport details:

$ git log -1
commit 8ad38519a4bf5b0aeeb1ad79040788229ae3328a
Author: Nick Carboni <[email protected]>
Date:   Tue Jun 26 10:15:38 2018 -0400

    Merge pull request #251 from abellotti/support_openid_connect
    
    Adding support for OpenID-Connect
    (cherry picked from commit 72dfee62cc3f9111baa1a776eea1c5f8bff13888)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jvlcek jvlcek jvlcek left review comments

@carbonin carbonin carbonin approved these changes

Assignees

@carbonin carbonin

Labels
enhancement hammer/backported template
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@abellotti @jvlcek @miq-bot @carbonin @simaishi