Skip to content
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

coreos-ostree-importer: switch to plain http(s) url #66

Merged
merged 3 commits into from
Dec 20, 2019

Conversation

dustymabe
Copy link
Member

We'll send the messages with a plain http(s) url so that this importer
doesn't need to be configured with AWS credentials.

We'll send the messages with a plain http(s) url so that this importer
doesn't need to be configured with AWS credentials.
@dustymabe
Copy link
Member Author

Also, switch to Fedora 31

@lucab
Copy link

lucab commented Dec 18, 2019

@dustymabe can we use https://builds.coreos.fedoraproject.org/ as the base URL perhaps?

@dustymabe
Copy link
Member Author

@dustymabe can we use https://builds.coreos.fedoraproject.org/ as the base URL perhaps?

yep, added a commit on top that updates the example fedora message.

Copy link
Member

@jlebon jlebon left a comment

Choose a reason for hiding this comment

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

Just one comment, otherwise LGTM!

s3.download_file(bucket, key, filepath)
# Grab file from the url
logger.info(f"Downloading object from url: {url}")
urllib.request.urlretrieve(url, filepath)
Copy link
Member

Choose a reason for hiding this comment

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

Just a side note, but we should also download the signature and verify it before importing. E.g. we can add a commit-sig to the message?

Copy link
Member Author

Choose a reason for hiding this comment

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

We already check the sha256sum. The only thing I can think that the signature file would help us with is to make sure only signed content got into the repo (i.e. if we're worried about an attacker trying to send a message to import invalid content), but I can think of even better ways to do that too. One of them would be to verify the commit in the ostree repo after the file was untarred, but before it was imported.

Copy link
Member

Choose a reason for hiding this comment

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

One of them would be to verify the commit in the ostree repo after the file was untarred, but before it was imported.

Hmm, I thought that was already the case. We definitely should do this too if not. Ahh OK yup, we need to explicitly do --gpgverify.

We already check the sha256sum. The only thing I can think that the signature file would help us with is to make sure only signed content got into the repo (i.e. if we're worried about an attacker trying to send a message to import invalid content)

One argument for requiring a signature on the tarball itself is to sanity check that it's not malicious before trying to extract it. Actually, even the docs for TarFile.extractall() has a warning related to this.

So I think we should do both.

Copy link
Member Author

Choose a reason for hiding this comment

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

so:

  • remove checksum and apply signature checking for the tarball
  • add commit checking in extracted ostree repo

?

Copy link
Member Author

Choose a reason for hiding this comment

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

pushing that bit to a followup: #68

@dustymabe dustymabe merged commit 0390c45 into coreos:master Dec 20, 2019
@dustymabe dustymabe deleted the dusty branch December 20, 2019 04:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants