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

support sign and auth for request #323

Merged
merged 10 commits into from
Feb 7, 2020
Merged

support sign and auth for request #323

merged 10 commits into from
Feb 7, 2020

Conversation

CodingSinger
Copy link
Member

enhance request security, support custom signing and verification.
refer apache/dubbo#5461

@CodingSinger
Copy link
Member Author

Please don't merge, I will add unit tests later.

@codecov-io
Copy link

codecov-io commented Jan 19, 2020

Codecov Report

Merging #323 into develop will decrease coverage by 0.22%.
The diff coverage is 74.82%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #323      +/-   ##
===========================================
- Coverage    66.88%   66.65%   -0.23%     
===========================================
  Files          123      128       +5     
  Lines         7597     7771     +174     
===========================================
+ Hits          5081     5180      +99     
- Misses        2024     2083      +59     
- Partials       492      508      +16
Impacted Files Coverage Δ
config/service_config.go 71.96% <100%> (+0.53%) ⬆️
filter/filter_impl/auth/accesskey_storage.go 100% <100%> (ø)
filter/filter_impl/auth/provider_auth.go 52.94% <52.94%> (ø)
filter/filter_impl/auth/consumer_sign.go 60% <60%> (ø)
filter/filter_impl/auth/sign_util.go 69.23% <69.23%> (ø)
filter/filter_impl/auth/default_authenticator.go 80% <80%> (ø)
common/url.go 70.71% <86.66%> (+2.08%) ⬆️
protocol/dubbo/pool.go 75.81% <0%> (-6.52%) ⬇️
protocol/dubbo/client.go 63.75% <0%> (-5%) ⬇️
protocol/dubbo/listener.go 56.75% <0%> (-4.83%) ⬇️
... and 9 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4de49ef...bd6ee75. Read the comment docs.

common/url.go Show resolved Hide resolved
filter/filter_impl/auth/authenticator.go Outdated Show resolved Hide resolved
filter/filter_impl/auth/authenticator_test.go Outdated Show resolved Hide resolved
filter/filter_impl/auth/sign_util.go Outdated Show resolved Hide resolved
filter/filter_impl/auth/accesskey_storage_test.go Outdated Show resolved Hide resolved
filter/filter_impl/auth/consumer_sign_test.go Outdated Show resolved Hide resolved
filter/filter_impl/auth/consumer_sign.go Outdated Show resolved Hide resolved
filter/filter_impl/auth/provider_auth.go Outdated Show resolved Hide resolved
filter/filter_impl/auth/provider_auth_test.go Outdated Show resolved Hide resolved
@CodingSinger CodingSinger changed the title [WIP] support sign and auth for request support sign and auth for request Jan 19, 2020
filter/auth_spi.go Outdated Show resolved Hide resolved
filter/auth_spi.go Outdated Show resolved Hide resolved
filter/auth_ext.go Outdated Show resolved Hide resolved
filter/filter_impl/auth/accesskey_storage.go Show resolved Hide resolved
Copy link
Contributor

@hxmhlt hxmhlt left a comment

Choose a reason for hiding this comment

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

I have two advise as follow:
1.Auth is not necessary so I suggest move all code in filter_impl folder.
2.It look that consumer get secret and access_key from url params and save them into dubbo attachmens. I wonder where these url params be set ?

common/constant/key.go Show resolved Hide resolved
common/url.go Show resolved Hide resolved
filter/auth_ext.go Outdated Show resolved Hide resolved
Copy link
Member

@flycash flycash left a comment

Choose a reason for hiding this comment

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

Maybe you could move auth_ext to auth package and then put AccessKeyPair and AccesskeyStorage in a file named access_key.go and Authenticator in another file named authenticator.go.

filter/auth_ext.go Outdated Show resolved Hide resolved
@zouyx
Copy link
Member

zouyx commented Feb 1, 2020

Could you add some comment for public struct and method?

Copy link
Member

@zouyx zouyx left a comment

Choose a reason for hiding this comment

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

LGTM

common/url.go Show resolved Hide resolved
Copy link
Contributor

@hxmhlt hxmhlt left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@flycash flycash left a comment

Choose a reason for hiding this comment

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

LGTM

@hxmhlt hxmhlt merged commit 9398a2e into apache:develop Feb 7, 2020
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.

6 participants