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

Validate WRP.Source for inbound messages #507

Merged
merged 6 commits into from
Aug 4, 2020
Merged

Conversation

joe94
Copy link
Member

@joe94 joe94 commented Jul 31, 2020

@joe94 joe94 marked this pull request as draft July 31, 2020 01:04
@joe94 joe94 self-assigned this Jul 31, 2020
@joe94 joe94 changed the title first draft Validate WRP.Source for inbound messages Jul 31, 2020
@codecov
Copy link

codecov bot commented Jul 31, 2020

Codecov Report

Merging #507 into main will increase coverage by 0.02%.
The diff coverage is 70.21%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #507      +/-   ##
==========================================
+ Coverage   86.58%   86.61%   +0.02%     
==========================================
  Files         186      186              
  Lines        8321     8360      +39     
==========================================
+ Hits         7205     7241      +36     
- Misses        912      913       +1     
- Partials      204      206       +2     
Impacted Files Coverage Δ
device/options.go 88.88% <55.55%> (-6.67%) ⬇️
device/manager.go 59.85% <69.69%> (+2.33%) ⬆️
device/metrics.go 100.00% <100.00%> (ø)
device/drain/drainer.go 96.96% <0.00%> (+1.51%) ⬆️
device/device.go 69.81% <0.00%> (+1.88%) ⬆️

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 054e02e...dcfe2de. Read the comment docs.

@JC000
Copy link

JC000 commented Jul 31, 2020

As long as it isn't some sort of coding error on our part, there shouldn't be a device identifier that is different between a message (simple event message or otherwise) source and the device identifier of the "connection" (connection context). The only possible exception is an empty source. I think it's OK to populate an empty source field, but a different source is an indication of something malicious. Log a security exception and drop the message on the floor.

@joe94 joe94 force-pushed the feature/wrpSourceValidation branch from 5beae25 to 2444d66 Compare August 4, 2020 20:22
@joe94 joe94 marked this pull request as ready for review August 4, 2020 20:24
@@ -27,6 +27,10 @@ const (
DefaultDeviceMessageQueueSize = 100
)

type wrpSourceCheckConfig struct {
Copy link
Member

Choose a reason for hiding this comment

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

why is this private?

Copy link
Member Author

Choose a reason for hiding this comment

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

We don't need it elsewhere outside the package. We do need the "Type" to be exported for viper to work properly.

@joe94 joe94 merged commit 25b0ffb into main Aug 4, 2020
@joe94 joe94 deleted the feature/wrpSourceValidation branch August 4, 2020 21:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants