Share parsed mbedtls_x509_crt and mbedtls_pk_context between sessions
#75
Conversation
| fn default() -> Self { | ||
| Self::new() | ||
| } | ||
| } | ||
|
|
||
| impl Certificates<'_> { | ||
| impl Certificates { | ||
| /// Create a new instance of [Certificates] with no certificates whatsoever | ||
| pub const fn new() -> Self { |
There was a problem hiding this comment.
Is this a real use-case? Can the library actually operate without any certs?
There was a problem hiding this comment.
I added it to deal with the builder pattern. I'm still not sure if it's the best way, considering if we have a copy and no copy variant, with different lifetimes, I don't know how can we reconcile both functionalities under the same builder.
There was a problem hiding this comment.
The lifetimes are covariant though so a single lifetime would do. If you want to keep the builder pattern, new would return a Certificates<'static> which would either stay like that (copy) or would be changed to Certificates<'a> by a builder non-copy method, where 'a would be a generic lifetime capturing the actual x509 lifetime (which can still end up being static after all).
esp-mbedtls/src/lib.rs
Outdated
| @@ -265,7 +386,7 @@ pub struct Certificates<'a> { | |||
| /// that will be used to verify the client's certificate during the handshake. | |||
| /// When set to [None] the server will not request nor perform any verification | |||
| /// on the client certificates. Only set when you want to use client authentication. | |||
There was a problem hiding this comment.
"only set this when you want to use client authentication".
Hmm, is this correct? My understanding is that if you are running a TLS client (say - an HTTPS client), you might still need a bunch of CA certificates so as to verify the certificate that the server is providing to you? I don't think the server is obliged at all to give you the CA cert, it only gives you its own cert signed by the CA cert but then the CA cert (in fact - a whole bundle of these) needs to be configured in the client?
Where I'm going with that is that both client and server (the server needs it when client auth is enabled) need (optional) access to something that ESP-IDF calls a "certificate bundle" which is - to oversimplify - a X509[] array which is just a bunch of (CA or interim) certificates.
I think we are kind of missing the whole notion of a "certificate bundle" actually... Also, the certificate bundle is always the same and shared across everything, so perhaps it needs to be modeled with its own type. CertificatesBundle or something.
There was a problem hiding this comment.
This seems to be a documentation error. It is indeed required nevertheless, and the optional certificate part for a server should actually be for server certficates, and not CA chain.
Currently X509 seems to support passing multiple chained certificates in the same parsing to create a bundle, but we might also extend the API to allow passing in a list of X509[] if the need is there.
EDIT: Although I'm confused because in mTLS examples we "enable" mTLS by adding a CA Chain. But this might be due to:
esp-mbedtls/esp-mbedtls/src/lib.rs
Lines 456 to 465 in 03458c3
There was a problem hiding this comment.
Currently X509 seems to support passing multiple chained certificates in the same parsing to create a bundle, but we might also extend the API to allow passing in a list of X509[] if the need is there.
My understanding is that a certificate chain (each cert in the chain signed by the following one) is a different thing from a bundle, which is a set of disparate root certs, as in google's, Mozilla's, verisign's etc. but I could be wrong.
There was a problem hiding this comment.
You could be right. I don't have much knowledge on the bundle properties, and even less knowledge about how ESP-IDF implements it.
|
I added I also exposed the certificate verification mode to Session so that users can set it. It is optional because sane defaults exists for this setting. That being said, that makes the Currently client examples fail with verification failed so I have to figure out why that is. Server examples work fine. |
|
So I'm still unsatisfied with the general API of this PR and general project. I refactored a proprietary app we're using, and I still have the following concerns:
|
Why is it optional? What does passing
Worrying about breaking changes - especially at this minor level - is too optimistic IMO given the maturity level of the library. Also changing how
Why are you leaning on builders so quickly? 3 or 4 or even 5 parameters provided at construction time is still OK IMO. Also this is embedded, so small compromises I think can be made in the name of efficiency. I would actually go with the auth_mode current setup, except for either making it non-optional if possible, or moving it to the end of the arguments list.
As per above, I wouldn't bother.
Isn't this just a matter of putting in some extra code in |
I agree that the current API is counter intuitive. Auth_mode should be optional because there are sane defaults for this setting which are There's A user would only really want to change these for:
Yeah, I think we can use getters on Certificates to return the pointers directly, wrapped in options |
If we want a cleaner api, we might try fusing |
|
Good idea. Perhaps a |
AnthonyGrondin
force-pushed
the
feat/X509-memory-improvement
branch
from
11a02ed to
75d9920
Compare
|
I'm getting pretty happy with the result so far.
One edge-case I found is that I've calculated the memory cost difference of this to be about the size of the certificate slice, 2048KB in the example below. That is: While if they were parsed one after the other, that would be something like: If a user really wants to save those bytes, they can use the nocopy variant. |
|
I've added feedback regarding the lifetimes of Also, I don't see why we should delay replacing |
AnthonyGrondin
changed the title
mbedtls_x509_crt and mbedtls_pk_context between sessionsmbedtls_x509_crt and mbedtls_pk_context between sessions
|
Thank you very much for your review. I replaced |
|
I hope you don't mind me jumping in, but a small question I think is related here. Will the changes you make will allow utilizing PSRAM memory for esp-mbedtls? Previous version afaik used the esp-wifi allocations and therefore the DRAM while technically I understand it could use PSRAM for at least some of the operations (unlike esp-wifi that must use DRAM). |
Great question that does not have a simple answer. The TL;DR is: it should work without explicit alignment. This is so because This works great for RiscV32 and Cortex M where For With that said, The C compiler (at least in release) does NOT complain on unaligned Anyway, long story short, I say let's just use |
|
@AnthonyGrondin BTW if this is ready from your POV shall I merge? |
|
@AnthonyGrondin If you are interested in the ... especially further down in the thread where I start pinging igrr from the Espressif team. |
|
Code seems ready but we still need to do some testing. I think the client examples broke and I need to figure out why. |
|
@AnthonyGrondin I think before thinking about connection splitting, we probably need to merge this one first... |
AnthonyGrondin
force-pushed
the
feat/X509-memory-improvement
branch
from
a6d4cdd to
9e87130
Compare
|
I rebased on top of master and tested the examples on esp32s3. mTLS client examples are still broken but they were broken on |
|
One last thing I'd like to see implemented here, is sharing certificates across different I have a case where I renew certificates on a device, so both certificates and private key are different, but the CA Chain is the same, and it would be a waste of memory to create and alloc another instance, if one exists already in memory. I'm still trying to think how it could be implemented. Since we use alloc anyways, maybe some internal RC |
... except we actually no longer use ( |
I did a local refactor, removing the no_copy variants of Maybe I could derive let mut session = Session::new(
&mut socket,
SessionConfig::new(
Mode::Client {
servername: SERVERNAME,
},
TlsVersion::Tls1_3,
),
Certificates::new()
.with_certificates(&crt, &pk)
.with_ca_chain(&ca_chain),
tls.reference(),
)and let the user deal with the lifetime management of 3 different variables, instead of a single one. EDIT: Or I could entirely remove the pub fn with_certificates(
&mut self,
certificate: &'d MbedTLSX509Crt<'_>,
private_key: &'d PkContext,
) -> Self {
self.certificate = Some(certificate);
self.private_key = Some(private_key);
self
}
pub fn with_ca_chain(&mut self, ca_chain: &'d MbedTLSX509Crt<'_>) -> Self {
self.ca_chain = Some(ca_chain);
self
}And then have: let mut session = Session::new(
&mut socket,
SessionConfig::new(
Mode::Client {
servername: SERVERNAME,
},
TlsVersion::Tls1_3,
)
.with_certificates(&crt, &pk)
.with_ca_chain(&ca_chain),
tls.reference(),
) |
Can you push your refactor somewhere? This sounds fishy. All lifetimes are covariant, so they should be possible to reduce to a single lifetime, unless I'm missing something. |
|
I pushed the refactor to https://github.com/esp-rs/esp-mbedtls/tree/refactor/x509-memory-improvement-test I changed the |
You mean this line: esp-mbedtls/examples/edge_server.rs Line 162 in 217f6d6 Yes, it can't work, but I wonder how A few questions/comments:
|
|
Update: No, |
|
Ah sorry, you probably said what I'm saying here:
(if you mean crt and private_key) Yes, that's unavoidable with this design. Alternative: The point of this equilibristic is, if you want to have an easy life and use the allocator, with the thus-generified certificates you would also be able to |
|
... so you could even roll-your-own type-alias: But the point is, |
|
My use case is a bit unsafe to starts with. I wrap the
I haven't found a safe easy way to do it but so far this seems to work for me. I only need to improve the memory usage. I think in my final implementation I'll use a critical section or some blocking async to ensure that the client isn't used elsewhere during this certificate swap trickery. I could also create another client instance, but my current architecture uses a singleton for simplicity. EDIT: Initially, I was thinking about doing something like: enum X509CrtInner {
Owned(*mut mbedtls_x509_crt),
Borrowed(*mut mbedtls_x509_crt),
}
struct MbedTLSX509Crt<'d> {
crt: X509CrtInner,
_t: PhantomData<&'d ()>,
}
impl Drop for MbedTLSX509Crt<'_> {
fn drop(&mut self) {
unsafe {
match self.crt {
X509CrtInner::Owned(ptr) => {
mbedtls_x509_crt_free(ptr);
mbedtls_free(ptr as *const _);
}
X509CrtInner::Borrowed(_) => {
// Do nothing
}
}
}
}
}But managing the lifetime seemed to be an issue, and this is basically almost like reimplementing |
enum X509CrtInner {
Owned(*mut mbedtls_x509_crt),
Borrowed(*mut mbedtls_x509_crt),
}Wouldn't a more correct variant of the above be something like: enum X509CrtInner<'d> {
Owned(*mut mbedtls_x509_crt),
Borrowed(*mut mbedtls_x509_crt, PhantomData<&'d ()>),
}A bit like the However if you want to "swap in" the cert, ==== In any case, the only safe variant of what you are trying to achieve would be to fetch-renew-save-in-NVS the new certs in a completely separate loop/task, and then signal the HTTP client task to re-start itself. Meaning, bring down the HTTP client, re-load the (now new) certs from NVS into the static or whatever storage, and then start the client again? Swapping the certs in the HTTP client when they are behind a |
- Session now exposes an optional `auth_mode` parameter to select the certificate verification mode. - Update Certificates to handle no-copy variants of certificates parsing. - Add DER certificates for testing no-copy variants
- Remove duplicate justfile step for faster building
AnthonyGrondin
force-pushed
the
feat/X509-memory-improvement
branch
from
9e87130 to
d0d565a
Compare
… LLVM extended installation
|
@AnthonyGrondin I'm not sure where we are landing with this in the end. Did you give up (for now) on sharing cert and private_key across multiple Certificates instances (either with a simple & or with a Borrow generic so that in the latter case the user could optionally also have the Certificate own these)? |
I've solved the issues of renewing certificates on my side, using a state machine. As for sharing certificates between different instances, I did test both taking enum X509CrtInner<'d> {
Owned(*mut mbedtls_x509_crt),
Borrowed(*mut mbedtls_x509_crt, PhantomData<&'d ()>),
}Both ways I wasn't so quite happy with the API, but I can take a look back at it. In any ways, we would need to make |
|
There is still the Update: and would need to be pubic, yes. |
Status: Still work in progress
The main intent behind this PR is to save memory allocated per session by reusing resources that could be shared between sessions, such as certificates.
This adds two new wrapper structs to initiate and parse X509 certs and Private keys, and manage the cleanup when they are dropped. Session now takes
Certificatesby reference so the same set ofCertificatescan be shared between sessions.Also
Certificateshave been refactored to parse during init, and to contain the pointer to the allocated memory. One of my long term goal would be to be able to fetch data from the parsed certificates such as the expiring date, without needing extra external dependencies. The config struct is another few 200 bytes that could be saved by sharing it per session, but this would need to keep in memory what parameters were used. Or extend theCertificatesbuilder mecanism so thatCertificatesown that config.