Skip to content
This repository has been archived by the owner on May 21, 2024. It is now read-only.

get_garage_sign.py: Point at new AWS bucket. #1619

Merged
merged 3 commits into from
Mar 30, 2020

Conversation

pattivacek
Copy link
Collaborator

No description provided.

@lbonn
Copy link
Contributor

lbonn commented Mar 27, 2020

There is also a size field, we could at least check it. And because it goes through TLS it's maybe good enough for integrity (detects partial downloads).

Signed-off-by: Patrick Vacek <patrickvacek@gmail.com>
Signed-off-by: Patrick Vacek <patrickvacek@gmail.com>
@codecov-io
Copy link

codecov-io commented Mar 27, 2020

Codecov Report

Merging #1619 into master will increase coverage by 0.06%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1619      +/-   ##
==========================================
+ Coverage   82.49%   82.56%   +0.06%     
==========================================
  Files         189      189              
  Lines       12004    12004              
==========================================
+ Hits         9903     9911       +8     
+ Misses       2101     2093       -8
Impacted Files Coverage Δ
src/libaktualizr/storage/sqlstorage.cc 77.77% <0%> (+0.09%) ⬆️
src/libaktualizr/primary/sotauptaneclient.cc 90.68% <0%> (+0.11%) ⬆️
src/libaktualizr/storage/sqlstorage_base.cc 78.52% <0%> (+2.68%) ⬆️
src/libaktualizr/storage/sqlstorage_base.h 100% <0%> (+40%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 740da18...a95bf11. Read the comment docs.

md5sum offered no security, since it was read from the same place as the
archive, and since it is no longer available, use the size instead for a
modicum of integrity. You can still provide a sha256sum if desired.

Signed-off-by: Patrick Vacek <patrickvacek@gmail.com>
@pattivacek
Copy link
Collaborator Author

New bucket is ready, I think this can be safely merged now if we want.

Copy link
Contributor

@zabbal zabbal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes looks fine to me (N. B: have not tested if it actually works) compared to what was before: the md5 offers no security nowadays even if it was downloaded out-of-band.

I've got few questions however:

  • why hardcode it at all, why not add another command-line option with default value of hardcoded URL?
  • why bother with "LastModified" checks - can we ask to add fixed tuf-cli-latest.tgz which would always link to the newest tuf-cli-v1.2.3.tgz every time the new version is uploaded?

Also, the rename commit seems to be entirely orthogonal and can be sent as a separate PR, but that's just minor nitpicking from my side.

@zabbal
Copy link
Contributor

zabbal commented Mar 30, 2020

On a related note, is there a place where we document the URL? Some stable page like docs.ota.connect or smth like that which we can refer to in the help message so users can manually check and fix things (provided we expose hardcoded value as command line option) if things go south again?

@lbonn
Copy link
Contributor

lbonn commented Mar 30, 2020

@zabbal

Also, the rename commit seems to be entirely orthogonal and can be sent as a separate PR, but that's just minor nitpicking from my side.

We prefer not to open too many small PRs if not necessary. Typically, for a related trivial change like this one, having it in a separate commit is enough for our purpose. We would split if there were some big contentious changes and a smaller commit that is more urgent and would need to be fast-tracked for example.

In this case, having two PRs would also create unnecessary headaches with merge conflicts and rebasing.

@pattivacek
Copy link
Collaborator Author

why hardcode it at all, why not add another command-line option with default value of hardcoded URL?

It's an option, but I never considered it'd be necessary before. I'll consider that.

why bother with "LastModified" checks - can we ask to add fixed tuf-cli-latest.tgz which would always link to the newest tuf-cli-v1.2.3.tgz every time the new version is uploaded?

Yes, that could also be an option, but that's outside of aktualizr. :)

is there a place where we document the URL?

Nope.

Merging for now, happy to think about how to further future-proof this.

@pattivacek pattivacek merged commit a09561b into master Mar 30, 2020
@pattivacek pattivacek deleted the feat/tuf-cli-aws-bucket branch March 30, 2020 15:18
@pattivacek
Copy link
Collaborator Author

why hardcode it at all, why not add another command-line option with default value of hardcoded URL?

It's an option, but I never considered it'd be necessary before. I'll consider that.

@zabbal BTW not the same as what you suggested, but there's already support for downloading the archive yourself and providing the path to the script/CMake.

@zabbal
Copy link
Contributor

zabbal commented Mar 31, 2020

there's already support for downloading the archive yourself and providing the path to the script/CMake

Cool, where can I read more about it?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants