[Poppler-bugs] [Bug 94376] Implement digital signature support (glib frontend)

[bugzilla-daemon at freedesktop.org]

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>