[investigation] HTTP signature query params issues

[tsmethurst]

While GtS's implementation of http signatures is technically correct (#107 (comment)), Mastodon and many other implementations do not include query parameters when generating http signatures. This leads to some incompatibilities with GoToSocial when it comes to fetching status replies.

We should investigate how to alleviate this incompatibility from GtS's side, while still adhering correctly to the rfc.

My initial idea about this would be to maintain current behavior for first attempts, BUT: if an incoming request with query parameters fails http sig authentication, retry it without including the query params in the reconstructed signature, and if an outgoing request with query params gets a 401 returned, retry it without including query params in the outgoing signature.

[benjojo]

I am using https://humungus.tedunangst.com/r/honk and it appears to suffer from this issue as well.

Might I suggest (since the rest of the ecosystem appears to do the Mastodon(?) way) that you validate "both" methods (the technically correct sig, and the Mastodon style one)?

I am unable to follow some accounts due to this and it appears the only instances that have this issue are GTS ones, and it's quite annoying to miss out on some accounts

[tsmethurst]

Related: mastodon/mastodon#18638

[NyaaaWhatsUpDoc]

Just noting here, i will be handling this task once we've finalized funding with NLNet for this proposed task :)

[tsmethurst]

Oh great, I forgot we'd included that :)

[milas]

Hi! I am a happy GoToSocial user, and I decided to give fixing this a shot a bit back, and have been running a fork on my instance with the changes for a bit.

I followed the approach outlined in the original issue:

My initial idea about this would be to maintain current behavior for first attempts, BUT: if an incoming request with query parameters fails http sig authentication, retry it without including the query params in the reconstructed signature, and if an outgoing request with query params gets a 401 returned, retry it without including query params in the outgoing signature.

The changes on the GtS side are fairly small, the go-fed/httpsig changes were a little trickier when keeping semver compatibility:

• feat: add option to exclude query params from signature go-fed/httpsig#31

Apologies for not posting here sooner, and I won't be at all offended if you opt to go with a different implementation!

For reference, here's more or less what the GtS changes look like when using my go-fed/httpsig fork:

commit 3b265efb1985e09e9a9f80eb09f7a41a1e5f30cb
Author: Milas Bowman <devnull@milas.dev>
Date:   Thu Dec 7 23:11:03 2023 -0500

    feat: add Mastodon-compatible HTTP signature fallback

diff --git a/internal/federation/authenticate.go b/internal/federation/authenticate.go
index fe611af8..d604f10f 100644
--- a/internal/federation/authenticate.go
+++ b/internal/federation/authenticate.go 
@@ -510,23 +515,30 @@ var signingAlgorithms = []httpsig.Algorithm{
 }
 
 // verifyAuth verifies auth using generated verifier, according to pubkey and our supported signing algorithms.
