| 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 | expand |
| Context | Check | Description |
|---|---|---|
| andriy/default | pending | |
| andriy/make | success | Make finished |
| andriy/make_fate | success | Make fate finished |
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
> 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".
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
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 --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 }, \