Finish revoke_certificate and add specs

1. Add a try catch in the final revoke_certificate function
2. Also delete the certificate from persistent memory when it is done revoked

include/ejabberd_acme.hrl

@@ -10,25 +10,40 @@
 	  id  :: list(),
 	  key :: jose_jwk:key()
 	 }).
+-type data_acc() :: #data_acc{}.
 
 -record(data_cert, {
 	  domain  :: bitstring(),
-	  pem     :: bitstring(),
+	  pem     :: pem(),
-	  path    :: bitstring()
+	  path    :: string()
 	 }).
+-type data_cert() :: #data_cert{}.
 
+%%
+%% Types
+%%
 
+%% The main data type that ejabberd_acme keeps
+-type acme_data() :: proplist().
 
+%% The list of certificates kept in data
+-type data_certs() :: proplist(bitstring(), data_cert()).
+
+%% The certificate saved in pem format
+-type pem() :: bitstring().
 
 -type nonce() :: string().
 -type url() :: string().
 -type proplist() :: [{_, _}].
+-type proplist(X,Y) :: [{X,Y}].
 -type dirs() :: #{string() => url()}.
 -type jws() :: map().
 -type handle_resp_fun() :: fun(({ok, proplist(), proplist()}) -> {ok, _, nonce()}).
 
 -type acme_challenge() :: #challenge{}.
 
+%% Options
 -type account_opt() :: string().
+-type verbose_opt() :: string().
+
 
--type pem_certificate() :: bitstring().

src/ejabberd_acme.erl

@@ -48,129 +48,7 @@ is_valid_verbose_opt("plain") -> true;
 is_valid_verbose_opt("verbose") -> true;
 is_valid_verbose_opt(_) -> false.
 
-%%
-%% Revoke Certificate
-%%
 
