Improve error reporting and logging when processing remote accounts #15605

ClearlyClaire · 2021-01-20T11:00:28Z

When discovering/fetching/processing a remote account fails, without adding additional instrumentation to the code, users, developers and admins alike get very few information on the actual reason behind the failure.

This is especially troublesome for fediverse server developers that get very few feedback from Mastodon regarding what exactly failed.

This PR reworks the error handling in the SignatureVerification concern, ResolveAccountService and ActivityPub::FetchRemoteAccountService services, as well as related classes to return meaningful error messages to ActivityPub queries and log additional information at the “debug” loglevel.

app/controllers/concerns/signature_verification.rb

app/services/activitypub/fetch_remote_account_service.rb

app/controllers/concerns/signature_verification.rb

ClearlyClaire · 2021-02-12T13:58:40Z

DeepSource doesn't like the multiple guarded raise very much, but I think they do make sense in this context.

ClearlyClaire · 2021-02-23T22:14:20Z

ping @Gargron

Gargron

I don't want to knock this PR down but I'm not sure 🤔

Gargron · 2021-04-06T13:39:21Z

app/controllers/concerns/signature_verification.rb

- rescue Mastodon::HostValidationError
- nil
+ rescue Mastodon::PrivateNetworkAddressError => e
+ raise SignatureVerificationError, "Requests to private network addresses are disallowed (tried to query #{e.host})"


Hm, is this safe to disclose..? What if you were testing ways to trick the private network checks?

I think it's safe to disclose, yes. Unless the admin has specifically allowed an address, the check is the same for everyone and can easily be inferred from the code. And in case the admin has allowed an address, it'd be rather easy to check for common ports anyway.

Gargron · 2021-04-06T13:43:13Z

app/controllers/concerns/signature_verification.rb

+ rescue Mastodon::PrivateNetworkAddressError => e
+ raise SignatureVerificationError, "Requests to private network addresses are disallowed (tried to query #{e.host})"
+ rescue Mastodon::HostValidationError, ActivityPub::FetchRemoteAccountService::Error, ActivityPub::FetchRemoteKeyService::Error, Webfinger::Error => e
+ raise SignatureVerificationError, e.message


I'd be in favour of smarter use of the Rails logger for debugging purposes but I am a little uneasy at disclosing each of these errors to the public. I understand that this information would be helpful to someone building an ActivityPub implementation but it could also provide information to an attacker prodding the endpoint for weaknesses...

I have a hard time thinking about weaknesses that it would disclose, and I'd say that any vulnerability in Mastodon could be found directly in the code, and then exploiting it would be extremely unlikely to be made easier by having such error messages.

weex · 2021-08-22T22:21:19Z

Bump on codeclimate

app/lib/request.rb

codeclimate · 2021-08-23T11:32:04Z

app/services/activitypub/fetch_remote_account_service.rb

 SUPPORTED_TYPES = %w(Application Group Organization Person Service).freeze

 # Does a WebFinger roundtrip on each call, unless `only_key` is true
- def call(uri, id: true, prefetched_body: nil, break_on_redirect: false, only_key: false)
+ def call(uri, id: true, prefetched_body: nil, break_on_redirect: false, only_key: false, suppress_errors: true)


Avoid parameter lists longer than 5 parameters. [6/5]

I wonder if we should set CountKeywordArgs to false in rubocop's config

ClearlyClaire · 2021-10-22T18:44:35Z

snarfed/bridgy-fed#77 too would have been easier to figure out with such a PR merged

Defaults to true (to preserve previous behavior). If set to false, errors will be raised instead of caught, allowing the caller to be informed of what went wrong.

…yloads

…oded data

ClearlyClaire · 2022-07-28T11:44:54Z

@Gargron I still think this would be useful

ClearlyClaire · 2022-09-19T23:08:25Z

@Gargron I would very much like if you could finally review this before I need to change remote account processing to support groups

Fix regression from mastodon#15605

Fix regression from #15605

…astodon#15605) * Add a more descriptive PrivateNetworkAddressError exception class * Remove unnecessary exception class to rescue clause * Remove unnecessary include to JsonLdHelper * Give more neutral error message when too many webfinger redirects * Remove unnecessary guard condition * Rework how “ActivityPub::FetchRemoteAccountService” handles errors Add “suppress_errors” keyword argument to avoid raising errors in ActivityPub::FetchRemoteAccountService#call (default/previous behavior). * Rework how “ActivityPub::FetchRemoteKeyService” handles errors Add “suppress_errors” keyword argument to avoid raising errors in ActivityPub::FetchRemoteKeyService#call (default/previous behavior). * Fix Webfinger::RedirectError not being a subclass of Webfinger::Error * Add suppress_errors option to ResolveAccountService Defaults to true (to preserve previous behavior). If set to false, errors will be raised instead of caught, allowing the caller to be informed of what went wrong. * Return more precise error when failing to fetch account signing AP payloads * Add tests * Fixes * Refactor error handling a bit * Fix various issues * Add specific error when provided Digest is not 256 bits of base64-encoded data * Please CodeClimate * Improve webfinger error reporting

