2006-11-11 Marcus Brinkmann <marcus@g10code.de>

	* src/error-mapping.c (scute_gpg_err_to_ck): Report error on debug
	stream.
	* src/slots.c (add_object): New function.
	(slot_init): Rewritten using add_object.
	* src/gpgsm.c: Include "gpgsm.h".
	(struct search): Replace members ATTRP, ATTR_COUNTP, PRV_ATTRP,
	PRV_ATTR_COUNTP by CERT_GET_CB, HOOK.
	(search_cb): Rewritten to add all certificates for a certain key
	(scute_gpgsm_get_cert): Take a callback function instead of
	attribute pointers.
	* gpgsm.h (scute_gpgsm_get_cert): Adjust prototype.	
	and also the certificate chain.
	* src/cert.h (scute_gpgsm_search_certs_by_fpr): New prototype.
	(struct cert): New member chain_id.
	* src/cert-gpgsm.c (struct search_ctx_by_grip): Rename to ...
	(struct search_ctx): ... this.  Rename field GRIP to PATTERN, and
	add new field FIELD.
	(search_certs_by_grip): Rename function to ...
	(search_certs): ... this.
	(scute_gpgsm_search_certs_by_fpr): Change user of the above.
	(scute_gpgsm_search_certs_by_fpr): New function.
	(search_certs_line): Store chain ID.

ChangeLog

@@ -1,3 +1,28 @@
+2006-11-11  Marcus Brinkmann  <marcus@g10code.de>
+
+	* src/error-mapping.c (scute_gpg_err_to_ck): Report error on debug
+	stream.
+	* src/slots.c (add_object): New function.
+	(slot_init): Rewritten using add_object.
+	* src/gpgsm.c: Include "gpgsm.h".
+	(struct search): Replace members ATTRP, ATTR_COUNTP, PRV_ATTRP,
+	PRV_ATTR_COUNTP by CERT_GET_CB, HOOK.
+	(search_cb): Rewritten to add all certificates for a certain key
+	(scute_gpgsm_get_cert): Take a callback function instead of
+	attribute pointers.
+	* gpgsm.h (scute_gpgsm_get_cert): Adjust prototype.	
+	and also the certificate chain.
+	* src/cert.h (scute_gpgsm_search_certs_by_fpr): New prototype.
+	(struct cert): New member chain_id.
+	* src/cert-gpgsm.c (struct search_ctx_by_grip): Rename to ...
+	(struct search_ctx): ... this.  Rename field GRIP to PATTERN, and
+	add new field FIELD.
+	(search_certs_by_grip): Rename function to ...
+	(search_certs): ... this.
+	(scute_gpgsm_search_certs_by_fpr): Change user of the above.
+	(scute_gpgsm_search_certs_by_fpr): New function.
+	(search_certs_line): Store chain ID.
+
 2006-11-07  Marcus Brinkmann  <marcus@g10code.de>
 
 	* src/p11-gettokeninfo.c (C_GetTokenInfo): Fix assignment.

README

@@ -11,6 +11,7 @@ TOC
 * Prerequisites
 * Installation
 * Client Authentication
+* Troubleshooting
 * Features and Limitations
 * Development
 * Mozilla Bugs
@@ -261,6 +262,35 @@ passphrase needs to be provided when starting up Apache), and
 server.crt has "CN=localhost" as part of its DN for this example.
 
 
+Troubleshooting
+===============
+
+Symptom: Loading the Scute security device in the security device
+manager of Firefox fails with "Unable to load module".
+
+Solution: Make sure that Scute is correctly installed, and that all
+libraries and executables are available.  Make sure that gpg-agent is
+running and can be found via the environment variable GPG_AGENT_INFO.
+
+
+Symptom: Client authentication fails with "<example.com> has received
+an incorrect or unexpected message.  Error code: -12227".
+
+Solution: Make sure that the correct OpenPGP card is inserted and the
+certificate available in GPGSM.  Check that the OpenPGP card is
+detected correctly in the security device manager and the
+corresponding certificate is displayed in the certificate manager of
+Firefox.
+
+
+Symptom: The OpenPGP card is detected and displayed in the security
+device manager in Firefox, but no corresponding certificate is
+displayed in the certificate manager of Firefox.
+
+Solution: Make sure that the corresponding certificate is imported in
+GPGSM.
+
+
 Features and Limitations
 ========================
 

