Refactor cache_objects() loop and object type handling

Reviewed-by: Tomas Mraz <tomas@openssl.org>
Reviewed-by: David von Oheimb <david.von.oheimb@siemens.com>
(Merged from https://github.com/openssl/openssl/pull/28382)

diff --git a/crypto/x509/by_store.c b/crypto/x509/by_store.c
index 3fa347178733b966603dfa220e9d268943034fcc..a969e3aa05b8c813262aa64f5168195ef64986f5 100644 (file)
--- a/crypto/x509/by_store.c
+++ b/crypto/x509/by_store.c
@@ -25,7 +25,7 @@ DEFINE_STACK_OF(CACHED_STORE)
 static int cache_objects(X509_LOOKUP *lctx, CACHED_STORE *store,
                          const OSSL_STORE_SEARCH *criterion, int depth)
 {
-    int ok = 0;
+    int ok = 1;
     OSSL_STORE_CTX *ctx;
     X509_STORE *xstore = X509_LOOKUP_get_store(lctx);
 
@@ -63,12 +63,16 @@ static int cache_objects(X509_LOOKUP *lctx, CACHED_STORE *store,
 
         /* NULL means error or "end of file".  Either way, we break. */
         if (info == NULL)
+            /*
+             * Cannot rely on OSSL_STORE_error() here:
+             * file_load() incorrectly reports an error at EOF
+             */
             break;
 
         infotype = OSSL_STORE_INFO_get_type(info);
-        ok = 0;
 
-        if (infotype == OSSL_STORE_INFO_NAME) {
+        switch (infotype) {
+        case OSSL_STORE_INFO_NAME:
             /*
              * This is an entry in the "directory" represented by the current
              * uri.  if |depth| allows, dive into it.
@@ -81,27 +85,26 @@ static int cache_objects(X509_LOOKUP *lctx, CACHED_STORE *store,
                 substore.propq = store->propq;
                 ok = cache_objects(lctx, &substore, criterion, depth - 1);
             }
-        } else {
-            /*
-             * We know that X509_STORE_add_{cert|crl} increments the object's
-             * refcount, so we can safely use OSSL_STORE_INFO_get0_{cert,crl}
-             * to get them.
-             */
-            switch (infotype) {
-            case OSSL_STORE_INFO_CERT:
-                ok = X509_STORE_add_cert(xstore,
-                                         OSSL_STORE_INFO_get0_CERT(info));
-                break;
-            case OSSL_STORE_INFO_CRL:
-                ok = X509_STORE_add_crl(xstore,
-                                        OSSL_STORE_INFO_get0_CRL(info));
-                break;
-            }
+            break;
+        /*
+         * We know that X509_STORE_add_{cert|crl} increments the object's
+         * refcount, so we can safely use OSSL_STORE_INFO_get0_{cert,crl}
+         * to get them.
+         */
+        case OSSL_STORE_INFO_CERT:
+            ok = X509_STORE_add_cert(xstore, OSSL_STORE_INFO_get0_CERT(info));
+            break;
+        case OSSL_STORE_INFO_CRL:
+            ok = X509_STORE_add_crl(xstore, OSSL_STORE_INFO_get0_CRL(info));
+            break;
+        default:
+            /* Ignore all other types (PKEY, PUBKEY, PARAMS) */
+            break;
         }
 
         OSSL_STORE_INFO_free(info);
         if (!ok)
-            break;
+            break; /* stop on first failure */
     }
     OSSL_STORE_close(ctx);
 
@@ -275,7 +278,7 @@ static int by_store_subject(X509_LOOKUP *ctx, X509_LOOKUP_TYPE type,
  */
 
 static X509_LOOKUP_METHOD x509_store_lookup = {
-    "Load certs from STORE URIs",
+    "Load certificates and CRLs from OSSL_STORE URIs",
     NULL,                        /* new_item */
     by_store_free,               /* free */
     NULL,                        /* init */

diff --git a/doc/man3/X509_LOOKUP_hash_dir.pod b/doc/man3/X509_LOOKUP_hash_dir.pod
index 22a599692549f222a51df8677c9421b5cb93d59b..35160e87c537f89d7ed0343e7d1a5a088784dad9 100644 (file)
--- a/doc/man3/X509_LOOKUP_hash_dir.pod
+++ b/doc/man3/X509_LOOKUP_hash_dir.pod
@@ -26,8 +26,9 @@ X509_load_cert_crl_file_ex, X509_load_cert_crl_file
 
 =head1 DESCRIPTION
 
-B<X509_LOOKUP_hash_dir> and B<X509_LOOKUP_file> are two certificate
-lookup methods to use with B<X509_STORE>, provided by OpenSSL library.
+B<X509_LOOKUP_hash_dir> and B<X509_LOOKUP_file>, and B<X509_LOOKUP_store> are
+certificate and CRL lookup methods to use with B<X509_STORE>, provided by
+OpenSSL library.
 
 Users of the library typically do not need to create instances of these
 methods manually, they would be created automatically by
@@ -125,6 +126,9 @@ It works with the help of URIs, which can be direct references to
 certificates or CRLs, but can also be references to catalogues of such
 objects (that behave like directories).
 
+B<X509_LOOKUP_store> ignores in its input anything else besides certificates and
+CRLs since OpenSSL 4.0.
+
 This method overlaps the L</File Method> and L</Hashed Directory Method>
 because of the 'file:' scheme loader.
 It does no caching of its own, but can use a caching L<ossl_store(7)>

diff --git a/test/recipes/25-test_verify_store.t b/test/recipes/25-test_verify_store.t
index 346396a628bb8012016ae4f951b668a844fe0475..6338b862b5544c7316044ab9f823b552745d1f42 100644 (file)
--- a/test/recipes/25-test_verify_store.t
+++ b/test/recipes/25-test_verify_store.t
@@ -14,7 +14,7 @@ use OpenSSL::Test::Utils;
 
 setup("test_verify_store");
 
-plan tests => 10;
+plan tests => 11;
 
 my $dummycnf = srctop_file("apps", "openssl.cnf");
 my $cakey = srctop_file("test", "certs", "ca-key.pem");
@@ -23,6 +23,7 @@ my $ukey = srctop_file("test", "certs", "ee-key.pem");
 my $cnf = srctop_file("test", "ca-and-certs.cnf");
 my $CAkey = "keyCA.ss";
 my $CAcert="certCA.ss";
+my $CAobjects="objectsCA.ss"; # DH params, public key, private key, certificate
 my $CAserial="certCA.srl";
 my $CAreq="reqCA.ss";
 my $CAreq2="req2CA.ss"; # temp
@@ -38,7 +39,7 @@ SKIP: {
          -key          => $cakey,
          -keyout       => $CAkey );
 
-    skip 'failure', 8 unless
+    skip 'failure', 9 unless
         x509( 'convert request into self-signed cert',
               qw(-req -CAcreateserial -days 30),
               qw(-extensions v3_ca),
@@ -47,30 +48,45 @@ SKIP: {
               -signkey  => $CAkey,
               -extfile  => $cnf );
 
-    skip 'failure', 7 unless
+    skip 'failure', 8 unless
         x509( 'convert cert into a cert request',
               qw(-x509toreq),
               -in       => $CAcert,
               -out      => $CAreq2,
               -signkey  => $CAkey );
 
-    skip 'failure', 6 unless
+    skip 'failure', 7 unless
         req( 'verify request 1',
              qw(-verify -noout -section userreq),
              -config    => $dummycnf,
              -in        => $CAreq );
 
-    skip 'failure', 5 unless
+    skip 'failure', 6 unless
         req( 'verify request 2',
              qw(-verify -noout -section userreq),
              -config    => $dummycnf,
              -in        => $CAreq2 );
 
-    skip 'failure', 4 unless
-        verify( 'verify signature',
+    skip 'failure', 5 unless
+        verify( 'verify cert using CAstore including just the cert',
                 -CAstore => $CAcert,
                 $CAcert );
 
+    open(my $out, '>', $CAobjects) or die $!;
+    my $pubkey = qx(openssl x509 -pubkey -noout -in $CAcert);
+    print $out $pubkey;
+    my @files;
+    push @files, srctop_file("test", "certs", "dhp2048.pem")
+        unless disabled("dh");
+    push @files, ($CAkey, $CAcert);
+    print $out do { local @ARGV = @files; <> };
+    close $out or die $!;
+
+    skip 'failure', 4 unless
+        verify( 'verify cert using CAstore including cert and extra stuff',
+                -CAstore => $CAobjects,
+                $CAcert );
+
     skip 'failure', 3 unless
         req( 'make a user cert request',
              qw(-new -section userreq),