Fix regression from mastodon#15605

…astodon#15605) * Add a more descriptive PrivateNetworkAddressError exception class * Remove unnecessary exception class to rescue clause * Remove unnecessary include to JsonLdHelper * Give more neutral error message when too many webfinger redirects * Remove unnecessary guard condition * Rework how “ActivityPub::FetchRemoteAccountService” handles errors Add “suppress_errors” keyword argument to avoid raising errors in ActivityPub::FetchRemoteAccountService#call (default/previous behavior). * Rework how “ActivityPub::FetchRemoteKeyService” handles errors Add “suppress_errors” keyword argument to avoid raising errors in ActivityPub::FetchRemoteKeyService#call (default/previous behavior). * Fix Webfinger::RedirectError not being a subclass of Webfinger::Error * Add suppress_errors option to ResolveAccountService Defaults to true (to preserve previous behavior). If set to false, errors will be raised instead of caught, allowing the caller to be informed of what went wrong. * Return more precise error when failing to fetch account signing AP payloads * Add tests * Fixes * Refactor error handling a bit * Fix various issues * Add specific error when provided Digest is not 256 bits of base64-encoded data * Please CodeClimate * Improve webfinger error reporting

Fix regression from mastodon#15605

innonate · 2022-12-28T14:34:12Z

https://github.com/mastodon/mastodon/blob/main/app/controllers/concerns/signature_verification.rb#L167 has an error in it. It validates the string length of base64 encoded digests, but it checks for a length of 32 vs 64, making it impossible for requests to get to the next validation.

ClearlyClaire · 2022-12-31T09:45:11Z

https://github.com/mastodon/mastodon/blob/main/app/controllers/concerns/signature_verification.rb#L167 has an error in it. It validates the string length of base64 encoded digests, but it checks for a length of 32 vs 64, making it impossible for requests to get to the next validation.

That's not true. digest_size is the size of the decoded base64, not that of the base64 representation. So 32 bytes is what's expected there.

…astodon#15605) * Add a more descriptive PrivateNetworkAddressError exception class * Remove unnecessary exception class to rescue clause * Remove unnecessary include to JsonLdHelper * Give more neutral error message when too many webfinger redirects * Remove unnecessary guard condition * Rework how “ActivityPub::FetchRemoteAccountService” handles errors Add “suppress_errors” keyword argument to avoid raising errors in ActivityPub::FetchRemoteAccountService#call (default/previous behavior). * Rework how “ActivityPub::FetchRemoteKeyService” handles errors Add “suppress_errors” keyword argument to avoid raising errors in ActivityPub::FetchRemoteKeyService#call (default/previous behavior). * Fix Webfinger::RedirectError not being a subclass of Webfinger::Error * Add suppress_errors option to ResolveAccountService Defaults to true (to preserve previous behavior). If set to false, errors will be raised instead of caught, allowing the caller to be informed of what went wrong. * Return more precise error when failing to fetch account signing AP payloads * Add tests * Fixes * Refactor error handling a bit * Fix various issues * Add specific error when provided Digest is not 256 bits of base64-encoded data * Please CodeClimate * Improve webfinger error reporting

Fix regression from mastodon#15605

ClearlyClaire force-pushed the fixes/improved-error-messages branch 2 times, most recently from fad57fb to fcbe642 Compare January 20, 2021 15:28

ClearlyClaire marked this pull request as ready for review January 20, 2021 15:51

ClearlyClaire marked this pull request as draft January 20, 2021 16:14

ClearlyClaire force-pushed the fixes/improved-error-messages branch 4 times, most recently from 309cb33 to 3c8ad37 Compare January 20, 2021 19:32

ClearlyClaire marked this pull request as ready for review January 20, 2021 19:54

ClearlyClaire requested a review from Gargron January 26, 2021 09:09

Gargron requested changes Feb 11, 2021

View reviewed changes

app/controllers/concerns/signature_verification.rb Show resolved Hide resolved

app/services/activitypub/fetch_remote_account_service.rb Outdated Show resolved Hide resolved

app/controllers/concerns/signature_verification.rb Outdated Show resolved Hide resolved

ClearlyClaire force-pushed the fixes/improved-error-messages branch 2 times, most recently from 66506b9 to 3d2435b Compare February 12, 2021 13:54

ClearlyClaire force-pushed the fixes/improved-error-messages branch from 3d2435b to a3895bc Compare March 1, 2021 18:33

ClearlyClaire force-pushed the fixes/improved-error-messages branch from a3895bc to 7842738 Compare March 19, 2021 14:31

Gargron reviewed Apr 6, 2021

View reviewed changes

ClearlyClaire force-pushed the fixes/improved-error-messages branch 2 times, most recently from 6e2551d to d1084c0 Compare May 31, 2021 12:39

ClearlyClaire requested a review from Gargron August 5, 2021 11:52

