Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make the location of OpenSSL configurable #201

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Make the location of OpenSSL configurable #201

wants to merge 3 commits into from

Conversation

jlouis
Copy link

@jlouis jlouis commented Sep 1, 2017

Apple devices as well as older software installations of Linux are
likely to have too old OpenSSLs for the token signing to work. Fix
this by making the location of the OpenSSL binary configurable in the
configuration knobs.

We needed this patch for two reasons: Apple, and older Linux distributions. I'm just suggesting it as a patch for now, and I think the _path component is a bad name. Probably rather call it openssl_executable or something such. Also, it needs documentation.

However, if those things were fixed, would you guys be willing to take this patch in?

Apple devices as well as older software installations of Linux are
likely to have too old OpenSSLs for the token signing to work. Fix
this by making the location of the OpenSSL binary configurable in the
configuration knobs.
Command = "printf '" ++
binary_to_list(Data) ++
"' | openssl dgst -binary -sha256 -sign " ++ KeyPath ++ " | base64",
"' | " ++ Openssl ++ " dgst -binary -sha256 -sign " ++ KeyPath ++ " | base64",
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Line 41 is too long: "' | " ++ Openssl ++ " dgst -binary -sha256 -sign " ++ KeyPath ++ " | base64",.

@ferigis
Copy link
Member

ferigis commented Sep 1, 2017

Hi @jlouis, Thanks for using apns4erl!

Actually the new HTTP2 APNs API requires to have openssl updated since it needs using TLS 1.2+

Do you think we will need this patch having in mind openssl should be updated? If the answer is yes, of course we are willing to take it in :)

Thanks

@jlouis
Copy link
Author

jlouis commented Sep 2, 2017

If you use something like "brew" on OSX, the Erlang binary will be compiled with a recent OpenSSL, supporting TLS 1.2+ and so on. But the openssl(1) binary will be stashed away in a subdirectory and not be in the path. The reason is that they don't want to mess with the Apple-supplied /usr/bin/openssl I guess. And the OpenSSL story is a mess where you often have multiple versions of it installed, depending on what it supports. Add to that the recent LibreSSL stuff on top, and we are set for endless headache :) My guess is this is also likely to happen on Linux machines, one way or the other.

By making the executable into a knob you can turn, it is far easier to configure the system in an operations-situation. Leave the existing OpenSSL installation be on the machine, build an Erlang with its own linkage to a newer openssl, and arrange the location of the binary to be somewhere else. This leaves the existing installation untouched, but provides a newer TLS for the Erlang installation. Perhaps in the future with a mature docker or kubernetes, this is less useful. But I think it should go in for now.

Using openssl_path could be interpreted as a directory to the
bin-directory, not the `openssl(1)` command itself. Correct this by
using a better name.
@jlouis
Copy link
Author

jlouis commented Sep 7, 2017

This should fix the naming of the OpenSSL command and also make elvis happy.

Remaining is to document this in the README file. Do you want me to go ahead and do that?

@@ -13,6 +13,9 @@
]},
{modules, []},
{mod, {apns_app, []}},
{env, [
{openssl_command, "openssl"}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should write some doc about this in the README explaining it exists and how it works.

@paulo-ferraz-oliveira
Copy link
Contributor

Hi. Was this option finally made available or not? This is still a valid concern for me (only now started using this lib.).

@jlouis
Copy link
Author

jlouis commented Oct 24, 2020

I don't think so. I ended writing my own APNS bindings for the company, but it isn't open source I'm afraid, so I didn't pursue this patch too much in the process. One particular reason was that I wanted something else than Chatterbox as the implementation of the HTTP2 transport.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants