-
Notifications
You must be signed in to change notification settings - Fork 229
[WIP] feat: implement brute force protection for public shares #11864
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
Draft
jvillafanez
wants to merge
5
commits into
master
Choose a base branch
from
brute_force_protection_link
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Draft
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
4ed6545
feat: implement brute force protection for public shares
jvillafanez 83e70ef
chore: adjust config description and add changelog entry
jvillafanez 8370cfa
chore: add readme explaining the feature
jvillafanez 27d7cfa
chore: adjust formatting for codacy
jvillafanez 5fee98c
feat: propagate GRPC metadata across go-micro services
jvillafanez File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
15 changes: 15 additions & 0 deletions
15
changelog/unreleased/enhancement-brute-force-protection-links.md
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,15 @@ | ||
| Enhancement: Implement brute force protection for public links | ||
|
|
||
| Public links will be protected by default, allowing up to 5 wrong password | ||
| attempts per hour. If such rate is exceeded, the link will be blocked for | ||
| all the users until the failure rate goes below the configured threshold | ||
| (5 failures per hour by default, as said). | ||
|
|
||
| The failure rate is configurable, so it can be 10 failures each 2 hours | ||
| or 3 failures per minute. | ||
|
|
||
| Note that the protection will apply per service replica, so one replica | ||
| might be blocked while another replica is fully functional. | ||
|
|
||
| https://github.com/owncloud/ocis/pull/11864 | ||
| https://github.com/owncloud/reva/pull/460 | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,66 @@ | ||
| package metadata | ||
|
|
||
| import ( | ||
| "context" | ||
| "strings" | ||
|
|
||
| "github.com/owncloud/reva/v2/pkg/rgrpc" | ||
| "go-micro.dev/v4/server" | ||
| "google.golang.org/grpc/metadata" | ||
| ) | ||
|
|
||
| const ( | ||
| // Prefix used to auto propagate GRPC metadata keys across servers. | ||
| // This is used in the NewHandlerWrapper and NewSubscriberWrapper, and | ||
| // it's expected to work in go-micro services. | ||
| // It needs to match the prefix used in reva for the same purpose. | ||
| AutoPropPrefix = rgrpc.AutoPropPrefix | ||
| ) | ||
|
|
||
| // NewHandlerWrapper propagates the grpc metadata. | ||
| func NewHandlerWrapper() server.HandlerWrapper { | ||
| return func(h server.HandlerFunc) server.HandlerFunc { | ||
| return func(ctx context.Context, req server.Request, rsp interface{}) error { | ||
| md, ok := metadata.FromIncomingContext(ctx) | ||
| if !ok { | ||
| md = metadata.MD{} | ||
| } | ||
|
|
||
| pairs := make([]string, 0, md.Len()*2) | ||
| for key, values := range md { | ||
| if strings.HasPrefix(key, AutoPropPrefix) { | ||
| for _, value := range values { | ||
| pairs = append(pairs, key, value) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| newctx := metadata.AppendToOutgoingContext(ctx, pairs...) | ||
| return h(newctx, req, rsp) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // NewSubscriberWrapper propagates the grpc metadata | ||
| func NewSubscriberWrapper() server.SubscriberWrapper { | ||
| return func(next server.SubscriberFunc) server.SubscriberFunc { | ||
| return func(ctx context.Context, msg server.Message) error { | ||
| md, ok := metadata.FromIncomingContext(ctx) | ||
| if !ok { | ||
| md = metadata.MD{} | ||
| } | ||
|
|
||
| pairs := make([]string, 0, md.Len()*2) | ||
| for key, values := range md { | ||
| if strings.HasPrefix(key, AutoPropPrefix) { | ||
| for _, value := range values { | ||
| pairs = append(pairs, key, value) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| newctx := metadata.AppendToOutgoingContext(ctx, pairs...) | ||
| return next(newctx, msg) | ||
| } | ||
| } | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,29 @@ | ||
| # storage-publiclink | ||
|
|
||
| ## Brute Force Protection | ||
|
|
||
| The brute force protection will prevent access to public links if wrong | ||
| passwords are entered. The implementation is very similar to a rate limiter, | ||
| but taking into account only wrong password attempts. | ||
|
|
||
| By default, you're allowed a maximum of 5 failed attempts | ||
| (`STORAGE_PUBLICLINK_BRUTEFORCE_MAXATTEMPTS=5`) in 1 hour | ||
| (`STORAGE_PUBLICLINK_BRUTEFORCE_TIMEGAP=1h`). You can adjust those values to | ||
| your liking in order to define the failure rate threshold (5 failures per | ||
| hour, by default). | ||
|
|
||
| If the failure rate threshold is exceeded, the public link will be blocked | ||
| until such rate goes below the threshold. This means that it will remain | ||
| blocked for an undefined time: a couple of seconds in the best case, or up to | ||
| `STORAGE_PUBLICLINK_BRUTEFORCE_TIME` in the worst case. | ||
|
|
||
| If the public link is blocked by the brute force protection, it will be | ||
| blocked for all the users. | ||
|
|
||
| In case of multiple service replicas, the brute force protection won't share | ||
| any data among the replicas and the failure rate will apply per replica. This | ||
| means that a replica might be blocked due to high failure rate while the rest | ||
| work fine. | ||
|
|
||
| As said, this feature is enabled by default, with a 5 failures per hour rate. | ||
| If you want to disable this feature, set the related configuration values to 0. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have questions on the behavior:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Public links are intended for external anonymous access, and the security team decided that trying to block access by IP could be bypassed, so the access will be blocked for everyone, including regular logged-in users.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the insight 😄
Addon question:
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Depends. I would expect ongoing Office session to not break. But I would expect the user cannot upload/download the file from this point. @jvillafanez is this the actual behaviour?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really? First you successfully login, then you cant do anything without being notified?
As logged in user, I would expect at least a notification on the screen. Then it would be ok.
Would this also affect any running up/download? Means, they will get interrupted and the session closed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No login involved - this is about public links.
Ideally yes - but from a security POV not needed.
I would expect not. Once the download has started it uses a different sort of access token that is not bound to the user. So download should succeed. Not sure about Upload. Needs to be tested.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll need to double-check, but I think the office session will work normally. Pretty sure that almost all the operations will target the office server, and when the office server needs to contact oCIS it will use the provided reva access token, which should still be valid (unless it's expired). There shouldn't be any public share authentication needed while in the office app.
I haven't tested all cases, but taking into account that there are no related changes in web, the behavior seems to be fine: you can edit the file (assuming enough permissions), upload the file correctly to the server with the changes, BUT you get a "resource not found" error when you exit the editor.