-
Notifications
You must be signed in to change notification settings - Fork 2.1k
pkg/{c25519,driver_cryptocell_310}: use dlcache for fetching zip archives, update pkg/Makefile.http example
#21813
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
Conversation
mguetschow
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for tackling this! Always impressive to find unexpected tools in RIOT, but actually rather unimpressive to not find them documented anywhere.
I'd have expected documentation about it on https://doc.riot-os.org/group__pkg.html. I then found there is a https://github.com/RIOT-OS/RIOT/blob/master/pkg/Makefile.http which is supposed to serve as a template. We should at least mention it there - or even implement $(DOWNLOAD_TO_FILE) with $(DLCACHE) under the hood. What do you think?
Or we deprecate |
Do you have a proposal what I should add to that documentation? |
pkg/Makefile.http example
Which is basically the same, but |
No, I think it is fine if it is used in |
Done. I would also like to extend the |
mguetschow
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM now, please squash!
Oh sure, feel free to do so! |
4fe8c18 to
5054cfb
Compare
5054cfb to
c2cdbb5
Compare
pkg/c25519/Makefile
Outdated
| PKG_EXT = zip | ||
| PKG_LICENSE = PD | ||
| PKG_SHA512 = dbfb4285837ab2ea3d99c448b22877cc7a139ccbaebb1de367e2bec1fd562fe629b389d86603915448078b8fd7e631c8fc9a7d126eb889a1ba0c17611369b190 | ||
| PKG_MD5 = 2f19396f8becb44fe1cd5e40111e3ffb |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The collision resistance of MD5 is really really poor. Especially when the transport is HTTP, there would be a real security benefit if we would compare the downloaded file against a SHA512 or similar.
In addition to that: I think md5sum is less likely to be part of a default installation than sha512sum these days.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, that only leaves one issue: I can't calculate the new checksum of the binary linked in mcuboot, because the link is dead:
Lines 10 to 11 in 5ff98db
| MCUBOOT_BIN_URL ?= http://download.riot-os.org/mynewt.mcuboot.bin | |
| MCUBOOT_BIN_MD5 ?= 0c71a0589bd3709fc2d90f07a0035ce7 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This wouldn't hold up this PR, because the dlcache script will always (try to) redownload if no checksum is given. The fact that there is nothing to download is something else 😅
c2cdbb5 to
1cb09eb
Compare
6764a30 to
54c0738
Compare
The dlcache script is useful to avoid unnecessary repeated downloads of zip archives, similar to the git-cache scripts. The command order was changed to stay compatible with DOWNLOAD_TO_FILE and the only other usage of $(DLCACHE) was changed accordingly. To avoid the risk of collisions, dlcache now uses SHA512 sums instead of MD5. If no SHA512 checksum is given to dlcache.sh, it will just act as $(DOWNLOAD_TO_FILE).
54c0738 to
a30ce22
Compare
|
Thx for spotting and addressing the issue ❤️ Our CI really has been a naughty before with causing so much pointless traffic 😅 |

Contribution description
In the past we've been causing A LOT of traffic on the Nordic website by continually fetching the nRF5_SDK for every build cycle where the Cryptocell driver is built. After they kicked us out (although I don't know if that's actually because of us), we changed to a community hosted server in #21635.
This still causes a lot of unnecessary traffic and
skyleafis ~160GB rx traffic each month for the CI.I accidentally discovered that we have a script for caching http(s) downloads, which is only used by one package so far.
Initially I wanted to create a new
pkg/pkg_http.mkfor that purpose, but that became too frustrating tbh.Edit: Further changes:
dlcachescript is now used in place of the old selection ofwgetandcurlin the$(RIOTBASE)/Makefile.include.dlcachescript now uses SHA512 instead of MD5.dlcachewas changed to be backwards compatible with$(DOWNLOAD_TO_FILE).Testing procedure
I'm not sure how to test it in the CI, but locally you have to do the following steps:
On the second build, it doesn't fetch anything as expected:
If we delete the files and folders in
build/we can observe howdlcachetakes the file from cache:The same test procedure applies to
pkg/c25519with thetests/pkg/c25519test.Issues/PRs references
Depends on RIOT-OS/riotdocker#262 .merged.