Merge branch 'legoscia/crl-check-without-dp' into maint

* legoscia/crl-check-without-dp:
  Be suspicious of certificates without CRL DPs
  Refactor ssl_crl_SUITE: extract crl_verify_error/6

OTP-13378

2 files changed, 70 insertions, 21 deletions

diff --git a/lib/ssl/src/ssl_handshake.erl b/lib/ssl/src/ssl_handshake.erl
index e9e140836b..e98073080a 100644
--- a/lib/ssl/src/ssl_handshake.erl
+++ b/lib/ssl/src/ssl_handshake.erl
@@ -2072,12 +2072,9 @@ crl_check(OtpCert, Check, CertDbHandle, CertDbRef, {Callback, CRLDbHandle}, _) -
 	      ],
     case dps_and_crls(OtpCert, Callback, CRLDbHandle, ext) of
 	no_dps ->
-	    case dps_and_crls(OtpCert, Callback, CRLDbHandle, same_issuer) of
-		[] ->
-		    valid; %% No relevant CRL existed
-		DpsAndCRls ->
-		    crl_check_same_issuer(OtpCert, Check, DpsAndCRls, Options)		
-	    end;
+	    crl_check_same_issuer(OtpCert, Check,
+				  dps_and_crls(OtpCert, Callback, CRLDbHandle, same_issuer),
+				  Options);
 	DpsAndCRLs ->  %% This DP list may be empty if relevant CRLs existed 
 	    %% but could not be retrived, will result in {bad_cert, revocation_status_undetermined}
 	    case public_key:pkix_crls_validate(OtpCert, DpsAndCRLs, Options) of
diff --git a/lib/ssl/test/ssl_crl_SUITE.erl b/lib/ssl/test/ssl_crl_SUITE.erl
index 44580be1ff..5b86027210 100644
--- a/lib/ssl/test/ssl_crl_SUITE.erl
+++ b/lib/ssl/test/ssl_crl_SUITE.erl
@@ -53,7 +53,7 @@ groups() ->
      {idp_crl, [], basic_tests()}].
 
 basic_tests() ->
-    [crl_verify_valid, crl_verify_revoked].
+    [crl_verify_valid, crl_verify_revoked, crl_verify_no_crl].
 
 
 init_per_suite(Config) ->
@@ -186,11 +186,6 @@ crl_verify_revoked(Config)  when is_list(Config) ->
 
     {ClientNode, ServerNode, Hostname} = ssl_test_lib:run_where(Config),
 
-    Server = ssl_test_lib:start_server_error([{node, ServerNode}, {port, 0}, 
-					      {from, self()}, 
-					      {options, ServerOpts}]),
-    Port = ssl_test_lib:inet_port(Server),
-    
     ssl_crl_cache:insert({file, filename:join([PrivDir, "erlangCA", "crl.pem"])}),
     ssl_crl_cache:insert({file, filename:join([PrivDir, "otpCA", "crl.pem"])}),
     
@@ -206,16 +201,55 @@ crl_verify_revoked(Config)  when is_list(Config) ->
 			   {verify, verify_peer}]
 		  end,	
     
-    Client = ssl_test_lib:start_client_error([{node, ClientNode}, {port, Port}, 
-     					      {host, Hostname},
-   					      {from, self()}, 
-     					      {options, ClientOpts}]),
-    receive
-	{Server, AlertOrColse} ->
-	    ct:pal("Server Alert or Close ~p", [AlertOrColse])
-    end,	    
-    ssl_test_lib:check_result(Client, {error, {tls_alert, "certificate revoked"}}).
+    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 = ?config(cert_dir, Config),
+    Check = ?config(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"])}],
+    ClientOpts =  case ?config(idp_crl, Config) of 
+		      true ->	       
+			  [{cacertfile, filename:join([PrivDir, "server", "cacerts.pem"])},
+			   {crl_check, Check},
+			   {crl_cache, {ssl_crl_cache, {internal, [{http, 5000}]}}},
+			   {verify, verify_peer}];
+		      false ->
+			  [{cacertfile, filename:join([PrivDir, "server", "cacerts.pem"])},
+			   {crl_check, Check},
+			   {verify, verify_peer}]
+		  end,			  
+    {ClientNode, ServerNode, Hostname} = ssl_test_lib:run_where(Config),
+
+    %% In case we're running an HTTP server that serves CRLs, let's
+    %% rename those files, so the CRL is absent when we try to verify
+    %% it.
+    %%
+    %% If we're not using an HTTP server, we just need to refrain from
+    %% adding the CRLs to the cache manually.
+    rename_crl(filename:join([PrivDir, "erlangCA", "crl.pem"])),
+    rename_crl(filename:join([PrivDir, "otpCA", "crl.pem"])),
+
+    %% The expected outcome when the CRL is missing depends on the
+    %% crl_check setting.
+    case Check of
+        true ->
+            %% The error "revocation status undetermined" gets turned
+            %% into "bad certificate".
+            crl_verify_error(Hostname, ServerNode, ServerOpts, ClientNode, ClientOpts,
+                             "bad certificate");
+        peer ->
+            crl_verify_error(Hostname, ServerNode, ServerOpts, ClientNode, ClientOpts,
+                             "bad certificate");
+        best_effort ->
+            %% In "best effort" mode, we consider the certificate not
+            %% to be revoked if we can't find the appropriate CRL.
+            crl_verify_valid(Hostname, ServerNode, ServerOpts, ClientNode, ClientOpts)
+    end.
 
 crl_verify_valid(Hostname, ServerNode, ServerOpts, ClientNode, ClientOpts) ->
     Server = ssl_test_lib:start_server([{node, ServerNode}, {port, 0}, 
@@ -236,6 +270,22 @@ crl_verify_valid(Hostname, ServerNode, ServerOpts, ClientNode, ClientOpts) ->
     ssl_test_lib:close(Server),
     ssl_test_lib:close(Client).
 
+crl_verify_error(Hostname, ServerNode, ServerOpts, ClientNode, ClientOpts, ExpectedAlert) ->
+    Server = ssl_test_lib:start_server_error([{node, ServerNode}, {port, 0},
+					      {from, self()},
+					      {options, ServerOpts}]),
+    Port = ssl_test_lib:inet_port(Server),
+
+    Client = ssl_test_lib:start_client_error([{node, ClientNode}, {port, Port},
+					      {host, Hostname},
+					      {from, self()},
+					      {options, ClientOpts}]),
+    receive
+	{Server, AlertOrClose} ->
+	    ct:pal("Server Alert or Close ~p", [AlertOrClose])
+    end,
+    ssl_test_lib:check_result(Client, {error, {tls_alert, ExpectedAlert}}).
+
 %%--------------------------------------------------------------------
 %% Internal functions ------------------------------------------------
 %%--------------------------------------------------------------------
@@ -259,3 +309,5 @@ make_dir_path(PathComponents) ->
 		"",
 		PathComponents).
 
+rename_crl(Filename) ->
+    file:rename(Filename, Filename ++ ".notfound").