auth: new api auth implementation by wbpcode · Pull Request #36968 · envoyproxy/envoy

wbpcode · 2024-11-04T16:06:41Z

Commit Message: auth: new api auth implementation
Additional Description:

To close #34877

Risk Level: low. New extension.
Testing: unit, integration.
Docs Changes: added.
Release Notes: added.
Platform Specific Features: n/a.

Signed-off-by: wangbaiping/wbpcode <wangbaiping@bytedance.com>

Signed-off-by: wangbaiping(wbpcode) <wangbaiping@bytedance.com>

repokitteh-read-only · 2024-11-04T16:06:54Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy/|docs/root/api-docs/).
envoyproxy/api-shepherds assignee is @markdroth
CC @envoyproxy/api-watchers: FYI only for changes made to (api/envoy/|docs/root/api-docs/).
CC @envoyproxy/dependency-shepherds: Your approval is needed for changes made to (bazel/.*repos.*\.bzl)|(bazel/dependency_imports\.bzl)|(api/bazel/.*\.bzl)|(.*/requirements\.txt)|(.*\.patch).
envoyproxy/dependency-shepherds assignee is @moderation

🐱

Caused by: #36968 was opened by wbpcode.

see: more, trace.

wbpcode · 2024-11-04T16:12:55Z

+  message Credential {
+    // The value of the unique API key.
+    string api_key = 1 [(validate.rules).string = {min_len: 1}];
+
+    // The unique id or identity that used to identify the client or consumer.
+    string client_id = 2 [(validate.rules).string = {min_len: 1}];
+  }
+
+  message Credentials {
+    repeated Credential entries = 1;
+  }
+
+  // Api credentials used to authenticate the clients.
+  Credentials credentials = 1 [(udpa.annotations.sensitive) = true];


The api key credenticals has clear structure. It would be better to use a typed message rather than DataSource.

It's OK to change the API because the #36709 was just merged.

FWIW, this implies that only an xDS-update will change the credentials.
If that is the intended behavior, that's fine by me, although I can see some (not huge) benefits of using DataSource (e.g., fine-granularity of credentials updates, and using env-vars).

@adisuissa Yeah. I know. I prefer the explicitly defined structures rather than plain string or bytes and I think the xDS should be the first way to update the configuration.

And specific to the DataSource advantages, there are two possible compromised ways:

Use the DataSource as config field type but require it's content to keep the structure of the Crendentials. We can load it by the loadFromJsonString directly to a Crendentials proto type.

Add a new DataSource config field in the Credentials future if some users require env/file support. By this way, we can use both xDS or local file/env to configure this filter.

Both are ok to me (and a inclided to the second way), which one would you prefer?

Signed-off-by: wangbaiping(wbpcode) <wangbaiping@bytedance.com>

…pi-auth

wbpcode · 2024-11-06T15:26:10Z

cc @envoyproxy/api-shepherds

Signed-off-by: wangbaiping(wbpcode) <wangbaiping@bytedance.com>

wbpcode · 2024-11-06T17:15:50Z

    - source/extensions/compression/zstd/common/dictionary_manager.h
    - source/extensions/filters/http/adaptive_concurrency/controller
    - source/extensions/filters/http/admission_control
+    - source/extensions/filters/http/api_key_auth


This is necessary because the ExceptionFreeFactoryBase cannot handle the case where the route specific configuration loading will throw the exception.

Signed-off-by: wangbaiping(wbpcode) <wangbaiping@bytedance.com>

adisuissa

Thanks!
Left a few high-level comments about the API.
I'll review the rest of the code once the API is more stable.

adisuissa · 2024-11-07T15:41:19Z

+  message Credential {
+    // The value of the unique API key.
+    string api_key = 1 [(validate.rules).string = {min_len: 1}];
+
+    // The unique id or identity that used to identify the client or consumer.
+    string client_id = 2 [(validate.rules).string = {min_len: 1}];
+  }
+
+  message Credentials {
+    repeated Credential entries = 1;
+  }
+
+  // Api credentials used to authenticate the clients.
+  Credentials credentials = 1 [(udpa.annotations.sensitive) = true];


