Conversation
vkareh
left a comment
There was a problem hiding this comment.
I haven't tested this yet, but it seems like a super useful feature for document attestation without having to rely on Adobe.
From this code it doesn't seem like I can yet validate the signatures, only sign, but that could be a logical next phase.
I pointed out in the review a lack of GError propagation - this is probably quite important in terms of making sure any issues during signing are surfaced through the UI, especially since it cannot yet validate signatures in Atril, so the user may think they have signed but cannot verify unless they use a different PDF viewer with support for that.
| static PopplerCertificateInfo * | ||
| find_poppler_certificate_info_by_id (EvCertificateInfo *ev_cert_info) | ||
| { | ||
| GList *certs = poppler_get_available_signing_certificates (); |
There was a problem hiding this comment.
This needs to be freed somewhere.
| if (!cert_info) | ||
| return; |
There was a problem hiding this comment.
Should this propagate a GError? Without checking that error == NULL in ev_document_signatures_sign, it will end up calling on_document_signed and assume it succeeded...
| ret = g_strdup (gtk_entry_get_text (GTK_ENTRY (entry))); | ||
| gtk_widget_destroy (dialog); | ||
|
|
||
| return ret; |
There was a problem hiding this comment.
If user cancels the dialog, this would return an empty string rather than NULL
| #if !defined (__EV_ATRIL_DOCUMENT_H_INSIDE__) && !defined (ATRIL_COMPILATION) | ||
| #error "Only <atril-document.h> can be included directly." | ||
| #endif |
There was a problem hiding this comment.
I'm not sure I understand this...
There was a problem hiding this comment.
This enforces library users to only include libdocument/atril-document.h. It is in all libdocument header files.
|
I've built the latest atril git snapshot 39f1f91 with this patch applied. I get File → Digital Signing, but it is greyed out. I've already had $HOME/.pki/nssdb before (with also my personal certificate, with which I can sign PDF in Adobe Reader). Just to make sure, I've generated and added "Test Signer" certificate as instructed above. "Digital Signing" menu entry is still greyed out. I've also opened already signed PDF file to try signature verification part, but "Digital Signing" menu entry is greyed out as well. System is Rocky Linux 9.7. What might be the reason? |
it could be that in Rocky Linux |
|
In RHEL / Fedora, libpoppler.so is a part of devel package (which is normally not needed / installed in user desktops), I guess. I've therfore checked: and I get: is this OK? |
|
yes, but maybe you have an older pobbler version? when you setup the build in atril
You need to have at least version |
|
I've checked my build log, there is no such line containing " But yes, poppler is version 21.01.0 only in Rocky 9.7 at the moment. Thank you for an explanation / support. |
|
I've backported Rocky 10.1 poppler 24.02.0 to Rocky Linux 9.7, and then atril with this poppler-glib-devel ver. 24. I've been asked for a nss password, selected a certificate and get "Digital Signing" now. I've also successfully signed a PDF document (at about normal size rectangle I've been warned about it being too small, and made a huge rectangle after, what looks quite silly now...). I don't get any option to verify a signature though (after reopening this signed document, I got a warning that document is signed, but the signature can not be verified...). |
|
I've noticed NSS password is now required whenever I open any PDF. It is quite annoying, as I expect to sign only about 1/1000 of all PDFs opened. Maybe it would be better if NSS password is required only after Digital Signature action (sign or verify) is initiated? |
|
This would be unacceptable to those who never sign or interact with signed documents
unless a null passphrase can be set and the interface bypassed.
|
|
I'm not sure a null passphrase is an option. Considering I've already had the NSS database in my home directory (before testing that), with my government issued certificate already in it, I guess it is used by some other applications (I'd say Firefox is the first candidate, or some of the dedicated signing applications I have) and there shall be some security to protect such certificates. Plus, it would be a bit strange to request from atril users to first set a null NSS passphrase before they can normally use atril as by now, or? |
|
Then we just have to ensure that this does not ask for a passphrase on encountering an unsigned document,
or leave this out until someone comes up with a fix.
|
|
I've been asked for a passphrase also while opening an already signed document... |
|
no worries, I will fix this issue before this PR gets merged :) |
|
Now, the NSS database password prompt appears only when it is actually required:
If libsecret is available, the password is cached in the GNOME Keyring. This means you only need to enter your password once, the first time the NSS database is unlocked. |
|
It works as expected now, thank you. It is also possible to draw a small rectangle now. But, if it is too small for the preselected font size, some text is simply cropped (on the right and / or at the bottom). I'm not sure it is OK like that - it would probably be better to reduce the font size (as long as the text can still be anyhow readable) or to refuse signing (request to draw a wider / taller rectangle)? And, a nice addition, if it would not be too complicated to implement - an old Adobe Reader 11 has the ability to add a drawing (scanned hand written signature) to a digital signature (it is positioned on the left side, and signature text / details right to it). Looks really cool. Adobe Reader mentioned also adds an Adobe logo as a watermark / background below the signature text, and it looks quite fancy. Atril logo (the one as on Atril icon) watermark would probably make such a signature look really professional... ;-) |
cool :) yeah this are features I want to look into in the future, but to not further complicate this PR, I would add those features in other PRs later on |
inspired by https://gitlab.gnome.org/GNOME/evince/-/merge_requests/488
Testing Digital Signing in Atril
Prerequisites
Install NSS tools (needed to manage the certificate database that poppler uses):
1. Create a Self-Signed Test Certificate
Poppler uses the system NSS database at
~/.pki/nssdb. The database already exists; we just need to add a test certificate.2. Import the Certificate into the NSS Database
You should see "Test Signer" in the list.
3. Mark the Certificate as Trusted for Signing
4. Run Atril and Test
# Run the freshly built atril ./build/shell/atril ./atril/test/test-links.pdfSteps in the UI:
/tmp/signed.pdf)Verify the signature was written:
Or use a recent version of Okular which has signing support.