2.3 algorithm should always check id==keyId, and should absolutize `publicKey.id` before this check

[trwnh]

https://swicg.github.io/activitypub-http-signature/#how-to-obtain-a-signature-s-public-key

the way it's written, handling a key document in step 5 has you jumping back to step 2 with an actor id, and then step 4 skips ahead to step 7

what you want to be doing is verifying that the actor's publicKey includes an id matching the http-sig's keyId you started with, not to throw away the key in the interim

step 7 should make it clear that the key in publicKey may be referenced by id, and that you should either a) have this key in-memory while this algorithm is processing, or b) that you need to refetch it from cache (step 2) or from network (step 3, but you should already have it from your first pass of the algorithm...)

i understand what the intent is with the "go back and run this algorithm with an actor" bit, but as written it's inefficient.

there is also an implication in step 1 that you should be discarding url fragments, despite this being unnecessary and also needing the url fragment later on in step 7.

furthermore the id==keyId comparison needs to make sure that the publicKey.id is converted to an absolute URI first, since it could be relative or absolute

---

a more correct algorithm should be:

• Initialize variables KEY_ID, DOCUMENT, PUBLIC_KEY, ACTOR_ID.
• Set KEY_ID to the value of the keyId parameter of the HTTP Signature.
• Dereference KEY_ID using an HTTP GET. Caching may be used. Store the result in memory as DOCUMENT.
• Set PUBLIC_KEY to the value of the publicKeyPem property in DOCUMENT, if present.
• Set ACTOR_ID to the id of the controller or owner property in DOCUMENT, if present.
• If PUBLIC_KEY has not been set: Set PUBLIC_KEY to the value of the publicKey property in DOCUMENT, if present.
• If PUBLIC_KEY is an array: Filter the array for an item whose absolutized id matches KEY_ID.
• If PUBLIC_KEY is an object:
  • If ACTOR_ID has not been set: Set ACTOR_ID to the id of the controller or owner property of that object, if present.
  • Set PUBLIC_KEY to the value of the publicKeyPem property of that object, if present. Otherwise, unset PUBLIC_KEY.
• If ACTOR_ID has not been set: Set ACTOR_ID to the value of KEY_ID, removing the URL fragment if any.
• Dereference ACTOR_ID using an HTTP GET. Caching may be used. Store the result in memory as DOCUMENT.
• Set PUBLIC_KEY to the value of the publicKey property in DOCUMENT, if present.
• If PUBLIC_KEY is an array: Filter the array for an item whose absolutized id matches KEY_ID.
• If PUBLIC_KEY is an object:
  • If the id of that object does not match KEY_ID, abort processing with a KeyNotMatchingError.
  • Set PUBLIC_KEY to the value of the publicKeyPem property of that object, if present. Otherwise, unset PUBLIC_KEY.
• If PUBLIC_KEY is not set, abort processing with a KeyNotFoundError.
• PEM-decode the value of PUBLIC_KEY and return this PEM-decoded value along with ACTOR_ID.

[TallTed]

@trwnh — Your suggested algorithm (including the sub-steps) would be better as an ordered list, than as the current unordered, as bullets should be executable in any order, which is not usually the case for an algorithm.

[snarfed]

This is great, thank you for the details @trwnh! And @TallTed, true, but @trwnh's algorithm is pretty clearly intended to be ordered in spirit, I think we can give them the benefit of the doubt here.

I'm all for resolving ambiguities here. Fragment handling is a good example, I can definitely change the language to clarify that you keep it for comparing against ids like in step 7.

I am a bit reluctant to make it quite this detailed and in the weeds though. This new algorithm reads more like pseudocode than prose, and I don't know if I want to go quite that far. It is an algorithm, agreed, but it's not a normative standard, and it leans heavily on existing standards and other established patterns like caching, array vs object handling, fetching objects by id if they're not inlined, etc.

Regardless, I'll take another pass at it and incorporate a lot of this. Thank you!

[trwnh]

yeah, the list is intended to be ordered but while typing it on my phone it was too inconvenient to use numbers. (also my pronouns are they/them :x)

re: taking another pass and incorporating stuff, i think the important bits are:

• note in step 7 that even for single values, the id needs to match the original keyId. right now, someone can use something like keyId="http://actor.example/#not-the-real-key" and land on "publicKey": {"id": "#the-real-key"} and never detect the error
  • note in step 7 that the id may be relative or absolute, so you need to absolutize it before comparing publicKey.id to keyId
• mention/define the possible failure states. if the publicKey.id didn't match the keyId, then call this out as a "key not matching" error. if no suitable publicKey is present (i.e. it is missing publicKeyPem or otherwise missing entirely), then call this out as a "key not found" error.