FWIW, this implies that only an xDS-update will change the credentials.
If that is the intended behavior, that's fine by me, although I can see some (not huge) benefits of using DataSource (e.g., fine-granularity of credentials updates, and using env-vars).

adisuissa · 2024-11-07T15:57:31Z

+envoy.filters.http.api_key_auth:
+  categories:
+  - envoy.filters.http
+  security_posture: robust_to_untrusted_downstream


This will probably need a signoff from the security group.

cc @envoyproxy/security-team

Normally for filters which contain parsers and this one does have a small parser, a fuzzer is required to claim the robustness to unstrusted downstream.

I am not familiar to the fuzz test. But I add one by refering to jwt fuzz. Hope it make sense.

Co-authored-by: Adi (Suissa) Peleg <adip@google.com> Signed-off-by: code <wbphub@gmail.com>

Signed-off-by: wangbaiping(wbpcode) <wangbaiping@bytedance.com>

wbpcode · 2024-11-19T02:54:48Z

wait any basically used to wait author's response. change it to wait review.

adisuissa

One API question, but otherwise LGTM

adisuissa · 2024-11-19T13:40:15Z

+  // then all authenticated clients are allowed. This provides very limited but simple authorization.
+  // If more complex authorization is required, then use the :ref:`HTTP RBAC filter
+  // <config_http_filters_rbac>` instead.
+  repeated string allowed_clients = 2;


The filter will try to authenticate the client by the override_config (if exists) and filter level configuration first. After the client is authenticated, we will try to check if it's limited by the allowed_clients (if exists and non-empty).

IMHO this is somewhat fragile, and may lead to inconsistencies when new fields are added.
Specifically, IIUC, if both fields are set, the client needs both in the credentials list inside the override_config and in the allowed_clients list.
I understand the motivation behind adding one of the fields, but I'm not sure I fully grok when would adding both fields be useful.
To be more concrete, looking at the example below, it is unclear to me why the {one_key, one_client} pair is useful in the override config (other than maybe receiving a different HTTP response code).

….proto Co-authored-by: Adi (Suissa) Peleg <adip@google.com> Signed-off-by: code <wbphub@gmail.com>

Signed-off-by: wangbaiping(wbpcode) <wangbaiping@bytedance.com>

wbpcode · 2024-11-19T18:40:46Z

/retest

…pi-auth

Signed-off-by: wangbaiping(wbpcode) <wangbaiping@bytedance.com>

wbpcode · 2024-11-23T09:16:43Z

cc @adisuissa , Hi, adi, the comments are updated and could you take a look at this again? Thanks. 🙏

yanavlasov · 2024-11-25T21:13:05Z

Waiting for @adisuissa review
/wait-any

adisuissa · 2024-11-25T21:46:51Z

+  // Route specific APIKeyAuth configuration. This is optional and could be used to **override** the
+  // filter level ApiKeyAuth configuration **partly**.
+  //
+  // For example, if non-empty ``credentials`` is set in the ``override_config``, then the
+  // ``credentials`` in the filter level configuration will be ignored and the ``credentials`` in
+  // this configuration will be used.


I don't think partial override is the right way to go. If a message of type ApiKeyAuth is provided, then it should replace the one define above entirely. The reason I'm against this is that if a new field is added in the future to ApiKeyAuth where it won't make sense to do a partial override, this will complicate the use of this field and the override.

If an override to a specific field is desired, then an additional field can be added to ApiKeyAuthPerRoute. For example repeated Credential credentials that will override only the credentials defined above.

If an override to a specific field is desired, then an additional field can be added to ApiKeyAuthPerRoute. For example repeated Credential credentials that will override only the credentials defined above.

Yeah, it's required.

Then I think I can remove the override_config and use the repeated Credential credentials and repeated KeySource key_sources directly.

adisuissa · 2024-11-25T21:48:30Z

+  // .. note::
+  //   Setting this field and ``override_config.credentials`` at the same configuration entry is not
+  //   an error but also makes no sense because they provide similar functionality. Please only use
+  //   one of them at same configuration entry.


Why wouldn't this be an error?
IIUC all the options here are essentially a oneof. As this is a newly introduced feature I think it can either reject or declare the order (prioritization) of these fields.

