Future proof for x509 v0.4.0

[aforward]

A method from the x509 library has been removed

https://github.com/voltone/x509/commit/02aa79e3244deee42c3b0371b21cbc4ec8f05a6d

v0.4.0

Breaking changes

• [X509] Removed to_der/1, to_pem/1 and from_der/2

I have updated the library to use the underlying function, to reproduce the issue run mix deps.update --all and then you will see the following warning

warning: function X509.to_pem/1 is undefined or private
  lib/site_encrypt.ex:61

Or, drop the latest code in production and then see tihngs blow up.

[aforward]

I am testing, there might be a few other issues. I'll report back once I have run through all the possible breaking changes.

[aforward]

Sorry, my sunday got away from me. I have not finished tested against creating a legit call to confirm this will address all breaking changes form the upgrade to v0.4.0.

[sasa1977]

Question for @voltone: since X509.to_pem will be removed, is this the proper way to convert the chain to pem?

I have not finished tested against creating a legit call to confirm this will address all breaking changes form the upgrade to v0.4.0.

If you're referring to automated tests, they are currently mostly non-existent. I think, however, that implementing tests is a separate task, and should not be done as a part of this pull. For the moment, it would suffice to manually test the changes by starting the included demo project.

This is admittedly far from perfect, and it would be better if we had automated tests. There are open related issues #5 and #10. I think that addressing #5 would already take us far, because we'd at least get integration level tests, which are IMO the most important ones. But again, I think this should be tackled outside of this PR.

[aforward]

I am referring to a manual test, I have a sandbox environment (separate from my dev box) that I deploy against to generate a new certification; I didn't get a chance to finish that so I have not seen an end-to-end work yet.

[voltone]

Oops, I was sure I replaced all the deprecated functions in #14, but looks like I missed this one. Sorry!

I would keep all the X509 stuff in SiteEncrypt.Crypto, and return PEM binaries from self_signed_chain/1 (https://github.com/sasa1977/site_encrypt/blob/master/lib/acme_server/crypto.ex#L28):

%{
  ca_cert: Certificate.to_pem(ca_cert),
  server_cert: Certificate.to_pem(server_cert),
  server_key: PrivateKey.to_pem(server_key)
}

[sasa1977]

I am referring to a manual test, I have a sandbox environment (separate from my dev box) that I deploy against to generate a new certification; I didn't get a chance to finish that so I have not seen an end-to-end work yet.

Wouldn't the demo project which exists in this repo suffice?

I would keep all the X509 stuff in SiteEncrypt.Crypto, and return PEM binaries from self_signed_chain/1

Thanks for the quick response! That looks good to me. @aforward if you agree, then we could do it like @voltone suggests.

[aforward]

@sasa1977, demo or not; I just ran out of time for a full manual test :-(

But yes, let's close this PR in favour of the suggested approach. That said, I don't fully follow the suggested path forward, is this now a documentation issue (aka change our configs), or a PR change (or both).

[sasa1977]

That said, I don't fully follow the suggested path forward

If I properly understand @voltone, the idea is to replace the suggested line (https://github.com/sasa1977/site_encrypt/blob/master/lib/acme_server/crypto.ex#L28) with the suggested snippet:

%{
  ca_cert: Certificate.to_pem(ca_cert),
  server_cert: Certificate.to_pem(server_cert),
  server_key: PrivateKey.to_pem(server_key)
}

That way, the output of AcmeServer.Crypto.self_signed_chain returns pems as values, so there's no need to perform the conversion to PEM in the caller site (and the only such place is the one where you made your changes). So the caller site would end up looking like:

defp generate_self_signed_certificate!(config) do
  # ...

  [config.domain | config.extra_domains]
  |> AcmeServer.Crypto.self_signed_chain()
  |> Stream.map(fn {type, pem} -> {file_name(type), pem} end)
  |> Enum.each(&save_pem!(config, &1))
end

But yes, let's close this PR in favour of the suggested approach.

I still propose making the changes in this PR, so it's easier to find this discussion.

[aforward]

Taking a look now

[aforward]

Tested and appears to work as expected

IMPORTANT NOTES:
 - Congratulations! Your certificate and chain have been saved at:

[sasa1977]

Thanks for contributing!