Repository navigation
feat: SNI hostname allow list (:sni_allowed_hosts) - #5
Conversation
Add :sni_allowed_hosts config. When set to a non-empty list, only those hostnames (case-insensitive) trigger certificate resolution and ACME; any other SNI returns :undefined. Prevents spurious cert issuance from random client SNIs (e.g. internet scanners). Bump to v1.3.0.
There was a problem hiding this comment.
Code Review
This pull request introduces an optional SNI hostname allow list to prevent unauthorized certificate requests, updating the documentation and bumping the version to 1.3.0. The reviewer suggested moving the debug log inside the allow list check to reduce noise, optimizing the hostname comparison for better performance, and expanding the test suite to cover positive matches and case-insensitivity.
| if sni_host_allowlisted?(domain) do | ||
| sni_fun_allowed(domain) | ||
| else | ||
| :undefined | ||
| end |
There was a problem hiding this comment.
To better achieve the goal of reducing noise from internet scanners, consider moving the Logger.debug call (currently on line 53) inside this if block. This ensures that only allowlisted requests are logged. Note: You will also need to remove the redundant Logger.debug call on line 53.
if sni_host_allowlisted?(domain) do
Logger.debug("CertMagex: sni_fun called with domain: #{domain}")
sni_fun_allowed(domain)
else
:undefined
end
|
|
||
| defp host_in_sni_list?(domain, hosts) do | ||
| d = String.downcase(domain) | ||
| Enum.any?(hosts, &(String.downcase(&1) == d)) |
There was a problem hiding this comment.
Performing String.downcase/1 on every element of the allow list during every TLS handshake is inefficient. This suggestion adds a fast-path check for cases where the configuration already contains lowercase hostnames. For best performance, you should also recommend in the documentation that hostnames in the configuration be provided in lowercase.
d in hosts or Enum.any?(hosts, &(String.downcase(&1) == d))
| test "returns :undefined when the hostname is not in the allow list" do | ||
| previous = Application.get_env(:certmagex, :sni_allowed_hosts) | ||
| Application.put_env(:certmagex, :sni_allowed_hosts, ["good.example.com"]) | ||
|
|
||
| on_exit(fn -> | ||
| if previous, | ||
| do: Application.put_env(:certmagex, :sni_allowed_hosts, previous), | ||
| else: Application.delete_env(:certmagex, :sni_allowed_hosts) | ||
| end) | ||
|
|
||
| assert :undefined = CertMagex.sni_fun("scanner.example.com") | ||
| end |
There was a problem hiding this comment.
The current test cove 8000 rage for the new allow list feature is minimal. It would be beneficial to add test cases for:
- A positive match (hostname in allow list).
- Case-insensitive matching (e.g., mixed-case hostname in list).
- Behavior when the allow list is empty or unset (should allow all).
|
@cursor review the gemini comments and address as appropriate |
Summary
Adds an optional
:sni_allowed_hostsapplication config for CertMagex. When set to a non-empty list, only those hostnames (case-insensitive) go through the existing IP check and certificate cache/ACME path; any other TLS SNI returns:undefinedwithout ACME. When unset or[], behavior matches the previous default (all SNIs are considered).Motivation
This mirrors the allowlist pattern used in Diode Console via
Console.CertSni/PHX_ADDITIONAL_SNI_HOSTS, so apps can use plain&CertMagex.sni_fun/1without a wrapper module.Version
Bumps package to v1.3.0.
Testing
mix test:undefined