Skip to content

Commit

Permalink
fix: remove authentication headers from Connection
Browse files Browse the repository at this point in the history
  • Loading branch information
jongiddy committed Aug 3, 2021
1 parent 49f58bb commit 4d2ff09
Show file tree
Hide file tree
Showing 4 changed files with 46 additions and 7 deletions.
2 changes: 1 addition & 1 deletion Makefile
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@

format:
gofmt -w -s internal/*.go cmd/*.go
gofmt -w -s internal/**/*.go cmd/*.go

.PHONY: format
2 changes: 1 addition & 1 deletion internal/configuration/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -250,7 +250,7 @@ func (c *Config) SetOidcProvider() {
c.OIDCContext = context.Background()
provider, err := oidc.NewProvider(c.OIDCContext, c.ProviderUri)
if err != nil {
log.Fatal("failed to get provider configuration for %s: %v (hint: make sure %s is accessible from the cluster)", c.ProviderUri, err, c.ProviderUri)
log.Fatalf("failed to get provider configuration for %s: %v (hint: make sure %s is accessible from the cluster)", c.ProviderUri, err, c.ProviderUri)
}
c.OIDCProvider = provider
}
Expand Down
34 changes: 29 additions & 5 deletions internal/handlers/server.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,14 @@ package handlers

import (
"fmt"
"github.com/mesosphere/traefik-forward-auth/internal/api/storage/v1alpha1"
"github.com/mesosphere/traefik-forward-auth/internal/authentication"
"github.com/mesosphere/traefik-forward-auth/internal/configuration"
"net/http"
neturl "net/url"
"strings"

"github.com/mesosphere/traefik-forward-auth/internal/api/storage/v1alpha1"
"github.com/mesosphere/traefik-forward-auth/internal/authentication"
"github.com/mesosphere/traefik-forward-auth/internal/configuration"

"github.com/containous/traefik/pkg/rules"
"github.com/coreos/go-oidc"
"github.com/mesosphere/traefik-forward-auth/internal/authorization/rbac"
Expand Down Expand Up @@ -194,11 +195,34 @@ func (s *Server) AuthHandler(rule string) http.HandlerFunc {
for _, group := range groups {
w.Header().Add(impersonateGroupHeader, fmt.Sprintf("%s%s", s.config.GroupClaimPrefix, group))
}
w.Header().Set("Connection", cleanupConnectionHeader(w.Header().Get("Connection")))
}
w.WriteHeader(200)
}
}

var removeHeaders = map[string]bool{
strings.ToLower("Authorization"): true,
strings.ToLower(impersonateUserHeader): true,
strings.ToLower(impersonateGroupHeader): true,
}

// Traefik correctly removes any headers listed in the Connection header, but
// because it removes headers after forward auth has run, a specially crafted
// request can forward to the backend with the forward auth headers removed.
// Remove forward auth headers from the Connection header to ensure that they
// get passed to the backend.
func cleanupConnectionHeader(original string) string {
headers := strings.Split(original, ",")
passThrough := make([]string, 0, len(headers))
for _, header := range headers {
if remove := removeHeaders[strings.ToLower(strings.TrimSpace(header))]; !remove {
passThrough = append(passThrough, header)
}
}
return strings.TrimSpace(strings.Join(passThrough, ","))
}

// Handle auth callback
func (s *Server) AuthCallbackHandler() http.HandlerFunc {
return func(w http.ResponseWriter, r *http.Request) {
Expand Down Expand Up @@ -227,7 +251,7 @@ func (s *Server) AuthCallbackHandler() http.HandlerFunc {
provider := s.config.OIDCProvider

// Mapping scope
scope := []string{}
var scope []string
if s.config.Scope != "" {
scope = []string{s.config.Scope}
} else {
Expand Down Expand Up @@ -355,7 +379,7 @@ func (s *Server) authRedirect(logger *logrus.Entry, w http.ResponseWriter, r *ht
logger.Debug("Set CSRF cookie and redirect to OIDC login")

// Mapping scope
scope := []string{}
var scope []string
if s.config.Scope != "" {
scope = []string{s.config.Scope}
} else {
Expand Down
15 changes: 15 additions & 0 deletions internal/handlers/server_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -372,6 +372,21 @@ func TestAuthzDisabled(t *testing.T) {
assert.Equal(s.authzIsBypassed(r), true)
}

func TestCleanupConnection(t *testing.T) {
assert := assert.New(t)
tests := map[string]string{
"": "",
"Authorization": "",
"keep-alive": "keep-alive",
"keep-alive, AUTHORIZATION": "keep-alive",
"Authorization, Other": "Other",
"keep-alive, authorization, Other": "keep-alive, Other",
}
for original, expected := range tests {
assert.Equal(expected, cleanupConnectionHeader(original))
}
}

/**
* Utilities
*/
Expand Down

0 comments on commit 4d2ff09

Please sign in to comment.