Support single-subject attestations

[AA-Turner]

Closes #213, cc @phillmv.

A

[AA-Turner]

The gigantic diff is as I ran npm run bundle, which regenerated the index.js file -- let me know if this should be reverted.

A

[phillmv]

Hello! This was extremely prompt, so I feel I owe you a prompt review.

I hereby request some changes:

• please delete the your generated index.js; this file really ought to be built automatically inside Actions but………… if we haven't set that automation up, then it really needs to have been generated by one of our maintainers :)
• if you unwind the createSingle/Multi functions so createAttestation is called directly inside the if (inputs.singleSubjectAttestations) { statement then we don't need to modify the AttestResult type / the log functions don't have to be modified either
• I left some comments on the log functions, let's avoid modifying their signatures, keeps this diff even simpler.

Thanks!,

[phillmv]

if we unwind createSingle/Multi attestations, we can call this like we did before.

[AA-Turner]

I've now reverted this (logAttestation) to the previous state, passing the full subjects list instead of a per-attestation attestationSubjects. I'm not sure though that this is the right change to make.

The current design of actions/attest@v2 has a single attestation with multiple subjects. logAttestation is thus only ever called once, with the full list of subjects. This list of subjects matches the subjects attested in the single attestation. Should there only be a single artefact, the name and SHA256 digest are printed, else the number of subjects is printed.

This PR introduces a 1:1 relationship between subjects and attestations. We now call logAttestation once per attestation generated, but each time with the full list of subjects. We should thus print the name and digest of the single subject each time -- but we can't, as there's no easy way of linking the attested subject to the created attestation (I have looked through the fields of the Attestation type).

The result is that we only print the name and digest if there is exactly one subject being attested (i.e. the behaviour is the same regardless of this PR), and that for more than one subject we print "Attestation created for N subjects", N times. Clearly this is wrong.

As such, I think the right thing to do here is either restore attestationSubjects, or remove the == 1 check, and only print out the number of subjects. I understand not wanting to unnecessarily change the AttestationResult type, so the latter option may be the most pragmatic. (see below for context on why I contend that printing subjects is valuable, though).

---

The other half of this, and the reason behind my change to logSummary(), is that currently for a multiple-subject attestation the names of the said subjects aren't ever printed out. One can introspect the name by base64-decoding the DSSE Envelope's payload & extracting the inner subject key, but this isn't exactly ergonomic. Especially for glob input, I do feel that this action should print the names (and digests) of the attested subjects for overview and debugging.

Ideally, the underlying JavaScript / TypeScript code would be changed to allow easier access to the names of subjects, obviating the need for the custom attestationSubjects list, but I don't know if this is possible given that Sigstore specifications must be adhered to.

[phillmv]

As such, I think the right thing to do here is either restore attestationSubjects, or remove the == 1 check, and only print out the number of subjects.

Shall we remove the == 1 check and the need to pass in subjects at all 😅 ?

is that currently for a multiple-subject attestation the names of the said subjects aren't ever printed out.

Genuine curiosity: how and why do you need this output? Just entirely for debugging?

(For the record, my interest here is in just keeping a small and tidy PR).

I'd support a) ditching subjects as a param to logAttestation and just having a logSubjects that prints the list separately with formatSubjectDigest 🙂.

[AA-Turner]

As such, I think the right thing to do here is either restore attestationSubjects, or remove the == 1 check, and only print out the number of subjects.

Shall we remove the == 1 check and the need to pass in subjects at all 😅 ?

This might end up being the most pragmatic option for this PR.

is that currently for a multiple-subject attestation the names of the said subjects aren't ever printed out.

Genuine curiosity: how and why do you need this output? Just entirely for debugging?

(For the record, my interest here is in just keeping a small and tidy PR).

I agree small PRs are better. For the short term, the output is useful as a sanity check / for debugging. Longer term, it would be great to have some access to the names of subjects via the Attestation object, as my use-case involves converting to PEP 740 attestations for the Python Package Index, for which I need to know the filename of the subject.

I have pushed a logSubjects change as you suggest @phillmv.

[AA-Turner]

This was extremely prompt, so I feel I owe you a prompt review.

Thank you. I have no real experience with TypeScript, but it seems fairly straightforwards. I do apologise as I'm not particularly familiar with custom & practice in the TypeScript world, though.

please delete the your generated index.js

Done.

if you unwind the createSingle/Multi functions so createAttestation is called directly inside the if (inputs.singleSubjectAttestations) { statement then we don't need to modify the AttestResult type / the log functions don't have to be modified either

Done & reverted (see my mini-essay comment re attestationSubjects, though -- sorry!)

I left some comments on the log functions, let's avoid modifying their signatures, keeps this diff even simpler.

I believe everything is reverted. Still a larger diff than I'd like, but +60/-20 seems much more reasonable.

A

[phillmv]

I've seen your comments @AA-Turner and left you a suggestion which hopefully makes sense.