From 077abb4327b4b9e8a5bd3db98c05e6a6645579ce Mon Sep 17 00:00:00 2001
From: Jakub Witczak <kuba@erlang.org>
Date: Mon, 14 Aug 2023 15:48:48 +0200
Subject: [PATCH] public_key: REVIEW improve pubkey_ocsp:verify_responder_cert

---
 lib/public_key/src/pubkey_ocsp.erl        | 41 ++++++++++-------------
 lib/public_key/test/pubkey_ocsp_SUITE.erl |  2 +-
 lib/ssl/src/ssl_trace.erl                 |  3 +-
 lib/ssl/test/openssl_stapling_SUITE.erl   |  1 +
 4 files changed, 20 insertions(+), 27 deletions(-)

diff --git a/lib/public_key/src/pubkey_ocsp.erl b/lib/public_key/src/pubkey_ocsp.erl
index c030c095728b..d9ddb25aab3d 100644
--- a/lib/public_key/src/pubkey_ocsp.erl
+++ b/lib/public_key/src/pubkey_ocsp.erl
@@ -156,14 +156,18 @@ get_nonce_value([#'Extension'{
 get_nonce_value([_Extn | Rest]) ->
     get_nonce_value(Rest).
 
+verify_signature(_, _, _, [], _, _) ->
+    {error, ocsp_responder_cert_not_found};
 verify_signature(ResponseDataDer, SignatureAlgo, Signature,
-                      Certs, ResponderID, IssuerCert) ->
-    case find_responder_cert(ResponderID, Certs, IssuerCert) of
-        {ok, Cert} ->
-            do_verify_signature(
-                ResponseDataDer, Signature, SignatureAlgo, Cert);
-        {error, Reason} ->
-            {error, Reason}
+                 [Cert | TCerts], ResponderID, IssuerCert) ->
+    maybe
+        true ?= is_responder_cert(ResponderID, Cert),
+        true ?= is_authorized_responder(Cert, IssuerCert),
+        ok ?= do_verify_signature(ResponseDataDer, Signature, SignatureAlgo, Cert)
+    else
+        _->
+            verify_signature(ResponseDataDer, SignatureAlgo, Signature,
+                             TCerts, ResponderID, IssuerCert)
     end.
 
 verify_past_timestamp(Timestamp) ->
@@ -192,18 +196,12 @@ verify_next_update(asn1_NOVALUE) ->
 verify_next_update(NextUpdate) ->
     verify_future_timestamp(NextUpdate).
 
-find_responder_cert(_ResponderID, [], _) ->
-    {error, ocsp_responder_cert_not_found};
-find_responder_cert(ResponderID, [Cert | TCerts], IssuerCert) ->
-    maybe
-        true ?= is_responder_cert(ResponderID, Cert),
-        ok ?= verify_responder_cert(Cert, IssuerCert),
-        {ok, Cert}
-    else
-        _-> find_responder_cert(ResponderID, TCerts, IssuerCert)
-    end.
+is_responder_cert({byName, Name}, Cert) ->
+    public_key:der_encode('Name', Name) == get_subject_name(Cert);
+is_responder_cert({byKey, Key}, Cert) ->
+    Key == crypto:hash(sha, get_public_key(Cert)).
 
-verify_responder_cert(Cert, IssuerCert) ->
+is_authorized_responder(Cert, IssuerCert) ->
     Case1 =
         %% the CA who issued the certificate in question signed the
         %% response
@@ -224,7 +222,7 @@ verify_responder_cert(Cert, IssuerCert) ->
 
     case lists:any(fun(E) -> E() end, [Case1, Case2, Case3]) of
         true ->
-            ok;
+            true;
         _ ->
             not_authorized_responder
     end.
@@ -244,11 +242,6 @@ get_public_key_rec(#'OTPCertificate'{tbsCertificate = TbsCert}) ->
     PKInfo = TbsCert#'OTPTBSCertificate'.subjectPublicKeyInfo,
     PKInfo#'OTPSubjectPublicKeyInfo'.subjectPublicKey.
 
-is_responder_cert({byName, Name}, Cert) ->
-    public_key:der_encode('Name', Name) == get_subject_name(Cert);
-is_responder_cert({byKey, Key}, Cert) ->
-    Key == crypto:hash(sha, get_public_key(Cert)).
-
 get_subject_name(#'OTPCertificate'{tbsCertificate = TbsCert}) ->
     public_key:pkix_encode('Name', TbsCert#'OTPTBSCertificate'.subject, otp).
 
diff --git a/lib/public_key/test/pubkey_ocsp_SUITE.erl b/lib/public_key/test/pubkey_ocsp_SUITE.erl
index a1a8ff92abb0..6ef0fa741779 100644
--- a/lib/public_key/test/pubkey_ocsp_SUITE.erl
+++ b/lib/public_key/test/pubkey_ocsp_SUITE.erl
@@ -177,7 +177,7 @@ ocsp_test(Config) when is_list(Config) ->
     %% invalid responses
     OcspResponseWrongSignature =
         OcspResponse#'BasicOCSPResponse'{signature = <<"rubbish_signature">>},
-    {error, ocsp_response_bad_signature} =
+    {error, ocsp_responder_cert_not_found} =
         pubkey_ocsp:verify_response(OcspResponseWrongSignature,
                                     [?RESPONDER_CERT],
                                     ?NONCE,
diff --git a/lib/ssl/src/ssl_trace.erl b/lib/ssl/src/ssl_trace.erl
index 77614919aa8e..36b89fd08f0a 100644
--- a/lib/ssl/src/ssl_trace.erl
+++ b/lib/ssl/src/ssl_trace.erl
@@ -437,12 +437,11 @@ trace_profiles() ->
                         {cert_status_check, 5}]},
        {public_key, [{ocsp_extensions, 1}, {pkix_ocsp_validate, 4},
                      {otp_cert, 1}]},
-       {pubkey_ocsp, [{find_responder_cert, 3}, {do_verify_signature, 4},
+       {pubkey_ocsp, [{do_verify_signature, 4},
                       {verify_response, 4}, {verify_nonce, 2},
                       {verify_signature, 6},
                       {is_responder_cert, 2}, {find_single_response, 3},
                       {status, 1}, {match_single_response, 4},
-                      {verify_responder_cert, 2},
                       {designated_for_ocsp_signing, 1}]},
        {ssl, [{opt_stapling, 3}]},
        {ssl_certificate, [{verify_cert_extensions, 4}]},
diff --git a/lib/ssl/test/openssl_stapling_SUITE.erl b/lib/ssl/test/openssl_stapling_SUITE.erl
index 905690843b46..01902dd1e49e 100644
--- a/lib/ssl/test/openssl_stapling_SUITE.erl
+++ b/lib/ssl/test/openssl_stapling_SUITE.erl
@@ -124,6 +124,7 @@ init_per_testcase_helper(Testcase, Config0) ->
     ct:timetrap({seconds, 10}),
     Default = "otpCA",
     TestcaseMapping = #{staple_by_issuer => Default,
+                        staple_by_trusted => "erlangCA",
                         staple_by_designated => "b.server",
                         staple_not_designated => "a.server",
                         staple_wrong_issuer => "localhost"},