ClearlyClaire force-pushed the fixes/improved-error-messages branch from d1084c0 to ac3ea15 Compare August 23, 2021 11:30

codeclimate bot reviewed Aug 23, 2021

View reviewed changes

ClearlyClaire force-pushed the fixes/improved-error-messages branch from ac3ea15 to 52c1fef Compare August 23, 2021 11:45

ClearlyClaire mentioned this pull request Oct 22, 2021

Mastodon doesn't accept replies, wants a "public key" snarfed/bridgy-fed#77

Closed

ClearlyClaire force-pushed the fixes/improved-error-messages branch 2 times, most recently from f72f728 to ec49f62 Compare November 14, 2021 14:58

ClearlyClaire mentioned this pull request Nov 14, 2021

Fix confusing error when webfinger request returns empty document #16986

Merged

ClearlyClaire added 10 commits May 21, 2022 14:12

Fix Webfinger::RedirectError not being a subclass of Webfinger::Error

08feaad

Add suppress_errors option to ResolveAccountService

211ca41

Defaults to true (to preserve previous behavior). If set to false, errors will be raised instead of caught, allowing the caller to be informed of what went wrong.

Return more precise error when failing to fetch account signing AP pa…

8b814bb

…yloads

Add tests

9199e93

Fixes

c91c585

Refactor error handling a bit

21498b6

Fix various issues

95697a2

Add specific error when provided Digest is not 256 bits of base64-enc…

7d5ecf7

…oded data

Please CodeClimate

bf73ffb

Improve webfinger error reporting

35caf0a

ClearlyClaire force-pushed the fixes/improved-error-messages branch from ec49f62 to 35caf0a Compare May 21, 2022 12:13

Gargron approved these changes Sep 20, 2022

View reviewed changes

Gargron merged commit 1145dbd into mastodon:main Sep 20, 2022

ClearlyClaire added a commit to ClearlyClaire/mastodon that referenced this pull request Sep 21, 2022

Fix typo in SignatureVerification

7439c9f

Fix regression from mastodon#15605

ClearlyClaire mentioned this pull request Sep 21, 2022

Fix typo in SignatureVerification #19209

Merged

Gargron pushed a commit that referenced this pull request Sep 21, 2022

Fix typo in SignatureVerification (#19209)

84aff59

Fix regression from #15605

nametoolong pushed a commit to nametoolong/nuage that referenced this pull request Sep 22, 2022

Fix typo in SignatureVerification (mastodon#19209)

3c8f8ea

Fix regression from mastodon#15605

bearice mentioned this pull request Nov 11, 2022

fixes ArgumentError when proxy is used #20420

Merged

kadoshita pushed a commit to kadoshita/mastodon that referenced this pull request Nov 19, 2022

Fix typo in SignatureVerification (mastodon#19209)

d69dd93

Fix regression from mastodon#15605

ClearlyClaire added a commit to ClearlyClaire/mastodon that referenced this pull request Jul 6, 2023

Fix typo in SignatureVerification (mastodon#19209)

b5237f0

Fix regression from mastodon#15605

ClearlyClaire mentioned this pull request Dec 22, 2023

Fix Mastodon not correctly processing HTTP Signatures with query strings #28476

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve error reporting and logging when processing remote accounts #15605

Improve error reporting and logging when processing remote accounts #15605

ClearlyClaire commented Jan 20, 2021

ClearlyClaire commented Feb 12, 2021

ClearlyClaire commented Feb 23, 2021

Gargron left a comment

Gargron Apr 6, 2021

ClearlyClaire Apr 6, 2021

Gargron Apr 6, 2021

ClearlyClaire Apr 6, 2021

weex commented Aug 22, 2021

codeclimate bot Aug 23, 2021

ClearlyClaire Aug 23, 2021

ClearlyClaire commented Oct 22, 2021

ClearlyClaire commented Jul 28, 2022

ClearlyClaire commented Sep 19, 2022

innonate commented Dec 28, 2022

ClearlyClaire commented Dec 31, 2022

Improve error reporting and logging when processing remote accounts #15605

Improve error reporting and logging when processing remote accounts #15605

Conversation

ClearlyClaire commented Jan 20, 2021

ClearlyClaire commented Feb 12, 2021

ClearlyClaire commented Feb 23, 2021

Gargron left a comment

Choose a reason for hiding this comment

Gargron Apr 6, 2021

Choose a reason for hiding this comment

ClearlyClaire Apr 6, 2021

Choose a reason for hiding this comment

Gargron Apr 6, 2021

Choose a reason for hiding this comment

ClearlyClaire Apr 6, 2021

Choose a reason for hiding this comment

weex commented Aug 22, 2021

codeclimate bot Aug 23, 2021

Choose a reason for hiding this comment

ClearlyClaire Aug 23, 2021

Choose a reason for hiding this comment

ClearlyClaire commented Oct 22, 2021

ClearlyClaire commented Jul 28, 2022

ClearlyClaire commented Sep 19, 2022

innonate commented Dec 28, 2022

ClearlyClaire commented Dec 31, 2022