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.

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

Ignore entries starting with "data:" #32

Merged
merged 2 commits into from Apr 18, 2021

Conversation

bblanchon
Copy link
Contributor

Hi @valdergallo,

Thank you very much for this awesome project.

I had a problem with my conf files containing data:$variable entries: they were renamed to myfile.conf.nokey.
This Pull Request adds a test to ignore files starting with data:

I included some explanation in the commit message:

Since version 1.15.10, Nginx supports the loading of SSL certificates and secret keys from variables.
From the documentation: "The value data:$variable can be specified instead of the file, which loads a certificate from a variable without using intermediate files."

For example, this feature allows using an empty certificate for the "default" server: https://serverfault.com/a/1044022/111986

Best regards,
Benoit

Since version 1.15.10, Nginx supports the loading of SSL certificates and secret keys from variables.
From the documentation: "The value data:$variable can be specified instead of the file, which loads a certificate from a variable without using intermediate files."

For example, this feature allows using an empty certificate for the "default" server: https://serverfault.com/a/1044022/111986
@JonasAlfredsson
Copy link
Owner

JonasAlfredsson commented Apr 17, 2021

Hi Benoit,

This was an unknown feature of Nginx for me!
I will look into this a bit more tomorrow, but an open question I have (that you might know the answer to) is if we should make some kind of check that this variable is defined (at least set to empty string)? Will Nginx panic (like in the case when a file on disk does not exist) or does it handle it more gracefully?

//Jonas

@JonasAlfredsson JonasAlfredsson self-assigned this Apr 17, 2021
@JonasAlfredsson JonasAlfredsson added the enhancement New feature or request label Apr 17, 2021
@bblanchon
Copy link
Contributor Author

I didn't try, but I'm pretty sure Nginx will panic if the variable is undefined.

I don't think you can (nor should) do anything about it.
An undefined variable is comparable to a syntax error: it's up to the user to fix it.

@JonasAlfredsson
Copy link
Owner

Some small things I will add here as a notes for myself in the future:

  • Using variables implies that a certificate will be loaded for each SSL handshake, and this may have a negative impact on performance.
  • When the certificate has a static path, it is loaded at init time which runs as root. When using a variable, it is loaded at runtime which generally runs as www or nginx so it is likely not going to have the permissions.
  • Inappropriate use of the data: syntax may have its security implications, such as writing secret key data to error log.

Source 1: https://serverfault.com/a/956167
Source 2: https://nginx.org/en/docs/http/ngx_http_ssl_module.html#ssl_certificate

@JonasAlfredsson
Copy link
Owner

Some entries allow a couple of special directives beyond file, which just means a filepath:

If we implement this change we should perhaps include the engine directive as well.

@bblanchon
Copy link
Contributor Author

Indeed, I didn't know about the engine: entries; I pushed a second commit for that.

Note: the sentence "using variables implies that a certificate will be loaded for each SSL handshake" only applies to file paths that include a variable, which is not the same as a data: entry. No file is involved in a data: entry.

@JonasAlfredsson
Copy link
Owner

JonasAlfredsson commented Apr 18, 2021

Nice!
I will merge this, but I will move some things around afterwards and add log messages for the sake of easier identification of what is happening in the future.

Thanks for the contribution, and informing me about this pretty neat feature :)

@JonasAlfredsson JonasAlfredsson merged commit cf1ac40 into JonasAlfredsson:master Apr 18, 2021
@bblanchon
Copy link
Contributor Author

You're welcome, Jonas.
Thanks again for maintaining this project!

@bblanchon bblanchon deleted the ignore-data-entries branch April 18, 2021 15:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants