Skip to content

fix: normalize CRLF line endings in .htpasswd to prevent xDS rejection#8567

Open
Teja079 wants to merge 1 commit intoenvoyproxy:mainfrom
Teja079:fix/basicauth-crlf-htpasswd
Open

fix: normalize CRLF line endings in .htpasswd to prevent xDS rejection#8567
Teja079 wants to merge 1 commit intoenvoyproxy:mainfrom
Teja079:fix/basicauth-crlf-htpasswd

Conversation

@Teja079
Copy link
Copy Markdown
Contributor

@Teja079 Teja079 commented Mar 21, 2026

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Fixes #

Release Notes: Yes/No

@Teja079 Teja079 requested a review from a team as a code owner March 21, 2026 21:25
Copilot AI review requested due to automatic review settings March 21, 2026 21:25
@netlify
Copy link
Copy Markdown

netlify bot commented Mar 21, 2026

Deploy Preview for cerulean-figolla-1f9435 canceled.

Name Link
🔨 Latest commit 334971e
🔍 Latest deploy log https://app.netlify.com/projects/cerulean-figolla-1f9435/deploys/69bf0cd197d19f00081811a6

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR aims to prevent Envoy xDS rejection when BasicAuth .htpasswd secrets are created on Windows by normalizing CRLF line endings before the data is handed to Envoy.

Changes:

  • Normalize \r\n to \n for BasicAuth users secret data during translation.
  • Add a unit test case covering CRLF line endings for validateHtpasswdFormat.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File Description
internal/gatewayapi/securitypolicy.go Normalizes htpasswd line endings prior to validation/IR construction.
internal/gatewayapi/securitypolicy_test.go Adds a CRLF test case for htpasswd format validation.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +2016 to +2020
// Normalize CRLF to LF to handle .htpasswd files generated on Windows.
// Without this, the \r character gets included in the hash string, causing
// Envoy to reject the xDS config with "invalid SHA hash length" errors.
usersSecretBytes = []byte(strings.ReplaceAll(string(usersSecretBytes), "\r\n", "\n"))

Copy link

Copilot AI Mar 21, 2026

Choose a reason for hiding this comment

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

usersSecretBytes normalization currently converts []byte -> string -> []byte, which always makes at least two full copies even when there are no CRLFs. Consider using bytes.ReplaceAll (or a bytes.Contains guard) to avoid unconditional allocations/copies for large secrets.

Copilot uses AI. Check for mistakes.
Comment on lines +2016 to +2019
// Normalize CRLF to LF to handle .htpasswd files generated on Windows.
// Without this, the \r character gets included in the hash string, causing
// Envoy to reject the xDS config with "invalid SHA hash length" errors.
usersSecretBytes = []byte(strings.ReplaceAll(string(usersSecretBytes), "\r\n", "\n"))
Copy link

Copilot AI Mar 21, 2026

Choose a reason for hiding this comment

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

This normalization only replaces CRLF (\r\n). If the secret contains stray \r characters (e.g., mixed line endings or a final line ending with \r only), they will still be passed through to Envoy and can trigger the same rejection. Consider also stripping standalone \r (e.g., replace all \r with empty) or normalizing more generally.

Suggested change
// Normalize CRLF to LF to handle .htpasswd files generated on Windows.
// Without this, the \r character gets included in the hash string, causing
// Envoy to reject the xDS config with "invalid SHA hash length" errors.
usersSecretBytes = []byte(strings.ReplaceAll(string(usersSecretBytes), "\r\n", "\n"))
// Strip carriage returns to handle .htpasswd files generated on Windows.
// Without this, the \r character gets included in the hash string, causing
// Envoy to reject the xDS config with "invalid SHA hash length" errors.
usersSecretBytes = []byte(strings.ReplaceAll(string(usersSecretBytes), "\r", ""))

Copilot uses AI. Check for mistakes.
Comment on lines +722 to +726
{
name: "valid htpasswd with CRLF line endings (Windows-style)",
htpasswd: "user1:{SHA}W6ph5Mm5Pz8GgiULbPgzG37mj9g=\r\nuser2:{SHA}qUqP5cyxm6YcTAhz05Hph5gvu9M=\r\n",
wantError: false,
},
Copy link

Copilot AI Mar 21, 2026

Choose a reason for hiding this comment

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

The new CRLF test case doesn’t actually validate the PR’s behavior change. validateHtpasswdFormat already calls strings.TrimSpace(line), which trims a trailing \r, so this test would pass even without the CRLF normalization in buildBasicAuth. Consider adding a unit test that exercises buildBasicAuth (or the IR/xDS generation) and asserts the emitted Users data contains no \r characters.

Copilot uses AI. Check for mistakes.
@codecov
Copy link
Copy Markdown

codecov bot commented Mar 21, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 74.17%. Comparing base (eadfe74) to head (334971e).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #8567      +/-   ##
==========================================
+ Coverage   74.15%   74.17%   +0.01%     
==========================================
  Files         242      242              
  Lines       37749    37750       +1     
==========================================
+ Hits        27992    28000       +8     
+ Misses       7804     7797       -7     
  Partials     1953     1953              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@arkodg arkodg requested a review from a team March 23, 2026 02:21
@jukie
Copy link
Copy Markdown
Contributor

jukie commented Mar 23, 2026

@Teja079 can you fix the conflicts and sign your commits?

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants