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

Allow st2web proxy auth mode to work in HA environments #6041

Merged
merged 3 commits into from
Oct 23, 2023

Conversation

floatingstatic
Copy link
Contributor

@floatingstatic floatingstatic commented Oct 22, 2023

It appears that proxy auth mode only works with REMOTE_USER set as a cgi environment variable. This does not appear to work for those of us using the HA helm chart. I saw #5766 but it does not appear this has been fixed or addressed yet.

I had previously worked around this by modifying st2auth with a custom standalone auth module that always returns true without inspecting the username but it seems it would be beneficial to the wider community to get this working with the built-in proxy mode in st2auth. My use case is to continue to use Google IAP (identity aware proxy) in GKE which can forward remote user info to st2web (nginx) without having to roll patched version of st2auth and st2web to support this.

@pull-request-size pull-request-size bot added the size/S PR that changes 10-29 lines. Very easy to review. label Oct 22, 2023
@arm4b arm4b added this to the 3.9.0 milestone Oct 22, 2023
Copy link
Member

@arm4b arm4b left a comment

Choose a reason for hiding this comment

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

Great stuff, thanks a lot @floatingstatic for the contribution!
Are there any Unit Tests that you could add to support this PR?

@pull-request-size pull-request-size bot added size/M PR that changes 30-99 lines. Good size to review. and removed size/S PR that changes 10-29 lines. Very easy to review. labels Oct 22, 2023
@floatingstatic
Copy link
Contributor Author

@armab good call. Added a unit test to cover this. Looks like there is already a test case that would cover lack of header and no remote user env var. Let me know if you think we need any other cases.

@floatingstatic
Copy link
Contributor Author

Theres one test that failed unrelated to my change: https://github.com/StackStorm/st2/actions/runs/6604759729/job/17939241233?pr=6041

Perhaps something flaky with that particular test? It seems it passed everywhere else.

Copy link
Member

@arm4b arm4b left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for the fix!

@arm4b arm4b requested a review from a team October 23, 2023 16:48
@arm4b arm4b enabled auto-merge October 23, 2023 19:03
@arm4b arm4b added this to In progress in StackStorm v3.8.1 via automation Oct 23, 2023
@arm4b arm4b removed this from In progress in StackStorm v3.8.1 Oct 23, 2023
@arm4b arm4b merged commit 60d4d16 into StackStorm:master Oct 23, 2023
36 of 37 checks passed
@arm4b arm4b added this to In progress in StackStorm v3.8.1 via automation Oct 23, 2023
@arm4b arm4b moved this from In progress to Done in StackStorm v3.8.1 Oct 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement size/M PR that changes 30-99 lines. Good size to review.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants