Skip to content
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

api: HTTP APIKey Auth Filter #36709

Merged
merged 15 commits into from
Nov 1, 2024
Merged

api: HTTP APIKey Auth Filter #36709

merged 15 commits into from
Nov 1, 2024

Conversation

sanposhiho
Copy link
Contributor

@sanposhiho sanposhiho commented Oct 20, 2024

This PR adds the API for HTTP APIKey Auth Filter that is proposed at #34877 and envoyproxy/gateway#2630.

Commit Message: api: HTTP APIKey Auth Filter
Risk Level: Low (only API)
Testing: WIP (will be done after we agree on the API)
Docs Changes: WIP
Release Notes: WIP
Platform Specific Features: No
Part of: #34877

Copy link

Hi @sanposhiho, welcome and thank you for your contribution.

We will try to review your Pull Request as quickly as possible.

In the meantime, please take a look at the contribution guidelines if you have not done so already.

🐱

Caused by: #36709 was opened by sanposhiho.

see: more, trace.

Copy link

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy/|docs/root/api-docs/).
envoyproxy/api-shepherds assignee is @mattklein123
CC @envoyproxy/api-watchers: FYI only for changes made to (api/envoy/|docs/root/api-docs/).

🐱

Caused by: #36709 was opened by sanposhiho.

see: more, trace.

@sanposhiho
Copy link
Contributor Author

/cc @wbpcode @arkodg @zhaohuabing @missBerg

@wbpcode
Copy link
Member

wbpcode commented Oct 20, 2024

/assign

@mattklein123
Copy link
Member

Can we potentially just include this functionality in the basic auth filter? We could have config in that filter to use different header names, etc. It seems not needed to have a new filter for this?

/wait

@wbpcode
Copy link
Member

wbpcode commented Oct 21, 2024

Can we potentially just include this functionality in the basic auth filter? We could have config in that filter to use different header names, etc. It seems not needed to have a new filter for this?

/wait

Basically, they have different logic and configuration format. Or we need to extend the meaning of basic_auth...

@mattklein123
Copy link
Member

Basically, they have different logic and configuration format. Or we need to extend the meaning of basic_auth...

The logic is basically identical just swapping out different header names. Doesn't seem worth it to have a new filter just to have different header names. Is there anything else different?

@wbpcode
Copy link
Member

wbpcode commented Oct 21, 2024

Basically, they have different logic and configuration format. Or we need to extend the meaning of basic_auth...

The logic is basically identical just swapping out different header names. Doesn't seem worth it to have a new filter just to have different header names. Is there anything else different?

Both the configration parsing and verification is a little different, although we can abstract them by a virtual interface. 🤔

@sanposhiho
Copy link
Contributor Author

sanposhiho commented Oct 21, 2024

I agree there's some similarity though, wouldn't it be weird to implement the features proposed here into BasicAuth filter since the responsibility of the filter would no longer be "Basic Auth"?

@wbpcode
Copy link
Member

wbpcode commented Oct 21, 2024

I agree there's some similarity though, wouldn't it be weird to implement the features proposed here into BasicAuth filter since the responsibility of the filter would no longer be "Basic Auth"?

Yeah, API Key Auth and Basic Auth are different authentication approaches. Or we need to extend the meaning of the basic_auth filter.
I will prefer to make this an independent filter.

If we insist on to merge them to one, then I think we need to document this very clear in the document of basic auth filter that the filter is not only be used for Basic Auth.

cc @mattklein123 WDYT

Signed-off-by: Kensei Nakada <[email protected]>
@zhaohuabing
Copy link
Member

zhaohuabing commented Oct 21, 2024

While basic authentication and API key authentication share some similarities, their authentication logic is fundamentally different:

  • Basic Auth authenticates a user by verifying both the username and password against a configured user list.
  • API Key Auth, on the other hand, authenticates a client by matching the API key provided in the request with a pre-configured list of valid keys.

From a UI perspective, it would also be clearer to separate these two methods into different APIs, as they serve distinct purposes and follow different workflows.

Some existing arts in this area:

// The header name to forward an authenticated user.
//
// If it is not specified, the username will not be forwarded.
string forward_username_header = 2
Copy link
Member

@zhaohuabing zhaohuabing Oct 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest removing this at the first run. We can use a metadata to pass it(username or clientid) to later filters if it's required in the future.

@mattklein123
Copy link
Member

OK fair enough we can do different filters.

@sanposhiho sanposhiho marked this pull request as draft October 24, 2024 00:23
@sanposhiho
Copy link
Contributor Author

Will make it ready for review again when the implementation is ready.

@sanposhiho
Copy link
Contributor Author

sanposhiho commented Oct 31, 2024

Discussed with @wbpcode.
Given I'm busy right now towards the upcoming K8s code freeze and I won't be able to take time for this until Kubecon NA is over, I converted this PR to the API-only one, and will pass the implementation part over to @wbpcode.

Thanks @wbpcode for the help!

@sanposhiho sanposhiho marked this pull request as ready for review October 31, 2024 13:25
@sanposhiho sanposhiho changed the title feat: HTTP APIKey Auth Filter api: HTTP APIKey Auth Filter Oct 31, 2024
@sanposhiho
Copy link
Contributor Author

Updated the PR description + the title

@sanposhiho sanposhiho requested a review from wbpcode October 31, 2024 13:27
Signed-off-by: Kensei Nakada <[email protected]>
sanposhiho and others added 6 commits November 1, 2024 11:39
Signed-off-by: Kensei Nakada <[email protected]>
Signed-off-by: Kensei Nakada <[email protected]>
Signed-off-by: Kensei Nakada <[email protected]>
Signed-off-by: wangbaiping(wbpcode) <[email protected]>
Signed-off-by: wangbaiping(wbpcode) <[email protected]>
Signed-off-by: wangbaiping(wbpcode) <[email protected]>
@repokitteh-read-only repokitteh-read-only bot added the deps Approval required for changes to Envoy's external dependencies label Nov 1, 2024
Copy link

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 @mattklein123

🐱

Caused by: #36709 was synchronize by wbpcode.

see: more, trace.

Signed-off-by: wangbaiping(wbpcode) <[email protected]>
@wbpcode wbpcode removed the deps Approval required for changes to Envoy's external dependencies label Nov 1, 2024
@repokitteh-read-only repokitteh-read-only bot removed the api label Nov 1, 2024
@wbpcode wbpcode merged commit 72b7507 into envoyproxy:main Nov 1, 2024
21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants