-
Notifications
You must be signed in to change notification settings - Fork 537
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Making fingerprint part of SDP created by createOffer/Answer #427
Conversation
Tested to work with FF. |
I updated a bit. Could someone please review this one (I think there are plenty of things in _crypto_utils.c that are not up to par)? |
@pererikb could you have a look at this? |
@stefanalund, @Rugvip has already promised to review. |
@@ -1158,14 +1158,18 @@ static GstElement *add_dtls_srtp_bin(OwrTransportAgent *transport_agent, guint s | |||
if (!is_encoder) { | |||
g_object_get(session, "dtls-certificate", &cert, NULL); | |||
g_object_get(session, "dtls-key", &key, NULL); | |||
g_print("transport_agent cert = %s\nkey = %s\n", cert, key); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These should be replaced by GST_CAT_DEBUG calls, i.e.
GST_CAT_INFO_OBJECT(_owrsession_debug, session, "transport_agent cert = %s\nkey = %s\n", cert, key);
This PR will be very useful for the project I'm developing - now that it has been reviewed, what kind of timeline are we looking at for getting these changes updated and eventually merged? A few days, a week or two, months... I know nothing is for sure but a rough idea would be really helpful in planning for further development. |
I'll try to address the comments tomorrow, I think (fingers crossed) we should get this one in within a few days. |
@Rugvip I addressed your comments (they were good, thanks) except for the error reporting one which I defered to a later error handing update. PTAL. |
g_value_init(¶ms[2], G_TYPE_STRING); | ||
|
||
if (errorDetected) { | ||
g_print("error detected \n"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general we should avoid g_print in committed code. All of these g_print statements should be replaced by GStreamer debug logging functions, so that we can show and hide them as needed, and provide some context. Especially this print statement will be very confusing unless you know exactly where you should be looking.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Rugvip I can easily change them all, but need some help. Should I use GST_DEBUG_OBJECT? What object should I tie it do? Or should I simply use GST_DEBUG()?
@Rugvip: have now changed the g_print's to GST_DEBUG's. PTAL. |
Just a minor thing, there's no need to mention the file in GST_DEBUG, as it's already a part of the log message, and there's also no need to add a newline. Otherwise, 👍 |
@Rugvip fixed those two items. Ready to merge? |
X509_sign (cert, key_pair, EVP_sha256 ()); | ||
|
||
if (!X509_digest(cert, fprint_type, fprint, &fprint_size)) { | ||
GST_DEBUG("Error, could not create certificate fingerprint"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GST_ERROR()
I was working on merging this but it seems that it does not rebase cleanly. There are conflicts with the code in master. Sorry for the inconvenience, but can you rebase on top of master? Then I will merge it. |
Thanks for working on this. I will rebase. |
…front to be standards compliant (offer/answer from createOffer/Answer contains them). mods. mods mods. Generating private key, cert and fingerpring in owr-crypto_utils works with this version (harcoded). And the offer from createOffer createAnswer can be sent off. Trying to use openssl. more tests. testing continues. more tests. testing testing. testing. testing. testing. test more testing. test. test. test test. test test. test test test test test test test test test test test test test test. test test test test test test test test test test test test test test test test test test test test test test test test test test test test test test test test test test test test test test test test test test test test test test test test test test test test test test test test test test
So, I think (fingers crossed) I managed to rebase. FWIW I have re-run the tests, and things still seem to work. |
It is done. :) |
Wohooo, this is huge! 😄 |
:-) |
I'm currently working on a major new version of Bowser as well that makes it much more reliable (using WKWebView). Combined with this PR we should soon have a Bowser that is significantly better. |
The assert added in add_dtls_srtp_bin() seems to break test-send-receive. |
Indeed. We'll have to fix that. |
First cut. The key and certificate is hard coded. Tested to work with FF (sending the SDPs from createO/A).