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

Fixes #30535 - Set HTTP headers for proxy requests #872

Merged
merged 1 commit into from
Oct 14, 2020
Merged

Fixes #30535 - Set HTTP headers for proxy requests #872

merged 1 commit into from
Oct 14, 2020

Conversation

hsahmed
Copy link

@hsahmed hsahmed commented Aug 19, 2020

No description provided.

@hsahmed hsahmed mentioned this pull request Aug 19, 2020
Merged
@hsahmed hsahmed force-pushed the master branch 3 times, most recently from 33856dd to cc2d829 Compare August 21, 2020 04:26
@hsahmed hsahmed force-pushed the master branch 2 times, most recently from bf32057 to 8bda021 Compare September 14, 2020 05:46
@hsahmed hsahmed force-pushed the master branch 4 times, most recently from 1781019 to 6c91300 Compare September 20, 2020 14:24
ekohl
ekohl reviewed Sep 22, 2020
View reviewed changes
Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

From a quick glance this looks correct. I'll be out until the next week, but I'll try to give it a spin then. It looks like this is backwards compatible which always makes me happy.

templates/lookup_identity.conf.erb Show resolved Hide resolved
@wiad
Copy link

wiad commented Sep 30, 2020 •
edited
Loading

I'm having some problems making this work, using mod_auth_cas. This module sets it's own headers, like HTTP_CAS_MAILADDRESS and I'm not succeding in using those headers to create the ones needed by Foreman (like HTTP_REMOTE_USER_EMAIL), they only turn out with a value of (null).

The ugly workaround is to edit app/services/sso/apache.rb and replace the headers there with my CAS headers.

sbuzonas
sbuzonas reviewed Oct 1, 2020
View reviewed changes
templates/auth_kerb.conf.erb Outdated
@@ -8,6 +8,14 @@
KrbAuthRealms <%= @facts['foreman_ipa']['default_realm'] %>
Krb5KeyTab <%= scope.lookupvar('::foreman::http_keytab') %>
KrbLocalUserMapping On

# Set headers for proxy requests
Copy link

Choose a reason for hiding this comment

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

Setting these here won't apply to the intercept for form login. I put them in lookup_identity which covers ^/users/(ext)?login$ and it fixes both form intercept and ticket authentication

@tbrisker
Copy link
Member

tbrisker commented Oct 6, 2020

@hsahmed could you update according to the latest comments please?

@hsahmed
Copy link
Author

hsahmed commented Oct 6, 2020

@hsahmed could you update according to the latest comments please?

I have moved the headers to lookup_identity, however I have not been able to test it yet. I will test and amend the commit later today.

@hsahmed
Copy link
Author

hsahmed commented Oct 8, 2020

I have moved the headers to lookup_identity and they're now working for both SSO and intercept login.

@tbrisker tbrisker requested a review from ekohl October 8, 2020 11:19
ehelms
ehelms approved these changes Oct 12, 2020
View reviewed changes
Copy link
Member

@ehelms ehelms left a comment

Choose a reason for hiding this comment

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

@ekohl final thoughts before merging?

@ehelms ehelms merged commit 6f5b99b into theforeman:master Oct 14, 2020
@sbuzonas
Copy link

Upon reapplying this patch to after an upgrade, I noticed an inconsistency. The unset REMOTE_USER_USER_GROUPS, but it's really passing REMOTE_USER_GROUPS

@tbrisker
Copy link
Member

Thanks for noticing @sbuzonas ! I've opened #896 to fix that.

@nbarrientos nbarrientos mentioned this pull request Apr 27, 2021
Closed
@emersonford emersonford mentioned this pull request Jun 1, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tbrisker tbrisker tbrisker left review comments

@sbuzonas sbuzonas sbuzonas left review comments

@ekohl ekohl ekohl approved these changes

@ehelms ehelms ehelms approved these changes

Assignees
No one assigned
Labels
Enhancement Needs testing
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@hsahmed @wiad @tbrisker @sbuzonas @ekohl @ehelms @theforeman-bot @wbclark