Repository navigation
Support for Let's encrypt IP address certs - #4
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces the capability to issue SSL/TLS certificates for IP addresses (both IPv4 and IPv6) using the Let's Encrypt ACME provider. This feature extends the existing certificate management system to handle non-domain identifiers, providing greater flexibility for securing services directly accessible via IP. The changes involve modifications to certificate generation logic, SNI handling, and certificate parsing to correctly identify and process IP-based requests. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request adds support for issuing Let's Encrypt certificates for IP addresses. The changes are well-structured, touching the main library, the ACMEv2 implementation, and Erlang helper functions. New tests have been added to cover the IP address functionality. My review includes a few suggestions to improve code clarity, reduce duplication, and follow language idioms. I've also noted a small documentation typo.
|
|
||
| @doc """ | ||
| Returns the SSL options for the given domain. This is useful for IP based SSL certificates. | ||
| Info: https://letsencrypt.org/2026/01/15/6day-and-ip-general-availability |
There was a problem hiding this comment.
There's a typo in the year in the Let's Encrypt URL. It should be 2024, not 2026.
Info: https://letsencrypt.org/2024/01/15/6day-and-ip-general-availability
| csr = | ||
| X509.CSR.new(key, "CN=#{domain}") | ||
| |> X509.CSR.to_der() | ||
| |> benc() | ||
| if CertMagex.ip?(domain) do | ||
| ip_binary = ip_string_to_binary!(domain) | ||
| # OTP's public_key uses the GeneralName choice type `iPAddress` (capital P). | ||
| # X509's typespec expects `{atom(), charlist()}` for tuple SAN entries; octets as a | ||
| # charlist satisfy Dialyzer and encode the same as a binary iPAddress. | ||
| san = | ||
| X509.Certificate.Extension.subject_alt_name([ | ||
| {:iPAddress, :binary.bin_to_list(ip_binary)} | ||
| ]) | ||
|
|
||
| X509.CSR.new(key, "CN=", extension_request: [san]) | ||
| |> X509.CSR.to_der() | ||
| |> benc() | ||
| else | ||
| X509.CSR.new(key, "CN=#{domain}") | ||
| |> X509.CSR.to_der() | ||
| |> benc() | ||
| end |
There was a problem hiding this comment.
The if/else block has some code duplication (|> X509.CSR.to_der() |> benc()). You can refactor this to make it more DRY by moving the common operations outside the conditional.
csr =
(if CertMagex.ip?(domain) do
ip_binary = ip_string_to_binary!(domain)
# OTP's public_key uses the GeneralName choice type `iPAddress` (capital P).
# X509's typespec expects `{atom(), charlist()}` for tuple SAN entries; octets as a
# charlist satisfy Dialyzer and encode the same as a binary iPAddress.
san =
X509.Certificate.Extension.subject_alt_name([
{:iPAddress, :binary.bin_to_list(ip_binary)}
])
X509.CSR.new(key, "CN=", extension_request: [san])
else
X509.CSR.new(key, "CN=#{domain}")
end)
|> X509.CSR.to_der()
|> benc()
| defp ip_string_to_binary!(ip_string) do | ||
| case :inet.parse_address(String.to_charlist(ip_string)) do | ||
| {:ok, {a, b, c, d}} -> | ||
| <<a, b, c, d>> | ||
|
|
||
| {:ok, {a, b, c, d, e, f, g, h}} -> | ||
| <<a::16, b::16, c::16, d::16, e::16, f::16, g::16, h::16>> | ||
|
|
||
| _ -> | ||
| raise "Invalid IP address: #{inspect(ip_string)}" | ||
| end | ||
| end |
There was a problem hiding this comment.
This helper function ip_string_to_binary!/1 is a duplicate of the one defined in test/certmagex_test.exs. Duplicating code can lead to maintenance issues where one is updated but the other is forgotten. While common for testing private functions, consider if this utility could be made public in a suitable module to be shared, or if a shared test helper module would be appropriate.
| IPStrings = | ||
| [ip_binary_to_string(Bin) || {ipAddress, Bin} <- Names] ++ | ||
| [ip_binary_to_string(Bin) || {iPAddress, Bin} <- Names], |
There was a problem hiding this comment.
These two list comprehensions can be combined into one for slightly better performance and conciseness by checking for both ipAddress and iPAddress tags in a single pass.
IPStrings = [ip_binary_to_string(Bin) || {Tag, Bin} <- Names, (Tag =:= ipAddress orelse Tag =:= iPAddress)],
| test "returns true for IPv4 addresses" do | ||
| assert CertMagex.ip?("192.168.1.1") == true | ||
| assert CertMagex.ip?("0.0.0.0") == true | ||
| end | ||
|
|
||
| test "returns true for IPv6 addresses" do | ||
| assert CertMagex.ip?("::1") == true | ||
| assert CertMagex.ip?("2001:db8::1") == true | ||
| end | ||
|
|
||
| test "returns false for domain names" do | ||
| assert CertMagex.ip?("example.com") == false | ||
| assert CertMagex.ip?("sub.example.com") == false | ||
| end |
There was a problem hiding this comment.
For better readability and more idiomatic Elixir code, you can use assert/1 for truthiness checks and refute/1 for falsiness checks, instead of comparing with == true or == false.
test "returns true for IPv4 addresses" do
assert CertMagex.ip?("192.168.1.1")
assert CertMagex.ip?("0.0.0.0")
end
test "returns true for IPv6 addresses" do
assert CertMagex.ip?("::1")
assert CertMagex.ip?("2001:db8::1")
end
test "returns false for domain names" do
refute CertMagex.ip?("example.com")
refute CertMagex.ip?("sub.example.com")
end
No description provided.