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

Digitally sign documents via software certificates #4129

Merged
merged 3 commits into from
Oct 21, 2024
Merged

Conversation

vmiklos
Copy link
Contributor

@vmiklos vmiklos commented Oct 16, 2024

Summary

This implements step 1 of digital signature signing, where the keystore is in richdocuments and the signing is performed in Collabora Online, not via some external signing service. This supports ODF, OOXML and PDF formats.

Checklist

  • Code is properly formatted: npm run stylelint passes
  • Sign-off message is added to all commits
  • Documentation (manuals or wiki) has been updated or is not required: this is not done in this PR, maybe we should link to the Collabora Online SDK to explain how to generate signing keys?

How to test

  1. Make sure to have a new enough COOL snapshot, so its /hosting/capabilities reports hasDocumentSigningSupport=true. This is true for online.git master as of >= 2024-11-09.

  2. Go to /settings/user/richdocuments

  3. Fill in the 3 new signing settings. For testing, you can generate the certificates with a script described at Digitally sign documents CollaboraOnline/online#9992 (comment) or just use these:

  1. Load e.g. an ODT file in Nextcloud Office. Go to the File tab, press the Signature button to open the Signatures dialog. Press the Sign Document button, select the detected certificate, press the Sign button: "the signatures in this document are valid" text appears.

Let me know if you would like any tweaks. Thanks.

Document signing needs to store keys as richdocuments settings. This
involves the signing key, certificate and the matching CA chain.

As a first step, add code to the personal settings to be able to set a
CA chain that issues the signing key / certificate.

Setting and getting the setting is possible after this; the setting is
not yet exposed in the WOPI CheckFileInfo response.

<CollaboraOnline/online#9992 (comment)>
has instructions on how to generate self-signed certificates for
document signing for development purposes. Related to nextcloud#4123

Signed-off-by: Miklos Vajna <vmiklos@collabora.com>
…FileInfo

This setting was already possible to read and write from the personal
settings UI, but was not available towards Collabora Online.

Other private user settings like the Zotero API key are exposed in the
WOPI CheckFileInfo reply.

Do the same here: if the feature is enabled in general and this is not a
public share, then include the signature CA setting in the CheckFileInfo
response.

The same still needs doing for the signature cert/key. Related to nextcloud#4123

Signed-off-by: Miklos Vajna <vmiklos@collabora.com>
…, too (fixes nextcloud#4123)

The CA chain for the document signing was already a user setting & it
was exposed in the WOPI CheckFileInfo, but the actual signing
certificate & key was missing, so signing was not possible.

These are typically in a similar PEM format using just ASCII characters,
so providing a textarea where the user can paste them sounds like a good
fit.

Add the read/write of this setting and also expose it as part of the
private user info in WOPI CheckFileInfo.

With this, once all 3 are configured, it's possible to sign a document
in Nextcloud Office, using the Signature button on the Home tab of the
notebookbar.

Signed-off-by: Miklos Vajna <vmiklos@collabora.com>
@@ -17,6 +17,16 @@ import { showError } from '@nextcloud/dialogs'
this.zoteroAPIKeySaveButton = document.getElementById('zoteroAPIKeySave')
this.zoteroAPIKeyRemoveButton = document.getElementById('zoteroAPIKeyRemove')

this.documentSigningCertInput = document.getElementById('documentSigningCertField')
Copy link
Member

Choose a reason for hiding this comment

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

It would be great if we can rather add this to the vue.js based frontend, but maybe that is something where @elzody can help moving that over

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, I was just tracing how the zotero API key setting does this, and this way seemed to be the way it works. Maybe move the zotero API key handling to the vue.js based frontend, and then I'm happy to do the same for these settings as well? Thanks.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, also wouldn't consider this a blocker then especially since zotero is also still there in the legacy part. We can migrate them together afterwards as well

@vmiklos
Copy link
Contributor Author

vmiklos commented Oct 17, 2024

An other problem I notice is that 5 cypress jobs are red, but it seems that the tip of 'main' has the same problem. Is it OK to ignore those failures for now? Thanks.

@juliusknorr
Copy link
Member

That is unfortunately known (due to larger refactorings and legacy code removal in server for the upcoming release), we're on that but can be ignored for now.

@juliusknorr juliusknorr added enhancement New feature or request 2. developing Work in progress labels Oct 17, 2024
@vmiklos
Copy link
Contributor Author

vmiklos commented Oct 18, 2024

OK thanks, good to know. Is there anything else I can help with to get this reviewed? I assume that it only makes sense to rebase the branch if there would be conflicts, correct? (Even if the github interface nags with an "update branch" button.)

Thanks.

@juliusknorr juliusknorr added 3. to review Ready to be reviewed and removed 2. developing Work in progress labels Oct 18, 2024
@juliusknorr
Copy link
Member

Yeah that is fine, I also requested a review from @elzody and lined it up for me

Copy link
Contributor

@elzody elzody left a comment

Choose a reason for hiding this comment

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

Works well on my end. Code looks good for now, will probably be reorganized in a future refactoring. Really cool feature

@elzody elzody merged commit d09ae22 into nextcloud:main Oct 21, 2024
66 of 71 checks passed
@vmiklos vmiklos deleted the sign branch October 21, 2024 12:36
@vmiklos
Copy link
Contributor Author

vmiklos commented Oct 21, 2024

@elzody thanks for the review! :-)

Copy link
Contributor

Hello there,
Thank you so much for taking the time and effort to create a pull request to our Nextcloud project.

We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process.

Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6

Thank you for contributing to Nextcloud and we hope to hear from you soon!

(If you believe you should not receive this message, you can add yourself to the blocklist.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Ready to be reviewed enhancement New feature or request feedback-requested
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Digitally sign documents via software certificates.
3 participants