Check the configuration file by default #13310
Conversation
| @@ -16,6 +16,10 @@ HOME = . | |||
| # Use this in order to automatically load providers. | |||
| openssl_conf = openssl_init | |||
|
|
|||
| # Comment out this if you deliberately want to ignore | |||
| # the configurations error | |||
| config_diagnostics = 1 | |||
There was a problem hiding this comment.
This should be commented out by default. The reason from #13303 applies:
The rationale for not using it in production is that it returns errors for malformed configuration files. This leads to DoS if the configuration is broken either by an attacker or by accident.
It's the same argument for ignoring errors when loading the configuration.
There was a problem hiding this comment.
As mentioned in #13303, I'm not sure I agree with the DoS argument.
There was a problem hiding this comment.
First, let the tests succeed...
There was a problem hiding this comment.
I'm thinking that there could be a DoS situation because of misconfiguration, and hiding the error certainly doesn't help anyone in that case
There was a problem hiding this comment.
Could such a misconfiguration prevent access to fix it though? I think that was the line @vdukhovni was following.
There was a problem hiding this comment.
Could such a misconfiguration prevent access to fix it though? I think that was the line @vdukhovni was following.
I'm struggling to see the line of thought here. We are talking about adding this to the default config file, which is in a known good state. If anyone changes it so that things are misconfigured, then this will immediately break. The fix is easy and obvious...roll back the previous change. It has surely got to be better to inform the sys admin asap that we detected a config problem.
What we cannot do is start providing diagnostics for random config files that are already deployed. If we did that then things might break and the fix may not be obvious.
There was a problem hiding this comment.
An mostly cosmetic error in the configuration file can signficantly impair a running system, breaking SSH, SMTP, and other critical services. Since we've not previously enforced mandatory validity of the configuration file, this would be a substantial change in semantics, and would need to be very prominently advertised in any release that makes configuration file errors "fatal".
I am not convinced that the configuration file is "critical" and that not running is better than running after ignoring an error in the file. Applications can explicitly load the config file and opt-in to error reporting, and we could provide tools to check the configuration...
There was a problem hiding this comment.
Since we've not previously enforced mandatory validity of the configuration file, this would be a substantial change in semantics,
Nor are we doing this now. Any existing config files would be unaffected. Anyone can remove the config_diagnostics line from the default config file if they wish it. Even with the line there, and in the event of config file errors, applications can continue anyway.
any release that makes configuration file errors "fatal".
They're not fatal even now. They're just "on the stack" as opposed to "not on the stack". If an application doesn't check for errors on the stack it will have no impact.
I am not convinced that the configuration file is "critical"
In 1.1.1 I'd probably agree. It is increasingly becoming critical. I expect many more people to be making config file changes in 3.0 than in 1.1.1. The impacts of a config file error are more significant in 3.0 as well. For example it could lead you to believe that you are running FIPS validated crypto when you are not.
There was a problem hiding this comment.
They're not fatal even now. They're just "on the stack" as opposed to "not on the stack". If an application doesn't check for errors on the stack it will have no impact.
Oh apparently I'm wrong on this point. The "config_diagnostics" setting actually causes OPENSSL_init_crypto() to fail where OPENSSL_INIT_LOAD_CONFIG is passed as a flag.
|
So now the team (maybe the OTC) should reach a consensus whether we should leave this option enabled by default (and disablable by vendors) or not. |
beldmit
added
the
approval: done
openssl-machine
added
approval: ready to merge
|
This pull request is ready to merge |
Reviewed-by: Matt Caswell <matt@openssl.org> (Merged from #13310)
|
Merged. Thanks! |
Checklist