-
Notifications
You must be signed in to change notification settings - Fork 60
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
Use implicit settings for lighttpd ≥1.4.68 #189
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
f35b63a
to
97385c1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I understand the changes either, could you please confirm the meaning behind:
{{#if (minver "1.4.68" form.serverVersion)}} | ||
{{#if output.serverPreferredOrder}} | ||
ssl.openssl.ssl-conf-cmd += ("Options" => "+ServerPreference") | ||
{{/if}} | ||
{{else}} | ||
ssl.openssl.ssl-conf-cmd += ("Options" => "{{#if output.serverPreferredOrder}}+{{else}}-{{/if}}ServerPreference") | ||
{{/if}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it worth differentiating? The JSON specs say either set preference, or don't set preference. Is it better to just omit it and leave to server default?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(I mean, the original +/- satisfied the specs exactly. Is there a reason starting ≥v1.4.68 for not setting -ServerPreference
explicitly?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@janbrasna the commit message cut-n-paste into the original description of this PR submission explains that it is best to use the lighttpd TLS defaults. You're welcome to disagree and I am happy to discuss further. However, I do not see how repeating that text a third time in this PR would be useful when I think that text already answers your question.
Is it better to just omit it and leave to server default?
Yes. I am a lighttpd developer and answer user questions on lighttpd forums. Yes, it is better to use lighttpd defaults.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah gotcha, so adding the complexity to the template logic this way makes the output configs actually simpler for newer versions, where you positively know you provide the same result. OK. It's not exactly readable from the partial, but if the end result is the same…
(SImilar issue was with certbot on httpd where locally overriding values explicitly set to defaults here was tricky.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gstrauss I sure read it… and expected the defaults to be unnecessary old protocols and such, not completely omitting @mozilla/server-side-tls's intermediate
cipher suites ;D that's why I raised the additional questions if I read it correctly and the resulting output is really meant that way and not a mistake.
src/templates/partials/lighttpd.hbs
Outdated
{{#if (minver "1.4.68" form.serverVersion)}} | ||
{{#if (includes "old" form.config)}} | ||
ssl.openssl.ssl-conf-cmd += ("CipherString" => "{{{join output.ciphers ":"}}}") | ||
{{/if}} | ||
{{else}} | ||
# TLS modules besides mod_openssl might name ciphers differently | ||
# See https://redmine.lighttpd.net/projects/lighttpd/wiki/Docs_SSL | ||
ssl.openssl.ssl-conf-cmd += ("CipherString" => "{{{join output.ciphers ":"}}}") | ||
{{/if}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This means not setting the cipher suites since ≥v1.4.68 unless old
, i. e. modern
and intermediate
won't set the suites at all according to json specs? Isn't the intention of the configs to explicitly set the cipher suites according to the specs, instead of leaving it to the server defaults?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gene1wood or do I read it wrong? I understand there are more and more great defaults provided by the servers, but that doesn't mean the generator should give up and not output the cipher suites according to the current .json recommendations (which is whole another discussion TBH).
Is there a consensus when the actual output should be left out for server defaults behind a version conditional?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@janbrasna as above, yes, it is better to use lighttpd secure TLS defaults.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@janbrasna if you have further questions how to configure lighttpd securely, please refer to the lighttpd TLS docs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gstrauss Nope, I don't have further questions how to configure lighttpd securely, I don't think I need to. Surprisingly that's not the issue.
My question is about this change, that basically disallows the output of https://statics.tls.security.mozilla.org/server-side-tls-conf-*.json
and leaves that to the vendor's definition. While claiming it's the @mozilla/server-side-tls's e. g. intermediate
… when it is no longer so;)
(even if the effectively used suites match today, that might not be true tomorrow — and that's why Mozilla's TLS specs are versioned and explicit, to be predictable, if needed for any given usecase e. g. PCI DSS testing etc.)
This PR basically reads for intermediate
and latestVersion
i. e. the default in the UI: "Claim it's Mozilla's https://wiki.mozilla.org/Security/Server_Side_TLS#Intermediate_compatibility_.28recommended.29 while in reality supplying Lighty's https://redmine.lighttpd.net/projects/lighttpd/wiki/Docs_SSL#Perfect-Forward-Secrecy-PFS …"
There are more vendors wishing the output not to mess with their secure TLS defaults so I'll open a broader discussion for it. I understand it, servers with great defaults should get their credit for having safe defaults, yet I don't see the need to cripple the json specs output to achieve this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"I will commit to updating lighttpd.hbs to match the Mozilla specs as soon as the Mozilla specs are updated." […] "Until then, they should be discarded and ignored in favor of current and maintained TLS recommendations from other qualified professional organizations."
@gstrauss You're happy to ignore them as you wish, and also to work on any other organization's docs and tools (or your forks thereof) in the meantime… and I'll be happy to continue reviewing this changeset once the upstream config data match your expectations so you don't have to hardcode your disagreement in the form of vandalizing the template output. (I'm perfectly fine with noting that in a comment though with the matching config included for reference for those envs that may need an exact match, so I'll look into that once I have some time to test drive a build with some of the recent changes…)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But the more important bit is — you'd probably need to do some of your own testing to make sure at least the publicly available test suites, when comparing to "Mozilla intermediate" or similar profile for OK/NOK or grading, won't complain about CCM8 and PSK suites that you include extra.
(I don't know honestly. They are all sorta specific use, not sure I'd be recommending them for general use without any more explanation — and not sure if any e.g. PCI testing suite wouldn't complain, for reasons unknown to me TBH, such as the plaintext username transfer in "pure" PSK for ≤TLS1.2 perhaps?)
@gstrauss How do you expose these? Are they enumerated to be explicitly enabled, or do you use some addition/subtraction based on defaults, so if they were not in default strings on the OS level, you're not adding them anyway…? Or…?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Read: If you need those, you need to know what to do and also enable them on purpose. We don't cater for such groups here in the configs. I'm not sure what all needs to be configured for them to even be able to end up negotiated — I'm only concerned about any automation/testing results that may stem from diverging from the exact restricted set in the jsons (as can be observed in the mozilla/server-side-tls#285 rationale — following any allowlists perhaps too literally…)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The omission of DHE-RSA-AES* is another topic, don't remember if just the DHE-RSA-CHACHA20 you provide is enough to support the sad SChannel compatibility when we had to keep the DHEs back in the day for RSA-only compatibility… will look into those, but quickly glancing https://learn.microsoft.com/en-us/windows/win32/secauthn/tls-cipher-suites-in-windows-7 it seems that would not support the oldest clients we claim unless ECDSA is used 🤷 so its presence without the others make no real difference in compatibility.
(and this is the exact reason I'm not willing to burn too much time making the configs "too implicit" and having to hunt for compatibility woes one by one… when we have a set that makes the conservative compatibility match the guidelines exactly, unless noted otherwise in the comments)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not really able to follow all the changes to Lighty defaults over time and with versions (hence why I'm hesitant to leave things to defaults and monitor changes constantly) but it seems that since the above comparison was posted, it's a bit stricter now — @gstrauss do I read it right that the PSK and DHE-RSA-CHACHA20 is gone now? And the presence of CCM/CCM8 is based on whether it was available in defaults before configuring the context? That would make the 3/24+ defaults reasonable, and only a note mentioning the absence of DHE thus the need for ECDSA chain to support the actual profile would suffice.
(BTW the actual Lighty cipher defaults are being documented under #PFS heading, it might be worth linking that section directly from the output comment as that's a valuable reference somewhat hard to find in general — as to why it's actually commented out, for comparison…)
The main benefit of not configuring the cipherstring explicitly would be enabling compatibility with other TLS libraries — and that's a very important improvement!
This comment was marked as resolved.
This comment was marked as resolved.
d106e8c
to
0b47781
Compare
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as duplicate.
This comment was marked as duplicate.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as duplicate.
This comment was marked as duplicate.
0b47781
to
97c1752
Compare
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as duplicate.
This comment was marked as duplicate.
1 similar comment
This comment was marked as duplicate.
This comment was marked as duplicate.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as outdated.
This comment was marked as outdated.
b5a5d46
to
b9686b4
Compare
This comment was marked as duplicate.
This comment was marked as duplicate.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
b9686b4
to
5d66972
Compare
This comment was marked as off-topic.
This comment was marked as off-topic.
5d66972
to
1b0dec2
Compare
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
src/templates/partials/lighttpd.hbs
Outdated
ssl.openssl.ssl-conf-cmd = ("MinProtocol" => {{#if (includes "TLSv1" output.protocols)}}"TLSv1"{{else if (includes "TLSv1.1" output.protocols)}}"TLSv1.1"{{else if (includes "TLSv1.2" output.protocols)}}"TLSv1.2"{{else}}"TLSv1.3"{{/if}}) | ||
{{#if (includes "TLSv1" output.protocols)}} | ||
ssl.openssl.ssl-conf-cmd = ("MinProtocol" => "TLSv1") | ||
{{else if (includes "TLSv1.1" output.protocols)}} | ||
ssl.openssl.ssl-conf-cmd = ("MinProtocol" => "TLSv1.1") | ||
{{else if (includes "TLSv1.2" output.protocols)}} | ||
{{#if (minver "1.4.78" form.serverVersion)}} | ||
ssl.openssl.ssl-conf-cmd = ("MinProtocol" => "TLSv1.2") | ||
{{else}} | ||
{{#unless (minver "1.4.56" form.serverVersion)}} | ||
ssl.openssl.ssl-conf-cmd = ("MinProtocol" => "TLSv1.2") | ||
{{/unless}} | ||
{{/if}} | ||
{{else if (includes "TLSv1.3" output.protocols)}} | ||
{{#unless (minver "1.4.78" form.serverVersion)}} | ||
ssl.openssl.ssl-conf-cmd = ("MinProtocol" => "TLSv1.3") | ||
{{/unless}} | ||
{{/if}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(This is one of the extremely complicated ways to "not emit defaults" at any cost, making the actual conditional spaghetti hard to maintain and understandable without some tribal knowledge. I'd much rather just print output.protocols[0]
as the lowest version /sorted/ and be done. TBD… Might be OK~ish tentatively if addressed once more powerful templating logic is available.)
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gstrauss Your main concern is forward compatibility by leaving out the defaults. In this case you generate a config for a version N, but just leaving out the defaults at that time, for version N, unknown what may come later. If that config content gets carried over to version N+1, can you still assure it won't break the expectations and recommendation tiers? (=You can't.) So I see the value in being explicit by not having the reality change behind the scenes for the user, as consistency and repeatability is one of the benefits of these configs.
If you configured intermediate
some time/versions ago (with TLS1.2 min left out as implicit), you'd basically get it disabled and be forced to modern
unknowingly over time and lighttpd updates once TLS1.2 is not the min default anymore. And that's not a feature, that's definitely a bug — the config won't be stable over time. (And you're aware of that, it's not an omission but definitely intentional… and I'm not willing to introduce intentional breakage or time bombs just to appease authors' perceptions of their own configs or defaults chosen — which are sort of irrelevant to the output of this tool.)
(This is borderline unreviewable for me as i can't even be sure the logic is not flawed… spending so much extra time on something i don't think should ever be the scope of the tool here…)
My opinion is this is doing the consumers disservice, but I don't see any value spending any more time trying to review it responsibly when I see it won't make any difference, so just the usual compromise:
Same as elsewhere, if you want to insist on not setting the value, at least emit it commented out with the note it's the default mentioning version numbers too — so when that changes over time, folks still know what is/was/should have been the explicit value and/or can uncomment it if there's a mismatch in their expectations.
(=Have them yell at server authors for changing things around. Not yell at @mozilla for generating crappy config boilerplates.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To demonstrate what I mean by inconsistent results:
I generate intermediate config for current 1.4.76 and use that — I get intermediate.
I use the same config in 1.4.78 once it comes out — and surprise, I get modern.
I have to come back to generate intermediate again this time for the newer 1.4.78 — and only then get intermediate back.
Basically the claim is the configs are more future proof, but in reality it makes them really compatible only with the version they were generated for — as there's so much logic that tries hard to leverage version-by-version changes to default, that once generated, it really should not be used with different server version, as you get different results than it was generated for.
I don't see it as positive, but at least with the comments stating moz config defaults at time of output and hardcoding the version it was generated for it should not confuse people too much.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NB: This changeset already includes the future defaults changing in a version yet unreleased — actually demonstrating the effect of the concerns above right here in the outputs how an intermediate
today will just stop working for TLSv1.2-only clients once you cross the version numbers hardcoded in the template to emit different configs, diverting from the guidelines in favor of server defaults on purpose again — but if you come back here and punch in your new version number, you'll have this time bomb removed and have a matching config again just fine. Escapes me.
We should try to widen the compatibility with different versions as much as we're able to, not making it as narrow and specific as possible. 🤷
I don't see the value in forcing people to come back to re-generate the template here after every upgrade.
Just wanted to pop in to say that the previous SSL Configuration Generator, circa 2015 or so, did use Javascript to generate its configurations. It was so complicated and messy that it was basically unusable after only supporting Apache and nginx and nobody wanted to touch it. It's certainly possible that you could restructure things better than it was before but I can confidently say that using Handlebars greatly simplified the design of the configuration generator and is what let it support as many things as it does today. |
@april thank you for the history. As an experiment, last week I rewrote all the .hbs partials into javascript in a development branch. The result saves over 70 KB (!) size in javascript over the Handlebars-generated javascript and also allows some shared code as well as simplification of conditions since javascript supports multiple conditions (e.g. For non-programmers, the javascript looks different than templates, but the different paradigm is similar to average Excel users putting data in simple tables and being unfamiliar with SQL. The javascript I wrote is not very complex since the result (each server config) is not that complex; evaluate some conditions and construct a string of the server configuration. However, the javascript is not a template and does not look like a template. To see the end result, the code should be run. Experiment created for discussion: (While I dabble in Javascript, it is not my primary language, so I am open to others sharing better idioms and approaches.) |
1b0dec2
to
ebde7a6
Compare
# lighttpd TLS defaults are widely supported by clients and should be preferred | ||
# See https://wiki.lighttpd.net/Docs_SSL | ||
# Uncomment to better match the less restricted Mozilla {{form.config}} spec. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wording +cipher naming compatibility +deeplink to PFS docs:
# lighttpd TLS defaults are widely supported by clients and should be preferred | |
# See https://wiki.lighttpd.net/Docs_SSL | |
# Uncomment to better match the less restricted Mozilla {{form.config}} spec. | |
# lighttpd uses secure TLS defaults, see https://wiki.lighttpd.net/Docs_SSL#Perfect-Forward-Secrecy-PFS | |
# If you need to match Mozilla {{form.config}} config exactly, uncomment below (OpenSSL compatibility only) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lighttpd uses secure TLS defaults
The lighttpd TLS defaults encompass more than simply the cipher list, so linking to PFS section is too specific for the intention of the statement.
If you need to match Mozilla {{form.config}} config exactly, uncomment below (OpenSSL compatibility only)
"OpenSSL compatibility only" is true of ssl-config-generator. lighttpd supports more than a handful of TLS libraries. While the suggestion is truthful, it is terse and likely to be confusing to consumers. I think it an overspecification of unnecessary information. If you use it with a different lighttpd TLS module instead of lighttpd mod_openssl, then it will either work or it will not work. In the latter case, lighttpd will emit an error, and the end-user can figure things out from there. Alternatively, we could overspecify and point to the server-side-tls mapping of cipher names between various libraries, but I think that is better in the wiki and not exploded into generated configs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it's an argument why the ciphers are commented out, a link to the default ciphers list should be considered a convenience to consumers who are told to follow the server defaults instead of the set the tool provides, to make the choice themselves. It's unnecessary to just send folks to the general docs and let them waste time figuring that out on their own where that info is. — If you read that section properly, you'll find out it's actually not a #PFS chapter, but a section explaining what defaults different lighty version use. Which is what I'm linking exactly (if you had a better heading to link, I'd happily do so, but it's all tucked away under PFS). Defaults for ciphers, defaults for TLS versions. All that's being added in this PR (or should I say removed?), and only changing that in this only instance.
(I'm not changing the link for other cases, only for the added/removed defaults, to link the defaults exactly. Other more verbose versions still output the general link as always.)
The note for OpenSSL–only compatibility was adapted from the same line of the existing config:
# TLS modules besides mod_openssl might name ciphers differently |
only simplified for brevity (comment length) to fit with the rest of the instructions, meaning uncommenting only works for implementations using the same cipher string format; not sure if it can be expressed more correctly without spanning three lines;] — you're always welcome to improve on my wording to better convey the meaning.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(if you had a better heading to link, I'd happily do so, but it's all tucked away under PFS)
The lighttpd TLS docs will be updated for the lighttpd Jan 2025 release which changes TLS defaults to MinProtocol => "TLSv1.3"
. An improper deep link might be broken, or might not longer point to the relevant portion of the document.
The note for OpenSSL–only compatibility was adapted from the same line of the existing config ... only simplified for brevity (comment length) to fit with the rest of the instructions
That was not clear to me, and I am intimately familiar with the material, so your "simplification for brevity" was not helpful.
I don't think we realistically would move away from using templates to turn the data into various forms of outputs (the charm of templating is the basic format is the target conf/yaml/ini/toml, with just the tokens adding some decisions here and there) — my guess is it's just the limits of Handlebars logic that started this, but the answer is not rewrite all of that into some bespoke spaghetti string manipulation, but address the individual limitations — probably considering a move from Handlebars to Mozilla's own Nunjucks (pretty much the same format, minimal impact, but adding the ability to set macros locally and call them with different values multiple times etc., set new variables, refer super() blocks etc. like in jinja2 that's used heavily around the org's projects), or the more low–level EJS that we already use in this project FWIW for the main render (that basically blends the JS with what's outside of the tokens, not adding any logic, you just write any javascript… but that has some migration overhead if we don't tweak the syntax options etc.)
I know the syntax;) the issue is when you start combining comments with whitespace control — most IDEs and syntax highlighters in general won't recognize the comments as comments then, which makes it a crappy experience. I removed most of my comments for that reason.
I'm sorry I don't follow what's the relation to EmberJS that we don't have here, and such framework use won't improve any of that (yes, any async approach and callbacks make the helpers a pain to use, and need decoupling from the webpack loader to point all that to own runtime instance manually etc. but we don't have to worry about that here). For performance there's template precompilation that I'm not sure we use. Macros and more logic is a HBS design choice that was OK when the templates started, but over time it turned out we'd need more; NJK or EJS covers that. In the meantime I quickly added a few helpers that I needed, and have no issues passing another helpers' output to them. 🤷
There's a lot of effort put into templating solutions to separate any output boilerplate from the actual code. I don't believe we should be storing the configuration formats with heavy escaping inside a waterfall of string operations. |
There is a balance to be struck. Templates make sense when the majority of the text is reused and the logic to fill in the template is relatively simple. Programmatic string concatenation makes sense where most of the output is conditional strings or when the logic to fill in the template might be more complex. |
@janbrasna I recognize that you disagree with and do not see the value in providing simpler guidance to people consuming ssl-config-generator. I do see the value and have quite a bit of experience supporting configuration questions for lighttpd. We can disagree on opinions here. I am going to move forwards with this PR. However, things are not set in stone. Future adjustments can be made based on feedback from users. |
lighttpd 1.4.56 and later default to MinProtocol TLSv1.2 lighttpd 1.4.68 and later default to a strict set of PFS ciphers and to -ServerPreference, since a strict set of PFS ciphers is the default. Simplify intermediate and modern configs for lighttpd 1.4.68 and later. lighttpd defaults will incrementally be made more secure in the future and using lighttpd secure defaults (without explicitly hard-coding the defaults at a point in time) will allow users to get more secure defaults along with lighttpd upgrades, instead of accidentally continuing to use explicitly set older, less-secure config settings.
list server.modules where modules are used lighttpd 1.4.56 and later ignore duplicates in server.modules, so uncomment those lines so that the modules are guaranteed to be loaded by the lighttpd config
lighttpd 1.4.78 will default to TLSv1.3
ebde7a6
to
c1e06d5
Compare
@april is there some place I could see the .js-based SSL Configuration Generator, circa 2015? This git repo has history only back to your initial commit in 2019, which is tagged v4.0 in the repo. If you have a moment, I'd also be interested in your high-level opinion of the part of SSL Config Generator that I converted to javascript underneath Handlebars:
Most of the .js helpers that I created from |
Updates lighttpd to 1.4.76
Leaves out explicit settings when server defaults based on its version can be used implicitly.
lighttpd defaults will incrementally be made more secure in the future and using lighttpd secure defaults (without explicitly hard-coding the defaults at a point in time) will allow users to get more secure defaults along with lighttpd upgrades, instead of accidentally continuing to use explicitly set older, less-secure config settings.
Adds future defaults that should be released in lighttpd with 1.4.78