-%% Add a try-catch to this stub
-revoke_certificate(CAUrl, Domain) ->
-    revoke_certificate0(CAUrl, Domain).
-
-revoke_certificate0(CAUrl, Domain) ->
-    BinDomain = list_to_bitstring(Domain),
-    case domain_certificate_exists(BinDomain) of
-	{BinDomain, Certificate} ->
-	    ?INFO_MSG("Certificate: ~p found!!", [Certificate]),
-	    ok = revoke_certificate1(CAUrl, Certificate),
-	    {ok, deleted};
-	false ->
-	    {error, not_found}
-    end.
-
-revoke_certificate1(CAUrl, Cert = #data_cert{pem=PemEncodedCert}) ->
-    {ok, _AccId, PrivateKey} = ensure_account_exists(),
-
-    Certificate = prepare_certificate_revoke(PemEncodedCert),
-    
-    {ok, Dirs, Nonce} = ejabberd_acme_comm:directory(CAUrl),
-    
-    Req = [{<<"certificate">>, Certificate}],
-    {ok, [], Nonce1} = ejabberd_acme_comm:revoke_cert(Dirs, PrivateKey, Req, Nonce),
-    ok.
-
-prepare_certificate_revoke(PemEncodedCert) ->
-    PemList = public_key:pem_decode(PemEncodedCert),
-    PemCertEnc = lists:keyfind('Certificate', 1, PemList),
-    PemCert = public_key:pem_entry_decode(PemCertEnc),
-    DerCert = public_key:der_encode('Certificate', PemCert),
-    Base64Cert = base64url:encode(DerCert),
-    Base64Cert.
-    
-domain_certificate_exists(Domain) ->
-    {ok, Certs} = read_certificates_persistent(),
-    lists:keyfind(Domain, 1, Certs).
-
-%%
-%% List Certificates
-%%
-
-list_certificates(Verbose) ->
-    try
-	list_certificates0(Verbose)
-    catch
-	throw:Throw ->
-	    Throw;
-	E:R ->
-	    ?ERROR_MSG("Unknown ~p:~p, ~p", [E, R, erlang:get_stacktrace()]), 
-	    {error, list_certificates}
-    end.
-
-list_certificates0(Verbose) ->
-    {ok, Certs} = read_certificates_persistent(),
-    case Verbose of
-	"plain" -> 
-	    [format_certificate(DataCert) || {_Key, DataCert} <- Certs];
-	"verbose" ->
-	    Certs
-    end.
-
-%% TODO: Make this cleaner and more robust
-format_certificate(DataCert) ->
-    #data_cert{
-       domain = DomainName,
-       pem = PemCert,
-       path = Path
-      } = DataCert,
-    
-    PemList = public_key:pem_decode(PemCert),
-    PemEntryCert = lists:keyfind('Certificate', 1, PemList),
-    Certificate = public_key:pem_entry_decode(PemEntryCert),
-
-    %% Find the commonName
-    _CommonName = get_commonName(Certificate),
-
-    %% Find the notAfter date
-    NotAfter = get_notAfter(Certificate),
-
-    format_certificate1(DomainName, NotAfter, Path).
-
-format_certificate1(DomainName, NotAfter, Path) ->
-    Result = lists:flatten(io_lib:format(
-			     "  Domain: ~s~n" 
-			     "    Valid until: ~s UTC~n" 
-			     "    Path: ~s", 
-			     [DomainName, NotAfter, Path])),
-    Result.
-
-get_commonName(#'Certificate'{tbsCertificate = TbsCertificate}) ->
-    #'TBSCertificate'{
-       subject = {rdnSequence, SubjectList}
-      } = TbsCertificate, 
-    
-    %% TODO: Not the best way to find the commonName
-    ShallowSubjectList = [Attribute || [Attribute] <- SubjectList],
-    {_, _, CommonName} = lists:keyfind(attribute_oid(commonName), 2, ShallowSubjectList),
-    
-    %% TODO: Remove the length-encoding from the commonName before returning it
-    CommonName.
-
-get_notAfter(#'Certificate'{tbsCertificate = TbsCertificate}) ->
-    #'TBSCertificate'{
-       validity = Validity
-      } = TbsCertificate,
-
-    %% TODO: Find a library function to decode utc time
-    #'Validity'{notAfter = {utcTime, UtcTime}} = Validity,
-    [Y1,Y2,MO1,MO2,D1,D2,H1,H2,MI1,MI2,S1,S2,$Z] = UtcTime,
-    YEAR = case list_to_integer([Y1,Y2]) >= 50 of
-	       true -> "19" ++ [Y1,Y2];
-	       _ -> "20" ++ [Y1,Y2]
-	   end,
-    NotAfter = lists:flatten(io_lib:format("~s-~s-~s ~s:~s:~s", 
-					   [YEAR, [MO1,MO2], [D1,D2],
-					    [H1,H2], [MI1,MI2], [S1,S2]])), 
-
-    NotAfter.
 
 %%
 %% Get Certificate
@@ -212,7 +90,7 @@ get_certificates0(CAUrl, "new-account") ->
 			       no_return().
 get_certificates1(CAUrl, PrivateKey) ->
     %% Read Config
-    {ok, Hosts} = get_config_hosts(),
+    Hosts = get_config_hosts(),
 
     %% Get a certificate for each host
     PemCertKeys = [get_certificate(CAUrl, Host, PrivateKey) || Host <- Hosts],
@@ -225,7 +103,7 @@ get_certificates1(CAUrl, PrivateKey) ->
     SavedCerts.
 
 -spec get_certificate(url(), bitstring(), jose_jwk:key()) -> 
-			     {'ok', bitstring(), pem_certificate()} | 
+			     {'ok', bitstring(), pem()} | 
 			     {'error', bitstring(), _}.
 get_certificate(CAUrl, DomainName, PrivateKey) ->
     ?INFO_MSG("Getting a Certificate for domain: ~p~n", [DomainName]),
@@ -243,7 +121,7 @@ get_certificate(CAUrl, DomainName, PrivateKey) ->
 -spec create_save_new_account(url()) -> {'ok', string(), jose_jwk:key()} | no_return().
 create_save_new_account(CAUrl) ->
     %% Get contact from configuration file
-    {ok, Contact} = get_config_contact(),
+    Contact = get_config_contact(),
 
     %% Generate a Key
     PrivateKey = generate_key(),
