Repository: scikit-learn/scikit-learn
Pull request: ENH Verify md5-checksums received from openml arff file metadata
<!--
Thanks for contributing a pull request! Please ensure you have taken a look at
the contribution guidelines: https://github.com/scikit-learn/scikit-learn/blob/master/CONTRIBUTING.md#pull-request-checklist
-->

#### Reference Issues/PRs
<!--
Example: Fixes #1234. See also #3456.
Please use keywords (e.g., Fixes) to create link to the issues or pull requests
you resolved, so that they will automatically be closed when your pull request
is merged. See https://github.com/blog/1506-closing-issues-via-pull-requests
-->
Fixes #11816 


#### What does this implement/fix? Explain your changes.

When fetching an open-ml dataset, metadata about the dataset is fetched via `https://openml.org/api/v1/json/data/`, which includes the latest file version to download, its md5-checksum, etc.

This PR adds functionality to verify the md5-checksum of the file downloaded to the one provided via the api. If the validation fails, it produces a `ValueError` stating the same.

#### Any other comments?
- Validation is done by default unless explicitly overriden
- Most files in the local tests `sklearn.datasets.tests.data.openml` folder do not match their checksums. This may be because of differences between versions of metadata vs actual file downloaded.

<!--
Please be aware that we are a loose team of volunteers so patience is
necessary; assistance handling other issues is very welcome. We value
all user contributions, no matter how minor they are. If we are slow to
review, either the pull request needs some benchmarking, tinkering,
convincing, etc. or more likely the reviewers are simply busy. In either
case, we ask for your understanding during the review process.
For more information, see our FAQ on this topic:
http://scikit-learn.org/dev/faq.html#why-is-my-pull-request-not-getting-any-attention.

Thanks for contributing!
-->


Problem statement:
Validate MD5 checksum of downloaded ARFF in fetch_openml
`fetch_openml` downloads the `data_set_description` from openml.org which includes an `md5_checksum` field. This should correspond to the MD5 checksum of the downloaded ARFF data. We should (optionally?) use `hashlib` to verify that the MD5 matches.

Hints from issue discussion:
I'll give this a go.
thanks. you may need to hack the md5s currently in
sklearn/datasets/tests/data/openml to match the truncated ARFF data there

Picking this to work on while in NYC WiMLDS Sprint Aug 24, 2019
I think this check should be optional. What do other core developers think? Forcing the verification ensures security against man in middle mocking but but adds some processing overhead. Should users pay this overhead by default?

Also, forcing the check as default might make genuine mocking for tests difficult too
I think having it true by default seems good but I'm fine with being able to disable it with an option.
I've written a hackish, not finished MR to give this a second try, see https://github.com/scikit-learn/scikit-learn/pull/15238. Comments welcome, but I won't have the time to work much on this over the next immediate days so don't expect this to be finished, shortly.

Commit message: Test should not modify local test-suite shared file

Patch to review:
diff --git a/sklearn/datasets/_openml.py b/sklearn/datasets/_openml.py
index 112cd9c0e525e..454df78ab4646 100644
--- a/sklearn/datasets/_openml.py
+++ b/sklearn/datasets/_openml.py
@@ -2,6 +2,7 @@
 import json
 import os
 import shutil
+import hashlib
 from os.path import join
 from warnings import warn
 from contextlib import closing
@@ -453,24 +454,44 @@ def _get_num_samples(data_qualities):
 
 
 def _load_arff_response(url, data_home, return_type, encode_nominal,
-                        parse_arff):
+                        parse_arff, md5_checksum):
     """Load arff data with url and parses arff response with parse_arff"""
     response = _open_openml_url(url, data_home)
 
     with closing(response):
         # Note that if the data is dense, no reading is done until the data
         # generator is iterated.
