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

Add SSL validation support for certs_keys #1260

Merged
merged 3 commits into from
Feb 17, 2025

Conversation

joshk
Copy link
Contributor

@joshk joshk commented Feb 16, 2025

The certs_keys config key was added recently(ish) to the Erlang SSL stack.

https://www.erlang.org/doc/apps/ssl/ssl.html#t:cert_key_conf/0

Verified

This commit was signed with the committer’s verified signature.
joshk Josh Kalderimis
The `certs_keys` config key was added recently(ish) to the Erlang SSL stack.

https://www.erlang.org/doc/apps/ssl/ssl.html#t:cert_key_conf/0
@josevalim
Copy link
Member

Thanks! Could we perhaps have a unit test for it?

@joshk
Copy link
Contributor Author

joshk commented Feb 16, 2025

Thanks! Could we perhaps have a unit test for it?

Sure, I'll work on that tomorrow. I couldn't find any tests for the keyfile and certfile logic, so didn't know if tests would be required for this.

How do you suggest I handle the OTP app logic?

  defp otp_app(options) do
    if app = options[:otp_app] do
      Application.app_dir(app)
    else
      fail("the :otp_app option is required when setting relative SSL certfiles")
    end
  end

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
Co-authored-by: José Valim <jose.valim@gmail.com>
@josevalim
Copy link
Member

Perhaps you could use :plug as the otp app itself and, if necessary, have it pick files from tmp? To be clear, it doesn't need to be an integration test, testing the configure function is enough. :)

@joshk
Copy link
Contributor Author

joshk commented Feb 16, 2025

Funny timing, I just thought about using plug as the app.

Cool cool, that was my plan. I'll just test configure, which validates and throws an error if the OTP app can't be found.

I'll have this done in the morning. I'm working on a nicer Acme integration to use for NervesHub and NervesCloud, and this is part of the puzzle.

Verified

This commit was signed with the committer’s verified signature.
joshk Josh Kalderimis
@joshk
Copy link
Contributor Author

joshk commented Feb 16, 2025

@josevalim done! although I did need to essentially revert your commit suggestion due to it not working as expected.

@joshk joshk requested a review from josevalim February 16, 2025 22:18
@josevalim josevalim merged commit 187266d into elixir-plug:main Feb 17, 2025
2 checks passed
@joshk
Copy link
Contributor Author

joshk commented Feb 17, 2025

@josevalim thank you for the merge! its almost like the good ol' days when you were mentoring me with Rails contributions :D

Do you have any plans for a new release?

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.

None yet

2 participants