-func verifyAuth(l *log.Entry, verifier httpsig.Verifier, pubKey *rsa.PublicKey) bool {
+func verifyAuth(l *log.Entry, verifier httpsig.VerifierWithOptions, pubKey *rsa.PublicKey) bool {
 	if pubKey == nil {
 		return false
 	}
 
-	// Loop through all supported algorithms.
-	for _, algo := range signingAlgorithms {
+	options := []httpsig.SignatureOption{
+		{ExcludeQueryStringFromPathPseudoHeader: false},
+		{ExcludeQueryStringFromPathPseudoHeader: true},
+	}
 
-		// Verify according to pubkey and algo.
-		err := verifier.Verify(pubKey, algo)
-		if err != nil {
-			l.Tracef("authentication NOT PASSED with %s: %v", algo, err)
-			continue
-		}
+	for _, opts := range options {
+		// Loop through all supported algorithms.
+		for _, algo := range signingAlgorithms {
 
-		l.Tracef("authenticated PASSED with %s", algo)
-		return true
+			// Verify according to pubkey and algo.
+			err := verifier.VerifyWithOptions(pubKey, algo, opts)
+			if err != nil {
+				l.Tracef("authentication NOT PASSED with %s: %v", algo, err)
+				continue
+			}
+
+			l.Tracef("authenticated PASSED with %s", algo)
+			return true
+		}
 	}
 
 	return false

[milas]

@NyaaaWhatsUpDoc Welcome back from break 😛

I know y'all were planning on getting funding for this, but I'm happy to help (as a volunteer) as well.

It looks like go-fed/httpsig is unmaintained. If I fork it and make some improvements (i.e. my PR mentioned ⬆️ & some of the other open PRs), would that be helpful?

[daenney]

Of note, Masto seems to have improved its behaviour: mastodon/mastodon#28476

[NyaaaWhatsUpDoc]

@NyaaaWhatsUpDoc Welcome back from break 😛

I know y'all were planning on getting funding for this, but I'm happy to help (as a volunteer) as well.

It looks like go-fed/httpsig is unmaintained. If I fork it and make some improvements (i.e. my PR mentioned ⬆️ & some of the other open PRs), would that be helpful?

if you're happy to jump on this then by all means go for it! as @daenney said Mastodon have improved their behaviour (which might in turn encourage others to actually correct their behaviour too, hopefully...), so it's not quite as urgent as a fix as it was. but if you do update (and maintain) an httpsig fork with these (and likely other useful changes too i'm sure :) ) then ofc that would be welcome! ❤️

EDIT:
just seen you posted the PR above, if you want me to go through it and give it a review let me know 👍

[ShadowJonathan]

We incorperated mastodon/mastodon#28476 into our mastodon fork, but the jobs returning 401s for GTS servers are still piling up. Would that fix work with some GTS servers, or is mastodon's fix currently not improving its behaviour/interaction with GTS?

[tsmethurst]

Mastodon's fix only works for verifying http signatures of requests going in to Mastodon, it doesn't work for signing outgoing requests to other servers from Mastodon. The latter still uses the existing way of signing by excluding the query parameters. This was for backwards compatibility reasons.

[ShadowJonathan]

is anything more needed on mastodon's side to make fetching replies work from GTS servers, or is the ball in GTS' court to fix it? or is it something else, like a privacy/actor delegation (fetching as instance vs fetching as user) issue?

[tsmethurst]

is anything more needed on mastodon's side to make fetching replies work from GTS servers, or is the ball in GTS' court to fix it?

Basically, Mastodon could consider signing http requests "properly" (including params), and falling back to signing them "improperly" (excluding params, existing behavior) when receiving a 401. Or the other way around. That would allow it to work with softwares doing things the "correct" way, while retaining backwards compatibility with old Mastodon versions and with other softwares doing things the "incorrect" way. (I include these terms in scare quotes because it's mostly a matter of interpretation.)

At the same time, GtS will likely do the same thing: try signing and validating signatures both ways, but always try the "correct" behavior first to hopefully succeed first time and not harm performance by having to create + validate signatures twice.

EDIT: I do think, at least in the short term, it would be a good idea for Mastodon to ensure that 401 jobs don't pile up and impact performance in any other regard. Certainly, there's no point retrying the same thing on a 401, or marking it as a failed job and indicating that in metrics, assuming that's what you mean by jobs piling up.

[tsmethurst]

@NyaaaWhatsUpDoc Welcome back from break 😛

I know y'all were planning on getting funding for this, but I'm happy to help (as a volunteer) as well.

It looks like go-fed/httpsig is unmaintained. If I fork it and make some improvements (i.e. my PR mentioned ⬆️ & some of the other open PRs), would that be helpful?

@milas I will fork the httpsig library into our SuperSeriousBusiness org as well (EDIT: done -- https://github.com/superseriousbusiness/httpsig), and we can start maintaining it the same way we now maintain the activity fork. Would you be OK with making PRs to that fork? Or would you rather fork and maintain it yourself? Either way is absolutely fine for me, just wanna get the ball rolling on this :)

[ShadowJonathan]

@tsmethurst

Certainly, there's no point retrying the same thing on a 401, or marking it as a failed job and indicating that in metrics, assuming that's what you mean by jobs piling up.

No, I mean this:

This dead job (filtered) queue piled as high as 7k in the last month before I cleared it all out again (We're using glitch-soc, a mastodon fork)

It grows fast, which is why I looked at why this error was occurring in the first place, and where I can look for to fix the problem (if there are any more mastodon issue or PR numbers i can keep track of, or any more GTS issues/prs I could look at)

[daenney]

When you queue a job in Sidekiq, as Masto does, and the job experiences an error and no retry behaviour is configured for it it'll get sent to the dead queue. By letting it land in the dead queue, admins can manually retry the job if they so want to.

How many items and for how long are held in the dead queue is a Sidekiq configuration. But Masto should probably ensure that jobs that 401 don't land in there in the first place, or clear them out somehow.

[ShadowJonathan]

mastodon/mastodon#28788 got merged, fixing the behaviour on mastodon's side

[tsmethurst]

Neat. That makes it less urgent for us to try both ways. We'll still do it, but at least in the meantime Mastodon's queues won't be getting clogged up as much. Hope it makes it into a release soon.

[renchap]

We plan for a 4.2.4 release early next week, which will include this fix.

[tsmethurst]

Very nice!

[tsmethurst]

First part of this (try to validate both methods of signing on incoming requests) PR'd above ^^

[milas]

Opened #2659 that will retry signed GET (e.g. for authorized fetch) without query params if first attempt fails with a 401. ⬆️

(edit: I wrote 403 originally but meant 401 🤦🏻)

[tsmethurst]

We now attempt and validate both types of HTTP signatures, for incoming and outgoing requests, so I am closing the issue :) Thanks everyone.