-        arff = _arff.load((line.decode('utf-8') for line in response),
+        actual_md5_checksum = hashlib.md5()
+
+        def _stream_checksum_generator(response):
+            for line in response:
+                actual_md5_checksum.update(line)
+                yield line.decode('utf-8')
+
+        stream = _stream_checksum_generator(response)
+
+        arff = _arff.load(stream,
                           return_type=return_type,
                           encode_nominal=encode_nominal)
-        return parse_arff(arff)
 
+        parsed_arff = parse_arff(arff)
 
-def _download_data_to_bunch(url, sparse, data_home, *,
-                            as_frame, features_list, data_columns,
-                            target_columns, shape):
-    """Download OpenML ARFF and convert to Bunch of data
-    """
+        # consume remaining stream, if early exited
+        for _ in stream:
+            pass
+
+        if actual_md5_checksum.hexdigest() != md5_checksum:
+            raise ValueError("md5 checksum of local file for " + url +
+                             " does not match description. "
+                             "Downloaded file could have been modified / "
+                             "corrupted, clean cache and retry...")
+
+        return parsed_arff
+
+
+def _download_data_to_bunch(url, sparse, data_home, *, as_frame, features_list,
+                            data_columns, target_columns, shape, md5_checksum):
+    """Download OpenML ARFF and convert to Bunch of data"""
     # NB: this function is long in order to handle retry for any failure
     #     during the streaming parse of the ARFF.
 
@@ -558,7 +579,8 @@ def postprocess(X, y, nominal_attributes):  # type:ignore
         _load_arff_response)(url, data_home,
                              return_type=return_type,
                              encode_nominal=not as_frame,
-                             parse_arff=parse_arff)
+                             parse_arff=parse_arff,
+                             md5_checksum=md5_checksum)
     X, y, frame, nominal_attributes = postprocess(*out)
 
     return Bunch(data=X, target=y, frame=frame,
@@ -817,7 +839,9 @@ def fetch_openml(name=None, *, version='active', data_id=None, data_home=None,
                                     as_frame=as_frame,
                                     features_list=features_list, shape=shape,
                                     target_columns=target_columns,
-                                    data_columns=data_columns)
+                                    data_columns=data_columns,
+                                    md5_checksum=data_description[
+                                        "md5_checksum"])
 
     if return_X_y:
         return bunch.data, bunch.target
