[WIP] RFC: Pluggable transport security #1924
Conversation
There was a problem hiding this comment.
Thanks for putting this together. Equally early thoughts from me below. Please feel encouraged to ping on slack/hangouts on the issue of who does the read - my thoughts on this are based on a vision of how the linked issue would be implemented and may not be very well explained.
|
|
||
| virtual Buffer::Instance& readBuffer() PURE; | ||
|
|
||
| virtual Buffer::Instance& writeBuffer() PURE; |
There was a problem hiding this comment.
These should all definitely be commented up - I assume you were just waiting until the APIs were hashed out?
There was a problem hiding this comment.
Yes, this is still WIP and of course I'll comment up once we agreed on API.
| public: | ||
| virtual ~TransportSecurity() {} | ||
| virtual std::string nextProtocol() const PURE; | ||
| virtual Connection::IoResult doReadFromSocket() PURE; |
There was a problem hiding this comment.
doReadAndDecrypt?
Reading from the socket is something I feel the connection should do. Reading and transforming is more of a crypto operation
There was a problem hiding this comment.
Also I'm really not thrilled about removing the read from the connectionImpl and having the transport-security do it using the raw fd. It makes it conceptually much harder to do something like #1630 where you may have a stream reading from another stream rather than from a socket. If the connection is supposed to represent the underlying byte stream it makes more conceptual sense to me to have the connection do the read and then pass it to the crypto layer which does decryption. That unfortunately doesn't play at all well with how we're currently doing SSL_reads. Is there a way to keep the logical "get data" separate from the decryption operation (without incurring extra copies)?
There was a problem hiding this comment.
I totally agree that not let transport-security do it using raw fd, and I would love to see that read/write from socket and encrypt/decrypt are decoupbled. But current SSL_do_handshake and SSL_read/write isn't in that way. I did this prototype is based on current implementation to pull out the interface.
I don't understand the part that #1630 will be harder, if you read from stream then I assume you'll use Http::AsyncClient, no?
@PiotrSikora do you have any idea?
There was a problem hiding this comment.
As written, I don't think this has anything to do with transport security. It's basically a pluggable connection handling module. This is potentially useful for people that want to port Envoy to proprietary environments. (This is basically my comment from the API PR).
If we have to go this direction, what do we think about just calling it what it is? Something like LowLevelSocket or whatever. Then we can have PlainTextLowLevelSocket, BoringSslLowLevelSocket, etc.?
There was a problem hiding this comment.
re: BoringSSL without fd, looking at gRPC TSI impl, it is doable with two BIO_s_mem, like what gRPC does here,
then interface will be just shuffling buffers with methods:
- doHandshake
- encryptBuffer
- decryptBuffer
This needs more work on current BoringSSL implementation, in that way the interface is pure transport security. WDYT?
There was a problem hiding this comment.
Matt's comments are spot on. I don't like "security interface" reading from a socket but if we rebrand it as socket helper or some such my objections go away :-)
If we go that route I think I'd have some other renaming in mind. For example, I'd suggest renaming nextProtocol() at it implies NPN, but a simple protocol() would be more neutral (and also useful for the proxying-random-things-over-H2 use case I have in mind :-) )
There was a problem hiding this comment.
Yeah I agree that current shape is more like low level socket. I'll try a bit to narrow this down to a real "security interface" if it is easy. If not I would rename this and config to LowLevelSocket per suggested.
There was a problem hiding this comment.
@lizan FWIW, I think we want the low level socket change no matter what. We have two new connection implementations coming (Windows and fd.io/DPDK/VPP) which I think could make good use of this. Not sure if this sways your opinion or not.
There was a problem hiding this comment.
@mattklein123 After digging with BoringSSL a while, and looked winsock, etc, I reached same conclusion here with LowLevelSocket. So yes, I will update this with renaming / comments etc.
There was a problem hiding this comment.
I agree with @mattklein123 here. We've got some exciting TCP acceleration coming down the pike from fd.io/VPP, and a pluggable LowLevelSocket would greatly ease that process :)
|
Closing this, will re-work on |
This is an almost complete rewrite of the Cronvoy main logic (CronetUrlRequest and CronetUploadDataStream). More cleanup will be done with next PRs, and much more documentation will also be added - the logic is quite subtile. Description: Reimplement Cronvoy to take advantage of the Explicit Flow Control Risk Level: None: only Cronvoy is changed Testing: CI - original tests are passing, and also all related @ignore Docs Changes: N/A Release Notes: N/A Signed-off-by: Charles Le Borgne [email protected] Signed-off-by: Charles Le Borgne <[email protected]> Signed-off-by: JP Simard <[email protected]>
This is an almost complete rewrite of the Cronvoy main logic (CronetUrlRequest and CronetUploadDataStream). More cleanup will be done with next PRs, and much more documentation will also be added - the logic is quite subtile. Description: Reimplement Cronvoy to take advantage of the Explicit Flow Control Risk Level: None: only Cronvoy is changed Testing: CI - original tests are passing, and also all related @ignore Docs Changes: N/A Release Notes: N/A Signed-off-by: Charles Le Borgne [email protected] Signed-off-by: Charles Le Borgne <[email protected]> Signed-off-by: JP Simard <[email protected]>
Early prototype of the interface for transport security and registry. Context for envoyproxy/data-plane-api#189
@mattklein123 @htuch @alyssawilk