| Message ID | 20210513213311.1870647-1-hzhou@ovn.org |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev,1/2] stream-ssl.c: Fix stream_ssl_set_key_and_cert. | expand |
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 <gmoodalbail@gmail.com> > Reported-at: https://mail.openvswitch.org/pipermail/ovs-discuss/2020-December/050859.html > Signed-off-by: Han Zhou <hzhou@ovn.org> > --- > 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'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/310a47ca-7f78-b5d1-1d3f-7e52ea0f5dd8@nutanix.com/ ? Bets regards, Ilya Maximets.
On Fri, May 14, 2021 at 2:43 AM Ilya Maximets <i.maximets@ovn.org> 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 <gmoodalbail@gmail.com> > > Reported-at: https://mail.openvswitch.org/pipermail/ovs-discuss/2020-December/050859.html > > Signed-off-by: Han Zhou <hzhou@ovn.org> > > --- > > 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/310a47ca-7f78-b5d1-1d3f-7e52ea0f5dd8@nutanix.com/ > ? > 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. Thanks, Han
On 5/15/21 12:13 AM, Han Zhou wrote: > > > On Fri, May 14, 2021 at 2:43 AM Ilya Maximets <i.maximets@ovn.org <mailto:i.maximets@ovn.org>> 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 <gmoodalbail@gmail.com <mailto:gmoodalbail@gmail.com>> >> > Reported-at: 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 <hzhou@ovn.org <mailto:hzhou@ovn.org>> >> > --- >> > 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/310a47ca-7f78-b5d1-1d3f-7e52ea0f5dd8@nutanix.com/ <https://patchwork.ozlabs.org/project/openvswitch/patch/310a47ca-7f78-b5d1-1d3f-7e52ea0f5dd8@nutanix.com/> >> ? >> > 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.
On Mon, May 17, 2021 at 3:01 PM Ilya Maximets <i.maximets@ovn.org> wrote: > > On 5/15/21 12:13 AM, Han Zhou wrote: > > > > > > On Fri, May 14, 2021 at 2:43 AM Ilya Maximets <i.maximets@ovn.org <mailto:i.maximets@ovn.org>> 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 <gmoodalbail@gmail.com <mailto: gmoodalbail@gmail.com>> > >> > Reported-at: 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 <hzhou@ovn.org <mailto:hzhou@ovn.org>> > >> > --- > >> > 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/310a47ca-7f78-b5d1-1d3f-7e52ea0f5dd8@nutanix.com/ < https://patchwork.ozlabs.org/project/openvswitch/patch/310a47ca-7f78-b5d1-1d3f-7e52ea0f5dd8@nutanix.com/ > > >> ? > >> > > 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. Thanks, Han
On 5/18/21 12:23 AM, Han Zhou wrote: > > > On Mon, May 17, 2021 at 3:01 PM Ilya Maximets <i.maximets@ovn.org <mailto:i.maximets@ovn.org>> wrote: >> >> On 5/15/21 12:13 AM, Han Zhou wrote: >> > >> > >> > On Fri, May 14, 2021 at 2:43 AM Ilya Maximets <i.maximets@ovn.org <mailto:i.maximets@ovn.org> <mailto:i.maximets@ovn.org <mailto:i.maximets@ovn.org>>> 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 <gmoodalbail@gmail.com <mailto:gmoodalbail@gmail.com> <mailto:gmoodalbail@gmail.com <mailto:gmoodalbail@gmail.com>>> >> >> > 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 <hzhou@ovn.org <mailto:hzhou@ovn.org> <mailto:hzhou@ovn.org <mailto:hzhou@ovn.org>>> >> >> > --- >> >> > 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/310a47ca-7f78-b5d1-1d3f-7e52ea0f5dd8@nutanix.com/ <https://patchwork.ozlabs.org/project/openvswitch/patch/310a47ca-7f78-b5d1-1d3f-7e52ea0f5dd8@nutanix.com/> <https://patchwork.ozlabs.org/project/openvswitch/patch/310a47ca-7f78-b5d1-1d3f-7e52ea0f5dd8@nutanix.com/ <https://patchwork.ozlabs.org/project/openvswitch/patch/310a47ca-7f78-b5d1-1d3f-7e52ea0f5dd8@nutanix.com/>> >> >> ? >> >> >> > 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.
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); }
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 <gmoodalbail@gmail.com> Reported-at: https://mail.openvswitch.org/pipermail/ovs-discuss/2020-December/050859.html Signed-off-by: Han Zhou <hzhou@ovn.org> --- lib/stream-ssl.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)