The authentication and authorization are two different phases. Even both credentials and allowed_clients are configured, the filter could handle them correctly as expected.

So, I don't think this is an error and don't deserve a configuration rejection. The prioritization also only make the implementation more complex and make no much sense.

Signed-off-by: wangbaiping(wbpcode) <wangbaiping@bytedance.com>

adisuissa

Thanks!
Left a minor API rephrase suggestion, but otherwise LGTM.

yanavlasov · 2024-11-26T15:44:25Z

/wait

Co-authored-by: Adi (Suissa) Peleg <adip@google.com> Signed-off-by: code <wbphub@gmail.com>

adisuissa

/lgtm api

wbpcode · 2024-11-27T02:20:10Z

@yanavlasov I think now this is ready for a final review or approval, thanks :)

yanavlasov

LGTM

yanavlasov · 2024-12-10T16:19:16Z

+    return ApiKeyAuthStats{ALL_API_KEY_AUTH_STATS(POOL_COUNTER_PREFIX(scope, prefix))};
+  }
+
+  ApiKeyAuthConfig default_config_;


I suggest being strict about const correctness of the configuration types as they are shared across multiple threads.

Let's land this PR first and create a new PR immediately.

wangbaiping/wbpcode added 2 commits November 2, 2024 11:06

enhanced api

9c77db7

Signed-off-by: wangbaiping/wbpcode <wangbaiping@bytedance.com>

complete basic development of api key auth

e543fb6

Signed-off-by: wangbaiping(wbpcode) <wangbaiping@bytedance.com>

wbpcode marked this pull request as draft November 4, 2024 16:06

repokitteh-read-only Bot added api deps Approval required for changes to Envoy's external dependencies labels Nov 4, 2024

repokitteh-read-only Bot assigned markdroth and moderation Nov 4, 2024

wbpcode commented Nov 4, 2024

View reviewed changes

Comment thread api/envoy/extensions/filters/http/api_key_auth/v3/api_key_auth.proto Outdated

wbpcode unassigned markdroth Nov 5, 2024

wangbaiping(wbpcode) added 2 commits November 6, 2024 15:22

complete the development and tests

58e74c0

Signed-off-by: wangbaiping(wbpcode) <wangbaiping@bytedance.com>

add change log

962515f

Signed-off-by: wangbaiping(wbpcode) <wangbaiping@bytedance.com>

wbpcode removed the deps Approval required for changes to Envoy's external dependencies label Nov 6, 2024

wbpcode marked this pull request as ready for review November 6, 2024 15:24

Merge branch 'main' of https://github.com/envoyproxy/envoy into dev-a…

1126c96

…pi-auth

repokitteh-read-only Bot added the deps Approval required for changes to Envoy's external dependencies label Nov 6, 2024

wbpcode unassigned moderation Nov 6, 2024

wbpcode assigned adisuissa Nov 6, 2024

wbpcode removed the deps Approval required for changes to Envoy's external dependencies label Nov 6, 2024

fix metadata

98e2ac2

Signed-off-by: wangbaiping(wbpcode) <wangbaiping@bytedance.com>

repokitteh-read-only Bot added the deps Approval required for changes to Envoy's external dependencies label Nov 6, 2024

wbpcode commented Nov 6, 2024

View reviewed changes

wangbaiping(wbpcode) added 2 commits November 7, 2024 08:47

fix docs

4d4605f

Signed-off-by: wangbaiping(wbpcode) <wangbaiping@bytedance.com>

revert unnecessary change

68232cd

Signed-off-by: wangbaiping(wbpcode) <wangbaiping@bytedance.com>

adisuissa reviewed Nov 7, 2024

View reviewed changes

wbpcode and others added 2 commits November 8, 2024 09:11

Update docs/root/configuration/http/http_filters/api_key_auth_filter.rst

0e2550e

Co-authored-by: Adi (Suissa) Peleg <adip@google.com> Signed-off-by: code <wbphub@gmail.com>

address comments

9eea7c8

Signed-off-by: wangbaiping(wbpcode) <wangbaiping@bytedance.com>

phlax reviewed Nov 8, 2024

View reviewed changes

