feat: implement API Key Auth security policy #4986

sanposhiho · 2024-12-31T08:55:56Z

What type of PR is this?

What this PR does / why we need it:

implement API Key Auth security policy.

Which issue(s) this PR fixes:

Fixes #2630

Release Notes: Yes

zhaohuabing · 2024-12-31T10:25:12Z

api/v1alpha1/api_key_auth_types.go

+	// are allowed. This provides very limited but simple authorization.
+	//
+	// +optional
+	AllowedClients []string `json:"allowedClients,omitempty"`


Are there any use cases where an API key is authenticated but not allowed to access the targeted Route/Gateway?

A use case I could come up is:

the gateway has API Key Auth configured globally. e.g., Credentials has admin: key1 and user: key2.

the route also has per-route API Key Auth, and you can define this route is only for admin with AllowedClients.

In such case, this route should only has admin: key1 credential attached. The HTTPRoute level SP can't use a "global" Credential at the Gateway. This is a design decision of EG that the more specific policy should override the higher level of policy, rather than merging with it.

apiVersion: gateway.envoyproxy.io/v1alpha1 kind: SecurityPolicy metadata: name: admin-route-sp spec: targetRef: group: gateway.networking.k8s.io kind: HTTPRoute name: admin-route apiKeyAuth: credentials: name: secret-with-admin-key keySources: ...

I see, I missed that point.
But, I'm not sure if it's the best interface for users that have to prepare secret per access group. Also, the same key for each client would be spread across different secrets.

So, I still see the benefit of AllowedClients there; users can have only one secret as a source of keys and they can select which key(s) is enabled on certain routes with AllowedClients.
What do you think?

users can have only one secret as a source of keys and they can select which key(s) is enabled on certain routes with AllowedClients.

I think your argument kind of makes sense 🙂, but I’d prefer not to include the AllowedClients field in the API for the following reasons:

Increased Complexity: Adding another field for users to configure complicates the API, making it less intuitive.

Potential Security Risk: The shared secret may include API keys not authorized to access an HTTPRoute. This poses a security risk, as the owner of an HTTPRoute might inadvertently or intentionally add such keys to AllowedClients.

@envoyproxy/gateway-maintainers please weigh in.

First of all, if a bad actor has an access to HTTPRoute or associated SecurityPolicy, they can just disable APIKeyAuth to allow access for unintended clients

according to EG's design, if the app dev service account can create a SecurityPolicy in the dev namespace where the HTTPRoute also lives, they should be able to override the auth, we are relying on users to set up kube RBAC based on their use case

my vote would be to make it look more like basicAuth https://gateway.envoyproxy.io/docs/api/extension_types/#basicauth
with one field credentials / keyRefs that can hold a list of secrets

we need to specify what the secret key and value should look like

Sorry, so what's your suggestion then, whether removing AllowedClients vs not?

yeah my vote is to rm it, I'll bring this API up in the community meeting tomorrow, to hear more thoughts from the community

@zhaohuabing and I discussed this in the community meeting today and we prefer if AllowedClients was removed from the initial API. In the future if we want to do API key based Authz, we could enhance then authorization API

gateway/api/v1alpha1/authorization_types.go

Line 55 in d23eab9

