Created attachment 269154 [details] [review]
tls certificate database support

This adds support for additional server certificate verification given a user certificate database file.

Created attachment 269155 [details] [review]
tls certificate database support

Forgot to show the error flags in latest patch.

Created attachment 269157 [details] [review]
tls certificate database support

Forgot to show the error flags in latest patch.

Created attachment 269158 [details] [review]
tls certificate database support 2nd fixup

Sorry, forgot to free tls_database_file and bugzilla is not helping.

Review of attachment 269158 [details] [review]:

Seems like a good idea, just some questions.

::: gst-libs/gst/rtsp/gstrtspconnection.c
@@ +230,3 @@
+
+    if (error)
+      goto verify_error;

Shouldn't this use the tls-validation-flags property here to ignore some errors but not others, like what happens in rtspsrc?

@@ +580,3 @@
+ * Sets the anchor certificate authorities database file. This
+ * certificate database will be used after a server certificate can't be
+ * verified with the default certificate database.

Does this require a single file with all certificates or can it also be a directory like /etc/ssl/certs ?

@@ +2251,3 @@
  *
  * Close and free @conn.
+ *

Please get rid of all these useless whitespace changes :)

(In reply to comment #5)
> Review of attachment 269158 [details] [review]:
> 
> Seems like a good idea, just some questions.
> 
> ::: gst-libs/gst/rtsp/gstrtspconnection.c
> @@ +230,3 @@
> +
> +    if (error)
> +      goto verify_error;
> 
> Shouldn't this use the tls-validation-flags property here to ignore some errors
> but not others, like what happens in rtspsrc?
> 

error is just a GError, but it's true that we should take into account the tls-validation-flags in here:

    accept = ((errors & rtspcon->tls_validation_flags) == 0);

i missed that, thanks!

> @@ +580,3 @@
> + * Sets the anchor certificate authorities database file. This
> + * certificate database will be used after a server certificate can't be
> + * verified with the default certificate database.
> 
> Does this require a single file with all certificates or can it also be a
> directory like /etc/ssl/certs ?
> 

Yes, it requires a single file with PEM encoded certificates.

https://developer.gnome.org/gio/2.32/GTlsFileDatabase.html#g-tls-file-database-new

Like the default database: /etc/ssl/certs/ca-certificates.crt

> @@ +2251,3 @@
>   *
>   * Close and free @conn.
> + *
> 
> Please get rid of all these useless whitespace changes :)

i have a deja vu here :-). we should get rid of those white spaces at some point.

Created attachment 269350 [details] [review]
tls certificate database support 3rd fixup

let's see now. added tls_validation_flags check and Since notes.

(In reply to comment #6)

> > Please get rid of all these useless whitespace changes :)
> 
> i have a deja vu here :-). we should get rid of those white spaces at some
> point.

Yes, please attach a second patch just doing that here :)

Created attachment 269358 [details] [review]
get rid of rtspconnectoin whitespaces

get rid of whitespaces.

Any reason to use the filename instead of a GTlsDatabase object in the API ?

(In reply to comment #10)
> Any reason to use the filename instead of a GTlsDatabase object in the API ?

Not really. I didn't think about it. :-) Good point. And this gives more flexibility to rtspconnection.

Created attachment 269477 [details] [review]
tls certificate database support 4th fixup

It now uses a GTlsDatabase directly instead of a file name.

Created attachment 269478 [details] [review]
get rid of rtspconnection superfluous whitespaces

Created attachment 269479 [details] [review]
tls certificate database support 5th fixup

I hate typos.

This could be used as a worked around to bug 724708.

Comment on attachment 269478 [details] [review]
get rid of rtspconnection superfluous whitespaces

commit 9121b16aa097b0de12368da336085d1ebccd5b7d
Author: Aleix Conchillo Flaqué <aleix@oblong.com>
Date:   Sun Feb 16 23:55:17 2014 -0800

    rtspconnection: get rid of superfluous whitespaces

Review of attachment 269479 [details] [review]:

::: gst-libs/gst/rtsp/gstrtspconnection.c
@@ +223,3 @@
+
+    if (error)
+      goto verify_error;

Because of this you will never validate it against the validation_flags below.

(In reply to comment #17)
> Review of attachment 269479 [details] [review]:
> 
> ::: gst-libs/gst/rtsp/gstrtspconnection.c
> @@ +223,3 @@
> +
> +    if (error)
> +      goto verify_error;
> 
> Because of this you will never validate it against the validation_flags below.

But you don't get a GError "error" if it can't verify, you just get "errors". May be the variable names are confusing.

I've tried it an it works as expected.

Comment on attachment 269479 [details] [review]
tls certificate database support 5th fixup

Oops, yes I missed the 's' :) Sorry

commit 0a115bd31f77a6b4b38127937c562f4145f8ccfc
Author: Aleix Conchillo Flaqué <aleix@oblong.com>
Date:   Sun Feb 16 17:39:35 2014 -0800

    rtspconnection: allow specifying a certificate database
    
    Two new functions have been added,
    gst_rtsp_connection_set_tls_database() and
    gst_rtsp_connection_get_tls_database(). The certificate database will be
    used when a certificate can't be verified with the default database.
    
    https://bugzilla.gnome.org/show_bug.cgi?id=724393

Created attachment 269740 [details] [review]
update windoes symbols file

It seems we also need this to make "make dist" happy.