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

app/deploy: Add --skip-branch-check #2819

Merged
merged 1 commit into from
May 11, 2021

Conversation

jlebon
Copy link
Member

@jlebon jlebon commented May 7, 2021

In Fedora CoreOS, updates are driven by Zincati and we thus completely
trust the information it gives us. The branch validation rpm-ostree does
is thus not necessary. It's also harmful in the case where the node is
extremely out of date because it may not be able to GPG verify the
commit at the tip of the branch (because the GPG key isn't yet in the
tree).

See: coreos/fedora-coreos-tracker#749

In Fedora CoreOS, updates are driven by Zincati and we thus completely
trust the information it gives us. The branch validation rpm-ostree does
is thus not necessary. It's also harmful in the case where the node is
extremely out of date because it may not be able to GPG verify the
commit at the tip of the branch (because the GPG key isn't yet in the
tree).

See: coreos/fedora-coreos-tracker#749
@jlebon jlebon force-pushed the pr/skip-branch-validation branch from 5b9c54b to 7c1072f Compare May 7, 2021 21:45
@jlebon jlebon changed the title app/deploy: add --skip-branch-check app/deploy: Add --skip-branch-check May 7, 2021
@lucab
Copy link
Contributor

lucab commented May 10, 2021

Cleared some doubts I had out-of-band, LGTM, but leaving some room if anybody else wants to review.

Copy link
Member

@cgwalters cgwalters left a comment

Choose a reason for hiding this comment

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

I'm OK with this. However: zincati metadata is only protected via standard TLS. Basically we're weakening our story around branch hijack attacks to TLS, and not TLS + GPG.

It'd help a lot for us to add an explicit offline signing to the Cincinnati metadata too.

@jlebon jlebon merged commit 5d79fbf into coreos:master May 11, 2021
@jlebon jlebon deleted the pr/skip-branch-validation branch May 11, 2021 13:18
@jlebon
Copy link
Member Author

jlebon commented May 11, 2021

Basically we're weakening our story around branch hijack attacks to TLS, and not TLS + GPG.

The thing is, branches are not actually super valuable for FCOS. Ultimately, we have to trust the commit hash that Zincati feeds us, because it's the update driver. Zincati isn't OSTree branch-aware and in the end the branches are more of an implementation detail. If we had coreos/zincati#498 for example, one could imagine actually doing the same thing the MCO does and pinning to commits (and then e.g. rpm-ostree upgrade would still work). Not saying we should do that, but we wouldn't be losing very much.

Note also that coreos/fedora-coreos-tracker#749 is proposing to have Zincati verify that the target commit is on the same stream, which ultimately is what we care most about from the value rpm-ostree's branch checking provided us.

It'd help a lot for us to add an explicit offline signing to the Cincinnati metadata too.

Yeah, agreed. Will file a tracker issue for this (since I can't find one already filed for it) to make sure it doesn't get lost. OK, filed coreos/fedora-coreos-tracker#826.

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