eccodes icon indicating copy to clipboard operation
eccodes copied to clipboard

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

Open shinji-s opened this issue 3 years ago • 6 comments

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.

shinji-s avatar Aug 18 '20 04:08 shinji-s

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 avatar Aug 18 '20 04:08 shinji-s

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

shinji-s avatar Aug 18 '20 07:08 shinji-s

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 avatar Aug 18 '20 22:08 shinji-s

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 avatar Aug 19 '20 14:08 shinji-s

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 avatar Aug 19 '20 16:08 shinji-s

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

shinji-s avatar Nov 11 '20 01:11 shinji-s