@@ -309,6 +187,8 @@ create_new_authorization(CAUrl, DomainName, PrivateKey) ->
 	    throw({error, DomainName, authorization})
     end.
 
+-spec create_new_certificate(url(), bitstring(), jose_jwk:key()) -> 
+				    {ok, bitstring(), pem()}.
 create_new_certificate(CAUrl, DomainName, PrivateKey) ->
     try
 	{ok, Dirs, Nonce0} = ejabberd_acme_comm:directory(CAUrl),
@@ -339,6 +219,7 @@ create_new_certificate(CAUrl, DomainName, PrivateKey) ->
 	    throw({error, DomainName, certificate})
     end.
 
+-spec ensure_account_exists() -> {ok, string(), jose_jwk:key()}.
 ensure_account_exists() ->
     case read_account_persistent() of
 	none ->
@@ -348,6 +229,152 @@ ensure_account_exists() ->
 	    {ok, AccId, PrivateKey}
     end.
 
+
+%%
+%% List Certificates
+%%
+-spec list_certificates(verbose_opt()) -> [string()] | [any()] | {error, _}.
+list_certificates(Verbose) ->
+    try
+	list_certificates0(Verbose)
+    catch
+	throw:Throw ->
+	    Throw;
+	E:R ->
+	    ?ERROR_MSG("Unknown ~p:~p, ~p", [E, R, erlang:get_stacktrace()]), 
+	    {error, list_certificates}
+    end.
+
+-spec list_certificates0(verbose_opt()) -> [string()] | [any()].
+list_certificates0(Verbose) ->
+    Certs = read_certificates_persistent(),
+    case Verbose of
+	"plain" -> 
+	    [format_certificate(DataCert) || {_Key, DataCert} <- Certs];
+	"verbose" ->
+	    Certs
+    end.
+
+%% TODO: Make this cleaner and more robust
+-spec format_certificate(data_cert()) -> string().
+format_certificate(DataCert) ->
+    #data_cert{
+       domain = DomainName,
+       pem = PemCert,
+       path = Path
+      } = DataCert,
+    
+    PemList = public_key:pem_decode(PemCert),
+    PemEntryCert = lists:keyfind('Certificate', 1, PemList),
+    Certificate = public_key:pem_entry_decode(PemEntryCert),
+
+    %% Find the commonName
+    _CommonName = get_commonName(Certificate),
+
+    %% Find the notAfter date
+    NotAfter = get_notAfter(Certificate),
+
+    format_certificate1(DomainName, NotAfter, Path).
+
+-spec format_certificate1(bitstring(), string(), string()) -> string().
+format_certificate1(DomainName, NotAfter, Path) ->
+    Result = lists:flatten(io_lib:format(
+			     "  Domain: ~s~n" 
+			     "    Valid until: ~s UTC~n" 
+			     "    Path: ~s", 
+			     [DomainName, NotAfter, Path])),
+    Result.
+
+-spec get_commonName(#'Certificate'{}) -> string().
+get_commonName(#'Certificate'{tbsCertificate = TbsCertificate}) ->
+    #'TBSCertificate'{
+       subject = {rdnSequence, SubjectList}
+      } = TbsCertificate, 
+    
+    %% TODO: Not the best way to find the commonName
+    ShallowSubjectList = [Attribute || [Attribute] <- SubjectList],
+    {_, _, CommonName} = lists:keyfind(attribute_oid(commonName), 2, ShallowSubjectList),
+    
+    %% TODO: Remove the length-encoding from the commonName before returning it
+    CommonName.
+
+-spec get_notAfter(#'Certificate'{}) -> string().
+get_notAfter(#'Certificate'{tbsCertificate = TbsCertificate}) ->
+    #'TBSCertificate'{
+       validity = Validity
+      } = TbsCertificate,
+
+    %% TODO: Find a library function to decode utc time
+    #'Validity'{notAfter = {utcTime, UtcTime}} = Validity,
+    [Y1,Y2,MO1,MO2,D1,D2,H1,H2,MI1,MI2,S1,S2,$Z] = UtcTime,
+    YEAR = case list_to_integer([Y1,Y2]) >= 50 of
+	       true -> "19" ++ [Y1,Y2];
+	       _ -> "20" ++ [Y1,Y2]
+	   end,
+    NotAfter = lists:flatten(io_lib:format("~s-~s-~s ~s:~s:~s", 
+					   [YEAR, [MO1,MO2], [D1,D2],
+					    [H1,H2], [MI1,MI2], [S1,S2]])), 
+
+    NotAfter.
+
+
+%%
+%% Revoke Certificate
+%%
+
+%% Add a try-catch to this stub
+-spec revoke_certificate(url(), string()) -> {ok, deleted} | {error, _}.
+revoke_certificate(CAUrl, Domain) ->
+    try
+	revoke_certificate0(CAUrl, Domain)
+    catch
+	throw:Throw ->
+	    Throw;
+	E:R ->
+	    ?ERROR_MSG("Unknown ~p:~p, ~p", [E, R, erlang:get_stacktrace()]), 
+	    {error, revoke_certificate}
+    end.	
+
+-spec revoke_certificate0(url(), string()) -> {ok, deleted} | {error, not_found}.
+revoke_certificate0(CAUrl, Domain) ->
+    BinDomain = list_to_bitstring(Domain),
+    case domain_certificate_exists(BinDomain) of
+	{BinDomain, Certificate} ->
+	    ?INFO_MSG("Certificate: ~p found!!", [Certificate]),
+	    ok = revoke_certificate1(CAUrl, Certificate),
+	    {ok, deleted};
+	false ->
+	    {error, not_found}
+    end.
+
+-spec revoke_certificate1(url(), data_cert()) -> ok.
+revoke_certificate1(CAUrl, Cert = #data_cert{pem=PemEncodedCert}) ->
+    {ok, _AccId, PrivateKey} = ensure_account_exists(),
+
+    Certificate = prepare_certificate_revoke(PemEncodedCert),
+    
+    {ok, Dirs, Nonce} = ejabberd_acme_comm:directory(CAUrl),
+    
+    Req = [{<<"certificate">>, Certificate}],
+    {ok, [], Nonce1} = ejabberd_acme_comm:revoke_cert(Dirs, PrivateKey, Req, Nonce),
+    ok = remove_certificate_persistent(Cert),
+    ok.
+
+-spec prepare_certificate_revoke(pem()) -> bitstring().
+prepare_certificate_revoke(PemEncodedCert) ->
+    PemList = public_key:pem_decode(PemEncodedCert),
+    PemCertEnc = lists:keyfind('Certificate', 1, PemList),
+    PemCert = public_key:pem_entry_decode(PemCertEnc),
+    DerCert = public_key:der_encode('Certificate', PemCert),
+    Base64Cert = base64url:encode(DerCert),
+    Base64Cert.
+
+-spec domain_certificate_exists(bitstring()) -> {bitstring(), data_cert()} | false.    
+domain_certificate_exists(Domain) ->
+    Certs = read_certificates_persistent(),
+    lists:keyfind(Domain, 1, Certs).
+
+
 %%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%
 %%
 %% Certificate Request Functions
@@ -508,13 +535,16 @@ get_challenges(Body) ->
     {<<"challenges">>, Challenges} = proplists:lookup(<<"challenges">>, Body),
     Challenges.
 
+-spec not_before_not_after() -> {binary(), binary()}.
 not_before_not_after() ->
-    %% TODO: Make notBefore and notAfter like they do it in other clients
+    %% TODO: Make notBefore and notAfter configurable somewhere
     {MegS, Sec, MicS} = erlang:timestamp(),
-    NotBefore = xmpp_util:encode_timestamp({MegS-1, Sec, MicS}),
+    NotBefore = xmpp_util:encode_timestamp({MegS, Sec, MicS}),
-    NotAfter = xmpp_util:encode_timestamp({MegS+1, Sec, MicS}),
+    %% The certificate will be valid for 60 Days after today
+    NotAfter = xmpp_util:encode_timestamp({MegS+5, Sec+184000, MicS}),
     {NotBefore, NotAfter}.
 
+-spec to_public(jose_jwk:key()) -> jose_jwk:key().
 to_public(PrivateKey) ->
     jose_jwk:to_public(PrivateKey).
     %% case jose_jwk:to_key(PrivateKey) of
@@ -530,7 +560,7 @@ to_public(PrivateKey) ->
 %% to_public(PrivateKey) ->
 %%     jose_jwk:to_public(PrivateKey).
 
-
+-spec is_error(_) -> boolean().
 is_error({error, _}) -> true;
 is_error(_) -> false.
 
@@ -539,7 +569,7 @@ is_error(_) -> false.
 %% Handle the persistent data structure
 %%
 %%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%
-
+-spec data_empty() -> [].
 data_empty() ->
     [].
 
@@ -547,6 +577,7 @@ data_empty() ->
 %% Account
 %%
 
+-spec data_get_account(acme_data()) -> {ok, list(), jose_jwk:key()} | none.
 data_get_account(Data) ->
     case lists:keyfind(account, 1, Data) of
 	{account, #data_acc{id = AccId, key = PrivateKey}} ->
@@ -555,6 +586,7 @@ data_get_account(Data) ->
 	    none
     end.
 
+-spec data_set_account(acme_data(), {list(), jose_jwk:key()}) -> acme_data().
 data_set_account(Data, {AccId, PrivateKey}) -> 
     NewAcc = {account, #data_acc{id = AccId, key = PrivateKey}},
     lists:keystore(account, 1, Data, NewAcc).
@@ -563,23 +595,32 @@ data_set_account(Data, {AccId, PrivateKey}) ->
 %% Certificates
 %% 
 
+-spec data_get_certificates(acme_data()) -> data_certs().
 data_get_certificates(Data) ->
     case lists:keyfind(certs, 1, Data) of
 	{certs, Certs} ->
-	    {ok, Certs};
+	    Certs;
         false ->
-	    {ok, []}
+	    []
     end.
 
+-spec data_set_certificates(acme_data(), data_certs()) -> acme_data().
 data_set_certificates(Data, NewCerts) -> 
     lists:keystore(certs, 1, Data, {certs, NewCerts}).
 
 %% ATM we preserve one certificate for each domain
+-spec data_add_certificate(acme_data(), data_cert()) -> acme_data().
 data_add_certificate(Data, DataCert = #data_cert{domain=Domain}) ->
-    {ok, Certs} = data_get_certificates(Data),
+    Certs = data_get_certificates(Data),
     NewCerts = lists:keystore(Domain, 1, Certs, {Domain, DataCert}),
     data_set_certificates(Data, NewCerts).
 
+-spec data_remove_certificate(acme_data(), data_cert()) -> acme_data().
+data_remove_certificate(Data, DataCert = #data_cert{domain=Domain}) ->
+    Certs = data_get_certificates(Data),
+    NewCerts = lists:keydelete(Domain, 1, Certs),
+    data_set_certificates(Data, NewCerts).
+
 
 %%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%
 %%
@@ -587,14 +628,17 @@ data_add_certificate(Data, DataCert = #data_cert{domain=Domain}) ->
 %%
 %%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%
 
+-spec persistent_file() -> file:filename().
 persistent_file() ->
     MnesiaDir = mnesia:system_info(directory),
     filename:join(MnesiaDir, "acme.DAT").
 
-%% The persistent file should be rread and written only by its owner
+%% The persistent file should be read and written only by its owner
+-spec persistent_file_mode() -> 384.
 persistent_file_mode() ->
     8#400 + 8#200. 
 
+-spec read_persistent() -> {ok, acme_data()} | no_return().
 read_persistent() ->
     case file:read_file(persistent_file()) of
 	{ok, Binary} ->
@@ -607,6 +651,7 @@ read_persistent() ->
 	    throw({error, Reason})
     end.
 
+-spec write_persistent(acme_data()) -> ok | no_return().
 write_persistent(Data) ->
     Binary = term_to_binary(Data),
     case file:write_file(persistent_file(), Binary) of
@@ -616,6 +661,7 @@ write_persistent(Data) ->
 	    throw({error, Reason})
     end.    
 
+-spec create_persistent() -> ok | no_return().
 create_persistent() ->
     Binary = term_to_binary(data_empty()),
     case file:write_file(persistent_file(), Binary) of
@@ -631,30 +677,40 @@ create_persistent() ->
 	    throw({error, Reason})
     end.        
 
+-spec write_account_persistent({list(), jose_jwk:key()}) -> ok | no_return().
 write_account_persistent({AccId, PrivateKey}) ->
     {ok, Data} = read_persistent(),
     NewData = data_set_account(Data, {AccId, PrivateKey}),
     ok = write_persistent(NewData).
 
+-spec read_account_persistent() -> {ok, list(), jose_jwk:key()} | none.
 read_account_persistent() ->
     {ok, Data} = read_persistent(),
     data_get_account(Data).
 
+-spec read_certificates_persistent() -> data_certs().
 read_certificates_persistent() ->
     {ok, Data} = read_persistent(),
     data_get_certificates(Data).
 
+-spec add_certificate_persistent(data_cert()) -> ok.
 add_certificate_persistent(DataCert) ->
     {ok, Data} = read_persistent(),
     NewData = data_add_certificate(Data, DataCert),
     ok = write_persistent(NewData).
 
+-spec remove_certificate_persistent(data_cert()) -> ok.
+remove_certificate_persistent(DataCert) ->
+    {ok, Data} = read_persistent(),
+    NewData = data_remove_certificate(Data, DataCert),
+    ok = write_persistent(NewData).
 
+-spec save_certificate({ok, bitstring(), binary()} | {error, _, _}) -> {ok, bitstring(), saved}.
 save_certificate({error, _, _} = Error) ->
     Error;
 save_certificate({ok, DomainName, Cert}) ->
     try
-	{ok, CertDir} = get_config_cert_dir(),
+        CertDir = get_config_cert_dir(),
 	DomainString = bitstring_to_list(DomainName),
 	CertificateFile = filename:join([CertDir, DomainString ++ "_cert.pem"]),
 	%% TODO: At some point do the following using a Transaction so
@@ -676,6 +732,7 @@ save_certificate({ok, DomainName, Cert}) ->
 	    {error, DomainName, saving}
     end.
 
+-spec write_cert(file:filename(), binary(), bitstring()) -> {ok, bitstring(), saved}.
 write_cert(CertificateFile, Cert, DomainName) ->
     case file:write_file(CertificateFile, Cert) of
 	ok ->
@@ -686,41 +743,45 @@ write_cert(CertificateFile, Cert, DomainName) ->
 	    throw({error, DomainName, saving})
     end.
 
+-spec get_config_acme() -> [{atom(), bitstring()}].
 get_config_acme() ->
     case ejabberd_config:get_option(acme, undefined) of
 	undefined ->
 	    ?ERROR_MSG("No acme configuration has been specified", []),
 	    throw({error, configuration});
         Acme ->
-	    {ok, Acme}
+	    Acme
     end.
 
+-spec get_config_contact() -> bitstring().
 get_config_contact() ->
-    {ok, Acme} = get_config_acme(),
+    Acme = get_config_acme(),
     case lists:keyfind(contact, 1, Acme) of
 	{contact, Contact} ->
-	    {ok, Contact};
+	    Contact;
 	false ->
 	    ?ERROR_MSG("No contact has been specified", []),
 	    throw({error, configuration_contact})
     end.
 
+-spec get_config_hosts() -> [bitstring()].
 get_config_hosts() ->
     case ejabberd_config:get_option(hosts, undefined) of
 	undefined ->
 	    ?ERROR_MSG("No hosts have been specified", []),
 	    throw({error, configuration_hosts});
         Hosts ->
-	    {ok, Hosts}
+	    Hosts
     end.
 
+-spec get_config_cert_dir() -> file:filename().
 get_config_cert_dir() ->
     case ejabberd_config:get_option(cert_dir, undefined) of
 	undefined ->
 	    ?ERROR_MSG("No cert_dir configuration has been specified", []),
 	    throw({error, configuration});
         CertDir ->
-	    {ok, CertDir}
+	    CertDir
     end.
 
 %%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%