ssl: Refactor CRL handling functions

CRL handling was partially broken. This refator makes the code work again
and avoids falling back to scanning of the database if the CRL
issuer ID is known but not present.

Extended tests but further testing enhancments should be added
to get full coverage.

4 files changed, 125 insertions, 39 deletions

diff --git a/lib/ssl/src/ssl_certificate.erl b/lib/ssl/src/ssl_certificate.erl
index b5b0a23d85..674870dd15 100644
--- a/lib/ssl/src/ssl_certificate.erl
+++ b/lib/ssl/src/ssl_certificate.erl
@@ -89,7 +89,7 @@ trusted_cert_and_paths(Chain,  CertDbHandle, CertDbRef, PartialChainHandler) ->
               end, Paths).     
 
 %%--------------------------------------------------------------------
--spec certificate_chain(undefined | binary() | #'OTPCertificate'{} , db_handle(), certdb_ref()) ->
+-spec certificate_chain(undefined | binary() | #'OTPCertificate'{} , db_handle(), certdb_ref() | {extracted, list()}) ->
 			  {error, no_cert} | {ok, der_cert() | undefined, [der_cert()]}.
 %%
 %% Description: Return the certificate chain to send to peer.
diff --git a/lib/ssl/src/ssl_crl.erl b/lib/ssl/src/ssl_crl.erl
index 888a75bfd6..12d261bcdc 100644
--- a/lib/ssl/src/ssl_crl.erl
+++ b/lib/ssl/src/ssl_crl.erl
@@ -27,35 +27,40 @@
 -include("ssl_internal.hrl").
 -include_lib("public_key/include/public_key.hrl"). 
 
--export([trusted_cert_and_path/3]).
+-export([trusted_cert_and_path/4]).
 
-trusted_cert_and_path(CRL, {SerialNumber, Issuer},{_, {Db, DbRef}} = DbHandle) -> 
+trusted_cert_and_path(CRL, {SerialNumber, Issuer}, CertPath, {Db, DbRef}) ->
+    %% CRL issuer cert ID is known
     case ssl_pkix_db:lookup_trusted_cert(Db, DbRef, SerialNumber, Issuer) of
 	undefined ->
-	    trusted_cert_and_path(CRL, issuer_not_found, DbHandle);
+            %% But not found in our database
+	    search_certpath(CRL, CertPath, Db, DbRef);
 	{ok, {_, OtpCert}}  ->
 	    {ok, Root, Chain} = ssl_certificate:certificate_chain(OtpCert, Db, DbRef),
 	    {ok, Root,  lists:reverse(Chain)}
     end;
-trusted_cert_and_path(CRL, issuer_not_found, {CertPath, {Db, DbRef}}) -> 
-    case find_issuer(CRL, {certpath, 
-                           [{Der, public_key:pkix_decode_cert(Der,otp)} || Der <-  CertPath]}) of
-	{ok, OtpCert} ->
-	    {ok, Root, Chain} = ssl_certificate:certificate_chain(OtpCert, Db, DbRef),
-	    {ok, Root, lists:reverse(Chain)};
-	{error, issuer_not_found} ->
-	    trusted_cert_and_path(CRL, issuer_not_found, {Db, DbRef}) 
-    end;
-trusted_cert_and_path(CRL, issuer_not_found, {Db, DbRef} = DbInfo) -> 
-    case find_issuer(CRL, DbInfo) of
-	{ok, OtpCert} ->
-            {ok, Root, Chain} = ssl_certificate:certificate_chain(OtpCert, Db, DbRef),
-	    {ok, Root, lists:reverse(Chain)};
-         {error, issuer_not_found} ->
-             {error, unknown_ca}
-     end.
+trusted_cert_and_path(CRL, issuer_not_found, CertPath, {Db, DbRef}) ->
+    case search_certpath(CRL, CertPath, Db, DbRef) of
+	{error, unknown_ca} ->
+            Issuer = public_key:pkix_normalize_name(public_key:pkix_crl_issuer(CRL)),
+            IsIssuerFun =
+                fun({_Key, {_Der,ErlCertCandidate}}, Acc) ->
+                        verify_crl_issuer(CRL, ErlCertCandidate, Issuer, Acc);
+                   (_, Acc) ->
+                        Acc
+                end,
+            case search_db(IsIssuerFun, Db, DbRef) of
+                {ok, OtpCert} ->
+                    {ok, Root, Chain} = ssl_certificate:certificate_chain(OtpCert, Db, DbRef),
+                    {ok, Root, lists:reverse(Chain)};
+                {error, issuer_not_found} ->
+                    {error, unknown_ca}
+            end;
+        Result ->
+            Result
+    end.
 
-find_issuer(CRL, {certpath = Db, DbRef}) ->
+search_certpath(CRL, CertPath, Db, DbRef) ->
     Issuer = public_key:pkix_normalize_name(public_key:pkix_crl_issuer(CRL)),
     IsIssuerFun =
 	fun({_Der,ErlCertCandidate}, Acc) ->
@@ -63,15 +68,18 @@ find_issuer(CRL, {certpath = Db, DbRef}) ->
 	   (_, Acc) ->
 		Acc
 	end,
-    find_issuer(IsIssuerFun, Db, DbRef);
-find_issuer(CRL, {Db, DbRef}) ->
-    Issuer = public_key:pkix_normalize_name(public_key:pkix_crl_issuer(CRL)),
-    IsIssuerFun =
-	fun({_Key, {_Der,ErlCertCandidate}}, Acc) ->
-		verify_crl_issuer(CRL, ErlCertCandidate, Issuer, Acc);
-	   (_, Acc) ->
-		Acc
-	end,
+    case find_issuer(IsIssuerFun, certpath,
+                     [{Der, public_key:pkix_decode_cert(Der,otp)} || Der <- CertPath]) of
+	{ok, OtpCert} ->
+	    {ok, Root, Chain} = ssl_certificate:certificate_chain(OtpCert, Db, DbRef),
+	    {ok, Root, lists:reverse(Chain)};
+	{error, issuer_not_found} ->
+            {error, unknown_ca}
+    end.
+
+search_db(IsIssuerFun, _, {extracted, ExtractedCerts})->
+    find_issuer(IsIssuerFun, extracted, ExtractedCerts);
+search_db(IsIssuerFun, Db, DbRef) ->
     find_issuer(IsIssuerFun, Db, DbRef).
 
 find_issuer(IsIssuerFun, certpath, Certs) ->
diff --git a/lib/ssl/src/ssl_handshake.erl b/lib/ssl/src/ssl_handshake.erl
index 783f729386..de5490d232 100644
--- a/lib/ssl/src/ssl_handshake.erl
+++ b/lib/ssl/src/ssl_handshake.erl
@@ -1978,15 +1978,15 @@ cert_status_check(OtpCert, #{ocsp_state := #{ocsp_stapling := best_effort, %%TOD
 
 maybe_check_crl(_, #{crl_check := false}, _, _, _) ->
     valid;
-maybe_check_crl(_, #{crl_check := peer}, _, valid, _) -> %% Do not check CAs with this option.
+maybe_check_crl(_, #{crl_check := peer}, valid, _, _) -> %% Do not check CAs with this option.
     valid;
 maybe_check_crl(OtpCert, #{crl_check := Check, 
                      certdb := CertDbHandle,
                      certdb_ref := CertDbRef,
                      crl_db := {Callback, CRLDbHandle}}, _, CertPath, LogLevel) ->
     Options = [{issuer_fun, {fun(_DP, CRL, Issuer, DBInfo) ->
-				     ssl_crl:trusted_cert_and_path(CRL, Issuer, {CertPath,
-                                                                                 DBInfo})
+				     ssl_crl:trusted_cert_and_path(CRL, Issuer, CertPath,
+                                                                   DBInfo)
 			     end, {CertDbHandle, CertDbRef}}}, 
 	       {update_crl, fun(DP, CRL) ->
                                     case Callback:fresh_crl(DP, CRL) of
diff --git a/lib/ssl/test/ssl_crl_SUITE.erl b/lib/ssl/test/ssl_crl_SUITE.erl
index f9fac82962..c7897d9404 100644
--- a/lib/ssl/test/ssl_crl_SUITE.erl
+++ b/lib/ssl/test/ssl_crl_SUITE.erl
@@ -43,6 +43,10 @@
          crl_verify_valid/1,
          crl_verify_revoked/0,
          crl_verify_revoked/1,
+         crl_verify_valid_derCAs/0,
+         crl_verify_valid_derCAs/1,
+         crl_verify_revoked_derCAs/0,
+         crl_verify_revoked_derCAs/1,
          crl_verify_no_crl/0,
          crl_verify_no_crl/1,
          crl_hash_dir_collision/0,
@@ -84,7 +88,11 @@ groups() ->
      {crl_verify_crldp_crlissuer, [], [crl_verify_valid]}].
 
 basic_tests() ->
-    [crl_verify_valid, crl_verify_revoked, crl_verify_no_crl].
+    [crl_verify_valid,
+     crl_verify_revoked,
+     crl_verify_valid_derCAs,
+     crl_verify_revoked_derCAs,
+     crl_verify_no_crl].
 
 crl_hash_dir_tests() ->
     [crl_hash_dir_collision, crl_hash_dir_expired].
@@ -220,13 +228,13 @@ crl_verify_valid(Config) when is_list(Config) ->
 		   {cacertfile, filename:join([PrivDir, "server", "cacerts.pem"])}],
     ClientOpts =  case proplists:get_value(idp_crl, Config) of 
 		      true ->	       
-			  [{cacertfile, filename:join([PrivDir, "server", "cacerts.pem"])},
+			  [{cacertfile, filename:join([PrivDir, "client", "cacerts.pem"])},
 			   {crl_check, Check},
 			   {crl_cache, {ssl_crl_cache, {internal, [{http, 5000}]}}},
 			   {verify, verify_peer}];
 		      false ->
 			  proplists:get_value(crl_cache_opts, Config) ++
-			      [{cacertfile, filename:join([PrivDir, "server", "cacerts.pem"])},
+			      [{cacertfile, filename:join([PrivDir, "client", "cacerts.pem"])},
 			       {crl_check, Check},
 			       {verify, verify_peer}]
 		  end,			  
@@ -266,15 +274,79 @@ crl_verify_revoked(Config)  when is_list(Config) ->
     
     crl_verify_error(Hostname, ServerNode, ServerOpts, ClientNode, ClientOpts,
                      certificate_revoked).
+crl_verify_valid_derCAs() ->
+    [{doc,"Verify a simple valid CRL chain"}].
+crl_verify_valid_derCAs(Config) when is_list(Config) ->
+    PrivDir = proplists:get_value(cert_dir, Config),
+    Check = proplists:get_value(crl_check, Config),
+
+    CaCerts = der_cas(filename:join([PrivDir, "client", "cacerts.pem"])),
+
+    ServerOpts =  [{keyfile, filename:join([PrivDir, "server", "key.pem"])},
+                   {certfile, filename:join([PrivDir, "server", "cert.pem"])},
+                   {cacerts, der_cas(filename:join([PrivDir, "server", "cacerts.pem"]))}
+                  ],
+    ClientOpts =  case proplists:get_value(idp_crl, Config) of
+		      true ->
+			  [{cacerts, CaCerts},
+			   {crl_check, Check},
+			   {crl_cache, {ssl_crl_cache, {internal, [{http, 5000}]}}},
+			   {verify, verify_peer}];
+		      false ->
+			  proplists:get_value(crl_cache_opts, Config) ++
+			      [{cacerts, CaCerts},
+			       {crl_check, Check},
+			       {verify, verify_peer}]
+		  end,
+    {ClientNode, ServerNode, Hostname} = ssl_test_lib:run_where(Config),
+
+    ssl_crl_cache:insert({file, filename:join([PrivDir, "erlangCA", "crl.pem"])}),
+    ssl_crl_cache:insert({file, filename:join([PrivDir, "otpCA", "crl.pem"])}),
+
+    crl_verify_valid(Hostname, ServerNode, ServerOpts, ClientNode, ClientOpts).
+
+crl_verify_revoked_derCAs() ->
+    [{doc,"Verify a simple CRL chain when peer cert is reveoked"}].
+crl_verify_revoked_derCAs(Config)  when is_list(Config) ->
+    PrivDir = proplists:get_value(cert_dir, Config),
+    Check = proplists:get_value(crl_check, Config),
+
+    CaCerts = der_cas(filename:join([PrivDir, "revoked", "cacerts.pem"])),
+
+    ServerOpts = [{keyfile, filename:join([PrivDir, "revoked", "key.pem"])},
+		  {certfile, filename:join([PrivDir, "revoked", "cert.pem"])},
+		  {cacerts,  der_cas(filename:join([PrivDir, "server", "cacerts.pem"]))}],
+
+    {ClientNode, ServerNode, Hostname} = ssl_test_lib:run_where(Config),
+
+    ssl_crl_cache:insert({file, filename:join([PrivDir, "erlangCA", "crl.pem"])}),
+    ssl_crl_cache:insert({file, filename:join([PrivDir, "otpCA", "crl.pem"])}),
+
+    ClientOpts =  case proplists:get_value(idp_crl, Config) of
+		      true ->
+			  [{cacerts, CaCerts},
+			   {crl_cache, {ssl_crl_cache, {internal, [{http, 5000}]}}},
+			   {crl_check, Check},
+			   {verify, verify_peer}];
+		      false ->
+			  proplists:get_value(crl_cache_opts, Config) ++
+			      [{cacerts, CaCerts},
+			       {crl_check, Check},
+			       {verify, verify_peer}]
+		  end,
+
+    crl_verify_error(Hostname, ServerNode, ServerOpts, ClientNode, ClientOpts,
+                     certificate_revoked).
 
 crl_verify_no_crl() ->
     [{doc,"Verify a simple CRL chain when the CRL is missing"}].
 crl_verify_no_crl(Config) when is_list(Config) ->
     PrivDir = proplists:get_value(cert_dir, Config),
     Check = proplists:get_value(crl_check, Config),
+
     ServerOpts =  [{keyfile, filename:join([PrivDir, "server", "key.pem"])},
-      		  {certfile, filename:join([PrivDir, "server", "cert.pem"])},
-		   {cacertfile, filename:join([PrivDir, "server", "cacerts.pem"])}],
+                   {certfile, filename:join([PrivDir, "server", "cert.pem"])},
+		   {cacertfile,  filename:join([PrivDir, "server", "cacerts.pem"])}],
     ClientOpts =  case proplists:get_value(idp_crl, Config) of 
 		      true ->	       
 			  [{cacertfile, filename:join([PrivDir, "server", "cacerts.pem"])},
@@ -552,3 +624,9 @@ new_ca(FileName, CA1, CA2) ->
     Pem = public_key:pem_encode(E1 ++E2),
     file:write_file(FileName,  Pem),
     FileName.
+
+
+der_cas(CAcertsFile) ->
+    {ok, Pem} = file:read_file(CAcertsFile),
+    Decoded = public_key:pem_decode(Pem),
+    [DER || {_, DER, _} <- Decoded].