Enhancement Proposal: Authentication Filter #4235
Open
+1,075
−1
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
the
documentation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4235 +/- ##
==========================================
+ Coverage 86.08% 86.10% +0.01%
==========================================
Files 131 131
Lines 14162 14162
Branches 35 35
==========================================
+ Hits 12192 12194 +2
+ Misses 1766 1765 -1
+ Partials 204 203 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
salonichf5
requested changes
Nov 6, 2025
|
|
||
| ## Introduction | ||
|
|
||
| This document focus expliclty on Authentiaction (AuthN) and not Authorization (AuthZ). Authentiaction (AuthN) defines the verification of identiy. It asks the question, "Who are you?". This is different from Authorization (AuthZ), which preceeds Authentication. It asks the question, "What are you allowed to do". |
There was a problem hiding this comment.
Suggested change
| This document focus expliclty on Authentiaction (AuthN) and not Authorization (AuthZ). Authentiaction (AuthN) defines the verification of identiy. It asks the question, "Who are you?". This is different from Authorization (AuthZ), which preceeds Authentication. It asks the question, "What are you allowed to do". | |
| This document focuses expliclty on Authentication (AuthN) and not Authorization (AuthZ). Authentication (AuthN) defines the verification of identiy. It asks the question, "Who are you?". This is different from Authorization (AuthZ), which preceeds Authentication. It asks the question, "What are you allowed to do". |
| // +optional | ||
| Basic *BasicAuth `json:"basic,omitempty"` | ||
|
|
||
| // JWT configures JSON Web Token authentication (NGINX Plus). |
There was a problem hiding this comment.
is JWT a plus feature only?
Comment on lines
+125
to
+128
| const ( | ||
| AuthTypeBasic AuthType = "Basic" | ||
| AuthTypeJWT AuthType = "JWT" | ||
| ) |
There was a problem hiding this comment.
we should specify the nginx modules these constants associate with
| // BasicAuth configures HTTP Basic Authentication. | ||
| type BasicAuth struct { | ||
| // Secret is the name of the Secret containing htpasswd data. | ||
| // The Secret must be in the same namespace as this filter. |
There was a problem hiding this comment.
why does it have to be in the same namespace? we can always resolve the secret in different namespace, unless there is another limitation assumed?
There was a problem hiding this comment.
Can we follow the ReferenceGrant pattern for accessing Secrets in another namespace?
Comment on lines
+134
to
+150
| Secret string `json:"secret"` | ||
|
|
||
| // Key is the key within the Secret that contains the htpasswd data. | ||
| // Default: "htpasswd". | ||
| // | ||
| // +optional | ||
| Key *string `json:"key,omitempty"` | ||
|
|
||
| // Realm used by NGINX auth_basic; helps with logging and WWW-Authenticate. | ||
| // | ||
| // +optional | ||
| Realm *string `json:"realm,omitempty"` | ||
|
|
||
| // OnFailure customizes the 401 response for failed authentication. | ||
| // | ||
| // +optional | ||
| OnFailure *AuthFailureResponse `json:"onFailure,omitempty"` |
There was a problem hiding this comment.
directives should be specified for the cases that apply.
| Path string `json:"path"` | ||
|
|
||
| // Levels specifies the directory hierarchy for cached files. | ||
| // Example: "1:2". |
There was a problem hiding this comment.
like 1: is for most frequently used?
| type JWTTokenSource struct { | ||
| // Read token from Authorization header. Default: true. | ||
| // | ||
| // +optional |
There was a problem hiding this comment.
set defaults using kubebuilder: default
Comment on lines
+446
to
+463
| const ( | ||
| // AuthenticationFilterConditionTypeAccepted indicates that the AuthenticationFilter is accepted. | ||
| // | ||
| // Possible reasons for this condition to be True: | ||
| // * Accepted | ||
| // | ||
| // Possible reasons for this condition to be False: | ||
| // * Invalid | ||
| AuthenticationFilterConditionTypeAccepted AuthenticationFilterConditionType = "Accepted" | ||
|
|
||
| // AuthenticationFilterConditionReasonAccepted is used with the Accepted condition type when | ||
| // the condition is true. | ||
| AuthenticationFilterConditionReasonAccepted AuthenticationFilterConditionReason = "Accepted" | ||
|
|
||
| // AuthenticationFilterConditionReasonInvalid is used with the Accepted condition type when | ||
| // the filter is invalid. | ||
| AuthenticationFilterConditionReasonInvalid AuthenticationFilterConditionReason = "Invalid" | ||
| ) |
There was a problem hiding this comment.
If we’re aiming to follow community guidelines, should we also add a condition for resolved refs? Since we usually have a Secret referenced for auth, we should probably tell the user when that reference was successfully resolved. I’m not sure if we currently do that for Secrets – I’ve only seen this done for backendRefs and inferencePools. Not certain what the right behavior is here, but I wanted to call it out.
Comment on lines
+563
to
+572
| # Internal location for custom 401 response | ||
| location @basic_auth_failure { | ||
| add_header WWW-Authenticate 'Basic realm="Restricted"' always; | ||
| add_header Content-Type "text/plain; charset=utf-8" always; | ||
| add_header X-Content-Type-Options "nosniff" always; | ||
| add_header Cache-Control "no-store" always; | ||
| add_header Pragma "no-cache" always; | ||
| return 401 'Unauthorized'; | ||
| } | ||
| } |
There was a problem hiding this comment.
Suggested change
| # Internal location for custom 401 response | |
| location @basic_auth_failure { | |
| add_header WWW-Authenticate 'Basic realm="Restricted"' always; | |
| add_header Content-Type "text/plain; charset=utf-8" always; | |
| add_header X-Content-Type-Options "nosniff" always; | |
| add_header Cache-Control "no-store" always; | |
| add_header Pragma "no-cache" always; | |
| return 401 'Unauthorized'; | |
| } | |
| } | |
| # Internal location for custom 401 response | |
| location @basic_auth_failure { | |
| internal; | |
| add_header WWW-Authenticate 'Basic realm="Restricted"' always; | |
| add_header Content-Type "text/plain; charset=utf-8" always; | |
| add_header X-Content-Type-Options "nosniff" always; | |
| add_header Cache-Control "no-store" always; | |
| add_header Pragma "no-cache" always; | |
| return 401 'Unauthorized'; | |
| } | |
| } |
Comment on lines
+1021
to
+1026
| ### Auth failure behaviour | ||
|
|
||
| 3xx response codes should not be allowed and AuthenticationFilter.onFailure must not support redirect targets. This is to prevent to prevent open-redirect abuse. | ||
|
|
||
| 401 and 403 should be the only allowable auth failure codes. | ||
|
|
There was a problem hiding this comment.
did not see this as a rule at API level, we will allow this at API spec level?
github-project-automation
bot
moved this from 🆕 New
to 🏗 In Progress
in NGINX Gateway Fabric
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Proposed changes
Status set to Implementable after merging #4136
This document proposes a solution for enabling Authentication use cases through NGINX Gateway Fabric.
Closes #4052
Checklist
Before creating a PR, run through this checklist and mark each as complete.
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.