Conversation
![hikaflow[bot]](https://avatars.githubusercontent.com/in/1109659?s=60&v=4){kind=small}
| "Invalid automatic TLS enabling option value" | ||
| ))? | ||
| } | ||
| if used_properties.contains("enableAutomaticTLS") |
There was a problem hiding this comment.
Identical Code - Priority: Medium
The condition checking for enableAutomaticTLS is identical in functionality between ferron/src/server.rs and ferron/src/util/validate_config.rs. This could be refactored to avoid redundancy.
| if let Some(domain) = domain_yaml.as_str() { | ||
| if !domain.contains("*") { | ||
| // check if we even want auto-TLS for this domain | ||
| let want_auto_tls = host |
There was a problem hiding this comment.
Duplicate Code - Priority: High
The logic for handling enableAutomaticTLS is duplicated between ferron/src/server.rs and ferron/src/util/validate_config.rs. Consider extracting this logic into a shared utility function.
PR SummaryChanges Overview
Identified Issues
Recommendations
|
|
I'm rewriting Ferron, and added support for mixed automatic and manual TLS in the rewrite (Ferron 2.x, which is now in beta). // This is just an example
example.com {
// Automatic TLS (it's enabled by default)
root "/var/www/example"
}
manual.example.com {
// Manual TLS
tls "/etc/certs/example.crt" "/etc/certs/example.key"
root "/var/www/example"
} |
|
Looks nice! |
|
I have marked Ferron 1.x as "in maintenance mode" in the f3de200 commit, so I'm not sure if I'm going to merge your pull request, as it's against the development branch for Ferron 1.x... The functionality discussed about in the pull request is already supported in Ferron 2. |
2 participants
This is another PR that's meant to start a discussion, rather than accept the code as-is.
Basically, I found it really helpful to exclude some domains from auto-TLS while testing apps before they were ready for deployment.
In this case, I was migrating an app from one server to another, so it was impossible to get TLS certs for the app on the destination server since DNS hadn't been updated yet. But I still wanted to be able to test the app on the target server before finalizing the migration, but Ferron kept trying to get the TLS certs for every domain its hosts have and failing on the one domain with old DNS settings. It's common to edit your
/etc/hostsfile in these cases so you can test deployments locally without making them public yet, which gives you the effect of bypassing public DNS for a bit. So this patch allows configuring each domain to be included in auto-TLS, or not.But, the reason why I think this might not be a good PR as-is is because it only solves part of the problem. Once you skip some domains during auto-TLS, you still want to be able to access them through Ferron. Either over regular HTTP without a cert, or over HTTPs with a local self-signed cert.
If you've generally configured Ferron to use HTTPs, then that basically rules out regular HTTP since the configuration isn't granular enough to be per-host. So one possible improvement would be to add per-host HTTPs settings.
If you still want to use HTTPs for all your hosts, even in local testing, then ideally you could attach a self-signed cert to the host and use than instead of the auto-TLS one. However, editing the
sniconfiguration to add a self-signed cert to the host seemed to be ignored by Ferron in this case, possibly because auto-TLS was enabled. So another possible improvement here would be to allow Ferron to use thesnicerts in cases when auto-TLS has been disabled for a host.Hopefully that makes sense, and hopefully I've motivated the use case enough here. Let me know what you think, and if you have any questions.