Skip to content
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

TLS options #7

Open
MathiasKoch opened this issue Mar 31, 2020 · 13 comments
Open

TLS options #7

MathiasKoch opened this issue Mar 31, 2020 · 13 comments

Comments

@MathiasKoch
Copy link
Collaborator

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.

@thejpster
Copy link
Member

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.

@MathiasKoch
Copy link
Collaborator Author

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

  • Client certificate (x509) and private key
  • ALPN settings

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)

@thejpster
Copy link
Member

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?

@MathiasKoch
Copy link
Collaborator Author

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?

@thejpster
Copy link
Member

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]),
}

@MathiasKoch
Copy link
Collaborator Author

MathiasKoch commented Apr 11, 2020

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

  • Certificate
  • Identity
  • TlsConnector
  • TlsConnectorBuilder

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?)

@thejpster
Copy link
Member

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.

@MathiasKoch
Copy link
Collaborator Author

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

@MathiasKoch
Copy link
Collaborator Author

MathiasKoch commented Apr 15, 2020

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 rust-lang/rfcs#2262 and rust-lang-nursery/portability-wg#12 (comment)

core_io ref: https://github.com/jethrogb/rust-core_io

@thejpster
Copy link
Member

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.

@ryan-summers
Copy link
Member

ryan-summers commented Nov 14, 2022

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:

  1. Take a known, unchanging type (i.e. there is a single impl for a driver)
  2. Change their behavior based on whether or not the stack supports TLS

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

@MathiasKoch
Copy link
Collaborator Author

MathiasKoch commented Nov 17, 2022

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.

@ryan-summers
Copy link
Member

ryan-summers commented Nov 17, 2022

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants