diff mbox series

[FFmpeg-devel,4/4] lavf/tls: verify TLS connections by default whenever possible

Message ID 20200530035753.30241-4-rcombs@rcombs.me
State New
Headers show
Series [FFmpeg-devel,1/4] lavf/tls_openssl: add support for verifying the server hostname on >=1.1.0
Related show

Checks

Context Check Description
andriy/default pending
andriy/make success Make finished
andriy/make_fate success Make fate finished

Commit Message

rcombs May 30, 2020, 3:57 a.m. UTC
---
 libavformat/tls.c | 13 +++++++++++++
 libavformat/tls.h |  2 +-
 2 files changed, 14 insertions(+), 1 deletion(-)

Comments

Moritz Barsnick June 3, 2020, 7:32 a.m. UTC | #1
On Fri, May 29, 2020 at 22:57:53 -0500, rcombs wrote:
>      {"cafile",     "Certificate Authority database file", offsetof(pstruct, options_field . ca_file),   AV_OPT_TYPE_STRING, .flags = TLS_OPTFL }, \
> -    {"tls_verify", "Verify the peer certificate",         offsetof(pstruct, options_field . verify),    AV_OPT_TYPE_BOOL, { .i64 = 0 }, 0, 1, .flags = TLS_OPTFL }, \
> +    {"tls_verify", "Verify the peer certificate",         offsetof(pstruct, options_field . verify),    AV_OPT_TYPE_BOOL, { .i64 = -1 }, -1, 1, .flags = TLS_OPTFL }, \
>      {"cert_file",  "Certificate file",                    offsetof(pstruct, options_field . cert_file), AV_OPT_TYPE_STRING, .flags = TLS_OPTFL }, \

Strictly speaking, this is a change in behavior, so I would at least
appreciate a version bump.

The reasoning is that some hosts which used to work will stop doing so,
namely those with self-signed (untrusted) certificates, expired
certificates, and for clients with an outdated CA certificates store.
Yes, this new behavior is desired, but it suddenly "breaks stuff".

Cheers,
Moritz
rcombs June 3, 2020, 7:40 a.m. UTC | #2
> On Jun 3, 2020, at 02:32, Moritz Barsnick <barsnick@gmx.net> wrote:
> 
> On Fri, May 29, 2020 at 22:57:53 -0500, rcombs wrote:
>>     {"cafile",     "Certificate Authority database file", offsetof(pstruct, options_field . ca_file),   AV_OPT_TYPE_STRING, .flags = TLS_OPTFL }, \
>> -    {"tls_verify", "Verify the peer certificate",         offsetof(pstruct, options_field . verify),    AV_OPT_TYPE_BOOL, { .i64 = 0 }, 0, 1, .flags = TLS_OPTFL }, \
>> +    {"tls_verify", "Verify the peer certificate",         offsetof(pstruct, options_field . verify),    AV_OPT_TYPE_BOOL, { .i64 = -1 }, -1, 1, .flags = TLS_OPTFL }, \
>>     {"cert_file",  "Certificate file",                    offsetof(pstruct, options_field . cert_file), AV_OPT_TYPE_STRING, .flags = TLS_OPTFL }, \
> 
> Strictly speaking, this is a change in behavior, so I would at least
> appreciate a version bump.
> 
> The reasoning is that some hosts which used to work will stop doing so,
> namely those with self-signed (untrusted) certificates, expired
> certificates, and for clients with an outdated CA certificates store.
> Yes, this new behavior is desired, but it suddenly "breaks stuff".

Reasonable; what level should it be? I'd guess a minor bump? Though traditionally AVOption changes are micro.

> 
> Cheers,
> Moritz
> _______________________________________________
> ffmpeg-devel mailing list
> ffmpeg-devel@ffmpeg.org
> https://ffmpeg.org/mailman/listinfo/ffmpeg-devel
> 
> To unsubscribe, visit link above, or email
> ffmpeg-devel-request@ffmpeg.org with subject "unsubscribe".
Moritz Barsnick June 3, 2020, 10:32 a.m. UTC | #3
On Wed, Jun 03, 2020 at 02:40:53 -0500, Ridley Combs wrote:
> > Strictly speaking, this is a change in behavior, so I would at least
> > appreciate a version bump.
>
> Reasonable; what level should it be? I'd guess a minor bump? Though
> traditionally AVOption changes are micro.

