[Poppler-bugs] [Bug 94376] Implement digital signature support (glib frontend)
bugzilla-daemon at freedesktop.org
bugzilla-daemon at freedesktop.org
Sun Jun 26 07:16:33 UTC 2016
https://bugs.freedesktop.org/show_bug.cgi?id=94376
--- Comment #2 from Carlos Garcia Campos <carlosgc at gnome.org> ---
Comment on attachment 124622
--> https://bugs.freedesktop.org/attachment.cgi?id=124622
Expose signature verification in the glib frontend
Review of attachment 124622:
-----------------------------------------------------------------
Thanks for the patch and sorry for the delay to review it. I haven't followed
the core implementation, so I'm not sure how this API is expected to be used by
applications. I guess the general idea is that the document might contain one
or more signatures (defined as form fields), and applications can verify the
document by validating every form field, right?
::: poppler-tmp/poppler-0.45.0/glib/poppler-document.cc
@@ +1105,4 @@
> }
>
> /**
> + * poppler_document_count_signatures:
In glib API we normally use n_signatures instead of count
@@ +1119,5 @@
> +poppler_document_count_signatures(PopplerDocument *document)
> +{
> + g_return_val_if_fail (POPPLER_IS_DOCUMENT (document), 0);
> + return document->doc->getSignatureWidgets().size();
> +}
What's the point of providing the number of signatures, how is the user
expected to use this? Just to know whether the document needs to be verified?
@@ +1478,5 @@
> FALSE,
> G_PARAM_READABLE));
> + /**
> + * PopplerDocument::signed:
> + * Whether the document is signed.
This doesn't match the property definition. I'm not sure we need a property for
this, though.
::: poppler-tmp/poppler-0.45.0/glib/poppler-form-field.cc
@@ +703,5 @@
> +
> + new_info = g_new0(PopplerSignatureInfo, 1);
> + new_info->sig_status = sig_info->sig_status;
> + new_info->cert_status = sig_info->cert_status;
> + new_info->signer_name = sig_info->signer_name;
This is owned by the other PopplerSignatureInfo, if you don't duplicate the
string, freeing one structure would leave the other one with an invalid
pointer.
@@ +704,5 @@
> + new_info = g_new0(PopplerSignatureInfo, 1);
> + new_info->sig_status = sig_info->sig_status;
> + new_info->cert_status = sig_info->cert_status;
> + new_info->signer_name = sig_info->signer_name;
> + new_info->signing_time = sig_info->signing_time;
You can do *new_info = *sig_info and then copy manually only the fields needed,
the signer name in this case.
@@ +712,5 @@
> +
> +void
> +poppler_signature_info_free (PopplerSignatureInfo *siginfo)
> +{
> + g_free(siginfo);
And here you should also free the signer name
@@ +716,5 @@
> + g_free(siginfo);
> +}
> +
> +PopplerSignatureInfo *
> +poppler_form_field_signature_validate (PopplerFormField *sigField, gboolean doVerifyCert, gboolean forceRevalidation)
This method also need to be documented. What do the parameters mean? I prefer
to use flags oinstead of boolean parameters if possible.
@@ +725,5 @@
> + FormFieldSignature * sig_field = static_cast<FormFieldSignature*>(sigField->widget->getField());
> + SignatureInfo * sig_info = sig_field->validateSignature(doVerifyCert, forceRevalidation);
> +
> + PopplerSignatureInfo * poppler_sig_info;
> + poppler_sig_info = g_new0(PopplerSignatureInfo, 1);
Maybe this could be an out parameter, so that it can be stack allocated by the
caller. And the method could return the validation result.
@@ +772,5 @@
> + case CERTIFICATE_GENERIC_ERROR:
> + poppler_sig_info->cert_status = POPPLER_CERTIFICATE_GENERIC_ERROR;
> + break;
> + case CERTIFICATE_NOT_VERIFIED:
> + poppler_sig_info->cert_status = POPPLER_CERTIFICATE_NOT_VERIFIED;
So we are validating two things in the same function? Are really sig and cert
status part of the signature information?
@@ +776,5 @@
> + poppler_sig_info->cert_status = POPPLER_CERTIFICATE_NOT_VERIFIED;
> + break;
> + }
> +
> + poppler_sig_info->signer_name = sig_info->getSignerName();
The return value is owned by SignatureInfo, we should copy it, and
SignatureInfo should return a const char* instead.
--
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/poppler-bugs/attachments/20160626/d2f4c706/attachment.html>
More information about the Poppler-bugs
mailing list