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

Blob: Remove branch from cache universe #102

Merged
merged 1 commit into from
Nov 4, 2024
Merged

Conversation

dfederm
Copy link
Member

@dfederm dfederm commented Nov 4, 2024

The original intent was to match Azure Pipeline Caching which has a hierarchy of caches, where the current branch has a Read/Write cache, while parent branches like main would have a cache with read-only access. Details for the interested for how that works.

However, this simply added the branch name to the cache universe, which isolated but did not actually add any security. It also didn't add any of the hierarchy logic, meaning branches were completely isolated, which is not desirable.

This change just removes that logic.

@dfederm dfederm enabled auto-merge (squash) November 4, 2024 22:01
@johnterickson
Copy link
Collaborator

which isolated but did not actually add any security

Could you elaborate on this? The isolation was the security.

Specifically, how do we reasonably ensure that (unmerged) PR builds don't affect CI builds? Perhaps instead of completely removing the branch we can:

  1. Base the salt/namespace on the build type: PR vs CI e.g. GITHUB_EVENT_NAME
  2. "Flatten" the branch name to main/master vs not.

If the intent is to make it so we remove the isolation between CI and PR, I would be open to making that an opt-in option, but I think the default should be secure.

@dfederm dfederm merged commit 147bc0b into main Nov 4, 2024
6 checks passed
@dfederm dfederm deleted the dfederm/blob-remove-branch branch November 4, 2024 22:12
@lifengl
Copy link
Member

lifengl commented Nov 5, 2024

@johnterickson : this logic blocks using cache for developers on a topic branch. For security PR build should not merge cache with CI builds. Actually, based on your isolation logic, PR build should not have cache turned on at all. Put straight, if you have an isolated cache store for each PR branch, it would only benefit the PR build from same branch, if it happens to run multiple times. In all the other time, it will only create cache blobs which would never be used again in the future, once the PR is merged. Basically, it means the owner will pay for the cost to store them without any benefit. Overall, it maybe doesn't make sense to have PR build to store cache in the same binary storage at all.

@lifengl
Copy link
Member

lifengl commented Nov 5, 2024

On the other word, if changes within a PR can modify the build logic to a degree, then allowing the PR build to have permission to access/update the same storage account would be unsafe, which cannot be blocked by the additional branch isolation logic inside the plug-in, but should not give the PR build machine the SAS token to do any update.

Or, if the PR cannot touch the build logic, but can only provide code, then the bad code will only yield to some result files, which would never be used by any CI, as long as the code has never been merged in. Those results are only referenced by fingerprints which would not exist in the CI build.

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.

4 participants