Use the latest released Craft version for publishing releases

[rhcarvalho]

A more important issue is publish always using the latest master build of craft whereas action-prepare-release using the latest released version.

Originally posted by @BYK in #664 (comment)

[chadwhitacre]

This bit us in the 22.1.0 self-hosted release, because we didn't have the fix in getsentry/craft#335.

[jan-auer]

@chadwhitacre It's actually the other way around. This issue suggests to use released versions as opposed to the latest master branch build.

In the case of the 22.1.0 self-hosted release, action-prepare-release did exactly that and missed a merged but unreleased fix in craft.

[chadwhitacre]

Ah, good point. What do you think? Should we modify publish to use the latest release, or action-prepare-release to use the latest master build?

[jan-auer]

My thinking is to use latest master in both cases, yes.

The need for releases of craft originally arose when we were moving fast over in craft with insufficient CI. It simply broke repeatedly. The situation has improved, and there is no guarantee that if we require faster releases, that would also improve QA. In that sense, let's go with master commits and rather improve on CI / testing where necessary.

[chadwhitacre]

I started a PR: getsentry/action-prepare-release#22.

[chadwhitacre]

Dialing back out from getsentry/action-prepare-release#22 ...

Anyone with write access to the craft repo can already inject code into the publish repo (so actually we should limit who can push to master on craft to the same set of people that can manage publish)

Because of this, I'm having second thoughts about modifying action-prepare-release to use master vs. pinning a version in the publish repo. I don't think we want to lock down the craft repo, because it's (theoretically, at least?) not Sentry-specific, and anyways we have multiple Sentry engineers regularly working on it and cutting releases. If we need to introduce more friction, let's at least load up all the friction in the publish repo if we can. Right?

As a slight help, maybe action-prepare-release could pull a version number from the publish repo to avoid having to bump versions in two places?

[BYK]

Because of this, I'm having second thoughts about modifying action-prepare-release to use master vs. pinning a version in the publish repo. I don't think we want to lock down the craft repo, because it's (theoretically, at least?) not Sentry-specific, and anyways we have multiple Sentry engineers regularly working on it and cutting releases. If we need to introduce more friction, let's at least load up all the friction in the publish repo if we can. Right?

This is just unnecessary friction. Craft repo is already well-protected through branch protection rules. If you are going with that argument, then any repo can steal credentials via their bump-version.sh scripts so you have the same vulnerabilities.

[chadwhitacre]

any repo can steal credentials via their bump-version.sh scripts

Ouch. Good point.

[chadwhitacre]

any repo can steal credentials via their bump-version.sh scripts

The pre-release hook runs in the context of the releasing repo, not the publish repo, correct? Therefore no secrets concern there, but ... there is also a post-release hook that does run in the context of the publish repo, so the basic point stands: anyone who can get code into a releasing repo and trigger a release and convince an approver to approve it can steal secrets. Yes?

[chadwhitacre]

I met with @alek-sentry on the security considerations with running Craft master vs. pinning a version (and only allowing Release Approvers (a team with 8 members, basically me, Dan, and SDK eng mgmt) to bump).

On the prepare side craft is only run in the context of the releasing repo, so as with the pre-release hook there is no secrets concern one way or another.

On the publish side, if we go with master then anyone who can get code into craft master and trigger a release and convince an approver to approve it can steal secrets. Yes?

[BYK]

there is also a post-release hook that does run in the context of the publish repo, so the basic point stands: anyone who can get code into a releasing repo and trigger a release and convince an approver to approve it can steal secrets. Yes?

Yes 🙂

On the publish side, if we go with master then anyone who can get code into craft master and trigger a release and convince an approver to approve it can steal secrets. Yes?

Yes. I don't think pinning a version necessarily reduces the risk though. It gives you equivalent protection with your branch protection rules which you rely on getsentry/sentry and getsentry/getsentry repos which are as equally sensitive as the SDKs (if not more as that code goes into prod and has potential access to customer data).

[chadwhitacre]

I don't think pinning a version necessarily reduces the risk though.

Pinning the version of craft used in publish would mean that anyone who can get code into craft master and convince someone in the releng team (3 people) to approve a version bump PR in publish and trigger a release and convince an approver to approve it can steal secrets. Right?

Of course it would also mean that anyone who lands a craft patch would have to convince someone in releng to approve a version bump PR in publish before being able to benefit.

Tradeoff.

Anyway @alek-sentry is chewing on this and we'll circle back in a bit.

[chadwhitacre]

equivalent protection with your branch protection rules which you rely on getsentry/sentry and getsentry/getsentry repos which are as equally sensitive as the SDKs

Key diff being that only three people can approve PRs in publish vs. all of eng in other repos (not sure about SDKs).

[mitsuhiko]

I deleted a previous comment here as it did not apply much. Right now we pick the latest release that made it to docker and not the latest master. This in turn means that there is not much more security benefit to pinning as the release already needs to be approved by a release manager.

We do however want to make sure that the craft repo in general is sufficiently protected to what we like to avoid the likelihood that bad commits make it into it.

[jan-auer]

The docker image (latest tag) is pushed on every master build via GCB.

[BYK]

@chadwhitacre

Key diff being that only three people can approve PRs in publish vs. all of eng in other repos (not sure about SDKs).

Which makes it even more dangerous for getsentry/sentry and getsentry/getsentry, hence my point. You'd introduce friction and a bottleneck (with an additional and mundane load on the releng team) for a perceived protection. It is like adding an additional lock to your window where your front door is protected by a slide lock.

@mitsuhiko

We do however want to make sure that the craft repo in general is sufficiently protected to what we like to avoid the likelihood that bad commits make it into it.

💯

[BYK]

The docker image (latest tag) is pushed on every master build via GCB.

I think a good middle-ground here would be to change the :latest tag to mean the "latest released version", like we did for sentry et al and introduce a :nightly tag (again, like sentry). This would give us the protection and flexibility and would also help me as an external craft consumer as I don't trust nightly builds as much when I'm not involved with the project at all times 😅

[chadwhitacre]

It is like adding an additional lock to your window where your front door is protected by a slide lock.

Fair.

when I'm not involved with the project at all times

lol you're still pretty involved @BYK 😁 🤗