GNOME Bugzilla – Bug 614929
Verifying digital signatures
Last modified: 2018-05-22 13:50:52 UTC
PDF documents can be digitally signed, see: http://www.adobe.com/devnet/acrobat/pdfs/PDF32000_2008.pdf Section 12.8. Having Verification of these signatures supported in evince would be great since many bills are digitally signed this way nowadays.
Yes, the main problem is that it's not supported by poppler yet: https://bugs.freedesktop.org/show_bug.cgi?id=16770
Just a bump to indicate there are users waiting for this feature.
Does this bug also cover adding signatures to documents, or should I file a separate bug for that?
Best to ask under: https://bugs.freedesktop.org/show_bug.cgi?id=16770
Created attachment 223749 [details] [review] Create new interface in libdocument
Created attachment 223751 [details] [review] New job for getting signatures info
Created attachment 223752 [details] [review] Create new sidebar to contain the info
Created attachment 223753 [details] [review] Changes to the window to include the sidebar and new message at the top
Created attachment 223754 [details] [review] Implement the new interface in the pdf backend
Created attachment 223755 [details] [review] Little change to evince-document.h
Here they are, 6 patches that add support for digital signatures in pdf files. In backend/pdf/ev-poppler.cc there are some functions called not yet in upstream poppler, but they will land soon. That's the reason I didn't delete the test function there. This way you can quickly review the changes to evince not caring about poppler changes. Things to look for... Probably objects references and stuff like that I didn't really got my head around.
Review of attachment 223749 [details] [review]: New public API needs to have gtk-doc comments (including introspection annotations where appropriate), and added to libevdocument-sections.txt. And new classes need gtk-doc SECTION:… etc, and be added to libevdocument-docs.xml (in help/reference/libdocument/). ::: libdocument/ev-document-signatures.c @@ +18,3 @@ + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. + */ + Missing include for config.h @@ +27,3 @@ +{ +} + Missing gtk-doc comment. @@ +35,3 @@ + return iface->has_signatures (document_signatures); +} + Missing gtk-doc comment (and introspection annotation about the returned list type). ::: libdocument/ev-document-signatures.h @@ +2,3 @@ + * this file is part of evince, a gnome document viewer + * + * Copyright (C) 2012 Vasco Dias <contact@vascodias.me> Make that © (also in all the other files). ::: libdocument/ev-signature.c @@ +19,3 @@ + */ + +#include <config.h> "config.h" not <config.h> @@ +50,3 @@ + EvSignature *signature = EV_SIGNATURE (object); + + // clean all the private fields No C++ comments; use /* … */ @@ +52,3 @@ + // clean all the private fields + if (signature->priv->signer_name) { + g_free (signature->priv->signer_name); g_free is NULL safe, so no need to do if (foo) { g_free (foo); } @@ +152,3 @@ + g_object_class_install_property (g_object_class, + PROP_DOC_VALID, + g_param_spec_boolean ("is-doc-valid", Shouldn't this be named "valid-signature" or sth like that then? @@ +156,3 @@ + "If the document signed with the signature is valid", + FALSE, + G_PARAM_READWRITE | Really writable? @@ +162,3 @@ + g_object_class_install_property (g_object_class, + PROP_ID_KNOWN, + g_param_spec_boolean ("is-id-known", "signer-identity-known" or so? @@ +172,3 @@ + g_object_class_install_property (g_object_class, + PROP_SIGN_TIME, + g_param_spec_string ("sign-time", "signature-time" or so? There's really no need to use abbreviations here. Also, why string and not a GDateTime (param_spec_boxed with G_TYPE_DATE_TIME) ? ::: libdocument/ev-signature.h @@ +50,3 @@ + +struct _EvSignatureClass { + GObjectClass base_class; Add some padding here: /*< private >*/ gpointer padding[8];
Review of attachment 223751 [details] [review]: Need gtk-doc stuff (see previous comments). ::: libview/ev-jobs.c @@ +595,3 @@ + g_list_free (job->signatures); + job->signatures = NULL; + } g_list_free_full is simpler.
Review of attachment 223752 [details] [review]: ::: shell/ev-sidebar-signatures.c @@ +22,3 @@ + +#include <glib/gi18n.h> // ----- support for translation + No C++ comments. @@ +435,3 @@ + COL_HAS_ICON, TRUE, + COL_MAKE_BOLD, TRUE, + -1); Use gtk_tree_store_insert_with_values instead of append + set.
Review of attachment 223753 [details] [review]: ::: shell/ev-window.c @@ +789,3 @@ + EvWindow *window) +{ + // force the sidebar to show and open the signatures No C++ comments. @@ +868,3 @@ + const gchar *format, + ...) +{ Add a prototype for this, with a G_GNUC_PRINTF attribute, for type safety. @@ +882,3 @@ + area = ev_message_area_new (GTK_MESSAGE_INFO, + msg, + _("See detailed information..."), … instead of . . .
Review of attachment 223754 [details] [review]: ::: backend/pdf/ev-poppler.cc @@ +3364,3 @@ + break; + default: + g_warning ("error trying to get signature information"); Is this a runtime or programming error? In the former case, don't g_warning; in the latter case, just fix it. If it's for forward-compatibility, just don't warn (spew on console is bad).
Review of attachment 223755 [details] [review]: Squash this commit to the one that introduces this new header.
Review of attachment 223754 [details] [review]: ::: backend/pdf/ev-poppler.cc @@ +3364,3 @@ + break; + default: + g_warning ("error trying to get signature information"); When we call poppler to verify the signatures in the file errors can occur. This case is, for example, when the signature info in the pdf file is broken, mal-formed or any other strange errors appear when the openssl is trying to read that info. We have no way to get this information beforehand.
Review of attachment 223749 [details] [review]: ::: libdocument/ev-document-signatures.c @@ +27,3 @@ +{ +} + Sorry, but could you give an example of what I should do with gtk-doc? I'm new to this and none of the other files in the project have an example regarding this. The same for the other gtk-doc notes. ::: libdocument/ev-document-signatures.h @@ +2,3 @@ + * this file is part of evince, a gnome document viewer + * + * Copyright (C) 2012 Vasco Dias <contact@vascodias.me> All the other files have this with (C), are you sure you want me to change it? ::: libdocument/ev-signature.c @@ +156,3 @@ + "If the document signed with the signature is valid", + FALSE, + G_PARAM_READWRITE | I was told that to create an object that only accepts the properties at construct time it would require the writable flag. Am I wrong? @@ +172,3 @@ + g_object_class_install_property (g_object_class, + PROP_SIGN_TIME, + g_param_spec_string ("sign-time", That info comes directly from the pdf signature's metadata, it already comes as a string. ::: libdocument/ev-signature.h @@ +50,3 @@ + +struct _EvSignatureClass { + GObjectClass base_class; I don't think I should work aroung compiler or memory implementation details this way. All the other code in the project have it this way. But if you are really sure I should do it I will.
Review of attachment 223753 [details] [review]: ::: shell/ev-window.c @@ +868,3 @@ + const gchar *format, + ...) +{ Sorry, I don't get what I should do with this...
(In reply to comment #18) > Review of attachment 223754 [details] [review]: > > ::: backend/pdf/ev-poppler.cc > @@ +3364,3 @@ > + break; > + default: > + g_warning ("error trying to get signature information"); > > When we call poppler to verify the signatures in the file errors can occur. > > This case is, for example, when the signature info in the pdf file is broken, > mal-formed or any other strange errors appear when the openssl is trying to > read that info. > > We have no way to get this information beforehand. Then that should be a message to the user (using a GError to propagate the info back to the ev-window code), *not* a message on console (that no one reads). (In reply to comment #19) > Review of attachment 223749 [details] [review]: > > ::: libdocument/ev-document-signatures.c > @@ +27,3 @@ > +{ > +} > + > > Sorry, but could you give an example of what I should do with gtk-doc? /** * SECTION:ev-foobar * @short_description: Foor bar * * Some utility functions to do foo with bar. * * Since: 3.6 */ /** * ev_foo_bar: * @foo: a #EvFoo * * Does foo with bar. * * Since: 3.6 */ void ev_foo_bar (EvFoo *foo) { ... } > I'm new to this and none of the other files in the project have an example > regarding this. There are a *few*, e.g. ev-macros.c has SECTION , and most .c files have gtk-doc comments for the individual functions. And the format of libevdocument-sections.txt and libevdocument-docs.xml should be easy to understand. https://live.gnome.org/DocumentationProject/GtkDoc might help. > The same for the other gtk-doc notes. > > ::: libdocument/ev-document-signatures.h > @@ +2,3 @@ > + * this file is part of evince, a gnome document viewer > + * > + * Copyright (C) 2012 Vasco Dias <contact@vascodias.me> > > All the other files have this with (C), are you sure you want me to change it? Well I use © for new files, but ok. > ::: libdocument/ev-signature.c > @@ +156,3 @@ > + "If the document > signed with the signature is valid", > + FALSE, > + G_PARAM_READWRITE | > > I was told that to create an object that only accepts the properties at > construct time it would require the writable flag. Am I wrong? Oh, you're right. I had overlooked that. > @@ +172,3 @@ > + g_object_class_install_property (g_object_class, > + PROP_SIGN_TIME, > + g_param_spec_string ("sign-time", > > That info comes directly from the pdf signature's metadata, it already comes as > a string. I see that the poppler patch exposes this as a string in its API, but IMHO that's a bug. It should just construct a GDateTime from the string and expose that. > > ::: libdocument/ev-signature.h > @@ +50,3 @@ > + > +struct _EvSignatureClass { > + GObjectClass base_class; > > I don't think I should work aroung compiler or memory implementation details > this way. All the other code in the project have it this way. > But if you are really sure I should do it I will. We don't currently have this in many classes, but IMO no reason not to do it for new classes. And it's not a 'workaround', it's standard gobject practice. (In reply to comment #20) > Review of attachment 223753 [details] [review]: > > ::: shell/ev-window.c > @@ +868,3 @@ > + const gchar *format, > + ...) > +{ > > Sorry, I don't get what I should do with this... Add a prototype for it like this: static void ev_window_info_message_signatures (EvWindow *window, const gchar *format, ...) G_GNUC_PRINTF(2,3);
(In reply to comment #21) > (In reply to comment #18) > > Review of attachment 223754 [details] [review] [details]: > > > > ::: backend/pdf/ev-poppler.cc > > @@ +3364,3 @@ > > + break; > > + default: > > + g_warning ("error trying to get signature information"); > > > > When we call poppler to verify the signatures in the file errors can occur. > > > > This case is, for example, when the signature info in the pdf file is broken, > > mal-formed or any other strange errors appear when the openssl is trying to > > read that info. > > > > We have no way to get this information beforehand. > > Then that should be a message to the user (using a GError to propagate the info > back to the ev-window code), *not* a message on console (that no one reads). > I think I'll just delete the g_warning message then. Currently when this happens the signatures sidebar shows up without any content. If I wanted to use GError to handle this how should I go about it? > Oh, you're right. I had overlooked that. > > @@ +172,3 @@ > > + g_object_class_install_property (g_object_class, > > + PROP_SIGN_TIME, > > + g_param_spec_string ("sign-time", > > > > That info comes directly from the pdf signature's metadata, it already comes as > > a string. > > I see that the poppler patch exposes this as a string in its API, but IMHO > that's a bug. It should just construct a GDateTime from the string and expose > that. > Well, poppler just gives me what the openssl gets from the signature fields. Is there any easy way to convert from a string with that format to a GDateTime object? > > > > ::: libdocument/ev-signature.h > > @@ +50,3 @@ > > + > > +struct _EvSignatureClass { > > + GObjectClass base_class; > > > > I don't think I should work aroung compiler or memory implementation details > > this way. All the other code in the project have it this way. > > But if you are really sure I should do it I will. > > We don't currently have this in many classes, but IMO no reason not to do it > for new classes. And it's not a 'workaround', it's standard gobject practice. > I'll add it. I didn't find many examples or explanation of what it does... Care to ellaborate? Thanks > (In reply to comment #20) > > Review of attachment 223753 [details] [review] [details]: > > > > ::: shell/ev-window.c > > @@ +868,3 @@ > > + const gchar *format, > > + ...) > > +{ > > > > Sorry, I don't get what I should do with this... > > Add a prototype for it like this: > > static void > ev_window_info_message_signatures (EvWindow *window, > const gchar *format, > ...) G_GNUC_PRINTF(2,3); Ok, I'll add it. I don't really understand the documentation that I read about it, can you explain me what it does?
Created attachment 223918 [details] [review] Create new interface in libdocument v2
Created attachment 223919 [details] [review] New job for getting signatures info v2
Created attachment 223920 [details] [review] Create new sidebar to contain the info v2
Created attachment 223921 [details] [review] Changes to the window to include the sidebar and new message at the top v2
Created attachment 223922 [details] [review] Implement the new interface in the pdf backend v2
Created attachment 223923 [details] [review] Changes to the window to include the sidebar and new message at the top v2.1 Replacing this patch, I added the G_GNUC_PRINTF in the wrong place and didn't test it.
Created attachment 224018 [details] [review] Create new interface in libdocument v2.1 Changed the way the GENERIC_ERROR is handled. This way we can show information to the user.
Created attachment 224019 [details] [review] Implement the new interface in the pdf backend v2.1 Changed the way the GENERIC_ERROR is handled. This way we can show information to the user.
Created attachment 224020 [details] [review] Create new sidebar to contain the info v2.1 Handle the case when the signature comes without info.
Created attachment 224021 [details] [review] Implement the new interface in the pdf backend v2.1 Changed the way the GENERIC_ERROR is handled. This way we can show information to the user.
Another bug relating to digital signatures, maybe it would be possible to share some code to solve it to: https://bugzilla.gnome.org/show_bug.cgi?id=640185
Anything ready to be tested here yet?
Basic signature verification support has now been merged in Poppler: https://bugs.freedesktop.org/show_bug.cgi?id=16770
The support for poppler-glib frontend is not ready yet: https://bugs.freedesktop.org/show_bug.cgi?id=94376
-- GitLab Migration Automatic Message -- This bug has been migrated to GNOME's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/evince/issues/143.