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.

Sign up for GitHub

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

Jump to bottom

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 •
edited
Loading

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.

AlexStocks
AlexStocks reviewed Jan 19, 2020
View reviewed changes
common/url.go Show resolved Hide resolved
filter/filter_impl/auth/authenticator.go Outdated Show resolved Hide resolved
flycash
flycash reviewed Jan 19, 2020
View reviewed changes
filter/filter_impl/auth/authenticator_test.go Outdated Show resolved Hide resolved
filter/filter_impl/auth/sign_util.go Outdated Show resolved Hide resolved
zouyx
zouyx reviewed Jan 19, 2020
View reviewed changes
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
hxmhlt
hxmhlt reviewed Jan 20, 2020
View reviewed changes
filter/auth_spi.go Outdated Show resolved Hide resolved
zouyx
zouyx reviewed Jan 20, 2020
View reviewed changes
filter/auth_spi.go Outdated Show resolved Hide resolved
zouyx
zouyx reviewed Jan 20, 2020
View reviewed changes
filter/auth_ext.go Outdated Show resolved Hide resolved
filter/filter_impl/auth/accesskey_storage.go Show resolved Hide resolved
hxmhlt
hxmhlt reviewed Jan 21, 2020
View reviewed changes
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
flycash
flycash reviewed Jan 21, 2020
View reviewed changes
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
@CodingSinger CodingSinger force-pushed the auth branch 4 times, most recently from 1c49488 to a1f28b0 Compare January 28, 2020 08:18
@CodingSinger CodingSinger force-pushed the auth branch 3 times, most recently from 789eff4 to c031cb0 Compare January 31, 2020 13:45
@zouyx
Copy link
Member

zouyx commented Feb 1, 2020

Could you add some comment for public struct and method?

zouyx
zouyx approved these changes Feb 4, 2020
View reviewed changes
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
hxmhlt
hxmhlt approved these changes Feb 7, 2020
View reviewed changes
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

flycash
flycash approved these changes Feb 7, 2020
View reviewed changes
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
@CodingSinger CodingSinger mentioned this pull request Mar 16, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AlexStocks AlexStocks AlexStocks left review comments

@hxmhlt hxmhlt hxmhlt approved these changes

@flycash flycash flycash approved these changes

@zouyx zouyx zouyx approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@CodingSinger @codecov-io @zouyx @AlexStocks @flycash @hxmhlt