TODO

@@ -1,8 +1,4 @@
 * Undergoing clean-up:
-** table.{h,c}: Simplify, remove hurdism.
-** agent.{h,c}: Clean namespace.
-** gpgsm.c: Support more than one keygrip.
-** slots.{h,c}: Cleanup lookup function and the rest.
 ** p11-closeallsessions.c, p11-closesession.c, p11-findobjects.c,
    p11-findobjectsfinal.c, p11-findobjectsinit.c, p11-getattributevalue.c,
    p11-getmechanisminfo.c, p11-getmechanismlist.c, p11-getsessioninfo.c,
@@ -10,20 +6,24 @@
    p11-opensession.c, p11-sign.c, p11-signinit.c
 
 * Bugs or misfeatures:
-** If more than one certificate with the same keygrip is installed in
-   gpgsm, only the first one is found.  We probably should return them
-   all and let Mozilla choose among them (if Mozilla supports that,
-   otherwise we need to make it configurable).
+** Mozilla presents the other certificates in "Websites".  Only the
+   first one is presented in the certicate manager under "Personal".
+** Mozilla does not unload the right security token!!!
+** Duplicate certificates should be removed from the object list (this
+   can occur when including all certificate chains).
 
 * Missing features:
 ** Implement random number generation function C_GenerateRandom.
 ** Add canonical gnupg logging module.
+** Mozilla ignores the CKA_TRUSTED attribute to certificates, so
+   exporting the information from GPGSM (ISTRUSTED) will not be
+   useful.  It's unclear if this can be improved in a meaningful way.
 
 * Standard ambiguities, or non-conformance in the applications:
 ** If the token is removed, the current sessions are closed.  If then
    a new token is inserted, and the application calls C_OpenSession, a
-   previously used session handle may be reused.  It is not clear to
-   me what behaviour the standard specifies in this case.
+   previously used session handle may be reused.  It is not clear what
+   behaviour the standard specifies in this case.
 
 ** Mozilla NSS has this comment (and relies on the assumption):
   "check to see if the module has added new slots. PKCS 11 v2.20

src/agent.c

@@ -616,7 +616,6 @@ scute_agent_sign (char *grip, unsigned char *data, int len,
 
   err = assuan_transact (agent_ctx, "PKSIGN",
 			 pksign_cb, &sig, NULL, NULL, NULL, NULL);
-  printf ("Returning ERR = %u\n", err);
   if (err)
     return err;
 

src/cert-gpgsm.c

@@ -393,15 +393,10 @@ search_certs_line (struct search_ctx *ctx)
 	  if (fields >= 10 && strlen (field[9]) <= sizeof (cert->fpr) - 1)
 	    strcpy (cert->fpr, field[9]);
 
-#if 0
 	  /* Field 13 has the gpgsm chain ID (take only the first one).  */
-	  if (fields >= 13 && !key->chain_id && *field[12])
-	    {
-	      key->chain_id = strdup (field[12]);
-	      if (!key->chain_id)
-		return gpg_error_from_errno (errno);
-	    }
-#endif
+	  if (fields >= 13 && strlen (field[12])
+	      <= sizeof (cert->chain_id) - 1)
+	    strcpy (cert->chain_id, field[12]);
 	}
       break;
 
@@ -511,10 +506,13 @@ scute_gpgsm_search_certs (assuan_context_t ctx, cert_search_cb_t search_cb,
 }
 
 
