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]>> 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]>>
>> > 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 <[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]/>
>> ?
>>
> 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.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
