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

feat: rewrite hmac-auth plugin for usability #11581

Merged
merged 8 commits into from
Sep 19, 2024

Conversation

Revolyssup
Copy link
Contributor

@Revolyssup Revolyssup commented Sep 17, 2024

Currently the hmac-auth plugin has a lot of headers that need to be configured, which makes it confusing to use the plugin. This PR also makes it so that it follows the RFC.
This PR refactors the plugin as following:

  • Use single Authorization header instead of multiple headers for ease of use. Now you can provide 4 parameters(key_id,signature, headers,algorithm) in Authorization header separated by comma in any order, this makes it easy to use
  • change the name of access_key to key_id
  • simplifies the schema of consumer resource and moves most parameters at route/service level.(except key_id and secret_key)
  • Use @Request-target as mentioned in RFC in signing string.

@Request-target definition comes from: https://datatracker.ietf.org/doc/html/draft-ietf-httpbis-message-signatures-03#name-request-target
Signing string generation logic: https://datatracker.ietf.org/doc/html/draft-cavage-http-signatures-00#section-2.1.2

@dosubot dosubot bot added size:XXL This PR changes 1000+ lines, ignoring generated files. plugin labels Sep 17, 2024
@membphis
Copy link
Member

bad title, we can not use backport in this PR

pls add more description about why we need this PR?

@Revolyssup Revolyssup changed the title feat: backport hmac-auth plugin refactor feat: rewrite hmac-auth plugin to match rfc Sep 18, 2024
@Revolyssup Revolyssup changed the title feat: rewrite hmac-auth plugin to match rfc feat: rewrite hmac-auth plugin for usability Sep 18, 2024
@Revolyssup
Copy link
Contributor Author

bad title, we can not use backport in this PR

pls add more description about why we need this PR?

done

moonming
moonming previously approved these changes Sep 18, 2024
nic-6443
nic-6443 previously approved these changes Sep 18, 2024
membphis
membphis previously approved these changes Sep 18, 2024
Copy link
Member

@membphis membphis left a comment

Choose a reason for hiding this comment

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

LGTM, except some minor issues

apisix/plugins/hmac-auth.lua Show resolved Hide resolved
date = auth_data[5]
signed_headers = auth_data[6]
end
if not auth_string:match("^Signature") then
Copy link
Member

Choose a reason for hiding this comment

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

a high performance way: core.string.has_prefix

Copy link
Member

Choose a reason for hiding this comment

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

if not validated_consumer then
core.log.warn("client request can't be validated: ", err or "Invalid signature")
return 401, {message = "client request can't be validated"}
end

local consumer_conf = consumer.plugin(plugin_name)
consumer.attach_consumer(ctx, validated_consumer, consumer_conf)
core.log.info("hit hmac-auth rewrite")
Copy link
Member

Choose a reason for hiding this comment

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

we do not need to remove it

Copy link
Member

@membphis membphis left a comment

Choose a reason for hiding this comment

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

LGTM

@Revolyssup Revolyssup merged commit a393320 into apache:master Sep 19, 2024
31 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
plugin size:XXL This PR changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants