Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

perf(federation): (re) Ed25519署名に対応する #14278

Draft
wants to merge 38 commits into
base: develop
Choose a base branch
from

Conversation

tamaina
Copy link
Contributor

@tamaina tamaina commented Jul 20, 2024

Fix #14273
Fix of #13464

What

beta.1での課題をもとに挙動を改善しました

p1.a9z.devなどで運用中

  • 署名検証で同一名キーが見つからない場合のupdatePersonなどの最短間隔を12分から5分に短縮
  • inboxで署名検証が失格になった場合、キューを再試行しないようにしていたがするように
  • admin/show-userで公開鍵(リモートユーザー)/秘密鍵公開鍵ペア(ローカルユーザー)を確認できるように
  • updatePersonの後に発火するremoteUserUpdatedイベントにより、ApDbResolverServiceのpublicKeyキャッシュpublicKeyByUserIdCacheをリフレッシュするように(ユーザーごと)
  • deliverでinboxに投げて401が返ってきた場合、時計が狂っている場合などを考慮しリトライできるように
  • signature parseにおいてheaders.dateとサーバー時間のズレは両方向に300sを許容するように (headers.date - サーバー時間 <= 2000しか許容していなかったがこれだと問題が起きた)

Why

Ed25519は導入すべきなので

Additional info (optional)

CHANGELOG

  • Feat: 連合に使うHTTP SignaturesがEd25519鍵に対応するように perf(federation): Ed25519署名に対応する #13464
    • Ed25519署名に対応するサーバーが増えると、deliverで要求されるサーバーリソースが削減されます
      • ジョブキューのconfig設定のデフォルト値を変更しました。
        default.ymlでジョブキューの並列度を設定している場合は、従前よりもconcurrencyの値をより下げるとパフォーマンスが改善する可能性があります。
        • deliverJobConcurrency: 16 (←128)
        • deliverJobPerSec: 1024 (←128)
        • inboxJobConcurrency: 4 (←16)
        • inboxJobPerSec: 64 (←32)

Checklist

  • Read the contribution guide
  • Test working in a local environment
  • (If needed) Add story of storybook
  • (If needed) Update CHANGELOG.md
  • (If possible) Add tests

@tamaina tamaina marked this pull request as draft July 20, 2024 15:09
Copy link

codecov bot commented Jul 20, 2024

Codecov Report

Attention: Patch coverage is 37.56345% with 615 lines in your changes missing coverage. Please review.

Project coverage is 41.66%. Comparing base (763c708) to head (40756dc).

Files with missing lines Patch % Lines
...ackend/src/core/activitypub/ApDbResolverService.ts 28.57% 110 Missing ⚠️
packages/backend/src/core/UserKeypairService.ts 39.59% 90 Missing ⚠️
...end/src/core/activitypub/models/ApPersonService.ts 16.84% 79 Missing ⚠️
...kend/src/queue/processors/InboxProcessorService.ts 2.66% 73 Missing ⚠️
...nd/src/core/activitypub/ApDeliverManagerService.ts 21.68% 65 Missing ⚠️
...ges/backend/src/server/ActivityPubServerService.ts 7.89% 35 Missing ⚠️
...s/backend/src/core/activitypub/ApRequestService.ts 61.79% 34 Missing ⚠️
...nd/src/queue/processors/DeliverProcessorService.ts 0.00% 29 Missing ⚠️
packages/backend/src/queue/types.ts 0.00% 21 Missing ⚠️
...s/backend/src/core/FetchInstanceMetadataService.ts 62.50% 15 Missing ⚠️
... and 15 more
Additional details and impacted files
@@             Coverage Diff             @@
##           develop   #14278      +/-   ##
===========================================
+ Coverage    39.97%   41.66%   +1.68%     
===========================================
  Files         1561     1564       +3     
  Lines       197358   203506    +6148     
  Branches      3611     3677      +66     
===========================================
+ Hits         78889    84784    +5895     
- Misses      117897   118117     +220     
- Partials       572      605      +33     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.


🚨 Try these New Features:

Copy link
Contributor

github-actions bot commented Jul 20, 2024

このPRによるapi.jsonの差分

差分はこちら
--- base
+++ head
@@ -13130,6 +13130,67 @@
                           "roleId"
                         ]
                       }
+                    },
+                    "publicKeys": {
+                      "type": [
+                        "array",
+                        "null"
+                      ],
+                      "items": {
+                        "type": "object",
+                        "properties": {
+                          "userId": {
+                            "type": "string"
+                          },
+                          "keyId": {
+                            "type": "string"
+                          },
+                          "keyPem": {
+                            "type": "string"
+                          }
+                        },
+                        "required": [
+                          "userId",
+                          "keyId",
+                          "keyPem"
+                        ]
+                      }
+                    },
+                    "keyPairs": {
+                      "type": [
+                        "object",
+                        "null"
+                      ],
+                      "properties": {
+                        "userId": {
+                          "type": "string"
+                        },
+                        "publicKey": {
+                          "type": "string"
+                        },
+                        "privateKey": {
+                          "type": "string"
+                        },
+                        "ed25519PublicKey": {
+                          "type": [
+                            "string",
+                            "null"
+                          ]
+                        },
+                        "ed25519PrivateKey": {
+                          "type": [
+                            "string",
+                            "null"
+                          ]
+                        }
+                      },
+                      "required": [
+                        "userId",
+                        "publicKey",
+                        "privateKey",
+                        "ed25519PublicKey",
+                        "ed25519PrivateKey"
+                      ]
                     }
                   },
                   "required": [
@@ -13156,7 +13217,9 @@
                     "signins",
                     "policies",
                     "roles",
-                    "roleAssigns"
+                    "roleAssigns",
+                    "publicKeys",
+                    "keyPairs"
                   ]
                 }
               }
