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

Add MD5 checksum integrity check #5790

Merged
merged 5 commits into from
Mar 9, 2022
Merged

Conversation

yanchenm
Copy link
Contributor

@yanchenm yanchenm commented Mar 8, 2022

Ticket(s) Closed

  • Closes #

Description

Deals with https://sentry.io/organizations/whist/issues/2963782777/events/?project=5461239&referrer=slack. Error given in Sentry is that the salt header is missing (an indicator of a malformed config file). However, upon manual inspection of all the possible candidate files for both occurrences of the error, the salt header was present in all of them. I suspect then that the cause could be because of corrupted downloads which we do not currently check for.

This PR adds MD5 checksum validation to the config download process and allows up to 3 retries on checksum mismatches.

Implementation

Adds an md5 key to the object metadata in S3.

Documentation & Tests Added

Testing Instructions

  • Test that configs still persist correctly
  • Check that S3 object has an md5 tag

PR Checklist

  • Did the PR author fully test this PR end-to-end?
  • Did one PR reviewer fully test this PR end-to-end?
  • Did one PR reviewer conduct a thorough code design review?

@github-actions github-actions bot added 📁 Repo: services This PR/Issue modifies /services code 📁 Repo: backend This PR/Issue modifies /backend code labels Mar 8, 2022
@yanchenm yanchenm force-pushed the yanchen/config-integrity branch from 1b04dbc to 2a8918b Compare March 8, 2022 16:41
djsavvy
djsavvy previously approved these changes Mar 8, 2022
Copy link
Contributor

@djsavvy djsavvy left a comment

Choose a reason for hiding this comment

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

Great catch @yanchenm. Hopefully this sheds some light onto what's going on.

Code looks great as always.

@codecov
Copy link

codecov bot commented Mar 8, 2022

Codecov Report

Merging #5790 (f4163e4) into dev (a0bf931) will decrease coverage by 0.09%.
The diff coverage is 64.28%.

@@            Coverage Diff             @@
##              dev    #5790      +/-   ##
==========================================
- Coverage   54.22%   54.12%   -0.10%     
==========================================
  Files         190      190              
  Lines       24752    24696      -56     
  Branches       31       31              
==========================================
- Hits        13422    13367      -55     
- Misses      11116    11117       +1     
+ Partials      214      212       -2     
Flag Coverage Δ *Carryforward flag
backend-services 51.10% <64.28%> (-0.77%) ⬇️
backend-webserver 88.15% <ø> (ø) Carriedforward from adf09ab
frontend-core-ts 69.13% <ø> (ø) Carriedforward from adf09ab
protocol 51.81% <ø> (ø) Carriedforward from adf09ab

*This pull request uses carry forward flags. Click here to find out more.

Impacted Files Coverage Δ
...ices/host-service/mandelbox/configutils/storage.go 0.00% <0.00%> (ø)
...nd/services/host-service/mandelbox/user_configs.go 58.88% <81.81%> (+1.99%) ⬆️
backend/services/host-service/httpserver.go 80.10% <0.00%> (-2.05%) ⬇️
backend/services/host-service/host-service.go 41.30% <0.00%> (ø)

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 a0bf931...f4163e4. Read the comment docs.

@MauAraujo
Copy link

MauAraujo commented Mar 8, 2022

I tested this PR and got this warning while connecting the client app to my dev instance:

WARN Response has no supported checksum. Not validating response payload.

What causes the checksum to not be supported? Does this mean that, in this case, there is still a possibility for a mismatch that will not be detected?

Aside from that, the code is great and everything else works nice!

@yanchenm
Copy link
Contributor Author

yanchenm commented Mar 8, 2022

Yeah I saw this warning too and it's from the AWS SDK not our code. I'll look into what it means and why it happens

@philippemnoel
Copy link
Owner

Yeah I saw this warning too and it's from the AWS SDK not our code. I'll look into what it means and why it happens

according to: aws/aws-sdk-go-v2#1606 it's just an issue in v 1.25 which we are using, which has apparently been fixed. If we upgrade our go-sdk this should be fixed?

@yanchenm
Copy link
Contributor Author

yanchenm commented Mar 9, 2022

according to: aws/aws-sdk-go-v2#1606 it's just an issue in v 1.25 which we are using, which has apparently been fixed. If we upgrade our go-sdk this should be fixed?

Good catch, I'll try that

philippemnoel
philippemnoel previously approved these changes Mar 9, 2022
@philippemnoel philippemnoel merged commit efd1f06 into dev Mar 9, 2022
@philippemnoel philippemnoel deleted the yanchen/config-integrity branch March 9, 2022 19:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📁 Repo: backend This PR/Issue modifies /backend code 📁 Repo: services This PR/Issue modifies /services code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants