-
Notifications
You must be signed in to change notification settings - Fork 141
Enhancement Proposal: Authentication Filter #4235
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4235 +/- ##
==========================================
+ Coverage 86.09% 86.11% +0.02%
==========================================
Files 131 131
Lines 14171 14171
Branches 35 35
==========================================
+ Hits 12200 12203 +3
+ Misses 1767 1765 -2
+ Partials 204 203 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
|
||
| ## 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| 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". |
| const ( | ||
| AuthTypeBasic AuthType = "Basic" | ||
| AuthTypeJWT AuthType = "JWT" | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should specify the nginx modules these constants associate with
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean we should add a comment or rename the constants to reflect the module?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No the nginx module should be added as reference in the comment
// NGINX module: https://nginx.org/en/docs/http/ngx_http_auth_basic_module.html
| // 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we follow the ReferenceGrant pattern for accessing Secrets in another namespace?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment was mostly to reflect security constraints. I noticed I didn't make note of it elsewhere in the proposal though. Mainly, we want to make sure that the AuthenticaitonFilter can only reference secrets in the same namespace by default, and like @sjberman said we can support integration with ReferenceGrant to support cross-namespace where needed.
I'll add a section to the proposal to cover that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sjberman @salonichf5 The Security Considerations section should be updated now to talk about namespace isolation as well as cross-namespace support with ReferenceGrant
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we remove or amend this comment to reflect this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The spec should use the Gateway API's ObjectReference type instead of two separate Secret and SecretRef fields.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ciarams87 I'll amend the comment around this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sjberman good catch. The BasicAuth spec should just have a secretRef field so that it can work with reference grants.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think he meant using the secretRef type as SecretObjectReference
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nginx directives should be specified for the cases that apply.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea. I'll make that change.
| // Default: File. | ||
| // | ||
| // +optional | ||
| // +kubebuilder:validation:Enum=File;Remote |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same comment as below for specifying defaults and enum
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I think about it, it might not make sense for something like this to have a default. Users should probably be very explicit about which mode they want to set. What do you think?
| Path string `json:"path"` | ||
|
|
||
| // Levels specifies the directory hierarchy for cached files. | ||
| // Example: "1:2". |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
like 1: is for most frequently used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This specifies the directory depth of the cache file. In the case where you have a proxy_cache_path configuration like this:
proxy_cache_path /data/nginx/cache levels=1:2 keys_zone=one:10m;The file names in a cache will look like this: /data/nginx/cache/c/29/b7f54b2df7773722d382f4809d65029c
From the docs:
The levels parameter defines hierarchy levels of a cache: from 1 to 3, each level accepts values 1 or 2.
| type JWTTokenSource struct { | ||
| // Read token from Authorization header. Default: true. | ||
| // | ||
| // +optional |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
set defaults using kubebuilder: default
| ### 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
did not see this as a rule at API level, we will allow this at API spec level?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could likely add it at a CEL validation level. What are your thoughts? I've typically seen these kind of things handled by controller level validation, but it feels like CEL validation would be better if we want to reject the resource.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah I think we can have CEL validation to allow 401 and 403 to simplify things in the later step
ciarams87
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are mostly focussed on typos I spotted while doing a first pass, I haven't fully absorbed the content yet
|
|
||
| ## Introduction | ||
|
|
||
| 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". |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| 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". | |
| This document focuses explicitly on Authentication (AuthN) and not Authorization (AuthZ). Authentication (AuthN) defines the verification of identity. 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". |
| ## Use Cases | ||
|
|
||
| - As an Application Developer, I want to secure access to my APIs and Backend Applications. | ||
| - As an Application Developer, I want to enforce authenticaiton on specific routes and matches. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| - As an Application Developer, I want to enforce authenticaiton on specific routes and matches. | |
| - As an Application Developer, I want to enforce authentication on specific routes and matches. |
| ## API, Customer Driven Interfaces, and User Experience | ||
|
|
||
| This portion of the proposal will cover API design and interaction experience for use of Basic Auth and JWT. | ||
| This portioan also contains: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| This portioan also contains: | |
| This portion also contains: |
|
|
||
| 1. The Golang API | ||
| 2. Example spec for Basic Auth | ||
| - Example HTTPRoutes and NINGX configuration |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| - Example HTTPRoutes and NINGX configuration | |
| - Example HTTPRoutes and NGINX configuration |
| 3. Example spec for JWT Auth | ||
| - Example HTTPRoutes | ||
| - Examples for Local & Remote JWKS configration | ||
| - Example NINGX configuration for both Local & Remote JWKS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| - Example NINGX configuration for both Local & Remote JWKS | |
| - Example NGINX configuration for both Local & Remote JWKS |
|
|
||
| ### Key rotation | ||
|
|
||
| Users sholud be advised to regularly rotate their JWKS keys in cases where they chose to reference a local JWKS via a `secrefRef` or `configMapRef` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| Users sholud be advised to regularly rotate their JWKS keys in cases where they chose to reference a local JWKS via a `secrefRef` or `configMapRef` | |
| Users should be advised to regularly rotate their JWKS keys in cases where they chose to reference a local JWKS via a `secrefRef` or `configMapRef` |
| ### Auth failure default headers | ||
|
|
||
| Below are a list of default defensive headers for authentication failure reponses. | ||
| We may choose to include these headers by default for improved robustness in auth falure responses. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| We may choose to include these headers by default for improved robustness in auth falure responses. | |
| We may choose to include these headers by default for improved robustness in auth failure responses. |
| 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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had to look this up, I had never seen it before. It's been superseded by Cache-Control, I don't think we would need it, it's legacy from HTTP/1.0
| This allows users to reference an external authentication services, such as Keycloak, to handle the authentication requests. | ||
| While this API is available in the experimental channel, it is subject to change. | ||
|
|
||
| Our decision to go forward with our own `AuthenticationFilter` was to ensure we could quckly provide authenticaiton to our users while allowing us to closley monitor progress of the ExternalAuthFilter. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| Our decision to go forward with our own `AuthenticationFilter` was to ensure we could quckly provide authenticaiton to our users while allowing us to closley monitor progress of the ExternalAuthFilter. | |
| Our decision to go forward with our own `AuthenticationFilter` was to ensure we could quickly provide authentication to our users while allowing us to closely monitor progress of the ExternalAuthFilter. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be clear, this proposed AuthenticationFilter is to supply different auth methods than provided in the ExternalAuth filter, and is technically unrelated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sjberman that's not entirely true. If we went forward with ExternalAuth filter, we could build out an external auth service that uses NGINX. It wouldn't prevent us from supplying those auth methods.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But that is unrelated to this AuthenticationFilter. This filter is to configure our NGINX data plane to perform native auth, without the need for an external entity. The ExternalAuthFilter is for just using an external entity for auth, whatever that entity may be (it wouldn't be our direct NGINX data plane, but it could be an external NGINX instance).
These are two different solutions for implementing auth for the user, and are not technically related to each other. I only mention this because this sentence in the doc implies that this Filter is being implemented as a workaround to the ExternalAuth filter, but it's really not, it's a different auth solution. Just being pedantic.
|
|
||
| ### Documenting filter behavour | ||
|
|
||
| In regards to documentation of filter behavour with the `AuthenticationFilter`, the Gateway API documentation on filters states the following: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| In regards to documentation of filter behavour with the `AuthenticationFilter`, the Gateway API documentation on filters states the following: | |
| In regards to documentation of filter behaviour with the `AuthenticationFilter`, the Gateway API documentation on filters states the following: |
sjberman
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll do a more thorough review once I'm back online full time.
| // 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The spec should use the Gateway API's ObjectReference type instead of two separate Secret and SecretRef fields.
| // | ||
| // +optional | ||
| // +kubebuilder:default=401 | ||
| // +kubebuilder:validation:XValidation:message="statusCode must be 401 or 403",rule="self == null || self in [401, 403]" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you use an Enum on an integer instead of CEL? I haven't tried this, maybe it only works for strings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure. I'll see if it's possible though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think you should be able to since they resolve as strings?
| // Example: "auth_jwt_key_cache 10m;". | ||
| // | ||
| // +optional | ||
| KeyCache *string `json:"keyCache,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can use the Duration type that we have defined.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good. Out of curiosity, why do we have our own Duration type?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It allows us to perform validation on the format of it.
| // Example: "auth_jwt_key_cache 10m". | ||
| // | ||
| // +optional | ||
| KeyCache *string `json:"keyCache,omitempty"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can use the Duration type that we have defined.
|
|
||
| ### Attachment | ||
|
|
||
| Filters must be attached to a HTTPRoute at the `rules.matces` level. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| Filters must be attached to a HTTPRoute at the `rules.matces` level. | |
| Filters must be attached to a HTTPRoute at the `rules.matches` level. |
| Detailed header breakdown: | ||
|
|
||
| - Content-Type: "text/plain; charset=utf-8" | ||
| - This header explicitly set the body as plan text. This prevents browsers from treating the response as HTML or JavaScript, and is effective at mitigating Cross-side scrpting (XSS) through error pages |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| - This header explicitly set the body as plan text. This prevents browsers from treating the response as HTML or JavaScript, and is effective at mitigating Cross-side scrpting (XSS) through error pages | |
| - This header explicitly set the body as plain text. This prevents browsers from treating the response as HTML or JavaScript, and is effective at mitigating Cross-side scrpting (XSS) through error pages |
| ### Validation | ||
| When referencing an `AuthenticationFilter` in either a HTTPRoute or GRPCRoute, it is important that we ensure all configurable fields are validated, and that the resulting NGINX configuration is correct and secure |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| When referencing an `AuthenticationFilter` in either a HTTPRoute or GRPCRoute, it is important that we ensure all configurable fields are validated, and that the resulting NGINX configuration is correct and secure | |
| When referencing an `AuthenticationFilter` in either a HTTPRoute or GRPCRoute, it is important that we ensure all configurable fields are validated, and that the resulting NGINX configuration is correct and secure. |
|
|
||
| We should validated that only one `AuthenticationFilter` is referenced per-rule. Multiple references to an `AuthenticationFilter` in a single rule should result in an `Invalid` HTTPRoute/GRPCRoute, and the resource should be `Rejected`. | ||
|
|
||
| an `AuthenticationFilter` that sets a `onFailure.statusCode` to anything other than `401` or `403` should be rejected. This relates to the "Auth failure behaviour" section in the Security Condierations section. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| an `AuthenticationFilter` that sets a `onFailure.statusCode` to anything other than `401` or `403` should be rejected. This relates to the "Auth failure behaviour" section in the Security Condierations section. | |
| An `AuthenticationFilter` that sets a `onFailure.statusCode` to anything other than `401` or `403` should be rejected. This relates to the "Auth failure behaviour" section in the Security Considerations section. |
| This allows users to reference an external authentication services, such as Keycloak, to handle the authentication requests. | ||
| While this API is available in the experimental channel, it is subject to change. | ||
|
|
||
| Our decision to go forward with our own `AuthenticationFilter` was to ensure we could quckly provide authenticaiton to our users while allowing us to closley monitor progress of the ExternalAuthFilter. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be clear, this proposed AuthenticationFilter is to supply different auth methods than provided in the ExternalAuth filter, and is technically unrelated.
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.