Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Check the configuration file by default #13310

Closed
wants to merge 4 commits into from
Closed

Check the configuration file by default #13310

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3a761b5
Check the configuration file by default
beldmit Nov 4, 2020
547e2c0
fixup! Check the configuration file by default
beldmit Nov 4, 2020
cefda18
fixup! Check the configuration file by default
beldmit Nov 4, 2020
acefa36
fixup! Check the configuration file by default
beldmit Nov 4, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 4 additions & 0 deletions apps/openssl-vms.cnf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,10 @@ HOME = .
# Use this in order to automatically load providers.
openssl_conf = openssl_init

# Comment this out if you deliberately want to ignore
# the configuration errors
beldmit marked this conversation as resolved.
Show resolved Hide resolved
config_diagnostics = 1

# Extra OBJECT IDENTIFIER info:
# oid_file = $ENV::HOME/.oid
oid_section = new_oids
Expand Down
4 changes: 4 additions & 0 deletions apps/openssl.cnf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,10 @@ HOME = .
# Use this in order to automatically load providers.
openssl_conf = openssl_init

# Comment this out if you deliberately want to ignore
# the configuration errors
config_diagnostics = 1
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As mentioned in #13303, I'm not sure I agree with the DoS argument.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First, let the tests succeed...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could such a misconfiguration prevent access to fix it though? I think that was the line @vdukhovni was following.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


# Extra OBJECT IDENTIFIER info:
# oid_file = $ENV::HOME/.oid
oid_section = new_oids
Expand Down