Bug 2052985 - feat: add build_signing transform and artifact signing specs

[kryoseu]

Port the desktop and source build-signing transforms (add_signed_routes, define_upstream_artifacts) and the shared generate_specifications_of_artifacts_to_sign helper from gecko_taskgraph so they can be consumed by both Gecko and comm.

Android signing is left out from generate_specifications_of_artifacts_to_sign. Consumers handle it before delegating here since it depends on gecko-specific scriptworker code (generate_beetmover_upstream_artifacts).

[bhearsum]

I'm not certain how I feel about this. This is a forklift of existing gecko_taskgraph code into this repository, and it retains a lot of Gecko-isms. I don't want to let perfect by the enemy of good, but I would also prefer that we don't copy hacks into what we expect to be the long term home for this code.

I'm also not sure that I'm fond of the idea of putting the whole build_signing transform in here. That's a transform that is highly specialized, and unlikely to be generally useful, as it is very closely tied to a couple of specific kinds in gecko. A rename or possibly refactor of it might make me a little bit more comfortable with the idea.

One idea (which would also help in getting rid of some of the Gecko-isms) would be to not import this code as a transform, but pull out the core bits and have them called in transforms in both gecko and comm repositories. For example, the core bit of add_signed_routes is the inner-most for loop. Putting that in its own (pure) function would avoid the need for code here to know anything about the shippable attribute or enable-signing-routes.

I have some specific comments below, but I also think you may want to consider putting some of this code in comm instead of here. If you want a more immediate path forward, I strongly urge you to put the code you need into the comm repo for now.

[bhearsum]

shippable is a Gecko (and I guess comm) specific concept that doesn't belong here IMO.

[bhearsum]

gecko in the index route is also Gecko specific. I'm not sure this would even work for comm in its current form?

[bhearsum]

These are Gecko-isms that shouldn't live here (the same goes for names of other specific kinds).

[bhearsum]

This function in particular I do not think should move here in its current form. It has...not aged well to say the least, and I would prefer not to prolong its life.

[jcristau]

Agree with Ben that this shouldn't be moved, or not as-is.

[ahal]

I think it's worth stepping back and discussing what actually is the thing we want to share across all projects here. When it comes to signing, the mechanism we use is the signingscript workers. These workers have a schema that all task payloads must conform to.

In gecko (and comm), the thing that makes task payloads conform to the schema is already in mozilla-taskgraph:

mozilla-taskgraph/src/mozilla_taskgraph/worker_types.py

Line 165 in 6188440

def build_signing_payload(config, task, task_def):

So instead of copy/pasting transforms, I think the question we want to ask is what logic would make sense to tease out of the various signing transforms (across gecko, comm, and even other repos) and place into build_signing_payload instead.

[kryoseu]

Thanks folks for the comments. Agreed the forklift is wrong, and ahal's framing especially. I've prototyped a different split and force-pushed it over the previous commit, so the diff here is now just the new approach. Please let me know if this is a better direction and what your thoughts are in general.

The split: the library does only the mechanical part — insert a signed component into the dependency's index routes, and attach taskId/taskType to turn {paths, formats} specs into upstream-artifacts. The project decides what to sign, via job["signing-artifacts"]. So generate_specifications_of_artifacts_to_sign never moves, and util/signed_artifacts.py is out of the diff too. Route prefix comes from graph_config.scriptworker.signed-route-prefix. Configure none, get no signed routes, so it's inert for existing consumers.

Plus build_signing_payload honouring scriptworker.excluded-signing-formats:

scriptworker:
    signed-route-prefix: 'index.comm.v2'
    excluded-signing-formats: [gcp_prod_autograph_widevine, autograph_langpack]

Just to be clear, my goal here is getting comm_taskgraph to stop importing gecko_taskgraph, while maintaining good separation of concerns, and improving Gecko's side of things as much as possible too. For Comm, l10n_source_signing.py goes entirely (it imports gecko's add_signed_routes and re-implements define_upstream_artifacts), and remove_widevine / no_sign_langpacks become the config above. Deletion on our side, not code moved into yours.

Two things I'd like your call on:

