Add support for Basic Auth through AuthenticationFilter #4436

shaun-nx · 2025-12-10T14:36:08Z

Proposed changes

This change adds support for Basic Authentication using the new AuthenticationFilter CRD.

For both HTTPRoute and GRPCRoute resource, requests made to route rules that reference an AuthenticationFilter using type: Basic will require credentials sent as part of the request.

Example HTTP requests:

# With credentials provided
curl --resolve cafe.example.com:8080:127.0.0.1 http://cafe.example.com:8080/coffee1 -u user1:password1
Server address: 10.244.0.18:8080
Server name: coffee-5b9c74f9d9-bf6bl
Date: 10/Dec/2025:14:32:31 +0000
URI: /coffee1
Request ID: 1dab2e14a2893e71f9a5a4992b477adc

# Without credentials provided
curl --resolve cafe.example.com:8080:127.0.0.1 http://cafe.example.com:8080/coffee1
<html>
<head><title>401 Authorization Required</title></head>
<body>
<center><h1>401 Authorization Required</h1></center>
<hr><center>nginx</center>
</body>
</html>

Example GRPC requests:

# With credentials provided
grpcurl -plaintext -proto grpc.proto -authority bar.com  -H "Authorization: Basic dXNlcjE6cGFzc3dvcmQx" -d '{"name": "exact"}' ${GW_IP}:${GW_PORT} helloworld.Greeter/SayHello
{
  "message": "Hello exact"
}

# Without credential provided
grpcurl -plaintext -proto grpc.proto -authority bar.com -d '{"name": "exact"}' ${GW_IP}:${GW_PORT} helloworld.Greeter/SayHello
ERROR:
  Code: Unknown
  Message: unexpected HTTP status code received from server: 204 (No Content)

Closes #4312

Checklist

Before creating a PR, run through this checklist and mark each as complete.

I have read the CONTRIBUTING doc
I have added tests that prove my fix is effective or that my feature works
I have checked that all unit tests pass after adding my changes
I have updated necessary documentation
I have rebased my branch onto main
I will ensure my PR is targeting the main branch and pulling from my branch from my own fork

Release notes

If this PR introduces a change that affects users and needs to be mentioned in the release notes,
please add a brief note that summarizes the change.

Support Basic Auth through AuthenticationFilter

…auth

shaun-nx · 2025-12-10T14:39:19Z

internal/controller/state/graph/graph.go

+	NGFPolicies           map[PolicyKey]policies.Policy
+	SnippetsFilters       map[types.NamespacedName]*ngfAPIv1alpha1.SnippetsFilter
+	AuthenticationFilters map[types.NamespacedName]*ngfAPIv1alpha1.AuthenticationFilter


I had through about creating a map similar to NGCPolicies for our filter. I'm not 100% sure if it would make sense though and it will require a bit of re-wiring. Would love to know what every thinks.

You mean to combine Snippets and Auth into one map for filters? It may be possible, though they do behave differently.

The generic Policy type is used because all of our policies are treated essentially the same in terms of how we process and render them in the nginx conf. Snippets are a bit of a special case.

Okay, that's good to know. Thanks Saylor!
With that in mind lets avoid that change as part of this PR. It can be something we come back and consider in the future if we find we're making more filter CRDs

Copilot

Pull request overview

This PR implements support for Basic Authentication through a new AuthenticationFilter CRD. Routes (both HTTPRoute and GRPCRoute) can now reference an AuthenticationFilter to require Basic Auth credentials on requests. The implementation includes validation of the filter configuration, secret references, and NGINX configuration generation to enforce authentication at the location level.

Key Changes

Introduces the AuthenticationFilter CRD for configuring Basic Auth via secret references
Extends HTTPRoute and GRPCRoute processing to support the new filter type
Adds NGINX configuration generation for auth_basic directives and user file management

Reviewed changes

