Skip to content

WPB-21140: Add configurable large_client_header_buffers to nginz for large SAML responses #4816

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
Merged
julialongtin merged 4 commits into from
Oct 22, 2025
Merged

WPB-21140: Add configurable large_client_header_buffers to nginz for large SAML responses #4816

julialongtin merged 4 commits into from
Oct 22, 2025

Conversation

@sghosh23
Copy link
Contributor

@sghosh23 sghosh23 commented Oct 17, 2025 •
edited
Loading

  • Fixes "414 Request-URI Too Large" errors during SAML SSO login
  • when using HTTP Redirect binding with large SAMLResponse parameters
  • Add troubleshooting documentation to SSO docs

Checklist

  • Add a new entry in an appropriate subdirectory of changelog.d
  • Read and follow the PR guidelines

@sghosh23
Add configurable large_client_header_buffers to nginz for large SAML …
4d068f0 
…responses

- Fixes "414 Request-URI Too Large" errors during SAML SSO login
- when using HTTP Redirect binding with large SAMLResponse parameters
- Add troubleshooting documentation to SSO docs
@sghosh23
Add changelog
89d633a
@sghosh23 sghosh23 requested review from a team as code owners October 17, 2025 08:40
@zebot zebot added the ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist label Oct 17, 2025
julialongtin
julialongtin reviewed Oct 17, 2025
View reviewed changes
@sghosh23
Copy link
Contributor Author

sghosh23 commented Oct 17, 2025

In this PR I also added large_client_header_buffers: "4 8k" in the values.yaml of the chart that it can be configured easily without touching the charts/nginz/templates/conf/_nginx.conf.tpl template, thus the documentation follows this pattern.
And this configuration wont get released right away I guess. So do you think we should keep it like this or tell users to modify directly on the _nginx.conf.tpl file

@supersven
Copy link
Contributor

supersven commented Oct 17, 2025 •
edited
Loading

In this PR I also added large_client_header_buffers: "4 8k" in the values.yaml of the chart that it can be configured easily without touching the charts/nginz/templates/conf/_nginx.conf.tpl template, thus the documentation follows this pattern. And this configuration wont get released right away I guess. So do you think we should keep it like this or tell users to modify directly on the _nginx.conf.tpl file

In my opinion users (admins) shouldn't have to change the chart itself. This would them have to know about internals of the chart which may change. Also, it would be unintuitive - the interface for all foreign charts I touched so far has always been the values.yaml file.

supersven
supersven approved these changes Oct 17, 2025
View reviewed changes
Copy link
Contributor

@supersven supersven left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@emil-wire As nginx is always security sensitive ... Do you want to take a look as well?

@julialongtin julialongtin merged commit e4ea67f into develop Oct 22, 2025
10 checks passed
@julialongtin julialongtin deleted the document-saml-login-uri-issue branch October 22, 2025 10:00
@emil-wire
Copy link
Contributor

emil-wire commented Oct 23, 2025

@julialongtin @supersven @sghosh23
I need some changes please:

  • ideally this issue should be fixed in the IdP by switching to POST instead of GET
  • if that's not feasible, we need to make these assertions encrypted only if they come in via GET otherwise we run into issues where sensitive information could be logged in various places

This was referenced Nov 26, 2025
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@supersven supersven supersven approved these changes

@julialongtin julialongtin julialongtin approved these changes

Assignees

No one assigned

Labels

ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@sghosh23 @supersven @emil-wire @julialongtin @zebot