Hello William L.

Thank you for your answer.

On Mon, May 11, 2020 at 1:01 PM William Lallemand
<wlallem...@haproxy.com> wrote:
> I'm wondering if we coud have the timing directly in the logs or on the
> CLI, because I'm actually also doing this kind of things, and people
> could probably optimize the loading if they could see the loading time.

I agree it is something which was missing while doing my tests.
Remove/re-add my timer to do my tests each time I was doing some
modifications; to a point where I noted to ask you if you had some
tooling I missed.
So that's totally something I could do in a separate patch! What is
your preferred timing function?

> Your patch is really interesting in my opinion, a lot of people could
> benefits of something like that. Regarding the reload part, I have some
> ideas to pass the ckch structures to the new processes without reloading
> the files one the filesystem, but I didn't implement anything yet.

ok; interesting.

> I'm currently reworking the ssl_sock.c and .h files, and splitting them
> in several files, so you will probably need to rebase your patch on top
> of this. At the moment I separated this in 3 parts: 1 part for the
> crt-list/directories, 1 part for the ckch, and 1 part for the
> SSL_CTX/bind. I'm not happy yet with what I have, but I'll do my best to
> push that as soon as possible in the git repository, I don't want the
> 2.2 to be released in the current state of the files.

good to know, and that's mostly why I avoided too much changes on the
file to avoid having too much rebase work while evolving my
functionality.

> I don't really know the io_uring API in details so I trust you on the
> implementation.

Yes, I've been using it for a few months now, so I'm starting to
better understand how to properly use it. There are indeed a few
different options where you could easily get trapped, and making the
code more complicated.
I however was planning to ask Jens for a quick review when I will have
something more feature complete.

> Regarding the name of the file, ssl_load.c, it is really generic, I
> don't mind having a more specific file only for loading the SSL
> certificates with io_uring.

Yes I totally agree; as it was more of an experimentation I will do a
second path on the naming and improve that. As I also started some
poll changes, it also helps me to better name things to make elements
more reusable.

> I'm wondering if we couldn't push directly the certificate in an X509
> struture instead of storing it in a intermediate buffer, and use the
> ckch instead of a specific cert_iobuf. That just some thoughts, I don't
> know io_uring enough.

I will look into that and it was also on my todo but for now I
considered it ok for a first implementation. It mostly depends if we
are able to properly allocate memory at the right time, while keeping
the code clear enough. For now I considered it as a possible
incremental improvement.
Thanks for the input.

> The crtlist files could also benefits from this, since they can contain
> a lot of certificates too.

Yes, indeed, for now I limited myself to the cases I was concerned
about to avoid too much testing. I will improve that. I should have
warned about it in my commit message.

> I'm seeing that you are using in ssl_load_preparetree() the
> SSL_SOCK_KEYTYPES_NAME array, which is only used for merging a
> certificate bundle. I know that this can be confusing in the code
> because the bundle implementation is crappy. But in a directory we
> don't use any filename extension to load a PEM, we load everything as a
> PEM file with the exception of .key, .ocsp, .sctl and .issuer, which are
> loaded separatly with their own functions. (The cert_exts[] array is
> used for that in ssl_sock.c)

ok, good to recall all files should be taken into account; for now I
blindly implemented my only known case. This will be part of the above
point to take into account all possible options.

> I'm going to remove the support for multi-cert bundle once 2.2 is
> released, so it will simplify a lot of things. I encourage people
> writing new features to not support multi-cert bundles, more
> particularly on the CLI.

So you mean that
example.pem.rsa
example.pem.ecdsa
will be loaded separately as all the other certificates?
I'm not 100% sure what you meant behind removing support of "multi-cert bundle".

> Unfortunately your patch is too late for 2.2, but I think it could be
> great for the next release!

Yes, and it was clear for me since the beginning, I would not have the
time to finish things early enough.

Thanks a lot for your feedbacks. I will try to combine the one from
Willy and yours for a future submission. I also hope we could find a
good middle point between something perfect and something we could
improve along the time.

--
William

Reply via email to