Allow intercepting and injecting DTLS packets#766
Conversation
d60f060 to
a743812
Compare
16f2df2 to
787aa43
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #766 +/- ##
==========================================
- Coverage 82.66% 82.62% -0.05%
==========================================
Files 121 121
Lines 6853 6917 +64
==========================================
+ Hits 5665 5715 +50
- Misses 777 788 +11
- Partials 411 414 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
ad5d88c to
9a2f30a
Compare
| if hasHandshake { | ||
| if c.inboundHandshakePacketNotifier != nil { | ||
| // Would it be useful to know this was injected? | ||
| // Should this work on a copy? |
There was a problem hiding this comment.
I would perfer that we do not implement such logic here, thus the users of this functionality should handle it themselves. I think it's reasonable to assume that it's the same library that would both use inject and the notifier.
|
Sorry if I am being dense @fippo, but how is the experience different then a user getting DTLS traffic via existing APIs? |
| } | ||
| rawPackets = append(rawPackets, rawHandshakePackets...) | ||
| } else { | ||
| rawPacket, err := c.processPacket(pkt) |
There was a problem hiding this comment.
Non-handshake packets are still processed in the writeHandshakePackets. I assume it is only supposed to be done in the writePackets function after this refactor.
There was a problem hiding this comment.
if you mean change cipher spec which doesn't have a handshake content type then yes, it is handled here since they need to go into the same network packet as part of a handshake flight (terminology... this one isn't great)
config.go
Outdated
| // OutboundHandshakePacketInterceptor is an optional callback that can be set to | ||
| // intercept outgoing raw handshake packets. It is called with the raw packet bytes. | ||
| // The interceptor can decide to drop the packet by returning true. | ||
| OutboundHandshakePacketInterceptor func(packet []byte) bool |
There was a problem hiding this comment.
I suggest we could make the interceptors more general and allow for modification of handshake messages, similar to the message hooks above (I already know about some usecases, for example anti-fingerprinting).
OutboundHandshakePacketInterceptor func(packet []byte) []byte and InboundHandshakePacketInterceptor func(packet []byte) []byte. Rather than a bool deciding if we drop a packet, we can drop a packet if the returned byte buffer is empty.
There was a problem hiding this comment.
wouldn't such a use-case implement its own flight generator which allows way more flexibility?
conn_test.go
Outdated
| server, err := Server(dtlsnet.PacketConnFromConn(cb), cb.RemoteAddr(), &Config{ | ||
| Certificates: []tls.Certificate{serverCert}, | ||
| OutboundHandshakePacketInterceptor: func(packet []byte) bool { | ||
| client.InjectPacket(packet, ca.RemoteAddr()) |
There was a problem hiding this comment.
Could you make a seprate test for injected packets? Now the interceptor and the injection functionallity are tested closely together, I would suggest seperating them. Would there be any way of testing the interceptors without using the injection functionallity?
There was a problem hiding this comment.
Testing them in complete isolation is only going to result in weaker assertions like "number of handshake packets seen" whereas TestInboundNotifier asserts that what one connection is intercepting (or not) as part of the handshake is seen by the other connection.
@Sean-Der the exisiting API only allows reading and writing application data, not handshake data like what is needed here. The message hooks offers some of the functionality (intercepting ClientHello, ServerHello and CertificateRequest messsages), but not all handshake messages. Correct me if I'm wrong @fippo. Unless there is some other way of getting traffic than the Conn API. |
|
@theodorsm aye, thanks for explanation :) You should have full access to the traffic if you pass your own Then if you care about specific messages you can parse things via pkg/protocol I worry about offering APIs that don't match crypto/tls because honestly they are just way better designers than anything I have done :) |
|
@Sean-Der ahhh I overlooked this API, thanks, that's much cleaner! Does this solve your usecase @fippo (if so, we should close this PR)? You can take inspiration from pkg/net/net.go. |
I was aware of that but that API operates on []byte which means parsing again and since it does not know the handshake is done (easily), will have to do so for every packet. |
|
@fippo, with the approach in this PR we are still working with []byte beyond knowing it's a handshake message, further parsing must happen anyways. To filter out the handshake messages from the raw bytes from |
2c5c975 to
da78a87
Compare
|
@fippo I agree with Sean about not wanting to offer the InjectPacket API on Conn. However, I think imlementing a custom net.PacketConn that exposes a InjectHandshakePacket would be cleaner, we could offer this in our net package. This custom PacketConn could also support interceptors and notifications. I see crypto/tls has a API for exposing the underlying net.Conn , we could also offer this that would allow for the flexibility needed. |
|
@fippo another sticking point is that Justin is planning on using boringssl. So the more logic you can put into Sorry to burn your time :( I just think in the long run it will work better for everyone. I am hesitant to make this library complicated because non-WebRTC users and it gets so much scrutiny. Doing funky stuff in pion/webrtc ends up being easier. Tell me if I am completely off base though! I purposefully exported all the Handshake parsing code to make stuff easy. |
|
BoringSSL needs the same interface changes - a way to get notified about incoming packets and a way to inject a packet (which is a lot simpler there) but usually wouldn't concern itself with parsing of the bytes. |
|
new requirement which makes it even less of a fit for net.Conn: the "subscriber" needs to know when a flight is starting or complete and needs to be "flushed" (which is required to invalidate a flight in case of timeout + resend) - OpenSSL does flush but I slightly prefer a signal for start-of-flight but for compat reasons ... |
b2eb45d to
b50dd93
Compare
which allows embedding them into STUN.
|
(updated for options which wasn't too bad) |
which allows embedding them into STUN.