WIP: Refactor Comm-Decision as a comm-decision task kind - #827
Conversation
|
This should help getting chain of trust working with this new setup: https://treeherder.mozilla.org/jobs?repo=enterprise-firefox-pr&revision=bf67e1eb5d53925ca48e08eef31c649ad3fec59c |
|
I did a cursory review and added some comments. This approach seems plausible. Though I'll be honest that I don't feel confident that this isn't regressing security for other projects in some way. I'll need to do a deeper dive into it once ready. |
There was a problem hiding this comment.
I did a somewhat deep dive into this patch and I think it makes sense fundamentally. It'll allow chains like:
signing:build:docker-image:parent:parent
Or even:
signing:build:docker-image:parent:docker-image:parent
If the parent is not the root one (aka a non-decision task that runs Taskgraph), then we validate the task definition matches what is in its parent task's task-graph.json, and then continue down the chain validating the root decision task against .taskcluster.yml as normal.
In addition to supporting the Enterprise use case it seems like a good enough general improvement, though I'm unclear if we'll ever need it elsewhere.
| bool: True if the parent task is a nested one. | ||
|
|
||
| """ | ||
| return link.task_type in DECISION_TASK_TYPES and link.parent_task_id != link.task_id |
There was a problem hiding this comment.
The fact that taskId == taskGroupId is true for Decision tasks is just convention and is something that could be faked by an attacker. I'm unsure if it's ok to rely on this or not.
There was a problem hiding this comment.
On some deeper digging, we don't need to worry about an attacker faking this.. as literally anything about the task definitino can be faked and the safeguard against that is not to land malicious code..
But it is definitely possible for a Decision task to have parent_task_id != task_id, in which case we'd never properly vet it. So instead we should check whether the task has a valid extra.parent or not. If yes, then we can be reasonably confident it's an intermediate Decision task. If not, then it's the root one.
| # the comm one is defined in the gecko tree while it checks the comm | ||
| # repository out. So a task carrying both repositories may legitimately | ||
| # be sourced from either of them; it just has to be one of the two. | ||
| if (repo or comm_repo) and not (match_repo or match_comm_repo): |
There was a problem hiding this comment.
This appears to be loosening the requirements on the condition.
There was a problem hiding this comment.
Is this change relevant? It seems kind of unrelated to the other part. Maybe it belongs in a separate commit?
| verify_link_in_task_graph(chain, link, target_link) | ||
| try: | ||
| await verify_parent_task_definition(chain, link) | ||
| if is_nested_parent_task(link): |
There was a problem hiding this comment.
I don't love the term "nested" task.. Maybe it could be called is_root_parent_task() instead? Or just in-line this helper and avoid the name altogether.
No description provided.