type Principal struct {

api/v1alpha1/api_key_auth_types.go

sanposhiho · 2025-01-01T06:59:53Z

E2e test passed on my local and opened the PR for the review.

Signed-off-by: Kensei Nakada <[email protected]>

sanposhiho · 2025-01-01T07:01:51Z

api/v1alpha1/validation/securitypolicy_validate.go

@@ -57,9 +57,28 @@ func validateSecurityPolicySpec(spec *egv1a1.SecurityPolicySpec) error {
 		errs = append(errs, err)
 	}

+	if err := ValidateAPIKeyAuth(spec.APIKeyAuth); err != nil {


There's a bug that ValidateSecurityPolicy isn't called at all currently.
I sent another PR to patch that, #4987.

Signed-off-by: Kensei Nakada <[email protected]>

arkodg · 2025-01-02T20:52:30Z

api/v1alpha1/api_key_auth_types.go

+
+	// KeySources is where to fetch the key from the coming request.
+	// The value from the first source that has a key will be used.
+	KeySources []*KeySource `json:"keySources"`


I like OpenAPI's definiton which uses In
https://swagger.io/docs/specification/v3_0/authentication/api-keys/

in: type: Header name: "x-api-id"

another existing design pattern

the jwt field uses extractFrom https://gateway.envoyproxy.io/docs/api/extension_types/#jwtprovider

Well, generally speaking, I do prefer the current form in this kind of case at API design, especially for better future extensibility; it allows us to add a different structure later, e.g., let's say envoy supports a new key source fetching key from body (which Kong already supports), it might be like:

type KeySource struct { Header *string Query *string Cookie *string Body *BodyKeySource } type BodyKeySource struct { BodyType string // yaml or json. Field string // where we get API Key from. }

　

Also, extractFrom actually looks like very similar to KeySource here, are you suggesting renaming it from KeySources to ExtractFrom?

yeah suggesting we pick a name thats already used in the API / or industry and also try and use a union discriminator i.e.

extractFrom: type: Header header: <val>

e.g. https://gateway.envoyproxy.io/docs/api/extension_types/#ratelimitspec
https://github.com/kubernetes/enhancements/blob/master/keps/sig-api-machinery/1027-api-unions/README.md

Renamed from keySources to extractFrom.

Signed-off-by: Kensei Nakada <[email protected]>

arkodg · 2025-01-08T02:58:44Z

api/v1alpha1/api_key_auth_types.go

+	// This is an Opaque secret.
+	// Each API key is stored in the key representing the client id,
+	// which can be used in AllowedClients to authorize the client in a simple way.
+	Credentials gwapiv1.SecretObjectReference `json:"credentials"`


prefer CredentialRefs []gwapiv1.SecretObjectReference we can start and only support a single entry in the list, but it helps us leave room for expansion

arkodg · 2025-01-08T03:03:23Z

api/v1alpha1/api_key_auth_types.go

+// +kubebuilder:validation:XValidation:rule="(has(self.header) || has(self.query) || has(self.cookie))",message="one of header, query or cookie must be specified"
+type ExtractFrom struct {
+	// Header is the name of the header to fetch the key from.
+	// This field is optional, but only one of header, query or cookie is supposed to be specified.


needs a Type field - Header, QueryParams, Cookie

arkodg · 2025-01-08T03:07:52Z

api/v1alpha1/api_key_auth_types.go

+	// This field is optional, but only one of header, query or cookie is supposed to be specified.
+	//
+	// +optional
+	Query *string `json:"query,omitempty"`


prefer QueryParams here

envoy calls it parameters in some places and queryParameters in some places,

openAPI calls it query
https://swagger.io/docs/specification/v3_0/describing-parameters/#query-parameters

gateway api uses queryParams https://gateway-api.sigs.k8s.io/reference/spec/#gateway.networking.k8s.io%2fv1.HTTPRouteMatch

arkodg · 2025-01-08T03:09:41Z

api/v1alpha1/api_key_auth_types.go

+// APIKeyAuth defines the configuration for the API Key Authentication.
+type APIKeyAuth struct {
+	// Credentials is the Kubernetes secret which contains the API keys.
+	// This is an Opaque secret.


we should highlight the format of the Secret here , its a map of key - value pairs, is the map key the API Key and the map value the Client ID or vice versa

sanposhiho force-pushed the api-key-auth branch from 7da88c9 to ab9eb56 Compare December 31, 2024 09:32

zhaohuabing reviewed Dec 31, 2024

View reviewed changes

api/v1alpha1/api_key_auth_types.go Outdated Show resolved Hide resolved

zhaohuabing reviewed Dec 31, 2024

View reviewed changes

api/v1alpha1/api_key_auth_types.go Outdated Show resolved Hide resolved

sanposhiho force-pushed the api-key-auth branch 2 times, most recently from 999022c to 46d4f6a Compare January 1, 2025 06:59

sanposhiho marked this pull request as ready for review January 1, 2025 06:59

sanposhiho requested a review from a team as a code owner January 1, 2025 06:59

feat: implement API Key Auth security policy

47fed4a

Signed-off-by: Kensei Nakada <[email protected]>

sanposhiho force-pushed the api-key-auth branch from 46d4f6a to 47fed4a Compare January 1, 2025 07:00

sanposhiho commented Jan 1, 2025

View reviewed changes

doc: add a release note

eabdd76

Signed-off-by: Kensei Nakada <[email protected]>

sanposhiho force-pushed the api-key-auth branch from d7139e3 to eabdd76 Compare January 1, 2025 07:04

fix: regenerate manifests

c6f8f01

Signed-off-by: Kensei Nakada <[email protected]>

arkodg reviewed Jan 2, 2025

View reviewed changes

fix: rename KeySources to ExtractFrom

8a159e3

Signed-off-by: Kensei Nakada <[email protected]>

arkodg reviewed Jan 8, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: implement API Key Auth security policy #4986

feat: implement API Key Auth security policy #4986

sanposhiho commented Dec 31, 2024 •

edited

Loading

zhaohuabing Dec 31, 2024 •

edited

Loading

sanposhiho Dec 31, 2024 •

edited

Loading

zhaohuabing Dec 31, 2024 •

edited

Loading

sanposhiho Dec 31, 2024 •

edited

Loading

zhaohuabing Jan 2, 2025

arkodg Jan 2, 2025

sanposhiho Jan 5, 2025 •

edited

Loading

This comment was marked as outdated.

arkodg Jan 6, 2025

arkodg Jan 8, 2025

sanposhiho commented Jan 1, 2025

sanposhiho Jan 1, 2025

arkodg Jan 2, 2025

sanposhiho Jan 3, 2025 •

edited

Loading

arkodg Jan 3, 2025

sanposhiho Jan 5, 2025 •

edited

Loading

arkodg Jan 8, 2025

arkodg Jan 8, 2025

arkodg Jan 8, 2025

arkodg Jan 8, 2025

feat: implement API Key Auth security policy #4986

Are you sure you want to change the base?

feat: implement API Key Auth security policy #4986

Conversation

sanposhiho commented Dec 31, 2024 • edited Loading

zhaohuabing Dec 31, 2024 • edited Loading

Choose a reason for hiding this comment

sanposhiho Dec 31, 2024 • edited Loading

Choose a reason for hiding this comment

zhaohuabing Dec 31, 2024 • edited Loading

Choose a reason for hiding this comment

sanposhiho Dec 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sanposhiho Jan 5, 2025 • edited Loading

Choose a reason for hiding this comment

This comment was marked as outdated.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sanposhiho commented Jan 1, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sanposhiho Jan 3, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sanposhiho Jan 5, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sanposhiho commented Dec 31, 2024 •

edited

Loading

zhaohuabing Dec 31, 2024 •

edited

Loading

sanposhiho Dec 31, 2024 •

edited

Loading

zhaohuabing Dec 31, 2024 •

edited

Loading

sanposhiho Dec 31, 2024 •

edited

Loading

sanposhiho Jan 5, 2025 •

edited

Loading

sanposhiho Jan 3, 2025 •

edited

Loading

sanposhiho Jan 5, 2025 •

edited

Loading