1. Does the format exclusion belong in build_signing_payload? It's the only place reaching every signing kind, not just build_signing, which is why our cleanup transforms disappear, but also means it touches inputs outside this diff.
2. Where should the shippable gate go? add_signed_routes used to check dep_job.attributes.get("shippable") and I've dropped it, since that attribute can't live in a shared library. But nothing replaced it, so non-shippable signing tasks would now get signed routes they don't get today. My inclination is to have a gecko-side transform compute it per task from shippable and leave the library ignorant. Alternative is a config key here naming the attribute to gate on. Either way it needs settling before this lands, since it's a behaviour change on the Firefox side, as is the fact that Gecko will need signed-route-prefix: 'index.gecko.v2' added to taskcluster/config.yml, or it gets no signed routes at all.

[kryoseu]

The force-push was just a rebase onto 5.0.0 with no content change. So the pending questions above still stand.

[ahal]

Thanks, this looks much better! It all seems reasonable to me to take into mozilla-taskgraph

[ahal]

Can this instead be derived from config.graph_config["trust-domain"]? Or are the projects where they don't follow the standard format?

If it's just gecko and comm that will be using this to start and they can both be derived, I'd say let's start with that and we can implement signed-route-prefix later when needed

[ahal]

nit: I'd prefer we call these variables task / tasks

[ahal]

This will need to be added to GraphConfig schema (and signed-route-prefix above if we decide to keep that). The scriptworker namespace is a Gecko-ism, so instead you should add it to the schema that mozilla-taskgraph provides:

mozilla-taskgraph/src/mozilla_taskgraph/config.py

Line 19 in 3e5fef6

class MozillaGraphConfigSchema(tg.graph_config_schema):

I'd like to follow the format that the shipitscript uses, e.g:

signing:
    excluded-formats: ...
    route-prefix: ...   # if necessary

[ahal]

And to answer your questions:

Does the format exclusion belong in build_signing_payload? It's the only place reaching every signing kind, not just build_signing, which is why our cleanup transforms disappear, but also means it touches inputs outside this diff.

It seems reasonable to me.. Though I was wondering if it would make more sense for this to be a per-task config? You could set it in task-defaults in your .yml files to essentially make it a global setting

Where should the shippable gate go?

Yeah I think we'll want a transform on the gecko / comm side that does whatever filtering it needs to before these shared transforms are reached. If we really wanted to make the logic shared, there could be some task config that uses the attrmatch utility to do filtering in mozilla-taskgraph. That way we can filter here without needing to encode the concept of "shippable". But I'm a proponent of keeping changes small, so seems like it could be a good follow-up.

[kryoseu]

Thanks @ahal, appreciate the comments.

I've pushed an update addressing all of the above:

• The route prefix now derives from graph_config["trust-domain"], signed-route-prefix is gone. Both consumers do follow the standard layout: gecko emits index.gecko.v2.*, comm emits index.comm.v2.*.
• Renamed the transform args to tasks.
• Added a schema for the transforms' input, signing-artifacts, via transforms.add_validate. It's required: before, a project that forgot to populate it silently got upstreamArtifacts: [], essentially a signing task that signs nothing, with no error at graph time.
• Dropped the format exclusion entirely. More on that below.

On the exclusion: task-defaults can't reach these kinds, since gecko_taskgraph.transforms.signing validates against a schema (SigningDescriptionSchema) with no worker field. But more to the point, it couldn't deliver what I justified it with. It would only fire for scriptworker-signing, but mac signing kinds get switched to iscript, which is exactly where widevine comes from for mac, so remove_widevine has to stay either way. comm doesn't need it here regardless: once its kinds populate signing-artifacts themselves, they won't emit widevine or langpack formats at all.

One consequence worth calling out: it's a no-op for Firefox but not for Thunderbird. comm's routes are index.comm.v2.*, which the hardcoded index.gecko.v2 silently dropped, so comm has never published a signed route. Once comm's kinds adopt these transforms it would start publishing them, including on non-shippable deps now that the shippable gate is gone. So I'll do the gating comm-side as you suggest and keep comm's routes off in the porting patches. We can switch them on after assessing with the TB release team.

Shippable gate: agreed, project-side. And attrmatch as a follow-up sounds right to me.