Look for root last in list, not anywhere in list.

Even if verify_chain() should not return a root cert that is not last
in the chain, the code is now less confusing to read.

Also, rename normalise_chain/3 to verify_chain to reflect what it
actually does.

1 files changed, 36 insertions, 36 deletions

diff --git a/src/x509.erl b/src/x509.erl
index 0b32c4e..f44e36f 100644
--- a/src/x509.erl
+++ b/src/x509.erl
@@ -21,14 +21,14 @@
 -spec normalise_chain([binary()], [binary()]) -> {ok, [binary()]} |
                                                  {error, reason()}.
 normalise_chain(AcceptableRootCerts, CertChain) ->
-    case normalise_chain(AcceptableRootCerts, CertChain, ?MAX_CHAIN_LENGTH) of
+    case verify_chain(AcceptableRootCerts, CertChain, ?MAX_CHAIN_LENGTH) of
         {false, Reason} ->
             {error, Reason};
         {true, Root} ->
-            case lists:member(Root, CertChain) of
-                true ->
+            case lists:last(CertChain) of
+                Root ->
                     {ok, CertChain};
-                false ->
+                _ ->
                     {ok, CertChain ++ [Root]}
             end
     end.
@@ -68,8 +68,8 @@ detox(LeafDer, ChainDer) ->
 %%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%
 %% Private functions.
 
--spec normalise_chain([binary()], [binary()], integer()) ->
-                             {false, reason()} | {true, [binary()]}.
+-spec verify_chain([binary()], [binary()], integer()) ->
+                          {false, reason()} | {true, [binary()]}.
 %% @doc Verify that the leaf cert or precert has a valid chain back to
 %% an acceptable root cert. The order of certificates in the second
 %% argument is: leaf cert in head, chain in tail. Order of certs in first
@@ -77,10 +77,10 @@ detox(LeafDer, ChainDer) ->
 %%
 %% Return {false, Reason} or {true, Root} where Root is a CA cert from
 %% AcceptableRootCerts signing the root of the chain.
-normalise_chain(_, _, MaxChainLength) when MaxChainLength =< 0 ->
+verify_chain(_, _, MaxChainLength) when MaxChainLength =< 0 ->
     %% Chain too long.
     {false, chain_too_long};
-normalise_chain(AcceptableRootCerts, [TopCert], MaxChainLength) ->
+verify_chain(AcceptableRootCerts, [TopCert], MaxChainLength) ->
     %% Check root of chain.
     case lists:member(TopCert, AcceptableRootCerts) of
         true ->
@@ -96,9 +96,9 @@ normalise_chain(AcceptableRootCerts, [TopCert], MaxChainLength) ->
                 Root -> {true, Root}
             end
     end;
-normalise_chain(AcceptableRootCerts, [Cert|Rest], MaxChainLength) ->
+verify_chain(AcceptableRootCerts, [Cert|Rest], MaxChainLength) ->
     case signed_by_p(Cert, hd(Rest)) of
-        true -> normalise_chain(AcceptableRootCerts, Rest, MaxChainLength - 1);
+        true -> verify_chain(AcceptableRootCerts, Rest, MaxChainLength - 1);
         false -> {false, signature_mismatch}
     end.
 
@@ -414,21 +414,21 @@ valid_cert_test_() ->
               %% #'OTPCertificate'{}. The error is
               %% {invalid_choice_tag,{22,<<"US">>}}}} in
               %% 'OTP-PUB-KEY':Func('OTP-X520countryname', Value0).
-              ?_assertMatch({true, _}, normalise_chain(nth(1, Chains),
-                                                       nth(1, Chains), 10)),
+              ?_assertMatch({true, _}, verify_chain(nth(1, Chains),
+                                                    nth(1, Chains), 10)),
               %% Self-signed so fail.
               ?_assertMatch({false, root_unknown},
-                            normalise_chain(KnownRoots,
-                                            nth(2, Chains), 10)),
+                            verify_chain(KnownRoots,
+                                         nth(2, Chains), 10)),
               %% Leaf signed by known CA, pass.
-              ?_assertMatch({true, _}, normalise_chain(KnownRoots,
-                                                       nth(3, Chains), 10)),
+              ?_assertMatch({true, _}, verify_chain(KnownRoots,
+                                                    nth(3, Chains), 10)),
               %% Proper 3-depth chain with root in KnownRoots, pass.
               %% Bug CATLFISH-19 --> [info] rejecting "3ee62cb678014c14d22ebf96f44cc899adea72f1": chain_broken
               %% leaf sha1: 3ee62cb678014c14d22ebf96f44cc899adea72f1
               %% leaf Subject: C=KR, O=Government of Korea, OU=Group of Server, OU=\xEA\xB5\x90\xEC\x9C\xA1\xEA\xB3\xBC\xED\x95\x99\xEA\xB8\xB0\xEC\x88\xA0\xEB\xB6\x80, CN=www.berea.ac.kr, CN=haksa.bits.ac.kr
