Skip to content

Commit

Permalink
Restrict policies to non-duplicate routes
Browse files Browse the repository at this point in the history
Problem: Some NGINX directives are not applied or enforced when configured in an internal location. This occurs when redirecting or rewriting a request from an external location to an internal location.

Solution: Only accept a policy if the Route it targets is the only Route that matches the hostname, port, and path combination. If other Routes overlap, the policy will be rejected.

This allows us to apply policy configuration to the external location instead of the internal locations. We would limit the policies we accept rather than limiting which Routes we accept.

This is possible because, with the policy restriction, a policy cannot be applied to a Route that shares an external location with another Route.

However, for the otel module, we still require some internal location directives to be specified, so the policy generator has been refactored to account for this.

Finally, revert named locations back to internal locations. As part of this process, we've learned that named locations do not behave as expected.

Co-authored-by: Kate Osborn <[email protected]>
  • Loading branch information
sjberman and kate-osborn committed Jul 31, 2024
1 parent c96fa3f commit 273d0ab
Show file tree
Hide file tree
Showing 52 changed files with 1,146 additions and 570 deletions.
7 changes: 2 additions & 5 deletions internal/mode/static/handler.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@ import (
ngxConfig "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/config"
"github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/file"
"github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/runtime"
"github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/policies"
"github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state"
"github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/dataplane"
"github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/graph"
Expand Down Expand Up @@ -65,8 +64,6 @@ type eventHandlerConfig struct {
serviceResolver resolver.ServiceResolver
// generator is the nginx config generator.
generator ngxConfig.Generator
// policyConfigGenerator is the config generator for NGF policies.
policyConfigGenerator policies.ConfigGenerator
// k8sClient is a Kubernetes API client
k8sClient client.Client
// logLevelSetter is used to update the logging level.
Expand Down Expand Up @@ -196,7 +193,7 @@ func (h *eventHandlerImpl) HandleEventBatch(ctx context.Context, logger logr.Log
return
case state.EndpointsOnlyChange:
h.version++
cfg := dataplane.BuildConfiguration(ctx, graph, h.cfg.serviceResolver, h.cfg.policyConfigGenerator, h.version)
cfg := dataplane.BuildConfiguration(ctx, graph, h.cfg.serviceResolver, h.version)

h.setLatestConfiguration(&cfg)

Expand All @@ -207,7 +204,7 @@ func (h *eventHandlerImpl) HandleEventBatch(ctx context.Context, logger logr.Log
)
case state.ClusterStateChange:
h.version++
cfg := dataplane.BuildConfiguration(ctx, graph, h.cfg.serviceResolver, h.cfg.policyConfigGenerator, h.version)
cfg := dataplane.BuildConfiguration(ctx, graph, h.cfg.serviceResolver, h.version)

h.setLatestConfiguration(&cfg)

Expand Down
11 changes: 4 additions & 7 deletions internal/mode/static/manager.go
Original file line number Diff line number Diff line change
Expand Up @@ -49,12 +49,12 @@ import (
"github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/config"
"github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/metrics/collectors"
ngxcfg "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/config"
"github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/config/policies"
"github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/config/policies/clientsettings"
"github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/config/policies/observability"
ngxvalidation "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/config/validation"
"github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/file"
ngxruntime "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/runtime"
"github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/policies"
"github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/policies/clientsettings"
"github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/policies/observability"
"github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state"
"github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/resolver"
"github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/validation"
Expand Down Expand Up @@ -232,7 +232,6 @@ func StartManager(cfg config.Config) error {
usageSecret: usageSecret,
gatewayCtlrName: cfg.GatewayCtlrName,
updateGatewayClassStatus: cfg.UpdateGatewayClassStatus,
policyConfigGenerator: policyManager,
})

objects, objectLists := prepareFirstEventBatchPreparerArgs(
Expand Down Expand Up @@ -287,17 +286,15 @@ func StartManager(cfg config.Config) error {
func createPolicyManager(
mustExtractGVK kinds.MustExtractGVK,
validator validation.GenericValidator,
) *policies.Manager {
) *policies.CompositeValidator {
cfgs := []policies.ManagerConfig{
{
GVK: mustExtractGVK(&ngfAPI.ClientSettingsPolicy{}),
Validator: clientsettings.NewValidator(validator),
Generator: clientsettings.Generate,
},
{
GVK: mustExtractGVK(&ngfAPI.ObservabilityPolicy{}),
Validator: observability.NewValidator(validator),
Generator: observability.Generate,
},
}

Expand Down
25 changes: 18 additions & 7 deletions internal/mode/static/nginx/config/generator.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,9 @@ package config
import (
"path/filepath"

"github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/config/policies"
"github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/config/policies/clientsettings"
"github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/config/policies/observability"
"github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/file"
"github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/dataplane"
)
Expand Down Expand Up @@ -85,7 +88,12 @@ func (g GeneratorImpl) Generate(conf dataplane.Configuration) []file.File {
files = append(files, generatePEM(id, pair.Cert, pair.Key))
}

files = append(files, g.generateHTTPConfig(conf)...)
policyGenerator := policies.NewCompositeGenerator(
clientsettings.NewGenerator(),
observability.NewGenerator(conf.Telemetry),
)

files = append(files, g.generateHTTPConfig(conf, policyGenerator)...)

files = append(files, generateConfigVersion(conf.Version))

Expand Down Expand Up @@ -127,20 +135,23 @@ func generateCertBundleFileName(id dataplane.CertBundleID) string {
return filepath.Join(secretsFolder, string(id)+".crt")
}

func (g GeneratorImpl) generateHTTPConfig(conf dataplane.Configuration) []file.File {
func (g GeneratorImpl) generateHTTPConfig(
conf dataplane.Configuration,
generator policies.Generator,
) []file.File {
fileBytes := make(map[string][]byte)

for _, execute := range g.getExecuteFuncs() {
for _, execute := range g.getExecuteFuncs(generator) {
results := execute(conf)
for _, res := range results {
fileBytes[res.dest] = append(fileBytes[res.dest], res.data...)
}
}

files := make([]file.File, 0, len(fileBytes))
for filepath, bytes := range fileBytes {
for fp, bytes := range fileBytes {
files = append(files, file.File{
Path: filepath,
Path: fp,
Content: bytes,
Type: file.TypeRegular,
})
Expand All @@ -149,10 +160,10 @@ func (g GeneratorImpl) generateHTTPConfig(conf dataplane.Configuration) []file.F
return files
}

func (g GeneratorImpl) getExecuteFuncs() []executeFunc {
func (g GeneratorImpl) getExecuteFuncs(generator policies.Generator) []executeFunc {
return []executeFunc{
executeBaseHTTPConfig,
executeServers,
newExecuteServersFunc(generator),
g.executeUpstreams,
executeSplitClients,
executeMaps,
Expand Down
25 changes: 21 additions & 4 deletions internal/mode/static/nginx/config/http/config.go
Original file line number Diff line number Diff line change
@@ -1,11 +1,13 @@
package http

const InternalRoutePathPrefix = "/_ngf-internal"

// Server holds all configuration for an HTTP server.
type Server struct {
SSL *SSL
ServerName string
Locations []Location
Includes []string
Includes []Include
Port int32
IsDefaultHTTP bool
IsDefaultSSL bool
Expand All @@ -18,19 +20,28 @@ type IPFamily struct {
IPv6 bool
}

type LocationType string

const (
InternalLocationType LocationType = "internal"
ExternalLocationType LocationType = "external"
RedirectLocationType LocationType = "redirect"
)

// Location holds all configuration for an HTTP location.
type Location struct {
Path string
ProxyPass string
HTTPMatchKey string
HTTPMatchVar string
Type LocationType
ProxySetHeaders []Header
ProxySSLVerify *ProxySSLVerify
Return *Return
ResponseHeaders ResponseHeaders
Rewrites []string
Includes []string
Includes []Include
GRPC bool
Redirects bool
}

// Header defines an HTTP header to be passed to the proxied server.
Expand Down Expand Up @@ -101,7 +112,7 @@ type Map struct {
Parameters []MapParameter
}

// Parameter defines a Value and Result pair in a Map.
// MapParameter defines a Value and Result pair in a Map.
type MapParameter struct {
Value string
Result string
Expand All @@ -118,3 +129,9 @@ type ServerConfig struct {
Servers []Server
IPFamily IPFamily
}

// Include defines a file that's included via the include directive
type Include struct {
Name string
Content []byte
}
5 changes: 5 additions & 0 deletions internal/mode/static/nginx/config/maps_template.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,4 +24,9 @@ map $http_upgrade $connection_upgrade {
default upgrade;
'' close;
}
## Returns just the path from the original request URI.
map $request_uri $request_uri_path {
"~^(?P<path>[^?]*)(\?.*)?$" $path;
}
`
1 change: 1 addition & 0 deletions internal/mode/static/nginx/config/maps_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,7 @@ func TestExecuteMaps(t *testing.T) {
"map ${http_my_set_header} $my_set_header_header_var {": 0,
"map $http_host $gw_api_compliant_host {": 1,
"map $http_upgrade $connection_upgrade {": 1,
"map $request_uri $request_uri_path {": 1,
}

mapResult := executeMaps(conf)
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,80 @@
package clientsettings

import (
"fmt"
"text/template"

ngfAPI "github.com/nginxinc/nginx-gateway-fabric/apis/v1alpha1"
"github.com/nginxinc/nginx-gateway-fabric/internal/framework/helpers"
"github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/config/http"
"github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/config/policies"
)

var tmpl = template.Must(template.New("client settings policy").Parse(clientSettingsTemplate))

const clientSettingsTemplate = `
{{- if .Body }}
{{- if .Body.MaxSize }}
client_max_body_size {{ .Body.MaxSize }};
{{- end }}
{{- if .Body.Timeout }}
client_body_timeout {{ .Body.Timeout }};
{{- end }}
{{- end }}
{{- if .KeepAlive }}
{{- if .KeepAlive.Requests }}
keepalive_requests {{ .KeepAlive.Requests }};
{{- end }}
{{- if .KeepAlive.Time }}
keepalive_time {{ .KeepAlive.Time }};
{{- end }}
{{- if .KeepAlive.Timeout }}
{{- if and .KeepAlive.Timeout.Server .KeepAlive.Timeout.Header }}
keepalive_timeout {{ .KeepAlive.Timeout.Server }} {{ .KeepAlive.Timeout.Header }};
{{- else if .KeepAlive.Timeout.Server }}
keepalive_timeout {{ .KeepAlive.Timeout.Server }};
{{- end }}
{{- end }}
{{- end }}
`

// Generator generates nginx configuration based on a clientsettings policy.
type Generator struct{}

// NewGenerator returns a new instance of Generator.
func NewGenerator() *Generator {
return &Generator{}
}

// GenerateForServer generates policy configuration for the server block.
func (g Generator) GenerateForServer(pols []policies.Policy, _ http.Server) policies.GenerateResultFiles {
return generate(pols)
}

// GenerateForServer generates policy configuration for a normal location block.
func (g Generator) GenerateForLocation(pols []policies.Policy, _ http.Location) policies.GenerateResultFiles {
return generate(pols)
}

// GenerateForServer generates policy configuration for a normal location block.
func (g Generator) GenerateForInternalLocation(pols []policies.Policy) policies.GenerateResultFiles {
return generate(pols)
}

func generate(pols []policies.Policy) policies.GenerateResultFiles {
files := make(policies.GenerateResultFiles, 0, len(pols))

for _, pol := range pols {
csp, ok := pol.(*ngfAPI.ClientSettingsPolicy)
if !ok {
continue
}

files = append(files, policies.File{
Name: fmt.Sprintf("ClientSettingsPolicy_%s_%s.conf", csp.Namespace, csp.Name),
Content: helpers.MustExecuteTemplate(tmpl, csp.Spec),
})
}

return files
}
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,9 @@ import (

ngfAPI "github.com/nginxinc/nginx-gateway-fabric/apis/v1alpha1"
"github.com/nginxinc/nginx-gateway-fabric/internal/framework/helpers"
"github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/policies"
"github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/policies/clientsettings"
"github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/policies/policiesfakes"
"github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/config/http"
"github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/config/policies"
"github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/config/policies/clientsettings"
)

func TestGenerate(t *testing.T) {
Expand Down Expand Up @@ -166,7 +166,15 @@ func TestGeneratePanics(t *testing.T) {
g := NewWithT(t)

generate := func() {
clientsettings.Generate(&policiesfakes.FakePolicy{}, nil)
generator := clientsettings.NewGenerator()
generator.GenerateForServer([]policies.Policy{}, http.Server{})
}

g.Expect(generate).To(Panic())

generate = func() {
generator := clientsettings.NewGenerator()
generator.GenerateForLocation([]policies.Policy{}, http.Location{})
}

g.Expect(generate).To(Panic())
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import (
"github.com/nginxinc/nginx-gateway-fabric/internal/framework/conditions"
"github.com/nginxinc/nginx-gateway-fabric/internal/framework/helpers"
"github.com/nginxinc/nginx-gateway-fabric/internal/framework/kinds"
"github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/policies"
"github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/config/policies"
staticConds "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/conditions"
"github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/validation"
)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,9 @@ import (
"github.com/nginxinc/nginx-gateway-fabric/internal/framework/conditions"
"github.com/nginxinc/nginx-gateway-fabric/internal/framework/helpers"
"github.com/nginxinc/nginx-gateway-fabric/internal/framework/kinds"
"github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/config/policies/clientsettings"
"github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/config/policies/policiesfakes"
"github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/nginx/config/validation"
"github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/policies/clientsettings"
"github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/policies/policiesfakes"
staticConds "github.com/nginxinc/nginx-gateway-fabric/internal/mode/static/state/conditions"
)

Expand Down
Loading

0 comments on commit 273d0ab

Please sign in to comment.