-
Notifications
You must be signed in to change notification settings - Fork 820
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
RFC: new way of loading certificate files #785
Comments
Hello, This is not a bug and works as designed. Indeed the "crt" option takes a basename which will be used to search other files by adding suffixes (".issuer", ".ocsp", ".key" etc.) It won't try to remove the extension of your basename. https://cbonte.github.io/haproxy-dconv/2.2/configuration.html#ssl-load-extra-files
You can only do that with a cert dir with 'crt-base' but we could probably add a 'key-base' too, that could be backported in 2.2. The problem with a "key" directive is that it will apply on the whole bind line, and that's not a good idea because it will apply on all crt instances. Only a "key" directive in a crt-list file makes sense. |
Yes would be very nice, I am now scripting to auto generate the pems. I If you have key and cert dir and specifiy those files, you do not need to do anything more. Except for ca validation. You should also not force people the use of names. Although most follow standards, it is just never 'one size fits all'. I don't think I have ever seen any application, forcing me to use basename.key. I think it is even the opposite of logical what is now implemented. In my case all the file names are the same with a nice 3 letter different extension. Furthermore your basename usage is also inconsistent, logically you should apply .crt to it. So in my case it would be dev.marathon.mesos.crt.crt so I could configure it as dev.marathon.mesos and crt and key are appended. So I would rethink this ssl configuration, stick to file names easier configuration, less coding, less options for mistakes. |
I agree with you regarding the use of names, but we had some limitations:
This was the original design of the SSL configuration parsing in HAProxy 1.5, I honestly hates it as most people use separate files and directories instead of PEM. When the OCSP was implemented, we couldn't add a "ocsp" directive because of (2), so a .ocsp file is looked up using the basename in "crt". And every features followed the same design after that, becoming worse with the bundles, which could look after ".ecdsa.ocsp" for example. I won't be able to change the way we look at files because of backward compatibility. But the introduction of the "ssl-load-extra-files" directive was a first step to disable the auto discover and be able to specify the files with new directives in the future. Specifying the extra files manually by certificate won't be compatible with the "crt" directive, so we will need to use the "crt-list" keyword for that. A lot of things changed internally in HAProxy, we now have the concept of certificate storage, which is not really visible in the configuration. But what HAProxy does is that it will store all files related to a "crt' in the certificate storage and then load this storage for each new bind line it is used. So maybe what we need is to be able to declare a new certificate storage in a dedicated section. I don't have any definitive proposal for this, I'll probably open a new ticket with some thoughts. |
Good, interesting background info! |
After discussing this with several people here are more thoughts about this:
This is an example of what it could be:
|
How about using the
So you'd just have to pass |
We thought about it first, but that's not really convenient for people having thousands of certificates. What I suggested will be compatible with crt-list, however if we want to use a different name we could use an optional "name" parameter, for example:
|
I'm overall fine with this but I'm convinced that we really need to be able to name the section and probably to make it mandatory, like this:
This would allow to easily load all the certs mentioned in such a group (e.g. What I like with this format is that we can later think about extending it, for example by adding:
|
We talked about this, face to face with @wtarreau and decided to change a little bit the format:
|
In issue #785, users are reporting that it's not convenient to load a ".crt.key" when the configuration contains a ".crt". This option allows to remove the extension of the certificate before trying to load any extra SSL file (.key, .ocsp, .sctl, .issuer etc.) The patch changes a little bit the way ssl_sock_load_files_into_ckch() looks for the file.
This comment has been minimized.
This comment has been minimized.
In issue haproxy#785, users are reporting that it's not convenient to load a ".crt.key" when the configuration contains a ".crt". This option allows to remove the extension of the certificate before trying to load any extra SSL file (.key, .ocsp, .sctl, .issuer etc.) The patch changes a little bit the way ssl_sock_load_files_into_ckch() looks for the file. (cherry picked from commit 8e8581e) Signed-off-by: William Lallemand <[email protected]>
It works well. |
The problem is that the path of each file is not stored, only the path of the "crt" is indexed. So when you are doing a commit this is not a problem, but if you do a "set" with a ".key", haproxy is not able to know what was the extension of the corresponding crt. So it's a little bit difficult to fix with the current architecture, you will still need to set ".key" at the end of the current path, and we will have the same problem with the new "crt-store" keyword. I'll really don't want to introduce this in a 2.2 or 2.3 release without having a clean solution, so I'll revert the patch before it become a problem for the backward compatibility of the "set ssl" command. |
What I could do is change a little bit the behavior, we could force the "crt" file to have a ".crt" extension as a requirement for this feature. It's less flexible but it make more sense and it's easy to replace the extension by ".crt" before the lookup |
In order to be compatible with the "set ssl cert" command of the CLI, this patch restrict the ssl-load-extra-del-ext to files with a ".crt" extension in the configuration. Related to issue #785. Should be backported where 8e8581e ("MINOR: ssl: 'ssl-load-extra-del-ext' removes the certificate extension") was backported.
The above commit should fix this, I also added a reg-test which test this with "set ssl cert". |
Unfortunately I won't backport this to 2.2, the CLI still has the bundle code and I don't want to risk to break it, so I'm still reverting this in 2.2. |
The current workflow is also inconsistent with the highly opinionated K8s workflow. kubectl will by default create two keys called "tls.crt" and "tls.key" for a TLS secret.. Not "tls.crt.key", thus not working out of the box. Workaround (bash shell): kubectl create secret tls haproxy --cert=<(cat fullchain.pem privkey.pem) --key=privkey.pem |
For public information, why can't we just have a "key" keyword in the bind statement, explicitly specifying a key file?
I do not know the internal magic how the suffixes are resolved but I do not think the above statement is true or at least how it is incompatible with the suggested syntax below.
This would resolve 2 certificates (fullchain.pem and another.crt) and 2 keys (privkey.pem and another.{key,crt.key} [must exists if another.crt is not a cert bundle]). Another alternative could be to introduce a new keyword (which intuitively seems backwards but it gets the job done)
|
This was fixed in 2.3 with the "ssl-load-extra-del-ext" keyword.
Unfortunately there is no way to know which key is associated with which crt using this syntax, because there is no "declaration order" on a bind line, all options are applied for the whole line. That means if you change the order of the crt and key options it must behave the same way, so that won't work if you have multiple key and crt keywords on the same line.
I think it's kind of confusing but it's an interesting idea, we tend to avoid having multiple arguments to a keyword but we should maybe reconsider something like this. |
Unfortunately this would break a lot of configuration so that is not possible, we will probably proceed this way:
|
Thank you for the answer. In example.com.pem [key example.com.pem.key ocsp example.com.pem.ocsp] In bind *:443,[::]:443 ssl crt-list /etc/haproxy/certs/crt-list strict-sni |
It could work like this indeed, but nothing was pushed in the master repository yet, we'll probably try to have at least these two keywords for 2.6. |
Unfortunately the changes are too deep in the configuration parser, and will require to change a lot of things, it's a bit too late to be integrated into 2.6. |
Is this expected to land in |
I'd like to add here that the ability to specifiy the key as seperate configuration itemg would enable supporting hardware security modules via OpenSSL key engine (which is deprecated) or OpenSSL 3 store API with just a few lines of code. When changing the configuration please bear in mind that the OpenSSL 3 API supports URIs for key locations. So it would be great if URIs would be supported in addition to "plain" file paths. See #71 |
What is the current state about this topic? Its been 3 years and unfortunately I cannot find new / further information about this. I once read it was coming in 2.6, then that it was planned for 2.7. Now 2.8 is on the horizon. An official statement would be great about whether this topic is still on the list, being worked on or not considered. |
We made multiple experiments around this, but it was implying a lot of breakage on the CLI and the configuration. Nothing was merged yet and it's a bit too late for 2.8 since we are freezing the code. This change has big implications in the configuration system even if it seems like a simple configuration option. The topic is still on the list and we are conscious of the current limitation with the old openssl pkcs11 engine (#71) and people that need to modify their deployment scripts to concatenate the PEM. Also, don't forgot that even if you are not able to specify an absolute path, HAProxy is still able to load separated .crt and .key files. |
The "load" keyword in the "crt-store" section allows to define which files you want to load for a specific certificate definition. Ex: crt-store load name "site1" crt "site1.crt" key "site1.key" load crt "site2.crt" key "site2.key" frontend in bind *:443 ssl crt "site1" crt "site2.crt" This is part of the certificate loading which was discussed in #785.
For those interested there was no possible solution with the crt-list system which was going to work, so we stopped trying to make it work with an architecture which was going to be hackish... The "crt-store" section solution was kept, and a first version was pushed in a feature branch https://github.com/haproxy/haproxy/tree/20240208-split-key-cert . See the documentation 26ac95c There are still keywords to be implemented and changes to be done on the CLI, but this will basically looks like this:
|
so "name" is optional or derived how? edit: nm, i saw that documentation was already added to the commit. thanks looking forward to using tihs! |
would this help with being able to use a PKCS#11 URL for the key? |
@hholst80 the name is either defined or it will use the crt parameter like it was done previously. but this could change in the future before it reach the master branch. @tmccombs this will provide more flexibility in the certificate configuration, it will probably need some adaption for the PCKS#11 but I think that will help. |
'crt-store' is a new section useful to define the struct ckch_store. The "load" keyword in the "crt-store" section allows to define which files you want to load for a specific certificate definition. Ex: crt-store load name "site1" crt "site1.crt" key "site1.key" load crt "site2.crt" key "site2.key" frontend in bind *:443 ssl crt "site1" crt "site2.crt" This is part of the certificate loading which was discussed in #785.
I pushed a new version in the https://github.com/haproxy/haproxy/tree/20240227-crtlist-crtstore branch. It introduces the new concept of "inline" crt-list. This allow to declare a crt-list directly in the configuration, it can also uses keyword from the crt-store section. Example:
|
'crt-store' is a new section useful to define the struct ckch_store. The "load" keyword in the "crt-store" section allows to define which files you want to load for a specific certificate definition. Ex: crt-store load crt "site1.crt" key "site1.key" load crt "site2.crt" key "site2.key" frontend in bind *:443 ssl crt "site1.crt" crt "site2.crt" This is part of the certificate loading which was discussed in #785.
Small update about the actual state in 3.0. The documentation about crt-store was updated here: https://github.com/haproxy/haproxy/blob/04a42a92f4cf31adf66a0796309bcb429e28de2f/doc/configuration.txt#L4783
I'm closing this ticket, for HSM features please refer to ticket #71. |
Going to ask this here as I can't figure it out right now (running Having
and the
HAProxy errors out with:
Removing Am I overlooking something here? |
Try this.
|
Hello, as I mentioned in my previous post the crt-list syntax didn't change. The syntax is explained in the section: https://github.com/haproxy/haproxy/blob/04a42a92f4cf31adf66a0796309bcb429e28de2f/doc/configuration.txt#L16026 And there are some example below:
This is a very old syntax that exists since HAProxy 1.5. So basically the keywords must be used between the square brackets, with no exception with the compatible keywords from the crt-list load line.
We didn't change the crt-list syntax for now, but the patch which does it is ready, I'm only waiting for feedbacks about this new system. |
In ticket #785, people are still confused about how to use the crt-store load parameters in a crt-list. This patch adds an example. This must be backported in 3.0
I added an example in the documentation with the |
Yes, that did the trick. Thank you for the clarification! |
In ticket haproxy#785, people are still confused about how to use the crt-store load parameters in a crt-list. This patch adds an example. This must be backported in 3.0 (cherry picked from commit c79c312) Signed-off-by: Amaury Denoyelle <[email protected]>
nothing searches in private
#bind 0.0.0.0:8080 ssl crt "${HAPROXY_CRT_DIR}"/dev.marathon.mesos.crt ssl-load-extra-files key "${HAPROXY_KEY_DIR}"/dev.marathon.mesos.key
#bind 0.0.0.0:8080 ssl crt "${HAPROXY_CRT_DIR}"/dev.marathon.mesos.crt ssl-load-extra-files key
bind 0.0.0.0:8080 ssl crt "${HAPROXY_CRT_DIR}"/dev.marathon.mesos.crt
my key does not end on crt.key, just key (It is obvious from the name in front that it is already the key for that domain)
Why can't I specify like any other application my own crt and key file name? I don't get why you even want to automate this. Also the certs and private directories are different on distributions.
[ALERT] 210/124445 (30) : parsing [/etc/haproxy/haproxy.cfg:52] : 'bind 0.0.0.0:8080' : No Private Key found in '/etc/ssl/certs/dev.marathon.mesos.crt' or '/etc/ssl/certs/dev.marathon.mesos.crt.key'.
Is there an config option to specify a key dir and a cert dir?
The text was updated successfully, but these errors were encountered: