Fail mark-for-deployment when re-deploying same version without --wait-for-deployment - #4307
Fail mark-for-deployment when re-deploying same version without --wait-for-deployment#4307cuza wants to merge 8 commits into
Conversation
1a04794 to
ccc3674
Compare
ccc3674 to
0110d7e
Compare
nemacysts
left a comment
nemacysts
left a comment
There was a problem hiding this comment.
mostly just questions - i think this is basically exactly what we want otherwise!
| @patch( | ||
| "paasta_tools.cli.cmds.check_deploy_health.check_if_instance_is_done", | ||
| autospec=True, | ||
| ) | ||
| @patch( | ||
| "paasta_tools.cli.cmds.check_deploy_health.get_instance_configs_for_service_in_deploy_group_all_clusters", | ||
| autospec=True, | ||
| ) | ||
| @patch( | ||
| "paasta_tools.cli.cmds.check_deploy_health.get_currently_deployed_version", | ||
| autospec=True, | ||
| ) | ||
| @patch( | ||
| "paasta_tools.cli.cmds.check_deploy_health.validate_service_name", | ||
| autospec=True, | ||
| ) |
There was a problem hiding this comment.
i need to finish my auto-fix pre-commit branch, but can we swap these for context-manager patches? (that's what we normally do for new code since then you don't have to remember how the decorator order matches the param order :p)
...although, maybe this matter less now that llms write a lot of our tests :p
Co-authored-by: Luis Pérez <luisp@yelp.com>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| print(deployment_version) | ||
| print("Continuing anyway.") | ||
| print(f"Warning: {deploy_group} already set to {deployment_version}") | ||
| if not args.block: |
There was a problem hiding this comment.
🤖 Security Issue: Early-return path when re-deploying the same version without --wait-for-deployment bypasses the can_user_deploy_service LDAP authorization check at line 550. An unauthorized user can write a deploy tag by re-issuing a mark-for-deployment for the currently deployed version without --wait-for-deployment, skipping the LDAP group membership verification entirely.
Severity: HIGH
Category: authorization_bypass
Tool: ClaudeCode AI Security Analysis
Exploit Scenario: An attacker or unauthorized user who knows the currently deployed version (sha + image_version) of a service can run paasta mark-for-deployment -s <service> -l <deploy-group> -c <current-sha> --image-version <current-image-version> without --wait-for-deployment. Because the version matches old_deployment_version, the code takes the early return at line 514, calling mark_for_deployment() to force-push the deploy git tag and returning success — without ever reaching can_user_deploy_service() at line 550 which checks LDAP group membership. This could be chained: an attacker first re-deploys the same version to confirm access, then races a version change (or exploits a TOCTOU window) to deploy an unauthorized version.
Recommendation: Move the authorization check (can_user_deploy_service) before the same-version early-return block, or call it within the early-return path before invoking mark_for_deployment(). The authorization check should gate ALL paths that write deploy tags.
There was a problem hiding this comment.
hmm, i can't remember if writing a deploy tag in this way will trigger a bounce - if it does, we probably do want to make sure we've done the can_user_deploy_service check
There was a problem hiding this comment.
@nemacysts
mark_for_deployment does call trigger_deploys(), but since it's force-pushing the same SHA to the same tag, deployd sees no version change
There was a problem hiding this comment.
hmm, on_enter_start_deploy() calls mark_for_deployment() which calls get_paasta_tag_from_deploy_group() which adds the current timestamp to the tag - i'm admittedly a little rusty on the nitty gritty internals of how the tag handling works w.r.t. what is a noop and causes a bounce, but i'm a little worried that this will allow folks to bounce services that they shouldn't be able to bounce since the tag won't be exactly the same
…uza/PAASTA-18862 * 'u/cuza/PAASTA-18862' of github.com:Yelp/paasta: Update paasta_tools/cli/cmds/mark_for_deployment.py
2 participants
Prevent deployment when attempting to redeploy the same version without the
--wait-for-deploymentflag.