Add verify_ssl option to gluon.utils.download #11546
Conversation
There was a problem hiding this comment.
Considering this is a security related feature, I'd prefer to have a unit test and a warning printed if that check is disabled. I'd like to emphasize that we as a project should never have any code served with this feature disabled. We should even think whether we want to open that box of Pandora. I think this needs a proper discussion before we employ that functionality.
|
@marcoabreu I agree that checking SSL certificates is important in general and should be (and is) the default. Still, when providing a utility function in gluon, we should let users judge and give them the option to disable SSL verification when necessary. For example, when SHA1 hashes of the to be downloaded files are hardcoded, the authenticity of the files can be verified based on the hash and little is lost by disabling SSL verification. Again, I do not advocate to disable SSL support in general. However, there may be cases (such as external files which we must not redistribute hosted on a misconfigured server) where it is useful to disable SSL certificate verification. Without this PR, the download utility function is useless in those cases. |
|
LGTM |
| with warnings.catch_warnings(record=True) as warnings_: | ||
| mx.gluon.utils.download( | ||
| "https://mxnet.incubator.apache.org/", verify_ssl=False) |
There was a problem hiding this comment.
I would mock the call to requests to avoid doing network IO on a unit test.
|
Is better to mock it, in python is super simple. Using localhost can also fail if the port is used, so best to avoid it. Let's not overcomplicate things. Mocking the request is easy: https://stackoverflow.com/questions/15753390/how-can-i-mock-requests-and-the-response |
|
Thanks. I have now changed all |
leezu
force-pushed
the
verifyssl
branch
2 times, most recently
from
0a55026
to
072dbf2
Compare
|
Where are test dependencies currently tracked? This PR would introduce In case there is no centralized place to declare test/development dependencies, would it make sense to track these in the |
|
AFAIK windows CI slave images are still manually built. @marcoabreu might have update |
|
We can workaround this by adding pip install mock after the activate call on the Jenkinsfile . parametrization of windows host is starting now and will take a while. Yes they are manually built. |
Sometimes datasets may be hosted on servers that serve invalid SSL certificates.
* Add verify_ssl option to gluon.utils.download Sometimes datasets may be hosted on servers that serve invalid SSL certificates. * Add warning * Add test * Mock gluon.utils.download tests * Add Py2 mock dependency to Jenkinsfile
Description
Sometimes datasets may be hosted on servers that serve invalid SSL certificates. If so, this allow disabling the SSL certificate verification. The default behavior is not changed.
Checklist
Essentials
Please feel free to remove inapplicable items for your PR.
Changes