Improve creating SSL context for download (IDFGH-8705) #10147
Conversation
josesimoes
commented
Nov 9, 2022
josesimoes
commented
- This improves the SLL context creation for downloading files. It states the purpose for the context and loads the "internal" DigiCert certificate.
- Definitive fix for ASN1 tag error when downloading components (IDFGH-8120) #9618.
- With this change I've managed to successfully install v4.4.2.
tools/idf_tools.py
Outdated
| @@ -432,8 +432,7 @@ def download(url, destination): # type: (str, str) -> Optional[Exception] | |||
| # This works around the issue with outdated certificate stores in some installations. | |||
| if 'dl.espressif.com' in url or 'github.com' in url: | |||
| try: | |||
| ctx = ssl.create_default_context() | |||
| ctx.load_verify_locations(cadata=DIGICERT_ROOT_CERT) | |||
| ctx = ssl.create_default_context(purpose=Purpose.SERVER_AUTH, cadata=DIGICERT_ROOT_CERT) | |||
There was a problem hiding this comment.
I think we should also call ssl.load_default_certs in this case, otherwise we will only load the DIGICERT_ROOT_CERT.
This is necessary since IDF_MIRROR_PREFIX_MAP and IDF_GITHUB_ASSETS environment variables can be used to redirect the download URLs to a mirror, which may be using a different certificate than the one github.com or dl.espressif.com are using.
There was a problem hiding this comment.
Does purpose=Purpose.SERVER_AUTH make any difference? It is the default argument.
There was a problem hiding this comment.
I'm not a Python expert so I can't comment further... 😅
I do know that this change made the ANSI TAG error go away.
Maybe that's because of the cdata being passed here instead of load_verify_locations...
Feel free to test and change!
There was a problem hiding this comment.
Yes, most likely this has fixed your issue for the following reasons:
- When
cadatais passed tocreate_default_context, only that certificate is loaded into the context. Certificates provided by the OS are not loaded. - In your installation, python is unable to load one of the certificates from the OS certificate store.
Basically, by not loading the OS certificate store you are avoiding the error. However as I mentioned above, we still need the OS certificate store, since the download URLs may be redirected to a mirror which can use an arbitrary certificate.
There was a problem hiding this comment.
I guess that means that we cannot accept this PR. I have no suggest how to "fix" it.
tools/idf_tools.py
Outdated
| ctx = ssl.create_default_context() | ||
| ctx.load_verify_locations(cadata=DIGICERT_ROOT_CERT) | ||
| ctx = ssl.create_default_context(purpose=Purpose.SERVER_AUTH, cadata=DIGICERT_ROOT_CERT) | ||
| ssl.load_default_certs() | ||
| except AttributeError: | ||
| # no ssl.create_default_context or load_verify_locations cadata argument |
There was a problem hiding this comment.
Since these comments are for the changed calls, please remove them together with the try-except.
Also please squash the commits and keep only one commit.
following code review remove try catch
|
Please also use https://docs.espressif.com/projects/esp-idf/en/latest/esp32/contribute/install-pre-commit-hook.html in order to solve all the issues and comply with our coding style. Thank you for your contribution! |
github-actions
bot
changed the title
espressif-bot
added
Resolution: Won't Do