Copilot reviewed 30 out of 30 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
apis/v1alpha1/authenticationfilter_types.go	Adds constant for Basic Auth secret key
apis/v1alpha1/register.go	Registers AuthenticationFilter types with the scheme
internal/framework/kinds/kinds.go	Adds AuthenticationFilter kind constant
internal/controller/state/graph/authentication_filter.go	Implements validation and processing logic for AuthenticationFilters
internal/controller/state/graph/secret.go	Updates secret validation to accept Opaque secrets with "auth" key
internal/controller/state/graph/extension_ref_filter.go	Adds AuthenticationFilter support to extension ref validation
internal/controller/state/graph/common_filter.go	Updates filter resolution to use map-based resolver lookup
internal/controller/state/graph/httproute.go	Threads AuthenticationFilter support through HTTP route processing
internal/controller/state/graph/grpcroute.go	Threads AuthenticationFilter support through GRPC route processing
internal/controller/state/graph/route_common.go	Updates route building to accept AuthenticationFilters
internal/controller/state/graph/graph.go	Adds AuthenticationFilters to cluster state and graph
internal/controller/state/dataplane/types.go	Defines AuthenticationFilter and BasicAuth dataplane types
internal/controller/state/dataplane/convert.go	Implements conversion from graph to dataplane representation
internal/controller/state/dataplane/configuration.go	Threads authentication filters through configuration building
internal/controller/state/conditions/conditions.go	Adds condition constructors for AuthenticationFilter status
internal/controller/state/status/status_setters.go	Implements status setter for AuthenticationFilter
internal/controller/state/status/prepare_requests.go	Prepares status update requests for AuthenticationFilters
internal/controller/state/change_processor.go	Adds AuthenticationFilter to change processing
internal/controller/handler.go	Updates status handling to include AuthenticationFilters
internal/controller/manager.go	Registers AuthenticationFilter controller and initial sync
internal/controller/nginx/config/servers.go	Implements NGINX configuration for Basic Auth
internal/controller/nginx/config/servers_template.go	Adds template directives for auth_basic
internal/controller/nginx/config/http/config.go	Adds AuthBasic configuration types
internal/controller/nginx/config/generator.go	Generates auth user files for Basic Auth
charts/nginx-gateway-fabric/templates/clusterrole.yaml	Adds RBAC permissions for AuthenticationFilter resources
Test files	Updates tests to pass nil for new AuthenticationFilter parameters

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

Copilot · 2025-12-10T15:46:30Z

internal/controller/state/graph/secret.go

+	// TODO: Define our own Secret types for other auth methods (e.g., OAuth) as needed.
+	case secret.Type == apiv1.SecretTypeOpaque:
+		// Allow Opaque secrets specifically when they contain the "auth" key.
+		//nolint:revive // may need to consider our own secret types.


[nitpick] The inline comment 'may need to consider our own secret types' doesn't clearly explain why the revive lint is being suppressed. Consider updating this to be more specific about what revive is flagging and why it's acceptable in this context.

Suggested change

//nolint:revive // may need to consider our own secret types.

//nolint:revive // Suppress revive warning for empty branch: intentionally allow Opaque secrets with "auth" key for future support of custom secret types (e.g., OAuth). No action needed in this branch.

Copilot · 2025-12-10T15:46:31Z

internal/controller/state/graph/common_filter.go

+			// Will need to test how this behaves if we don't get a match.
+			extRefFilterResolver := extRefFilterResolvers[string(f.ExtensionRef.Kind)]


This TODO-style comment should be addressed before merging. If this is a known edge case that needs handling, add explicit error handling or validation. If testing has already confirmed the behavior is acceptable, remove the comment.

Suggested change

// Will need to test how this behaves if we don't get a match.

extRefFilterResolver := extRefFilterResolvers[string(f.ExtensionRef.Kind)]

extRefFilterResolver, ok := extRefFilterResolvers[string(f.ExtensionRef.Kind)]

if !ok {

err := field.NotFound(filterPath.Child("extensionRef.kind"), f.ExtensionRef.Kind)

errors.resolve = append(errors.resolve, err)

valid = false

continue

}

I put quite a few TODO comments in this PR. I'll come back to address and/or remove them after the review.

Copilot · 2025-12-10T15:46:31Z

internal/controller/state/change_processor.go