For behavioral changes, and changes to option handling, I am under the
impression that a micro bump is what is commonly used in libav*, though
https://ffmpeg.org/developer.html#Code
says "binary compatible change". Perhaps others can comment.

Cheers,
Moritz
Anton Khirnov June 5, 2020, 8:50 a.m. UTC | #4
Quoting Moritz Barsnick (2020-06-03 12:32:25)
> On Wed, Jun 03, 2020 at 02:40:53 -0500, Ridley Combs wrote:
> > > Strictly speaking, this is a change in behavior, so I would at least
> > > appreciate a version bump.
> >
> > Reasonable; what level should it be? I'd guess a minor bump? Though
> > traditionally AVOption changes are micro.
> 
> For behavioral changes, and changes to option handling, I am under the
> impression that a micro bump is what is commonly used in libav*, though
> https://ffmpeg.org/developer.html#Code
> says "binary compatible change". Perhaps others can comment.

It's a breaking change, I'd prefer postponing it until a major bump. And
it should be mentioned in APIchanges.
diff mbox series

Patch

diff --git a/libavformat/tls.c b/libavformat/tls.c
index 10e0792e29..3cf24ca056 100644
--- a/libavformat/tls.c
+++ b/libavformat/tls.c
@@ -64,6 +64,19 @@  int ff_tls_open_underlying(TLSShared *c, URLContext *parent, const char *uri, AV
 
     set_options(c, uri);
 
+    if (c->verify < 0) {
+        c->verify = c->listen;
+#if CONFIG_MBEDTLS
+        if (!c->listen && !c->ca_file) {
+            av_log(parent, AV_LOG_WARNING, "ffmpeg was configured with mbedTLS and no root CA store was provided,\n"
+                                           "so this connection will be made insecurely.\n"
+                                           "To make this connection securely, specify a path to a root bundle\n"
+                                           "with the 'ca_file' option.");
+            c->verify = 0;
+        }
+#endif
+    }
+
     if (c->listen)
         snprintf(opts, sizeof(opts), "?listen=1");
 
diff --git a/libavformat/tls.h b/libavformat/tls.h
index 6c2d025f6c..e4854c28da 100644
--- a/libavformat/tls.h
+++ b/libavformat/tls.h
@@ -45,7 +45,7 @@  typedef struct TLSShared {
 #define TLS_COMMON_OPTIONS(pstruct, options_field) \
     {"ca_file",    "Certificate Authority database file", offsetof(pstruct, options_field . ca_file),   AV_OPT_TYPE_STRING, .flags = TLS_OPTFL }, \
     {"cafile",     "Certificate Authority database file", offsetof(pstruct, options_field . ca_file),   AV_OPT_TYPE_STRING, .flags = TLS_OPTFL }, \
-    {"tls_verify", "Verify the peer certificate",         offsetof(pstruct, options_field . verify),    AV_OPT_TYPE_BOOL, { .i64 = 0 }, 0, 1, .flags = TLS_OPTFL }, \
+    {"tls_verify", "Verify the peer certificate",         offsetof(pstruct, options_field . verify),    AV_OPT_TYPE_BOOL, { .i64 = -1 }, -1, 1, .flags = TLS_OPTFL }, \
     {"cert_file",  "Certificate file",                    offsetof(pstruct, options_field . cert_file), AV_OPT_TYPE_STRING, .flags = TLS_OPTFL }, \
     {"key_file",   "Private key file",                    offsetof(pstruct, options_field . key_file),  AV_OPT_TYPE_STRING, .flags = TLS_OPTFL }, \
     {"listen",     "Listen for incoming connections",     offsetof(pstruct, options_field . listen),    AV_OPT_TYPE_BOOL, { .i64 = 0 }, 0, 1, .flags = TLS_OPTFL }, \