Rework API + verify metadata #2

Oneric · 2025-02-15T02:19:00Z

Oneric commented

2025-02-15 02:19:00 +00:00

This patchset reworks the API into a clean basis suitable to expand on, with optional communication between API user and adapter, without leaking any internal details and adds new functionality. It will enable improvements to Akkoma.

The current API is an utter mess and by necessity leaks internal details and forces adapter implementations to reparse signature headers just to figure out which key they should actually look up. At the same time, current API does not allow passing any metadata between the caller of verify/signature and the adapter in neither direction. This however is important for akkoma to be able to e.g. exit early if:

exit early if the key is on a blocked domain
exit early if the key is marked as deleted (HTTP 410)
fake a success for overly eager Mastodon if the former happened and the received doc is an actor Delete (in that case we never knew the actor and there’s nothing to do, but except for bleeding-edge mastodon insists on resending the Delete quite often until it gets a success

Furthermore the way pseudo headers are handled atm is very awkward, requiring explicit code for each pseudo header in both the lib and the API user! For example (opaque) is apparently sometimes used, but has no explicit code yet and would thus always fail verification.
Instead the new version now can handle any pseudo header during verification and explicit code only in the library is only needed to implement additional side-effects the pseudo header asks for, like e.g. checking a sender-determined expiry time (in addition to receiver-side signature age limits)

Speaking of expiry and time limits, so far the library never verified signature age or expiry, happily accepting a 100 year old signature even if it was explicitly marked as expiring 99 years ago. Now this is actually verified. Limits are configurable, by default max 2h old and a 40min clock skew leniency for future timestamps and expiry deadlines (since the latter might be affected by clock skew).

To avoid accepting vulnerable signatures and creating such ourselves, the presence of a minimal set of headers is now verified by the library.

Finally request-target aliases are now handled in the library which enables us to avoid attempting to refetch the key after each failed alias. Now a key is refetched at most once for one request, if and only if all request-target aliases failed to verify with the original key. Also alias can be lazily evaluated, allowing us to entirely skip database queries when objects are fetched by their canonical ID (the empirically most common case)

Further details in each commit. As usual this is best reviewed commit by commit.

This patchset reworks the API into a clean basis suitable to expand on, with optional communication between API user and adapter, without leaking any internal details and adds new functionality. It will enable improvements to Akkoma. The current API is an utter mess and by necessity leaks internal details and forces adapter implementations to reparse signature headers just to figure out which key they should actually look up. At the same time, current API does not allow passing any metadata between the caller of `verify`/`signature` and the adapter in neither direction. This however is important for akkoma to be able to e.g. exit early if: - exit early if the key is on a blocked domain - exit early if the key is marked as deleted (HTTP 410) - fake a success for overly eager Mastodon if the former happened and the received doc is an actor Delete (in that case we never knew the actor and there’s nothing to do, but except for bleeding-edge mastodon insists on resending the Delete quite often until it gets a success Furthermore the way pseudo headers are handled atm is very awkward, requiring explicit code for each pseudo header _in **both** the lib and the API user!_ For example `(opaque)` is apparently sometimes used, but has no explicit code yet and would thus always fail verification. Instead the new version now can handle any pseudo header during verification and explicit code _only in the library_ is only needed to implement additional side-effects the pseudo header asks for, like e.g. checking a sender-determined expiry time *(in addition to receiver-side signature age limits)* Speaking of expiry and time limits, so far the library never verified signature age or expiry, happily accepting a 100 year old signature even if it was explicitly marked as expiring 99 years ago. Now this is actually verified. Limits are configurable, by default max 2h old and a 40min clock skew leniency for future timestamps and expiry deadlines *(since the latter might be affected by clock skew)*. To avoid accepting vulnerable signatures and creating such ourselves, the presence of a minimal set of headers is now verified by the library. Finally request-target aliases are now handled in the library which enables us to avoid attempting to refetch the key after each failed alias. Now a key is refetched _at most_ once for one request, if and only if all request-target aliases failed to verify with the original key. Also alias can be lazily evaluated, allowing us to entirely skip database queries when objects are fetched by their canonical ID *(the empirically most common case)* Further details in each commit. As usual this is best reviewed commit by commit.

❤️ 1

Oneric added 12 commits

2025-02-15 02:19:01 +00:00

Bump CI to Elixir 1.13 5df2317d26

Update repo link 30c62bad55

Pleroma still hasn’t merged the (created) /expires) PR,
so it seems like we’ll carry our own fork for the foreseeable future

Update dependencies eab5ba344f

test: capture logs 0b09d51312

Rework library interface ce53b672fc

And also clean up internals while at it.
Now no more weird req_headers mangling is needed
with callers being required to reparse the signature just to know how to
mangle things or in case of the adapter to figure out what keys should
be fetched.

Unexpected pseudo headers also no longer break verification completely,
they’ll be processed such that verification works, albeit we won't
apply any intended side effects until full support is added.

The interface is now also prepared to handle request target aliases
inside the library in the future to avoid superfluous key refetches etc
on each failing alias. This and further improvments will be implemented
in future commits.

As this is breaking the existing API,
the major version is bumped from 0 to 1.

Basic support for request-target aliases a0c7d0297c

Avoid superfluous key refetches while testing request target aliases 65bcf4c389

Allow lazily evaluated request aliases 0bb5619614

If fallback aliases are expensive to compute but rarely actually needed
this will avoid the cost of ever computing it. Multiple aliases can be
lazily inserted at once by returning a list of all results.

In Akkoma computing display URL aliases requires a non-cached activity
lookup, but only very few requests will actually query objects by
display URL. We also need to add both a version with and without query
parameters.

Actually check signature metadata 60b9dc0c04

Verify all mandatory headers are used f582a2c8b7

Update README ca75c7059a

Bump Version 2adf4c0214

New features since API rework:
 - handling request-target aliases is now suppported
 - request-target aliases can be lazily evaluated
 - signature metadata like date is now checked
 - signatures missing mandatory headers are rejected due to being insecure
 - creation of signatures missing mandatory headers will raise an error

Oneric force-pushed api_rework from 2adf4c0214 to 9887ea6249

2025-02-15 23:08:34 +00:00

Compare

floatingghost reviewed

2025-02-21 15:28:42 +00:00

floatingghost left a comment

i've glanced over it but will likely need some more time to look in depth

ezpz comment attached

i've glanced over it but will likely need some more time to look in depth ezpz comment attached

.gitlab-ci.yml Outdated

					
				@ -2,3 +2,3 @@

				# SPDX-License-Identifier: LGPL-3.0-only

				image: elixir:1.7

				image: elixir:1.13

you can probably drop this commit entirely (and remove the ci config file) since we don't use it

oh right it’s GitLab specific ;^^
now deleted

oh right it’s GitLab specific ;^^ now deleted

Oneric marked this conversation as resolved

Oneric force-pushed api_rework from 9887ea6249 to c98a4df78b

2025-02-21 20:44:40 +00:00

Compare

Oneric referenced this pull request from AkkomaGang/akkoma

2025-03-01 19:53:21 +00:00

Rework HTTPSignatures and fix bridgy interop #874

floatingghost reviewed

2025-05-09 16:48:14 +00:00

floatingghost left a comment

thing i noticed re: skew

config/config.exs

					
				@ -10,0 +9,4 @@

				  config :http_signatures,

				    adapter: HTTPSignatures.TestAdapter,

				    # 20 years in seconds; to not just test our own signatures

				    # the test suite contains signatures from other real-world implementations

typo typo implementations

potentially stupid question: what’s the typo?
If it’s about the plural s, so far I’ve usually seen implementation used as a countable word when referring to distinct software projects following a common spec

potentially stupid question: what’s the typo? If it’s about the plural `s`, so far I’ve usually seen implementation used as a countable word when referring to distinct software projects following a common spec

i may be stupid

floatingghost marked this conversation as resolved

lib/http_signatures/http_signatures.ex

					
				@ -97,0 +197,4 @@

				  defp check_expiry(now, %{"(expires)" => expires_unix_seconds}) do

				    with {:ok, expires} <- unixstr_to_time(expires_unix_seconds),

				         true <- DateTime.diff(expires, now) > -@max_clock_skew do

you probably could (and likelly should?) use abs here to avoid the negative constant thing going on - this only allows one-way clock skew

abs(DateTime.diff(expires, now)) < @max_clock_skew would probably be ok

you probably could (and likelly should?) use `abs` here to avoid the negative constant thing going on - this only allows one-way clock skew `abs(DateTime.diff(expires, now)) < @max_clock_skew` would probably be ok

won’t this end up rejecting signature whose expiry date is set further into the future than now + max_clock_skew; i.e. rejecting signatures marked as still valid?

won’t this end up rejecting signature whose expiry date is set further into the future than `now + max_clock_skew`; i.e. rejecting signatures marked as still valid?

Oneric marked this conversation as resolved

lib/http_signatures/http_signatures.ex

					
				@ -97,0 +210,4 @@

				  defp check_created(now, %{"(created)" => created_unix_seconds}) do

				    with {:ok, created} <- unixstr_to_time(created_unix_seconds),

				         diff <- DateTime.diff(now, created),

				         {_, false} <- {:future, diff < -@max_clock_skew},

same dealio here

clock skew should only limit dates set into the future; max_sig_age already restricts past dates. The latter has by default a higher leniency than future dates. If this was using abs both future and past timestamps will be limited by max_clock_skew and max_sig_age will be useless.

clock skew should only limit dates set into the future; max_sig_age already restricts past dates. The latter has by default a higher leniency than future dates. If this was using `abs` both future and past timestamps will be limited by `max_clock_skew` and `max_sig_age` will be useless.

Oneric marked this conversation as resolved

Oneric merged commit c98a4df78b into main

2025-06-07 18:16:42 +00:00

Oneric deleted branch api_rework

2025-06-07 18:16:42 +00:00

Sign in to join this conversation.

No reviewers

No labels

No milestone

No project

No assignees

2 participants

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

AkkomaGang/http_signatures!2

Rows
Columns