-              ?_assertMatch({true, _}, normalise_chain(KnownRoots,
-                                                       nth(4, Chains), 3)),
+              ?_assertMatch({true, _}, verify_chain(KnownRoots,
+                                                    nth(4, Chains), 3)),
               %% Verify against self, pass.
               %% Bug CATLFISH-??, can't handle issuer keytype ECPoint.
               %% Issuer sha1: 6969562e4080f424a1e7199f14baf3ee58ab6abb
@@ -442,31 +442,31 @@ valid_cert_test_() ->
 
               %% Supposedly problematic chains from Google Aviator, fatal.
               %% 00459972: asn1: syntax error: sequence truncated
-              ?_assertMatch({true, _}, normalise_chain(nth(7, Chains),
-                                                       nth(7, Chains), 10)),
+              ?_assertMatch({true, _}, verify_chain(nth(7, Chains),
+                                                    nth(7, Chains), 10)),
               %% 1402673: x509: RSA modulus is not a positive number
-              ?_assertMatch({true, _}, normalise_chain(nth(8, Chains),
-                                                       nth(8, Chains), 10)),
+              ?_assertMatch({true, _}, verify_chain(nth(8, Chains),
+                                                    nth(8, Chains), 10)),
               %% 1345105: asn1: syntax error: IA5String contains invalid character
-              ?_assertMatch({true, _}, normalise_chain(nth(9, Chains),
-                                                       nth(9, Chains), 10)),
+              ?_assertMatch({true, _}, verify_chain(nth(9, Chains),
+                                                    nth(9, Chains), 10)),
               %% 1557693: asn1: structure error: integer too large
-              ?_assertMatch({true, _}, normalise_chain(nth(10, Chains),
-                                                       nth(10, Chains), 10)),
+              ?_assertMatch({true, _}, verify_chain(nth(10, Chains),
+                                                    nth(10, Chains), 10)),
 
               %% Supposedly problematic chains from Google Aviator, non-fatal.
               %% 16800: x509: negative serial number
               %% a.pem
-              ?_assertMatch({true, _}, normalise_chain(nth(11, Chains),
-                                                       nth(11, Chains), 10)),
+              ?_assertMatch({true, _}, verify_chain(nth(11, Chains),
+                                                    nth(11, Chains), 10)),
               %% 22487: x509: unhandled critical extension ([2 5 29 32])
               %% b.pem
-              ?_assertMatch({true, _}, normalise_chain(nth(12, Chains),
-                                                       nth(12, Chains), 10)),
+              ?_assertMatch({true, _}, verify_chain(nth(12, Chains),
+                                                    nth(12, Chains), 10)),
               %% 5198: x509: certificate contained IP address of length 8
               %% c.pem
-              ?_assertMatch({true, _}, normalise_chain(nth(13, Chains),
-                                                       nth(13, Chains), 10))
+              ?_assertMatch({true, _}, verify_chain(nth(13, Chains),
+                                                    nth(13, Chains), 10))
               ] end}.
 
 chain_test_() ->
@@ -480,19 +480,19 @@ chain_test(C0, C1) ->
      %% Root not in chain but in trust store.
      ?_assertEqual({ok, [C0, C1]}, normalise_chain([C1], [C0])),
      %% Same, but chain too long.
-     ?_assertEqual({false, chain_too_long}, normalise_chain([C1], [C0], 1)),
+     ?_assertEqual({false, chain_too_long}, verify_chain([C1], [C0], 1)),
      %% Root in chain and in trust store.
      ?_assertEqual({ok, [C0, C1]}, normalise_chain([C1], [C0, C1])),
      %% Same, but chain too long.
-     ?_assertEqual({false, chain_too_long}, normalise_chain([C1], [C0, C1], 1)),
+     ?_assertEqual({false, chain_too_long}, verify_chain([C1], [C0, C1], 1)),
      %% Root not in trust store.
      ?_assertEqual({error, root_unknown}, normalise_chain([], [C0, C1])),
      %% Selfsigned. Actually OK.
      ?_assertEqual({ok, [C0]}, normalise_chain([C0], [C0])),
      %% Same, still OK.
-     ?_assertEqual({true, C0}, normalise_chain([C0], [C0], 1)),
+     ?_assertEqual({true, C0}, verify_chain([C0], [C0], 1)),
      %% Max chain length 0 is not OK.
-     ?_assertEqual({false, chain_too_long}, normalise_chain([C0], [C0], 0))
+     ?_assertEqual({false, chain_too_long}, verify_chain([C0], [C0], 0))
     ].
 
 %%-spec read_certs(file:filename()) -> [string:string()].