-struct search_ctx_by_grip
+struct search_ctx_by_field
 {
-  /* The grip we are looking for.  */
-  const char *grip;
+  /* What we are searching for.  */
+  enum { SEARCH_BY_GRIP, SEARCH_BY_FPR } field;
+
+  /* The pattern we are looking for.  */
+  const char *pattern;
 
   cert_search_cb_t search_cb;
   void *search_cb_hook;
@@ -579,12 +577,13 @@ export_cert (char *fpr, struct cert *cert)
 
 
 static gpg_error_t
-search_certs_by_grip (void *hook, struct cert *cert)
+search_certs_by_field (void *hook, struct cert *cert)
 {
-  struct search_ctx_by_grip *ctx = hook;
+  struct search_ctx_by_field *ctx = hook;
   gpg_error_t err = 0;
 
-  if (!strcmp (ctx->grip, cert->grip))
+  if ((ctx->field == SEARCH_BY_GRIP && !strcmp (ctx->pattern, cert->grip))
+      || (ctx->field == SEARCH_BY_FPR && !strcmp (ctx->pattern, cert->fpr)))
     {
       if (strlen (cert->fpr) != 40)
 	return gpg_error (GPG_ERR_GENERAL);
@@ -612,17 +611,45 @@ scute_gpgsm_search_certs_by_grip (const char *grip,
   gpg_error_t err;
   assuan_context_t ctx;
   const char *argv[] = { "gpgsm", "--server", NULL };
-  struct search_ctx_by_grip search;
+  struct search_ctx_by_field search;
+
+  err = assuan_pipe_connect (&ctx, GPGSM_PATH, argv, NULL);
+  if (err)
+    return err;
+
+  search.field = SEARCH_BY_GRIP;
+  search.pattern = grip;
+  search.search_cb = search_cb;
+  search.search_cb_hook = search_cb_hook;
+
+  err = scute_gpgsm_search_certs (ctx, &search_certs_by_field, &search);
+  assuan_disconnect (ctx);
+  return err;
+}
+
+
+/* Invoke SEARCH_CB for each certificate found using assuan connection
+   CTX to GPGSM.  */
+gpg_error_t
+scute_gpgsm_search_certs_by_fpr (const char *fpr,
+				 cert_search_cb_t search_cb,
+				 void *search_cb_hook)
+{
+  gpg_error_t err;
+  assuan_context_t ctx;
+  const char *argv[] = { "gpgsm", "--server", NULL };
+  struct search_ctx_by_field search;
 
   err = assuan_pipe_connect (&ctx, GPGSM_PATH, argv, NULL);
   if (err)
     return err;
 
-  search.grip = grip;
+  search.field = SEARCH_BY_FPR;
+  search.pattern = fpr;
   search.search_cb = search_cb;
   search.search_cb_hook = search_cb_hook;
 
-  err = scute_gpgsm_search_certs (ctx, &search_certs_by_grip, &search);
+  err = scute_gpgsm_search_certs (ctx, &search_certs_by_field, &search);
   assuan_disconnect (ctx);
   return err;
 }

src/cert.h

@@ -79,6 +79,9 @@ struct cert
   /* The key grip.  */
   unsigned char grip[41];
 
+  /* The chain ID.  */
+  unsigned char chain_id[41];
+
   /* The certificate in DER format.  This is not entered by the search
      function, but afterwards by the filter before converting it into
      a PKCS #11 object.  */
@@ -100,6 +103,12 @@ gpg_error_t scute_gpgsm_search_certs_by_grip (const char *grip,
 					      cert_search_cb_t search_cb,
 					      void *search_cb_hook);
 
+/* Invoke SEARCH_CB for each certificate found using assuan connection
+   CTX to GPGSM.  */
+gpg_error_t scute_gpgsm_search_certs_by_fpr (const char *fpr,
+					     cert_search_cb_t search_cb,
+					     void *search_cb_hook);
+     
 
 /* From cert-object.c.  */
 

src/error-mapping.c

@@ -40,6 +40,7 @@
 #include <gpg-error.h>
 
 #include "cryptoki.h"
+#include "debug.h"
 
 #include "error-mapping.h"
 
@@ -67,6 +68,10 @@ scute_sys_to_ck (error_t err)
 CK_RV
 scute_gpg_err_to_ck (gpg_error_t err)
 {
+  if (err)
+    DEBUG ("Error occured: %s (%s)\n", gpg_strerror (err),
+	   gpg_strsource (err));
+
   switch (gpg_err_code (err))
     {
     case GPG_ERR_NO_ERROR:

src/gpgsm.c

@@ -47,15 +47,15 @@
 #include "cryptoki.h"
 #include "support.h"
 #include "cert.h"
+#include "gpgsm.h"
 
 
 struct search
 {
   bool found;
-  CK_ATTRIBUTE_PTR *attrp;
-  CK_ULONG *attr_countp;
-  CK_ATTRIBUTE_PTR *prv_attrp;
-  CK_ULONG *prv_attr_countp;
+  cert_get_cb_t cert_get_cb;
+  void *hook;
+
 };
 
 
@@ -63,26 +63,45 @@ static gpg_error_t
 search_cb (void *hook, struct cert *cert)
 {
   struct search *ctx = hook;
-  gpg_error_t err;
-  
-  /* FIXME: Support more than one certificate.  */
-  if (ctx->found)
-    return 0;
+  gpg_error_t err = 0;
 
-  /* Turn this into a certificate object.  */
-  err = scute_attr_cert (cert, ctx->attrp, ctx->attr_countp);
+  CK_ATTRIBUTE_PTR attrp;
+  CK_ULONG attr_countp;
+
+  /* Add the private key object only once.  */
+  if (!ctx->found)
+    {
+      err = scute_attr_prv (cert, &attrp, &attr_countp);
+      if (err)
+	return err;
+
+      err = (*ctx->cert_get_cb) (ctx->hook, attrp, attr_countp);
+      if (err)
+	{
+	  scute_attr_free (attrp, attr_countp);
+	  return err;
+	}
+
+      ctx->found = true;
+    }
+
+  /* Add the certificate chain recursively before adding the
+     certificate.  */
+  if (strcmp (cert->chain_id, cert->fpr))
+    err = scute_gpgsm_search_certs_by_fpr (cert->chain_id, search_cb, ctx);
+
+  /* Turn this certificate into a certificate object.  */
+  err = scute_attr_cert (cert, &attrp, &attr_countp);
   if (err)
     return err;
-
-  err = scute_attr_prv (cert, ctx->prv_attrp, ctx->prv_attr_countp);
+  
+  err = (*ctx->cert_get_cb) (ctx->hook, attrp, attr_countp);
   if (err)
     {
-      scute_attr_free (*ctx->attrp, *ctx->attr_countp);
-      *ctx->attrp = NULL;
-      *ctx->attr_countp = 0;
+      scute_attr_free (attrp, attr_countp);
+      return err;
     }
 
-  ctx->found = true;
   return err;
 }
 
@@ -92,23 +111,14 @@ search_cb (void *hook, struct cert *cert)
    and ATTR_COUNTP, and for the private key object in PRV_ATTRP
    and PRV_ATTR_COUNTP.  */
 gpg_error_t
-scute_gpgsm_get_cert (char *grip,
-		      CK_ATTRIBUTE_PTR *attrp, CK_ULONG *attr_countp,
-		      CK_ATTRIBUTE_PTR *prv_attrp, CK_ULONG *prv_attr_countp)
+scute_gpgsm_get_cert (char *grip, cert_get_cb_t cert_get_cb, void *hook)
 {
   gpg_error_t err;
   struct search search;
 
-  *attrp = NULL;
-  *attr_countp = 0;
-  *prv_attrp = NULL;
-  *prv_attr_countp = 0;
-
   search.found = false;
-  search.attrp = attrp;
-  search.attr_countp = attr_countp;
-  search.prv_attrp = prv_attrp;
-  search.prv_attr_countp = prv_attr_countp;
+  search.cert_get_cb = cert_get_cb;
+  search.hook = hook;
 
   err = scute_gpgsm_search_certs_by_grip (grip, search_cb, &search);
   

src/gpgsm.h

@@ -37,15 +37,20 @@
 
 #include "cryptoki.h"
 
+#include "table.h"
+
 
+/* The callback type invoked for each certificate found in the
+   search.  */
+typedef gpg_error_t (*cert_get_cb_t) (void *hook, 
+				      CK_ATTRIBUTE_PTR attrp,
+				      CK_ULONG attr_countp);
+
 /* Create the attributes required for a new certificate object.
    Returns allocated attributes for the certificate object in ATTRP
    and ATTR_COUNTP, and for the private key object in PRV_ATTRP
    and PRV_ATTR_COUNTP.  */
-gpg_error_t scute_gpgsm_get_cert (char *grip,
-				  CK_ATTRIBUTE_PTR *attrp,
-				  CK_ULONG *attr_countp,
-				  CK_ATTRIBUTE_PTR *prv_attrp,
-				  CK_ULONG *prv_attr_countp);
+gpg_error_t scute_gpgsm_get_cert (char *grip, cert_get_cb_t cert_get_cb,
+				  void *hook);
 
 #endif	/* GPGSM_H */

src/slots.c

@@ -354,49 +354,49 @@ slot_reset (slot_iterator_t id)
 }
 
 
+static gpg_error_t
+add_object (void *hook, CK_ATTRIBUTE_PTR attrp,
+	    CK_ULONG attr_countp)
+{
+  gpg_error_t err;
+  struct slot *slot = hook;
+  struct object *object;
+  unsigned int oidx;
+  void *objp;
+
+  err = scute_table_alloc (slot->objects, &oidx, &objp, NULL);
+  if (err)
+    return err;
+
+  object = objp;
+  object->attributes = attrp;
+  object->attributes_count = attr_countp;
+
+  return 0;
+}
+
+
 /* Initialize the slot after a token has been inserted.  SLOT->info
    must already be valid.  */
 static gpg_error_t
 slot_init (slot_iterator_t id)
 {
-  gpg_error_t err;
+  gpg_error_t err = 0;
   struct slot *slot = scute_table_data (slots, id);
-  struct object objects[2];
-  unsigned int oidxs[2];
-  void *objp;
 
-
-  err = scute_gpgsm_get_cert (slot->info.grip3,
-			      &objects[0].attributes,
-			      &objects[0].attributes_count,
-			      &objects[1].attributes,
-			      &objects[1].attributes_count);
-  if (err)
-    return scute_gpg_err_to_ck (err);
-  
-  err = scute_table_alloc (slot->objects, &oidxs[0], &objp, NULL);
-  if (err)
-    {
-      object_dealloc (&objects[0]);
-      object_dealloc (&objects[1]);
-      return err;
-    }
-  memcpy (objp, &objects[0], sizeof (objects[0]));
-  
-  err = scute_table_alloc (slot->objects, &oidxs[1], &objp, NULL);
+  err = scute_gpgsm_get_cert (slot->info.grip3, add_object, slot);
   if (err)
-    {
-      scute_table_dealloc (slot->objects, &oidxs[0]);
-      object_dealloc (&objects[1]);
-      return err;
-    }
-  memcpy (objp, &objects[1], sizeof (objects[1]));
-  
+    goto init_out;
+
   /* FIXME: Perform the rest of the initialization of the
      token.  */
   slot->token_present = true;
 
-  return 0;
+ init_out:
+  if (err)
+    slot_reset (id);
+
+  return err;
 }
 
 
@@ -436,13 +436,11 @@ slots_update_slot (slot_iterator_t id)
   if (gpg_err_code (err) == GPG_ERR_CARD_REMOVED
       || gpg_err_code (err) == GPG_ERR_CARD_NOT_PRESENT)
     /* Nothing to do.  */
-    ;
-  else if (err)
-    return scute_gpg_err_to_ck (err);
-  else
+    err = 0;
+  else if (err == 0)
     err = slot_init (id);
 
-  return CKR_OK;
+  return scute_sys_to_ck (err);
 }