@@ -81086,6 +81149,9 @@
               "string",
               "null"
             ]
+          },
+          "httpMessageSignaturesImplementationLevel": {
+            "type": "string"
           }
         },
         "required": [
@@ -81113,7 +81179,8 @@
           "faviconUrl",
           "themeColor",
           "infoUpdatedAt",
-          "latestRequestReceivedAt"
+          "latestRequestReceivedAt",
+          "httpMessageSignaturesImplementationLevel"
         ]
       },
       "GalleryPost": {

Get diff files from Workflow Page

* lastFetchedAtでの更新制限を弱めて再取得
* Reacquisition with weakened update limit at lastFetchedAt
*/
if (user.lastFetchedAt == null || user.lastFetchedAt < new Date(Date.now() - 1000 * 60 * 12)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

怪しいコード1

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

少なくとも時計が狂ってたケースではかなり怪しそう(それをすごく考慮するとややキリがないけど)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

時計が狂ってるとDigest検証とかもっと前の段階で落ちるような気もするけど

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(特定サーバーの話題だけど)その割にアプデ前では配送が落ちてないのが謎

Copy link
Contributor Author

@tamaina tamaina Jul 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

配送は落ちないと思う

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

じゃあここじゃないか

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

inboxで握りつぶすというものなので

(dd8561fで署名検証が失格になってもrecoverable errorとするようにはした

@tamaina
Copy link
Contributor Author

tamaina commented Jul 20, 2024

#14273 (comment)

dd41dd0 でremoteUserUpdatedが来た場合にpublicKeyキャッシュをパージするようにした

@tamaina

This comment was marked as off-topic.

@tamaina
Copy link
Contributor Author

tamaina commented Jul 20, 2024

(色々ごちゃごちゃ更新はしてるけどクリティカルなバグを見つけられてない

@tamaina
Copy link
Contributor Author

tamaina commented Jul 20, 2024

#14273 (comment), 88085fd

deliverでinboxに投げて401が返ってきた場合、時計が狂っている場合があるためリトライするようにした

@tamaina tamaina marked this pull request as ready for review July 20, 2024 17:53
Copy link
Contributor

@zyoshoka zyoshoka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

コード斜め読みしてたら見つけたので一応上げておきます

packages/backend/src/core/QueueService.ts Outdated Show resolved Hide resolved
packages/backend/src/core/QueueService.ts Outdated Show resolved Hide resolved
@tamaina
Copy link
Contributor Author

tamaina commented Jul 31, 2024

コード斜め読みしてたら見つけたので一応上げておきます

キューイング直前でいらんデータを弾くようにしたいのでこのままがいいかも…

@tamaina
Copy link
Contributor Author

tamaina commented Jul 31, 2024

conflict resolved

@tamaina
Copy link
Contributor Author

tamaina commented Jul 31, 2024

キューイング直前でいらんデータを弾くようにしたいのでこのままがいいかも…

いやキューデータ定義はPrivateKeyWithPemなのでこれでいいのかしら

Copy link
Contributor

api.jsonの差分作成中にエラーが発生しました。詳細はWorkflowのログを確認してください。

@tamaina
Copy link
Contributor Author

tamaina commented Sep 24, 2024

conflict resolved?

@github-actions github-actions bot added the packages/frontend Client side specific issue/PR label Sep 24, 2024
@tamaina
Copy link
Contributor Author

tamaina commented Sep 29, 2024

conflict resolved

@tamaina
Copy link
Contributor Author

tamaina commented Oct 15, 2024

Conflict resolved;

  • enableStatsForFederatedInstances: falseである場合、httpMessageSignaturesImplementationLevelは常に00 (RSA署名を使用)と扱うため、このPRの効果が期待できません。

@syuilo
Copy link
Member

syuilo commented Oct 15, 2024

初回の連合時にhttpMessageSignaturesImplementationLevelを取得するようにすればよさそう

@tamaina
Copy link
Contributor Author

tamaina commented Oct 15, 2024

初回の連合時に

既存の連合には効果ないとこのPRはあんまり意味ないと思っており

(オフトピ: そもそもenableStatsForFederatedInstances: falseにする目的があんまりわかってない、これでそこまでパフォーマンスが上がると思えず

@syuilo
Copy link
Member

syuilo commented Oct 15, 2024

既存の連合には効果ない

1%くらいの確率で情報を再取得するようにするとか

@syuilo
Copy link
Member

syuilo commented Oct 15, 2024

(オフトピ: そもそもenableStatsForFederatedInstances: falseにする目的があんまりわかってない、これでそこまでパフォーマンスが上がると思えず

deliverやinboxの処理が行われるたびにレコードのupdateが走るのは重いわね

@tamaina
Copy link
Contributor Author

tamaina commented Oct 15, 2024

deliverやinboxの処理が行われるたびにレコードのupdateが走る

isNotRespondingとnotRespondingSinceでupdateしてるだけなのか…

@tamaina
Copy link
Contributor Author

tamaina commented Oct 29, 2024

TODO: test-federationかく

@tamaina tamaina marked this pull request as draft October 29, 2024 13:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
packages/backend:test packages/backend Server side specific issue/PR packages/frontend Client side specific issue/PR packages/misskey-js
Projects
None yet
4 participants