Comment thread docs/root/configuration/http/http_filters/api_key_auth_filter.rst Outdated

adisuissa reviewed Nov 19, 2024

View reviewed changes

wbpcode and others added 3 commits November 19, 2024 22:03

Update api/envoy/extensions/filters/http/api_key_auth/v3/api_key_auth…

bb0291a

….proto Co-authored-by: Adi (Suissa) Peleg <adip@google.com> Signed-off-by: code <wbphub@gmail.com>

Update api/envoy/extensions/filters/http/api_key_auth/v3/api_key_auth…

2b93530

….proto Co-authored-by: Adi (Suissa) Peleg <adip@google.com> Signed-off-by: code <wbphub@gmail.com>

update docs

dd79eef

Signed-off-by: wangbaiping(wbpcode) <wangbaiping@bytedance.com>

adisuissa reviewed Nov 19, 2024

View reviewed changes

Comment thread api/envoy/extensions/filters/http/api_key_auth/v3/api_key_auth.proto Outdated

address comments

7aa8b7b

Signed-off-by: wangbaiping(wbpcode) <wangbaiping@bytedance.com>

wbpcode removed the wait:review Waiting for (further) review label Nov 22, 2024

wangbaiping(wbpcode) added 2 commits November 23, 2024 08:44

Merge branch 'main' of https://github.com/envoyproxy/envoy into dev-a…

b792580

…pi-auth

address comments

90654fb

Signed-off-by: wangbaiping(wbpcode) <wangbaiping@bytedance.com>

repokitteh-read-only Bot added the waiting:any label Nov 25, 2024

adisuissa reviewed Nov 25, 2024

View reviewed changes

repokitteh-read-only Bot removed the waiting:any label Nov 25, 2024

wangbaiping(wbpcode) added 2 commits November 26, 2024 09:01

address comments

6588a6d

Signed-off-by: wangbaiping(wbpcode) <wangbaiping@bytedance.com>

fix example config

0db5f59

Signed-off-by: wangbaiping(wbpcode) <wangbaiping@bytedance.com>

adisuissa reviewed Nov 26, 2024

View reviewed changes

Comment thread api/envoy/extensions/filters/http/api_key_auth/v3/api_key_auth.proto Outdated

Comment thread api/envoy/extensions/filters/http/api_key_auth/v3/api_key_auth.proto Outdated

repokitteh-read-only Bot added the waiting label Nov 26, 2024

update api_key_auth.proto

1935ac3

Co-authored-by: Adi (Suissa) Peleg <adip@google.com> Signed-off-by: code <wbphub@gmail.com>

repokitteh-read-only Bot removed the waiting label Nov 26, 2024

更新 api_key_auth.proto

2e57ab4

Co-authored-by: Adi (Suissa) Peleg <adip@google.com> Signed-off-by: code <wbphub@gmail.com>

adisuissa reviewed Nov 26, 2024

View reviewed changes

repokitteh-read-only Bot removed the api label Nov 26, 2024

yanavlasov approved these changes Dec 10, 2024

View reviewed changes

wbpcode merged commit d2dbc05 into envoyproxy:main Dec 10, 2024

wbpcode deleted the dev-api-auth branch December 10, 2024 16:33

Conversation

wbpcode commented Nov 4, 2024

Uh oh!

repokitteh-read-only Bot commented Nov 4, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wbpcode Nov 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

wbpcode commented Nov 6, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adisuissa left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

Uh oh!

Uh oh!

wbpcode commented Nov 19, 2024

Uh oh!

adisuissa left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

wbpcode commented Nov 19, 2024

Uh oh!

wbpcode commented Nov 23, 2024

Uh oh!

yanavlasov commented Nov 25, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wbpcode Nov 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

adisuissa left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

yanavlasov commented Nov 26, 2024

Uh oh!

adisuissa left a comment

Choose a reason for hiding this comment

Uh oh!

wbpcode commented Nov 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yanavlasov left a comment

Choose a reason for hiding this comment

wbpcode Nov 8, 2024 •

edited

Loading

wbpcode Nov 26, 2024 •

edited

Loading

wbpcode commented Nov 27, 2024 •

edited

Loading