+			{
+				gvk:       cfg.MustExtractGVK(&ngfAPIv1alpha1.AuthenticationFilter{}),
+				store:     newObjectStoreMapAdapter(clusterStore.AuthenticationFilters),
+				predicate: nil, // Not sure if this should be nil or if we should track references


This comment indicates uncertainty about the implementation. The predicate should be nil if you want to write status to all AuthenticationFilters (like SnippetsFilters), or it should filter based on references if you only want to update referenced ones. This should be resolved before merging.

Suggested change

predicate: nil, // Not sure if this should be nil or if we should track references

predicate: nil, // we always want to write status to AuthenticationFilters so we don't filter them out

For SnippetsFilter, we mention that "we always want to write status to SnippetsFilters so we don't filter them out". Would the same apply for AuthenticationFilter? I con't think of scenarios where we wouldn't want to write the status.

Yeah, probably.

Copilot · 2025-12-10T15:46:32Z

internal/controller/nginx/config/servers.go

+	logger := ctlrZap.New().WithName("update-location-auth-filter")
+
+	// TODO: Remove logging after debugging
+	if authenticationFilter == nil {
+		logger.Info("Missing AuthenticationFilter for location", "locationPath", location.Path)
+	} else if authenticationFilter.Basic == nil {
+		logger.Info("No Basic authentication configured for location", "locationPath", location.Path)
+	}
+
+	if authenticationFilter != nil {
+		logger.Info("Applying authentication filter to location", "locationPath", location.Path)


This function contains temporary debug logging that should be removed before merging (as noted by the TODO comment). Remove the logger initialization and all log statements on lines 730-740, 750-753.

internal/controller/nginx/config/servers.go

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

sjberman · 2025-12-10T21:44:58Z

apis/v1alpha1/authenticationfilter_types.go

 )

+const (
+	AuthKeyBasic = "auth" // Key in the Secret data for Basic Auth credentials.


Let's add a properly formatted comment above the field. And why "Basic" in the name?

Sounds good. I chose the word AuthKeyBasic so we can distinguish between the key for JWT when we introduce that. So that new key might be set to `AuthKeyJWT = "jwks.json"

I though we decided we were just going to use auth as the generic field name for both Basic and JWT?

The JWT is going to be base64 encoded anyway, and technically not a JSON format in the Secret itself.

sjberman · 2025-12-10T21:45:30Z

internal/controller/nginx/config/http/config.go

+type AuthBasic struct {
+	Realm string
+	Data  AuthBasicData
+}
+
+type AuthBasicData struct {
+	FileName string
+	FileData []byte
+}


Will need comments.

sjberman · 2025-12-10T21:46:31Z

internal/controller/nginx/config/generator.go

 	// nginxPlusConfigFile is the path to the file containing the NGINX Plus API config.
 	nginxPlusConfigFile = httpFolder + "/plus-api.conf"
+
+	basicAuthUserFile = configFolder + "/secrets/%s"


secretsFolder is already defined above

Also, we shouldn't need a global var for the user file name, it can just be generated on the fly as needed.

sjberman · 2025-12-10T21:56:11Z

internal/controller/nginx/config/servers.go

+	return location
+}
+
+func createExecuteResultsForAuthBasicUserFile(servers []http.Server) []executeResult {


Can we not follow the pattern that we use in generateCertBundle to create the secret files? (related to my other comment as well about that function)

sjberman · 2025-12-10T21:57:39Z

internal/controller/nginx/config/http/config.go

+
+type AuthBasicData struct {
+	FileName string
+	FileData []byte


The file data isn't needed on the location block. I wonder if we can structure this better. For example, the way we currently process TLS secret files. The server sets the file path, but the data isn't stored on the server Go struct at all. It's a separate field in the dataplane conf. See how we handle generateCertBundle.

Thanks @sjberman ! This is exactly the kind of thing I wanted to find in this review. 😄
I'll take a dig into that part of the code and see if we can simplify this.

sjberman · 2025-12-10T22:15:09Z

internal/controller/state/graph/authentication_filter.go

+			allErrs = append(allErrs, valErr)
+		}
+
+		resolvedSecrets := secretResolver.getResolvedSecrets()


getResolvedSecrets() is now being called twice when we build the graph. Let's just call it once to save on overhead.

sjberman · 2025-12-10T22:17:24Z

internal/controller/state/graph/secret.go

 	r.resolvedSecrets[nsname] = &secretEntry{
 		Secret: Secret{
 			Source:     secret,
 			CertBundle: certBundle,


This doesn't really make sense in the context of a basic auth secret.

sjberman · 2025-12-10T22:18:29Z

internal/controller/state/graph/authentication_filter.go

+				if secret == nil {
+					allErrs = append(allErrs, field.Invalid(field.NewPath("spec.basic.secretRef"), af.Spec.Basic.SecretRef.Name, msg))
+					break
+				}


Didn't we already verify this and add an error after we called resolve?

Addressed here

sjberman · 2025-12-10T22:18:56Z

internal/controller/state/graph/authentication_filter.go

+				if secret.Source == nil {
+					allErrs = append(allErrs, field.Invalid(field.NewPath("spec.basic.secretRef"), af.Spec.Basic.SecretRef.Name, msg))
+					break
+				}


Isn't this an internal coding error if this value is nil?

Addressed here

sjberman · 2025-12-10T22:19:16Z

internal/controller/state/graph/authentication_filter.go

+				if _, exists := secret.Source.Data[ngfAPI.AuthKeyBasic]; !exists {
+					msg = "referenced secret does not contain required 'auth' key"
+					allErrs = append(allErrs, field.Invalid(field.NewPath("spec.basic.secretRef"), af.Spec.Basic.SecretRef.Name, msg))
+				}
+				break


Similarly, doesn't resolve already verify this?

Had to look over it a few times but I think you're right. With the resolve function, we already know if we have a secret with the right type and key.

We also know if it exists or not, making the first part of the logic redundant.

Nice catch with that. I'll re-run a some checks and make sure everything still works.

Add support for Basic Auth through AuthenticationFilter

bb4607a

github-project-automation bot added this to NGINX Gateway Fabric Dec 10, 2025

nginx-bot bot added the release-notes label Dec 10, 2025

github-project-automation bot moved this to 🆕 New in NGINX Gateway Fabric Dec 10, 2025

github-actions bot added enhancement New feature or request helm-chart Relates to helm chart labels Dec 10, 2025

Merge branch 'feat/authentication-filter-basic-auth' into feat/basic-…

23ab285

…auth

shaun-nx commented Dec 10, 2025

View reviewed changes

shaun-nx requested a review from Copilot December 10, 2025 15:44

Copilot AI reviewed Dec 10, 2025

View reviewed changes

shaun-nx and others added 3 commits December 10, 2025 15:51

make generate-all

0a48fff

Set dest from location.AuthBasic.Data.FileName

0e5f51e

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Update dest value

5a4903c

sjberman reviewed Dec 10, 2025

View reviewed changes

shaun-nx added 12 commits December 11, 2025 09:26

Remove duplicate call to secret resolved

e35da99

Remove duplicate call to secret resolver

1b7dd25

Re-add removed comment

edb8348

Process custom secret type

816382d

Update basicAuthUserFile const

3fa25c0

Ensure secrets are resolved

d2f170b

Resolve nil pointer error

a681ba5

Remove redundant error handling

5effdbc

Do not resolve Opaque secrets

b8f26a6

Add secret namespace to file path

50dfd77

Update status variable name

5b6ccd1

Update logic for generating auth basic user file

12d6e54

	//nolint:revive // may need to consider our own secret types.
	//nolint:revive // Suppress revive warning for empty branch: intentionally allow Opaque secrets with "auth" key for future support of custom secret types (e.g., OAuth). No action needed in this branch.

		// Will need to test how this behaves if we don't get a match.
		extRefFilterResolver := extRefFilterResolvers[string(f.ExtensionRef.Kind)]

-			// Will need to test how this behaves if we don't get a match.
-			extRefFilterResolver := extRefFilterResolvers[string(f.ExtensionRef.Kind)]
+			extRefFilterResolver, ok := extRefFilterResolvers[string(f.ExtensionRef.Kind)]
+			if !ok {
+				err := field.NotFound(filterPath.Child("extensionRef.kind"), f.ExtensionRef.Kind)
+				errors.resolve = append(errors.resolve, err)
+				valid = false
+				continue
+			}

	predicate: nil, // Not sure if this should be nil or if we should track references
	predicate: nil, // we always want to write status to AuthenticationFilters so we don't filter them out

Add support for Basic Auth through AuthenticationFilter #4436

Are you sure you want to change the base?

Add support for Basic Auth through AuthenticationFilter #4436

Uh oh!

Conversation

shaun-nx commented Dec 10, 2025

Proposed changes

Checklist

Release notes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Key Changes

Reviewed changes

Uh oh!

Copilot AI Dec 10, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 10, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 10, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 10, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!