Add support for RTSP-over-SSL (RTSPS) #78
Conversation
|
Thank you for the contribution!
I'm happy to give guidance on those things if you can take the time to work through it. Before I get into the details, I'm assuming you've used this only with |
|
Yep, I'd like to get any guidance and improve, just keep in mind this is a side project for me and I'm not sure how much time I'll be able to allocate. |
|
Ok, I've gated the use of RTSPS so that any attempt to use it with UDP will generate an error. I've done it in the Session::setup method as it was the earliest point where both the url and the transport options were present together to avoid having to remember to check it in every client when processing command line arguments. |
There was a problem hiding this comment.
My apologies for just leaving this hanging for such a long time. Got buried in a pile of mail, and I haven't been active on my open source projects to notice it more proactively. I don't mean to ignore things, and feel free to remind me if I do...
I added some comments.
Would it also be possible to add a test?
| @@ -36,6 +36,8 @@ time = "0.1.43" | |||
| tokio = { version = "1.11.0", features = ["macros", "net", "rt", "time"] } | |||
| tokio-util = { version = "0.7.3", features = ["codec"] } | |||
| url = "2.2.1" | |||
| tokio-rustls = "0" | |||
There was a problem hiding this comment.
Can you make the new deps optional? I'd like to keep it minimal when folks don't want tls.
| @@ -94,14 +94,35 @@ fn read_offer() -> Result<RTCSessionDescription, Error> { | |||
|
|
|||
| async fn run() -> Result<(), Error> { | |||
| let opts = Opts::parse(); | |||
|
|
|||
| // Try to get credentials | |||
There was a problem hiding this comment.
- Can we do one logical change per commit?
- Does this work? iirc there's checking later on that explicitly errors out if the URL has credentials. I'd like to be able to just log the URL without worrying about putting sensitive stuff in the logs/tracing output, so we'd need to strip it out before passing the url along.
| }), | ||
| }; | ||
|
|
||
| //AB: This is a hack to work around some devices returning the realm attribute without a terminating double-quote |
There was a problem hiding this comment.
Can we do this in the http-auth crate instead?
| let stream = match (use_tls, &host) { | ||
| //Domain supported in both tls and non-tls case | ||
| (_, Host::Domain(h)) => TcpStream::connect((*h, port)).await, | ||
| //Numeric IP only supported in non-tls case |
There was a problem hiding this comment.
This might not be necessary—I think rustls supports ip addresses now. The caveat is that I'm not sure how validation works. Some callers may just want an option to turn validation completely off anyway, treating TLS more as a thing that some cameras may require rather than a real security measure...
I'm playing with some cameras that only do Secure RTSP (RTSPS) so I tried to adapt retina to support it.
I'm not sure you'll want to merge this in, because frankly:
tokio::Connectionstruct at the price of having to do quite a bit of pattern matching in the implementation ofStreamandSink