diff --git a/sklearn/datasets/tests/data/openml/1/api-v1-json-data-1.json.gz b/sklearn/datasets/tests/data/openml/1/api-v1-json-data-1.json.gz
index f75912bf2def7..ba544db491637 100644
Binary files a/sklearn/datasets/tests/data/openml/1/api-v1-json-data-1.json.gz and b/sklearn/datasets/tests/data/openml/1/api-v1-json-data-1.json.gz differ
diff --git a/sklearn/datasets/tests/data/openml/1119/api-v1-json-data-1119.json.gz b/sklearn/datasets/tests/data/openml/1119/api-v1-json-data-1119.json.gz
index 97ab0d1ce64f6..286e57672339a 100644
Binary files a/sklearn/datasets/tests/data/openml/1119/api-v1-json-data-1119.json.gz and b/sklearn/datasets/tests/data/openml/1119/api-v1-json-data-1119.json.gz differ
diff --git a/sklearn/datasets/tests/data/openml/2/api-v1-json-data-2.json.gz b/sklearn/datasets/tests/data/openml/2/api-v1-json-data-2.json.gz
index 22dfb6ff61c1b..921326973b212 100644
Binary files a/sklearn/datasets/tests/data/openml/2/api-v1-json-data-2.json.gz and b/sklearn/datasets/tests/data/openml/2/api-v1-json-data-2.json.gz differ
diff --git a/sklearn/datasets/tests/data/openml/292/api-v1-json-data-292.json.gz b/sklearn/datasets/tests/data/openml/292/api-v1-json-data-292.json.gz
index 888140f92b360..17caf8a8dcc7b 100644
Binary files a/sklearn/datasets/tests/data/openml/292/api-v1-json-data-292.json.gz and b/sklearn/datasets/tests/data/openml/292/api-v1-json-data-292.json.gz differ
diff --git a/sklearn/datasets/tests/data/openml/292/api-v1-json-data-40981.json.gz b/sklearn/datasets/tests/data/openml/292/api-v1-json-data-40981.json.gz
index 888140f92b360..9fd04cf4a73cb 100644
Binary files a/sklearn/datasets/tests/data/openml/292/api-v1-json-data-40981.json.gz and b/sklearn/datasets/tests/data/openml/292/api-v1-json-data-40981.json.gz differ
diff --git a/sklearn/datasets/tests/data/openml/40589/api-v1-json-data-40589.json.gz b/sklearn/datasets/tests/data/openml/40589/api-v1-json-data-40589.json.gz
index 9c71553ce5137..97089490a18c9 100644
Binary files a/sklearn/datasets/tests/data/openml/40589/api-v1-json-data-40589.json.gz and b/sklearn/datasets/tests/data/openml/40589/api-v1-json-data-40589.json.gz differ
diff --git a/sklearn/datasets/tests/data/openml/40675/api-v1-json-data-40675.json.gz b/sklearn/datasets/tests/data/openml/40675/api-v1-json-data-40675.json.gz
index 42b876f0a4723..6f21ade9d3eba 100644
Binary files a/sklearn/datasets/tests/data/openml/40675/api-v1-json-data-40675.json.gz and b/sklearn/datasets/tests/data/openml/40675/api-v1-json-data-40675.json.gz differ
diff --git a/sklearn/datasets/tests/data/openml/40966/api-v1-json-data-40966.json.gz b/sklearn/datasets/tests/data/openml/40966/api-v1-json-data-40966.json.gz
index 02b25d717f925..f1deb65592a9f 100644
Binary files a/sklearn/datasets/tests/data/openml/40966/api-v1-json-data-40966.json.gz and b/sklearn/datasets/tests/data/openml/40966/api-v1-json-data-40966.json.gz differ
diff --git a/sklearn/datasets/tests/test_openml.py b/sklearn/datasets/tests/test_openml.py
index 44fe392e42e74..64dafdded2661 100644
--- a/sklearn/datasets/tests/test_openml.py
+++ b/sklearn/datasets/tests/test_openml.py
@@ -146,6 +146,38 @@ def _fetch_dataset_from_openml(data_id, data_name, data_version,
     return data_by_id
 
 
+class MockHTTPResponse:
+    def __init__(self, data, is_gzip):
+        self.data = data
+        self.is_gzip = is_gzip
+
+    def read(self, amt=-1):
+        return self.data.read(amt)
+
+    def tell(self):
+        return self.data.tell()
+
+    def seek(self, pos, whence=0):
+        return self.data.seek(pos, whence)
+
+    def close(self):
+        self.data.close()
+
+    def info(self):
+        if self.is_gzip:
+            return {'Content-Encoding': 'gzip'}
+        return {}
+
+    def __iter__(self):
+        return iter(self.data)
+
+    def __enter__(self):
+        return self
+
+    def __exit__(self, exc_type, exc_val, exc_tb):
+        return False
+
+
 def _monkey_patch_webbased_functions(context,
                                      data_id,
                                      gzip_response):
@@ -160,37 +192,6 @@ def _monkey_patch_webbased_functions(context,
     path_suffix = '.gz'
     read_fn = gzip.open
 
-    class MockHTTPResponse:
-        def __init__(self, data, is_gzip):
-            self.data = data
-            self.is_gzip = is_gzip
-
-        def read(self, amt=-1):
-            return self.data.read(amt)
-
-        def tell(self):
-            return self.data.tell()
-
-        def seek(self, pos, whence=0):
-            return self.data.seek(pos, whence)
-
-        def close(self):
-            self.data.close()
-
-        def info(self):
-            if self.is_gzip:
-                return {'Content-Encoding': 'gzip'}
-            return {}
-
-        def __iter__(self):
-            return iter(self.data)
-
-        def __enter__(self):
-            return self
-
-        def __exit__(self, exc_type, exc_val, exc_tb):
-            return False
-
     def _file_name(url, suffix):
         return (re.sub(r'\W', '-', url[len("https://openml.org/"):])
                 + suffix + path_suffix)
@@ -1190,3 +1191,44 @@ def test_fetch_openml_with_ignored_feature(monkeypatch, gzip_response):
     # so we assert that we don't have the ignored feature in the final Bunch
     assert dataset['data'].shape == (101, 16)
     assert 'animal' not in dataset['feature_names']
+
+
+@pytest.mark.parametrize('as_frame', [True, False])
+def test_fetch_openml_verify_checksum(monkeypatch, as_frame, cache):
+    data_id = 2
+    _monkey_patch_webbased_functions(monkeypatch, data_id, True)
+
+    # create a modified (local) arff file
+    dataset_dir = os.path.join(currdir, 'data', 'openml', str(data_id))
+    original_data_path = os.path.join(dataset_dir,
+                                      'data-v1-download-1666876.arff.gz')
+    corrupt_copy = original_data_path + ".test_corrupt_arff"
+    with gzip.GzipFile(original_data_path, "rb") as orig_gzip, \
+         gzip.GzipFile(corrupt_copy, "wb") as modified_gzip:
+        data = bytearray(orig_gzip.read())
+        data[len(data)-1] = 37
+        modified_gzip.write(data)
+
+    # simulate request to return modified file
+    mocked_openml_url = sklearn.datasets._openml.urlopen
+
+    def swap_file_mock(request):
+        url = request.get_full_url()
+        if url.endswith('data/v1/download/1666876'):
+            return MockHTTPResponse(open(corrupt_copy, "rb"), is_gzip=True)
+        else:
+            return mocked_openml_url(request)
+
+    monkeypatch.setattr(sklearn.datasets._openml, 'urlopen', swap_file_mock)
+
+    # validate failed checksum
+    if as_frame:
+        pytest.importorskip('pandas')
+    with pytest.raises(ValueError) as exc:
+        sklearn.datasets.fetch_openml(data_id=data_id, cache=False,
+                                      as_frame=as_frame)
+    # exception message should have file-path
+    assert exc.match("1666876")
+
+    # cleanup fake local file
+    os.remove(corrupt_copy)


Code review comments:
diff_hunk: @@ -1190,3 +1191,44 @@ def test_fetch_openml_with_ignored_feature(monkeypatch, gzip_response):
     # so we assert that we don't have the ignored feature in the final Bunch
     assert dataset['data'].shape == (101, 16)
     assert 'animal' not in dataset['feature_names']
+
+
+@pytest.mark.parametrize('as_frame', [True, False])
+def test_fetch_openml_verify_checksum(monkeypatch, as_frame, cache):
+    data_id = 2
+    _monkey_patch_webbased_functions(monkeypatch, data_id, True)
+
+    # create a modified (local) arff file
+    dataset_dir = os.path.join(currdir, 'data', 'openml', str(data_id))
+    original_data_path = os.path.join(dataset_dir,
+                                      'data-v1-download-1666876.arff.gz')
+    corrupt_copy = original_data_path + ".test_corrupt_arff"
+    with gzip.GzipFile(original_data_path, "rb") as orig_gzip, \
+         gzip.GzipFile(corrupt_copy, "wb") as modified_gzip:
path: sklearn/datasets/tests/test_openml.py
text: @user1:
Flake8

```suggestion
    with gzip.GzipFile(original_data_path, "rb") as orig_gzip, \
            gzip.GzipFile(corrupt_copy, "wb") as modified_gzip:
```