Skip to content

Conversation

@umputun
Copy link
Owner

@umputun umputun commented Dec 6, 2025

Summary

  • Add per-route basic authentication following the existing OnlyFromIPs pattern
  • Routes can specify auth credentials via file provider (auth yaml field), docker (reproxy.auth label), or consul catalog (reproxy.auth tag)
  • Per-route auth takes precedence over global --basic-auth when configured
  • Format: comma-separated user:bcrypt_hash pairs (htpasswd compatible)

Example (file provider):

my-server.example.com:
  - { route: "^/admin/(.*)", dest: "http://backend:8080/$1", auth: "admin:$2y$..." }

Example (docker):

reproxy.auth=user1:$2y$...,user2:$2y$...

Copilot AI review requested due to automatic review settings December 6, 2025 09:56
Copy link
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 adds per-route basic authentication to reproxy, allowing different authentication credentials for different routes. The feature follows the existing pattern used by OnlyFromIPs and integrates seamlessly with the current architecture.

Key changes:

  • Added PerRouteAuth middleware that checks route-specific AuthUsers field for authentication
  • Modified global basic auth to skip routes with per-route auth configured
  • Extended all providers (file, docker, consul catalog) to parse and propagate auth configuration

Reviewed changes

Copilot reviewed 16 out of 16 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
app/proxy/per_route_auth.go New middleware implementing per-route basic authentication with constant-time validation
app/proxy/per_route_auth_test.go Comprehensive unit tests for per-route auth functionality
app/proxy/proxy.go Integrated per-route and global auth handlers into middleware chain with proper ordering
app/proxy/handlers.go Refactored global basic auth to skip routes with per-route auth; fixed strings.Split to strings.SplitN
app/proxy/handlers_test.go Added tests verifying global auth skips per-route auth routes
app/proxy/proxy_test.go Added end-to-end integration tests including default initialization scenario
app/discovery/discovery.go Added AuthUsers field to URLMapper and ParseAuth helper function
app/discovery/discovery_test.go Added tests for ParseAuth and verified AuthUsers preservation in extendMapper
app/discovery/provider/file.go Added auth YAML field parsing for file provider
app/discovery/provider/file_test.go Updated tests to verify auth field parsing
app/discovery/provider/testdata/config.yml Added example route with auth configuration
app/discovery/provider/docker.go Added reproxy.auth label parsing for docker provider
app/discovery/provider/docker_test.go Updated tests to verify auth label parsing
app/discovery/provider/consulcatalog/consulcatalog.go Added reproxy.auth tag parsing for consul catalog provider
app/discovery/provider/consulcatalog/consulcatalog_test.go Updated tests to verify auth tag parsing
README.md Added comprehensive documentation for global and per-route basic auth
Comments suppressed due to low confidence (1)

app/proxy/handlers.go:215

  • The credential validation logic (hashing username with SHA256, comparing with constant time, checking bcrypt password) is duplicated between globalBasicAuthHandler (lines 198-215) and PerRouteAuth.validateCredentials (lines 64-84 in per_route_auth.go). Consider extracting this into a shared helper function to improve maintainability and ensure consistency.
			passed := false
			for _, a := range allowed {
				alwElems := strings.SplitN(strings.TrimSpace(a), ":", 2)
				if len(alwElems) != 2 {
					continue
				}

				// hash to ensure constant time comparison not affected by username length
				usernameHash := sha256.Sum256([]byte(username))
				expectedUsernameHash := sha256.Sum256([]byte(alwElems[0]))

				expectedPasswordHash := alwElems[1]
				userMatched := subtle.ConstantTimeCompare(usernameHash[:], expectedUsernameHash[:])
				passMatchErr := bcrypt.CompareHashAndPassword([]byte(expectedPasswordHash), []byte(password))
				if userMatched == 1 && passMatchErr == nil {
					passed = true // don't stop here, check all allowed to keep the overall time consistent
				}
			}

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


func TestHttp_withPerRouteAuth(t *testing.T) {
port := rand.Intn(10000) + 40000
// test password hash for "secret123" generated with htpasswd -nbB test secret123
Copy link

Copilot AI Dec 6, 2025

Choose a reason for hiding this comment

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

The comment states "test password hash for 'secret123'" but the hash is actually for "passwd" (as indicated by the inline comment on the same line). The comment should be corrected to say "passwd" instead of "secret123".

Suggested change
// test password hash for "secret123" generated with htpasswd -nbB test secret123
// test password hash for "passwd" generated with htpasswd -nbB test passwd

Copilot uses AI. Check for mistakes.
umputun added a commit that referenced this pull request Dec 8, 2025
Remove unnecessary client.Do() call that made real HTTP requests
to example.com, causing CI failures on network issues.

Related to #237
@umputun umputun force-pushed the per-route-auth branch 6 times, most recently from e70df0b to 2906841 Compare December 9, 2025 08:11
add support for per-route basic authentication via AuthUsers field in URLMapper.
routes can now specify their own credentials independent of global basic auth.

- add PerRouteAuth handler with shared credential validation
- support AuthUsers field in file, docker, and consul-catalog providers
- document per-route auth configuration in README
@umputun umputun force-pushed the per-route-auth branch 2 times, most recently from cbc6cba to 6f78416 Compare December 9, 2025 08:20
- increase test timeout from 120s to 180s
- increase context timeouts to 10s
- increase waitForServer timeout to 5s with 100ms dial timeout
- increase inline Eventually timeouts to 5s
- add missing defer ds.Close() to prevent goroutine leaks
- remove flaky network call in TestHttp_matchHandler
@umputun umputun force-pushed the per-route-auth branch 4 times, most recently from c234e93 to fc59aa7 Compare December 11, 2025 07:25
- add tests for passwords containing colons
- add test for empty password edge case
- clarify test name for username with colon rejection
- fix comment precision in validateBasicAuthCredentials
- make ParseOnlyFrom filter empty entries like ParseAuth
@umputun umputun merged commit 9a70a1c into master Dec 11, 2025
3 checks passed
@umputun umputun deleted the per-route-auth branch December 11, 2025 07:36
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.

2 participants