auth: new api auth implementation #36968

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

api/envoy/extensions/filters/http/api_key_auth/v3/api_key_auth.proto

+  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?

api/envoy/extensions/filters/http/api_key_auth/v3/api_key_auth.proto

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

tools/code_format/config.yaml

@@ -172,6 +172,7 @@ paths:
    - 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

api/envoy/extensions/filters/http/api_key_auth/v3/api_key_auth.proto

+  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).

api/envoy/extensions/filters/http/api_key_auth/v3/api_key_auth.proto

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

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

source/extensions/extensions_metadata.yaml

+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

api/envoy/extensions/filters/http/api_key_auth/v3/api_key_auth.proto

source/extensions/filters/http/api_key_auth/api_key_auth.h

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

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

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

bazel/repositories_extra.bzl

test/extensions/filters/http/api_key_auth/integration_test.cc

adisuissa · 2024-11-08T21:03:51Z

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

+      credentials:
+        entries:
+        - api_key: one_key
+          client_id: one_client


Probably a dumb question: where does the "one_client" value coming from in the data-plane (how it the "related client" defined)?

Probably a dumb question: where does the "one_client" value coming from in the data-plane (how it the "related client" defined)?

It's hard to answer this quesion. the client_id is an identity that related to the key. It could be anything that could be used to identify a client/app or a group of clients/apps.

For a specific example, the gateway production may provides the ability to manage the credentials. And that credentials management module may allow users to create or define the client or consumer, and then, users could create some keys that related to the client/consumer.
In conclusion, it depends on the abstraction of the gateway production.

api/envoy/extensions/filters/http/api_key_auth/v3/api_key_auth.proto

source/extensions/filters/http/api_key_auth/api_key_auth.h

api/envoy/extensions/filters/http/api_key_auth/v3/api_key_auth.proto

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

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

wbpcode · 2024-11-09T08:26:49Z

/docs

repokitteh-read-only · 2024-11-09T08:26:53Z

Docs for this Pull Request will be rendered here:

https://storage.googleapis.com/envoy-pr/36968/docs/index.html

The docs are (re-)rendered each time the CI envoy-presubmit (precheck docs) job completes.

🐱

Caused by: a #36968 (comment) was created by @wbpcode.

see: more, trace.

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

…dev-api-auth

…pi-auth

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

wbpcode · 2024-11-09T11:45:55Z

/retest

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

adisuissa

Thanks!
Overall LGTM, mostly left minor comments.
This is now ready for a senior-maintainer pass (preferably someone with the domain background).

adisuissa · 2024-11-12T15:21:01Z

api/envoy/extensions/filters/http/api_key_auth/v3/api_key_auth.proto

@@ -28,27 +26,84 @@ option (xds.annotations.v3.file_status).work_in_progress = true;
 // .. code-block:: yaml
 //
 //    authentication_header: "X-API-KEY"


Where is the authentication_header config field defined?

sorry, this should be updated.

api/envoy/extensions/filters/http/api_key_auth/v3/api_key_auth.proto

source/extensions/filters/http/api_key_auth/api_key_auth.cc

api/envoy/extensions/filters/http/api_key_auth/v3/api_key_auth.proto

source/extensions/filters/http/api_key_auth/api_key_auth.cc

adisuissa · 2024-11-12T16:08:06Z

source/extensions/filters/http/api_key_auth/api_key_auth.h

+    absl::variant<Http::LowerCaseString, std::string> source_{""};
+    bool query_source_{};


This is a code smell that is due to the use of absl::variant over using OO polymorphism.

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

source/extensions/filters/http/api_key_auth/api_key_auth.h

test/extensions/filters/http/api_key_auth/api_key_auth_test.cc

….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-13T03:59:09Z

Hi, @yanavlasov could you take a look at this PR to give second pass when you get some free time. I saw you are maintainer of jwt and rbac, so should have lots of background knowledge about the authentication/authorizaton.

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

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

api/envoy/extensions/filters/http/api_key_auth/v3/api_key_auth.proto Outdated Show resolved Hide resolved

wbpcode unassigned markdroth Nov 5, 2024

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

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

docs/root/configuration/http/http_filters/api_key_auth_filter.rst Outdated Show resolved Hide resolved

adisuissa reviewed Nov 8, 2024

View reviewed changes

wbpcode and others added 3 commits November 9, 2024 10:16

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

96dbe4e

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

Update source/extensions/filters/http/api_key_auth/api_key_auth.h

3d909d0

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

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

d90a0ee

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

wbpcode added 5 commits November 9, 2024 08:48

address comments

a9a03ee

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

Merge branch 'dev-api-auth' of https://github.com/wbpcode/envoy into …

a0263a2

…dev-api-auth

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

6d72835

…pi-auth

more clear comment

1818af7

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

fix format and test

b93795c

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

fix format

98ed3ed

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

adisuissa reviewed Nov 12, 2024

View reviewed changes

wbpcode and others added 3 commits November 13, 2024 11:22

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

4d9973f

….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…

750bc3d

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

address comments

ab295f3

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

wbpcode assigned yanavlasov Nov 13, 2024

wbpcode added 2 commits November 13, 2024 06:01

fix docs format

b204906

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

fix format

6eb8935

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

auth: new api auth implementation #36968

auth: new api auth implementation #36968

wbpcode commented Nov 4, 2024

repokitteh-read-only bot commented Nov 4, 2024

wbpcode Nov 4, 2024

adisuissa Nov 7, 2024

wbpcode Nov 8, 2024 •

edited

Loading

wbpcode commented Nov 6, 2024

wbpcode Nov 6, 2024

adisuissa left a comment

adisuissa Nov 7, 2024

adisuissa Nov 7, 2024

wbpcode Nov 8, 2024

adisuissa Nov 8, 2024

wbpcode Nov 9, 2024 •

edited

Loading

wbpcode commented Nov 9, 2024

repokitteh-read-only bot commented Nov 9, 2024

wbpcode commented Nov 9, 2024

adisuissa left a comment

adisuissa Nov 12, 2024

wbpcode Nov 13, 2024

adisuissa Nov 12, 2024

wbpcode commented Nov 13, 2024 •

edited

Loading

		absl::variant<Http::LowerCaseString, std::string> source_{""};
		bool query_source_{};

auth: new api auth implementation #36968

Are you sure you want to change the base?

auth: new api auth implementation #36968

Conversation

wbpcode commented Nov 4, 2024

repokitteh-read-only bot commented Nov 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wbpcode Nov 8, 2024 • edited Loading

Choose a reason for hiding this comment

wbpcode commented Nov 6, 2024

Choose a reason for hiding this comment

adisuissa left a 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

Choose a reason for hiding this comment

wbpcode Nov 9, 2024 • edited Loading

Choose a reason for hiding this comment

wbpcode commented Nov 9, 2024

repokitteh-read-only bot commented Nov 9, 2024

wbpcode commented Nov 9, 2024

adisuissa left a 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

wbpcode commented Nov 13, 2024 • edited Loading

wbpcode Nov 8, 2024 •

edited

Loading

wbpcode Nov 9, 2024 •

edited

Loading

wbpcode commented Nov 13, 2024 •

edited

Loading