Open MathiasKoch opened 4 years ago
Hmm, TLS support is an interesting idea. I'm not sure if it should be baked in, or kept as part of the relevant crypto library. Certainly there's an argument for a generic stream, which can be implemented by a some_crate::TcpSocket, other_crate::TlsSocket and embedded_hal::Serial.
I don't really mean TLS support baked in as such. I haven't given it much thought, but to give a bit of context it originated from a desire to have my MQTT Client only rely on traits from embedded-nal, while still being able to take options like
Possibly even CA's
I think this makes sense, as these certificate pairs are very tightly coupled to the socket. I wouldn't expect embedded-nal to have any notion of how the TLS is set up, as that could be using any crypto library, offloading to some other chip (eg. through AT), or any other way.
A crate implementing embedded-nal traits could then implement TcpStack
& UdpStack
default, and put implementations of Ssl
behind a feature flag, if they pull in external dependencies (eg. rustls)
Ah, OK. That makes sense. I guess we should look at things like https://docs.rs/native-tls to see what sort of crypto APIs are common across different libraries. I imagine we probably want to take keys and certs as &str
in ASCII PEM format, rather than defining a specific X.509 set of types?
I guess we should look at things like https://docs.rs/native-tls to see what sort of crypto APIs are common across different libraries
Yup, that makes sense i think! I can see if i can find time to give a proposal some time during next week.
I imagine we probably want to take keys and certs as &str in ASCII PEM format, rather than defining a specific X.509 set of types?
That sounds fine by me, though we should probably consider &[u8]
instead of &str
, to support binary DER format as well as PEM?
If someone had read a bunch of bytes off the wire, it would be a shame to force them to verify them for UTF-8 correctness before handing them over, as any valid PEM is plain 7-bit ASCII anyway. So maybe we should take &[u8] for both PEM and DIR. Or have some enum:
enum Certificate<'a> {
Pem(&'a [u8]),
Der(&'a [u8]),
}
So maybe we should take &[u8] for both PEM and DER
I think taking &[u8] makes great sense for both yes. And your enum probably makes sense as well, as i suspect a lot of tls frameworks needs to know what format the data is coming in.
Also we need to remember an option to add a password for certificates.
One proposal could be to simply copy the relevant parts of native-tls's api? I think its the exact same thing we want here? Basically we want the stuff provided by
They might not need to look the same (The certificate could very well be the enum described above)
Should we distinguish between a TcpSocket
and its Tls enabled part? (Usually distinguished by TcpStream
and TlsStream
?)
Also we need to remember an option to add a password for ~certificates~.
Private Keys. Certificates are the public key half :)
Should we distinguish between a TcpSocket and its Tls enabled part?
They should both implement some generic Stream trait. It's a source of frustration that std::io isn't core::io, which means there is no real standard for implementing a stream on no_std
Rust. But my HTTP library shouldn't care if it has been given a TCP socket, a TLS socket, or a UART. All are octet-oriented, bi-directional, full-duplex, non-seekable pipes.
Private Keys. Certificates are the public key half :)
Ahh yeah, of course. Guess it went a bit too fast.
They should both implement some generic Stream trait. It's a source of frustration that std::io isn't core::io, which means there is no real standard for implementing a stream on no_std Rust. But my HTTP library shouldn't care if it has been given a TCP socket, a TLS socket, or a UART. All are octet-oriented, bi-directional, full-duplex, non-seekable pipes.
That's a good point! Regarding the std::io vs core::io, i guess that's something we will have to live with.. AFAIK there is no effort, nor plans about making it core::io, is there? Other than the core_io 3rd party crate
Would it make sense to create Stream
and TlsStream<Stream>
, implementing core_io::{Read, Write}
, allowing for somewhat easy swapping with core::io::{Read, Write}
, when they finally work out https://github.com/rust-lang/rfcs/issues/2262 and https://github.com/rust-lang-nursery/portability-wg/issues/12#issuecomment-382072568
core_io ref: https://github.com/jethrogb/rust-core_io
I'm wary of relying on anything nightly-only, so if there is an impl for core_io
, it should be behind a feature gate.
Now that https://github.com/drogue-iot/embedded-tls exists, I'm actually quite interested in leveraging TLS for devices running MQTT, and it seems to me like different traits are required for this.
For an MQTT library taking in a TcpClientStack
, there's no way for the library to know if the stack is TLS-enabled or not. However, the library needs to do different things for TLS (e.g. provide user/pass authentication) that it cannot do when TLS is not enabled (as sending user/pass without TLS would compromise the credentials). While we could rely on the user providing a TLS-enabled TcpClientStack
, this seems like an excellent foot-gun for users to expose themselves to cybersecurity concerns.
@MathiasKoch I saw that there was some work on this on your the fork of embedded-nal
. Has there been any progress as of yet on an implementation here that I could use as a reference?
After reviewing embedded-tls and the TLS handshake, I don't think any of the specifics of TLS belong in the embedded-nal. I think we just need an extension trait to TcpClientStack
for a TlsClientStack
. The actual X.509 cerficates and such can be provided to the actual stack via constructors for the implementor. I don't see any need to expose those APIs via the NAL.
An alternative approach is to leave the current traits as-is and allow crate authors to implement a full TcpClientStack
using TLS. Since I imagine the API of TcpClientStack
and TlsClientStack
would be identical, this essentially offloads the responsibility of checking TLS configuration to the user as opposed to using typestate to manage this.
I'm not convinced which approach would be better, as having a separate trait might cause library authors stress in figuring out how to implement their driver given either a TcpClientStack
or TlsClientStack
.
Perhaps the best approach is to instead add a new associated constant to TcpClientStack
indicating if TLS is enabled or not. This would then allow drivers to:
embedded-nal/tcp.rs
:
pub trait TcpClientStack {
const TLS_ENABLED: bool;
// ...
}
driver/lib.rs
:
struct DriverTakingNal<T: TcpClientStack> {
stack: T,
}
impl<T: TcpClientStack> DriverTakingNal<T> {
pub fn connect(&mut self, remote: IpAddress) {
let socket = self.stack.socket().unwrap();
if T::TLS_ENABLED {
socket.connect((remote, 8883)).unwrap();
} else {
socket.connect((remote, 1883)).unwrap();
}
}
}
Edit Edit: It seems to me like @MathiasKoch is pointing out that we may want to have specific TLS data associated with a socket as opposed to the entire network stack. Perhaps this could be done via a new API requirement on TcpSocket
that allows for custom-assigning certificates and/or TLS options? I'm not entirely convinced that TLS options should be unique on a per-socket basis yet, but I haven't dove into this yet
I think they absolutely need to be specific on a per-socket basis to be even remotely useful. Think about a super simple setup, where my IoT device needs to connect securely to MQTT (x.509 & TLS on port 8883 with hostname checking enabled), and at the same time do a simple HTTP request on an endpoint.
This would not be possible if the TLS settings are not per socket, as the HTTP request would end up with a TLS handshake with an x.509 & hostname checking that would fail every single time.
This is not even close to an advanced, far-fetched example, but merely a very common one from most IoT devices out there.
To your question on progress: None, as we ended up running with our own extensions to solve this at work. I would still like to see a proper abstraction, and I personally think the native-tls
way of doing it would fit super nicely in embedded as well, even though I agree it would add a lot of data structures to embedded-tls
. Meanwhile, I don't see that as an issue, if they are indeed generic for all implementations.
Just my 2 cents.
EDIT:
With the above said, I think it would make even more sense to rework the blocking traits into the same socket-driven API that the async ones are using, where embedded-io
is the abstraction of read/write. That would probably make it much easier to make a proper TLS abstraction as well, due to the factory like approach of the stack.
I think they absolutely need to be specific on a per-socket basis to be even remotely useful. Think about a super simple setup, where my IoT device needs to connect securely to MQTT (x.509 & TLS on port 8883 with hostname checking enabled), and at the same time do a simple HTTP request on an endpoint.
This would not be possible if the TLS settings are not per socket, as the HTTP request would end up with a TLS handshake with an x.509 & hostname checking that would fail every single time.
This is not even close to an advanced, far-fetched example, but merely a very common one from most IoT devices out there.
This is a good point. I failed to realize that if you had two sockets, one that needed TLS and one that didn't use TLS, the one using TLS may try to establish the TLS handshake when no TLS server is running, causing the process to fail. This is a very valid concern.
However, if you were able to differentiate between TLS-enabled and TLS-absent sockets, would two individual TLS-enabled sockets need different TLS parameters, or would it be feasible to associate all certificates/CAs with all TLS-enabled sockets? From what I understand, something like Chrome or Firefox would associate certificates/CAs with all TLS sockets and wouldn't have unique certificates on a per-connection basis.
Would it be feasible for us to go this approach, i.e. have a method to open a TLS socket and one to open a normal socket, then have TLS configs associated with all TLS sockets?
I ask about this approach because it seems to me like we could quite easily leverage something like embedded-tls
to make a generic TLS-enabled stack on top of any TcpClientStack
without too much effort, which would make TLS much more widely available to users without imposing high complexity on this library.
Would it make sense to add a third trait to allow "upgrading" a TcpSocket to a TlsSocket somehow? Not sure on the format, but i guess it should add a way of setting certificates etc.