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

Check integrity of downloaded data files and avoid running dependent tests if a broken file is found. #41

Open
wants to merge 12 commits into
base: develop
Choose a base branch
from

Conversation

shinji-s
Copy link
Contributor

Associate each data file with a known md5 checksum and have the checksum compared with one generated from a downloaded file in order to verify the integrity of the file. Downloading tests will fail if they find a corrupt/incomplete file.
Declare downloading tests to be fixtures so that dependent tests are not run if downloading tests fail and so that the the downloading tests get run even if subset of dependent tests are run (e.g. via ctest -R {some_test})

This patch also concerns about security. Since data files are served over plain HTTP without TLS, a man in the middle can serve a tweaked data file which exploits vulnerabilities in eccodes to compromise security of computers running the tests. The integrity check will lower the chance of that kind of attack succeeding.

Declare downloading tests to be fixtures so that dependant tests will not be run if there is incomplete/corrupted file.
@shinji-s shinji-s changed the title Check integrity of downloaded data files and avoid running dependent tests if a broken file is found. WIP: Check integrity of downloaded data files and avoid running dependent tests if a broken file is found. Aug 18, 2020
@shinji-s
Copy link
Contributor Author

Ouch. Seems there is problem in comparing checksum on Windows.
Is the output of 'cmake -E md5 ...' different on Windows from one on Linux?

@shinji-s
Copy link
Contributor Author

Can anybody try ctests with this patch on Windows to see what is going on?

@shinji-s
Copy link
Contributor Author

I bit the bullet and setup build environment in ever decreasing free space on my Windows drive. eccodes_download_gribs failed at the same position as it does in AppVeyor check. The cause was the difference in line terminating chars. The template file ecbuild/cmake/md5.in from which .md5 are generated contains CRLF but files generated by 'cmake -E md5sum' contain LF. If that is what is really happening in AppVeyor check, possible solutions would be;

  • (A) Add 'NEWLINE_STYLE LF' to 'configure_file( "${ECBUILD_MACROS_DIR}/md5.in" ${_p_NAME}.md5 @only )' in ecbuild/cmake/ecbuild_get_test_data.cmake.
  • (B) Git-check out ecbuild to force termination with LF or tweak the template file after check out.

@shinji-s
Copy link
Contributor Author

Solution B does not work because configure_file() forces platform specific line ending chars in the output if no NEWLINE_STYLE is specified.

@shinji-s
Copy link
Contributor Author

I think this PR is ready to be examined and subsequently be merged. It would be a tough decision to make; whether to merge this PR now or wait until the patch on ecbuild be merged and be made be public.
Let me know if the commits in this PR needs to be squashed.

@shinji-s shinji-s changed the title WIP: Check integrity of downloaded data files and avoid running dependent tests if a broken file is found. Check integrity of downloaded data files and avoid running dependent tests if a broken file is found. Aug 19, 2020
@shinji-s
Copy link
Contributor Author

Ecbuild repository was updated with my proposed change and therefore now the appveyor test of this project succeeds without patching ecbuild.

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.

1 participant