On 5/18/21 12:23 AM, Han Zhou wrote: > > > On Mon, May 17, 2021 at 3:01 PM Ilya Maximets <[email protected] > <mailto:[email protected]>> wrote: >> >> On 5/15/21 12:13 AM, Han Zhou wrote: >> > >> > >> > On Fri, May 14, 2021 at 2:43 AM Ilya Maximets <[email protected] >> > <mailto:[email protected]> <mailto:[email protected] >> > <mailto:[email protected]>>> wrote: >> >> >> >> On 5/13/21 11:33 PM, Han Zhou wrote: >> >> > From the description of this interface, one of the problems it tries to >> >> > solve is when one of the files is changed before the other: >> >> > >> >> > * But, if the private >> >> > * key is changed before the certificate (e.g. someone "scp"s or "mv"s >> >> > the new >> >> > * private key in place before the certificate), then OpenSSL would >> >> > reject that >> >> > * change, and then the change of certificate would succeed, but there >> >> > would be >> >> > * no associated private key (because it had only changed once and >> >> > therefore >> >> > * there was no point in re-reading it). >> >> > >> >> > * This function avoids both problems by, whenever either the >> >> > certificate or >> >> > * the private key file changes, re-reading both of them ... >> >> > >> >> > However, in the implement it used "&&" instead of "||", and so it was >> >> > in fact re-reading both of them only when both are changed. This patch >> >> > fixes it by using "||". >> >> > >> >> > Reported-by: Girish Moodalbail <[email protected] >> >> > <mailto:[email protected]> <mailto:[email protected] >> >> > <mailto:[email protected]>>> >> >> > Reported-at: >> >> > https://mail.openvswitch.org/pipermail/ovs-discuss/2020-December/050859.html >> >> > >> >> > <https://mail.openvswitch.org/pipermail/ovs-discuss/2020-December/050859.html> >> >> > >> >> > <https://mail.openvswitch.org/pipermail/ovs-discuss/2020-December/050859.html >> >> > >> >> > <https://mail.openvswitch.org/pipermail/ovs-discuss/2020-December/050859.html>> >> >> > Signed-off-by: Han Zhou <[email protected] <mailto:[email protected]> >> >> > <mailto:[email protected] <mailto:[email protected]>>> >> >> > --- >> >> > lib/stream-ssl.c | 2 +- >> >> > 1 file changed, 1 insertion(+), 1 deletion(-) >> >> > >> >> > diff --git a/lib/stream-ssl.c b/lib/stream-ssl.c >> >> > index 078fcbc3a..e67ccb4bd 100644 >> >> > --- a/lib/stream-ssl.c >> >> > +++ b/lib/stream-ssl.c >> >> > @@ -1215,7 +1215,7 @@ stream_ssl_set_key_and_cert(const char >> >> > *private_key_file, >> >> > const char *certificate_file) >> >> > { >> >> > if (update_ssl_config(&private_key, private_key_file) >> >> > - && update_ssl_config(&certificate, certificate_file)) { >> >> > + || update_ssl_config(&certificate, certificate_file)) { >> >> > stream_ssl_set_certificate_file__(certificate_file); >> >> > stream_ssl_set_private_key_file__(private_key_file); >> >> > } >> >> > >> >> >> >> Hi, Han. Thanks for working on this. >> >> >> >> This change might fix the issue, but I'm not sure that updating only >> >> one of the components makes much sense. I mean, certificate and private >> >> key should match, otherwise setup will be broken while the second >> >> component is not updated, IIUC. >> >> >> > I agree that it doesn't make sense to set mismatched cert & key. However, >> > I think it is not this interface's responsibility. This patch only >> > intended to fix the implementation to support what the description has >> > mentioned. What's in my mind was, the user who calls the interface (or the >> > end user) is responsible for providing matched cert & key files to make >> > sure the SSL works, but if they failed to do so at some circumstances, >> > they should still be able to correct the problem (by correcting the wrong >> > file) and then call this interface to fix it. The original implementation >> > doesn't work in this situation when user tries to correct the problem. >> > With this patch it will work when the mismatch files are corrected. >> > >> >> I'm not sure, but maybe you're looking for solution for the same problem >> >> as this patch tries to address: >> >> >> >> https://patchwork.ozlabs.org/project/openvswitch/patch/[email protected]/ >> >> >> >> <https://patchwork.ozlabs.org/project/openvswitch/patch/[email protected]/> >> >> >> >> <https://patchwork.ozlabs.org/project/openvswitch/patch/[email protected]/ >> >> >> >> <https://patchwork.ozlabs.org/project/openvswitch/patch/[email protected]/>> >> >> ? >> >> >> > Thanks for sharing. I didn't notice this before, but it looks indeed a >> > better solution for the above mentioned problem. However, there seems to >> > be a new problem in a corner case: if a user placed mismatched cert & key >> > files when the first time this interface is called, and later on fixes the >> > mismatch by correcting the wrong file (either cert or key, but not both), >> > then calling the interface again would not fix the problem because the >> > function would just return. It would work only if both files are changed >> > (e.g. generating a new pair, or if the user reads the code and gets the >> > tricks to pretend that both files are changed). I am not sure if this >> > corner case is of importance. Otherwise that patch looks better and I am >> > ok to drop my patch. >> >> Ugh. This corner case doesn't sound good. >> >> Unfortunately, I don't see a good way to fix that except >> for creation of a new SSL_CTX instance, setting a new >> certificate and checking if the key matches installed >> certificate with SSL_CTX_check_private_key(). >> >> I guess, full solution should look like a mix of patches >> plus additional check (just an illustration): >> >> if (update_ssl_config(cert) || update_ssl_config(key)) { >> tmp_ctx = SSL_CTX_new(); >> if (SSL_CTX_use_certificate_file(tmp_ctx, cert) >> && SSL_CTX_use_PrivateKey_file(tmp_ctx, key) >> && SSL_CTX_check_private_key(ctx)) { >> >> stream_ssl_set_certificate_file__(certificate_file); >> stream_ssl_set_private_key_file__(private_key_file); >> } else { >> /* Log some meaningfull rate-limited error. */ >> /* Rollback all changes made by update_ssl_config() */ >> } >> SSL_CTX_free(tmp_ctx); >> } >> >> >> What do you think? >> >> Best regards, Ilya Maximets. > > Looks good to me! This approach does solve both problems, and it is still > simple enough. Would you propose a formal patch, or do you want me or Thomas > to submit a new version? I am ok either way.
It would be great if one of you could prepare a new version with this kind of changes. Best regards, Ilya Maximets. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
