Skip to content

Verify signaling token keys #15332

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.

Sign up for GitHub

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

Jump to bottom
Merged
SystemKeeper merged 2 commits into from
Jun 16, 2025
Merged

Verify signaling token keys #15332

SystemKeeper merged 2 commits into from
Jun 16, 2025

Conversation

@SystemKeeper
Copy link
Contributor

@SystemKeeper SystemKeeper commented Jun 12, 2025 •
edited
Loading

🛠️ API Checklist

Error handling based on
https://github.com/nextcloud/server/blob/a8f46af20f4fccac0257eba950e70d0da96c4a5a/lib/private/Authentication/Token/PublicKeyTokenProvider.php#L559-L565

🏁 Checklist

  • ⛑️ Tests (unit and/or integration) are included or not possible
  • 📘 API documentation in docs/ has been updated or is not required
  • 🔖 Capability is added or not needed

@SystemKeeper SystemKeeper requested a review from Antreesy June 12, 2025 19:10
@SystemKeeper
Copy link
Contributor Author

SystemKeeper commented Jun 12, 2025

Federation is failing because of

623f2f0240016bbc142387741a44864ecb0458e2 is the first bad commit
commit 623f2f0240016bbc142387741a44864ecb0458e2
Author: Micke Nordin <[email protected]>
Date:   Fri Mar 14 09:53:16 2025 +0100

    feat(OCM-invites): Implementation of invitation flow
    
    This patchset:
    * implements the /invite-accepted endpoint
    * adds capabilities and inviteAceptDialog to the discovery
    * adds a FederatedInviteAcceptedEvent
    
    https://cs3org.github.io/OCM-API/docs.html?branch=v1.1.0&repo=OCM-API&user=cs3org#/paths/~1invite-accepted/post
    
    Co-authored-by: Anna <[email protected]>
    Co-authored-by: Côme Chilliet <[email protected]>
    Co-authored-by: Joas Schilling <[email protected]>
    Co-authored-by: Navid Shokri <[email protected]>
    Signed-off-by: Micke Nordin <[email protected]>

IOCMProvider -> ICapabilityAwareOCMProvider

@SystemKeeper SystemKeeper marked this pull request as ready for review June 13, 2025 05:03
@SystemKeeper SystemKeeper force-pushed the fix/14312/verify-signaling-token-keys branch from 01a5d58 to 53995f7 Compare June 13, 2025 09:15
Antreesy
Antreesy reviewed Jun 13, 2025
View reviewed changes
Copy link
Contributor

@Antreesy Antreesy left a comment

Choose a reason for hiding this comment

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

Looks good from manual tests

nickvergessen
nickvergessen reviewed Jun 16, 2025
View reviewed changes
Copy link
Member

@nickvergessen nickvergessen left a comment

Choose a reason for hiding this comment

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

I was about to comment that this should be part of the setup check, but I see you have both.

@SystemKeeper SystemKeeper force-pushed the fix/14312/verify-signaling-token-keys branch from b7fea0b to c0f0246 Compare June 16, 2025 12:42
@SystemKeeper SystemKeeper enabled auto-merge June 16, 2025 12:43
@SystemKeeper
Copy link
Contributor Author

SystemKeeper commented Jun 16, 2025

Should we backport to 31 and 30?

nickvergessen
nickvergessen reviewed Jun 16, 2025
View reviewed changes
lib/SetupCheck/HighPerformanceBackend.php Outdated
$publicKeyDerived = $this->talkConfig->deriveSignalingTokenPublicKey($privateKey, $alg);

if ($publicKey != $publicKeyDerived) {
return SetupResult::error($this->l->t('The stored public key for used algorithm %$1s does not match the stored private key. Run %$2s to fix the issue.', [$alg, '`occ talk:signaling:verify-keys --update`']));
Copy link
Member

@nickvergessen nickvergessen Jun 16, 2025

Choose a reason for hiding this comment

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

Sorry, I always mess this up:

Suggested change
return SetupResult::error($this->l->t('The stored public key for used algorithm %$1s does not match the stored private key. Run %$2s to fix the issue.', [$alg, '`occ talk:signaling:verify-keys --update`']));
return SetupResult::error($this->l->t('The stored public key for used algorithm %1$s does not match the stored private key. Run %2$s to fix the issue.', [$alg, '`occ talk:signaling:verify-keys --update`']));

@nickvergessen nickvergessen force-pushed the fix/14312/verify-signaling-token-keys branch from c0f0246 to 6f45ccb Compare June 16, 2025 13:13
@nickvergessen
Copy link
Member

nickvergessen commented Jun 16, 2025

/backport to stable31

@nickvergessen nickvergessen added this to the 🪺 Next Major (32) milestone Jun 16, 2025
@nickvergessen nickvergessen added 3. to review feature: api 🛠️ OCS API for conversations, chats and participants feature: signaling 📶 Internal and external signaling backends bug labels Jun 16, 2025
@SystemKeeper SystemKeeper merged commit e24d192 into main Jun 16, 2025
79 checks passed
@SystemKeeper SystemKeeper deleted the fix/14312/verify-signaling-token-keys branch June 16, 2025 13:31
@backportbot backportbot bot mentioned this pull request Jun 16, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Antreesy Antreesy Antreesy left review comments

@nickvergessen nickvergessen nickvergessen approved these changes

Assignees

No one assigned

Labels

3. to review bug feature: api 🛠️ OCS API for conversations, chats and participants feature: signaling 📶 Internal and external signaling backends

Projects

None yet

Milestone

v22.0.0-beta.1

Development

Successfully merging this pull request may close these issues.

System check should validate HPB public key is derived from private key

4 participants

@SystemKeeper @nickvergessen @Antreesy