-
Notifications
You must be signed in to change notification settings - Fork 180
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
Collaboration proofkeys #9366
Collaboration proofkeys #9366
Conversation
Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes. |
79eef4a
to
7caf11a
Compare
Unit tests seems quite difficult to add. We'll need to generate private keys in order to set the proof signature, and also to store the related public keys, which should be fetchable from somewhere (likely a dummy server). Some basic end-to-end test with any WOPI app with proof keys configured should be enough to know whether the feature works or not because any request done to the wopiserver will be rejected if the verification fails. If the proof keys are generated automatically when the WOPI app starts, then the integration should work without configuring any additional thing on our side. |
ed36e70
to
e004d99
Compare
General question: Are the proofkeys kept in memory? |
The actual proofkeys come from the "/hosting/discovery" endpoint of the WOPI app. According to Microsoft docs, the keys change from time to time (I've read somewhere that they change the keys each 12 hours, but I'm not sure), so the collaboration service will cache the keys in memory for 12 hours before requesting the new keys again. Even if the keys are cached individually in memory, it's just a local cache. There is no problem with scaling on our side. The collabotation service's replicas will eventually have the same keys. The proposed algorithm for the verification in the official docs also takes into account possible time shifts (you get the "latest" keys and one hour later the WOPI app changes the keys; or the WOPI app sends the proof with the previous key), so they shouldn't be any problem if we cache the keys within the time range of the rotation. The only potential problem is if we need to scale the WOPI app somehow, although I guess it's a problem the WOPI app should solve, not us. I mean, if we need to scale OnlyOffice to 3 instances, it's up to them how to do it, but I'd expect all the instances to share the same keys. |
Thank you for the explanation. That works for me. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From a code POV it looks good. I cannot test the fuctionality.
BTW: Which WebOffice suites are using ProofKeys?
Background of the question: We are using collabora as the default. I see no proofkeys on the hosting/discovery response. Therefore it might be better to keep the feature default off.
OnlyOffice have some keys set by default, although for production they should be generated (I'm not sure how, I was testing with the default ones) For Collabora, I think you need to run the As said, the keys are expected to rotate after a while, but I don't know if that will be taken care automatically by the WOPI app or the admin will need to do something else. If the key rotation happens faster than each 12 hours, the admin will need to adjust our |
For Collabora, I think the best option is for them to handle the generation of the proof keys on startup. They could provide an environment option to generate the keys. For the short term, I guess the easiest option is to include the following command in the docker-compose file (collabora section):
That should generate the proof keys right before starting Collabora. If the keys exist, the |
c20369a
to
990539e
Compare
Basic code handling is in the proofkeys package. The idea is to implement it as a middleware and hook it after the wopicontext middleware. Note that we need a way to switch this middleware off in case there are problems with the verification, even though it should be active by default.
Proofkeys feature can't be tested with the wopi validator for now. The "FakeOffice" doesn't provide the keys in the discovery endpoint, and the wopi validator doesn't seem to send any header with the proof.
58df958
to
d2f6679
Compare
Quality Gate passedIssues Measures |
Description
Add support for proof keys. These will be used to ensure that the calls coming to the collaboration service comes from a trusted source.
Most of the technical details can be found on https://learn.microsoft.com/en-us/microsoft-365/cloud-storage-partner-program/online/scenarios/proofkeys
The proofkeys verification will be enabled by default. This could cause problems if the WOPI app doesn't support the feature, it doesn't have the keys ready, or there are bugs in our side. In order to not disturb the system too much, this feature can be disabled on our side (collaboration service) so the verification won't happen and all the requests will be allowed. Note that you'll need to restart the collaboration service after changing its configuration.
In addition, the retrieved keys from the discovery (required for the verification) will be cached in memory for 12 hours by default. It's expected that the WOPI app rotates the keys so we'll need to fetch them again periodically. You can adjust the caching period in order to ensure we get updated keys periodically, otherwise the verification could fail.
Related Issue
No open issue
Motivation and Context
This should increase the security because all the allowed requests should come from a trusted source.
How Has This Been Tested?
Manually tested with OnlyOffice
Screenshots (if appropriate):
Types of changes
Checklist: