fix(crl): insert the CRLs individually when an URI is provided

[thalesmg]

Currently, if one uses ssl_crl_cache:insert/2 providing the URI of the distribution point of a CRL, when a connection is attempted, it fails with a {unexpected_error,function_clause}.

This traces to ssl_handshake:dps_and_crls/3, which eventually ends up calling public_key:der_decode/2 with a list-wrapped CRL DER binary instead of simply the DER binary.

[CLAassistant]

All committers have signed the CLA.

[github-actions[bot]]

CT Test Results

       2 files       64 suites   47m 21s :stopwatch:    737 tests    671 :heavy_check_mark:   66 :zzz: 0 :x: 3 498 runs  2 730 :heavy_check_mark: 768 :zzz: 0 :x:

Results for commit 97a8677c.

:recycle: This comment has been updated with latest results.

To speed up review, make sure that you have read Contributing to Erlang/OTP and that all checks pass.

See the TESTING and DEVELOPMENT HowTo guides for details about how to run test locally.

Artifacts

• Complete CT logs (Download Logs)
• HTML Documentation (Download HTML Docs)
• Windows Installer

// Erlang/OTP Github Action Bot

[thalesmg]

I've just realized that ssl_otp_crl_cache is of type set, not bag like ssl_otp_crl_issuer_mapping. So this change will persist only the last CRL of a list of CRLs.

I'll change the implementation to flatten the list that ssl_handshake:dps_and_crls/3 receives just before iterating over it.

[IngelaAndin]

I think flattening the list is symptom treatment. Would you make the patch do the following instead?

diff --git a/lib/ssl/src/ssl_crl_cache.erl b/lib/ssl/src/ssl_crl_cache.erl
index 095e3e8b44..7a8bcf5758 100644
--- a/lib/ssl/src/ssl_crl_cache.erl
+++ b/lib/ssl/src/ssl_crl_cache.erl
@@ -175,7 +175,7 @@ cache_lookup(URL, {{Cache, _}, _}) ->
     case ssl_pkix_db:lookup(string:trim(Path, leading, "/"), Cache) of
        undefined ->
            [];
-       CRLs ->
+       [CRLs] ->
            CRLs
     end.

That is we know that the internal table used in the internal implementation is an ets table of type set (pkix_db handles several ets tables ) and we can remove the outer list as our value which happens to be a list will be the one term inside. And is what ssl_crl_cache:lookup is supposed to return according to the documented callback API.

[thalesmg]

@IngelaAndin Thanks for the feedback.

I've updated the patch to use that: https://github.com/erlang/otp/pull/5996/commits/f3948cd90e84c59439c00bff473b5cd40adefbb3

[thalesmg]

I found an analogous bug, triggered when one tries to call ssl_crl_cache:delete/1 with an URI.

I'll push a similar fix and a test for that case.

https://github.com/erlang/otp/pull/5996/commits/29882533faaea4851cc0723e5bfb3e942139a3ba

[dgud]

You will need to remove the enclosing [] in fresh_crl since get_crls have already removed the list.

fresh_crl(#'DistributionPoint'{distributionPoint = {fullName, Names}}, CRL) ->
    case get_crls(Names, undefined) of
	not_available ->
	    CRL;
->	[NewCRL] ->
	    NewCRL
    end.

[thalesmg]

Hi! Do I need to do some other changes here?

[IngelaAndin]

Well, the test case is a bit sparse! It is only testing that the delete function does not crash. You could use the following code to find the reference of the CRL-cache and do lookups before and after the deletion to see that it works.

  {status, _, _, StatusInfo} = sys:get_status(whereis(ssl_manager)),
   [_, _,_, _, Prop] = StatusInfo,
   State = ssl_test_lib:state(Prop),
   case element(5, State) =  [_, _, _, {CRLCache,_}],

[IngelaAndin]

@thalesmg Thank you for the PR

[thalesmg]

Thanks for the feedback! :tada: :confetti_ball: :beers: