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

Move API OpenID-Connect support to Apache configuration #282

Merged
merged 4 commits into from
May 8, 2020

Conversation

jvlcek
Copy link
Member

@jvlcek jvlcek commented May 6, 2020

Fixes ManageIQ/manageiq#19866

This PR combined with others in other github repos will move the support for the ManageIQ API out of code and into the Apache configuration.

Dependent PRs
ManageIQ/manageiq-api#828
ManageIQ/manageiq#20131
ManageIQ/manageiq-appliance_console#117

@jvlcek jvlcek changed the title Move API OpenID-Connect support to Apache configuration [WIP] Move API OpenID-Connect support to Apache configuration May 6, 2020
@jvlcek
Copy link
Member Author

jvlcek commented May 6, 2020

@miq-bot add_label wip

Setting wip label until more testing is done.

@miq-bot miq-bot added the wip label May 6, 2020
@abellotti
Copy link
Member

As this is settling, we'd also need the equivalent change to:

https://github.com/ManageIQ/manageiq-appliance_console/blob/master/lib/manageiq/appliance_console/oidc_authentication.rb

And the same to @carbonin's operator.

to add support for the oidc_oauth_introspection_endpoint option for the new parameter.

@abellotti
Copy link
Member

abellotti commented May 6, 2020

I wish there's a way to get a default/derive value for oidc_oauth_introspection_endpoint without having to require it. Anyways, https://www.manageiq.org/docs/reference/latest/auth/openid_connect would also need updating.

@abellotti
Copy link
Member

for the CLI, if the oidc_provider_metadata_url specified ends with .well-known/openid-configuration we can present the following default for the oidc_oauth_introspection_endpoint:

oidc_provider_metadata_url.sub(/\/\.well-known\/openid-configuration$/, "/protocol/openid-connect/token/introspect")

They can always enter a different value, at least it would be the correct default for a Keycloak provider metadata url.

@@ -11,7 +11,7 @@ OIDCOAuthRemoteUserClaim username
OIDCOAuthClientID <%= oidc_client_id %>
OIDCOAuthClientSecret <%= oidc_client_secret %>
OIDCOAuthIntrospectionEndpoint <%= oidc_oauth_introspection_endpoint %>
OIDCOAuthIntrospectionEndpointAuth client_secret_post
OIDCOAuthIntrospectionEndpointAuth client_secret_basic
Copy link
Member

Choose a reason for hiding this comment

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

Nice !! 👍 common denominator between Keycloak and IAM.

@jvlcek
Copy link
Member Author

jvlcek commented May 8, 2020

for the CLI, if the oidc_provider_metadata_url specified ends with .well-known/openid-configuration we can present the following default for the oidc_oauth_introspection_endpoint:

oidc_provider_metadata_url.sub(/\/\.well-known\/openid-configuration$/, "/protocol/openid-connect/token/introspect")

They can always enter a different value, at least it would be the correct default for a Keycloak provider metadata url.

@abellotti

I'm not sure I like this idea or perhaps I just don't understand what you are suggesting.

I expect the CLI to be invoked from a script and require all the necessary parameters be supplied. If the CLI tries to derive a default for the introspect endpoint based on the metadata URL the CLI would need to be updated to prompt the user to confirm if the derived introspect endpoint, and all other supplied parameters, are correct before doing the configuration.

I think this flow would make usage awkward and the added logic to prompt the user to confirm the supplied and derived parameters are correct seems like overkill to me.

Or maybe I don't understand what you are suggesting.

Please explain if my understanding is inaccurate.

@miq-bot
Copy link
Member

miq-bot commented May 8, 2020

Checked commits jvlcek/manageiq-appliance@6ee6386~...b104ceb with ruby 2.5.7, rubocop 0.69.0, haml-lint 0.28.0, and yamllint
0 files checked, 0 offenses detected
Everything looks fine. 🍰

@jvlcek
Copy link
Member Author

jvlcek commented May 8, 2020

@miq-bot remove_label wip

@jvlcek jvlcek changed the title [WIP] Move API OpenID-Connect support to Apache configuration Move API OpenID-Connect support to Apache configuration May 8, 2020
@miq-bot miq-bot removed the wip label May 8, 2020
@Fryguy Fryguy merged commit 34c78fd into ManageIQ:master May 8, 2020
@Fryguy Fryguy self-assigned this May 8, 2020
@abellotti
Copy link
Member

abellotti commented May 8, 2020

Was actually thinking of appliance console/prompts (in use case), equivalent thought for CLI would be:

  • if they provide it, use it.
  • if they don't provide it and we can derive from provider metadata url, then we're good.
  • if they don't provide it and we can't derive, then fail since we need it.

Not sure if this will bite us in CLI mode if we can't see what's being applied

@Fryguy
Copy link
Member

Fryguy commented May 8, 2020

@abellotti I asked this question over in the appliance_console and pods PRs as well, but I think the decision to require things like the token inspection endpoint should be based on how many providers actually stray from the default / well known locations.

We should follow the 80/20 rule. If 80% of providers use the well known, then we should make it optional; if 20% use the well known we should make it required.

@Fryguy
Copy link
Member

Fryguy commented May 8, 2020

My thoughts stem less from the CLI and more from the pods CR, where every parameter adds a potential layer of confusion and less is more. However, even in the CLI, I'd argue that using sensible defaults (and documenting those in --help) is a much more user-friendly interface.

@jvlcek
Copy link
Member Author

jvlcek commented May 8, 2020

Was actually thinking of appliance console/prompts (in use case), equivalent thought for CLI would be:

  • if they provide it, use it.
  • if they don't provide it and we can derive from provider metadata url, then we're good.
  • if they don't provide it and we can't derive, then fail since we need it.

Not sure if this will bite us in CLI mode if we can't see what's being applied

Ah! I get it now. Will do! Thank you.

@abellotti
Copy link
Member

lookin' for any standard in the RFC's, nothing yet. If we're allowed to access it, a fourth bullet may be:

simaishi pushed a commit that referenced this pull request May 12, 2020
Move API OpenID-Connect support to Apache configuration

(cherry picked from commit 34c78fd)
@simaishi
Copy link
Contributor

Jansa backport details:

$ git log -1
commit 5a9e445c301376036eda9605a48086cbd4239879
Author: Jason Frey <fryguy9@gmail.com>
Date:   Fri May 8 16:33:48 2020 -0400

    Merge pull request #282 from jvlcek/oidc_to_httpd_config_issue_19866

    Move API OpenID-Connect support to Apache configuration

    (cherry picked from commit 34c78fd07945c609a3126c1198c74e0800be1697)

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.

Research moving API OpenID-Connect/OAuth2 support into Apache configuration
6 participants