Telodendria/Cytoplasm
Telodendria/Cytoplasm
3
3
Fork 5
Code Issues 17 Pull requests 2 Projects Releases 2 Packages Activity Actions

WIP: Adds MbedTLS support to Cytoplasm #54

Draft
lda wants to merge 9 commits from lda/Cytoplasm:add-mbed into master
pull from: lda/Cytoplasm:add-mbed
merge into: Telodendria:master
Conversation 1 Commits 9 Files changed 8 +451 -3
lda commented 2024-09-15 09:19:09 +00:00
Contributor
Copy link

Only issue with this IMO is that requiring the presence of CYTO_TLS_CA whenever a program may want to use TLS doesn't sound all that great, so maybe it should check off common paths when unset before complaining.

Only issue with this IMO is that requiring the presence of `CYTO_TLS_CA` whenever a program may want to use TLS doesn't sound all that great, so maybe it should check off common paths when unset before complaining.
lda added 2 commits 2024-09-15 09:19:10 +00:00
lda added 1 commit 2024-09-15 11:08:15 +00:00
lda added 2 commits 2024-09-15 11:09:44 +00:00
jordan reviewed 2024-09-15 22:24:43 +00:00
src/Tls/TlsMbedTLS.c Outdated
@ -0,0 +96,4 @@
}
/* Add a source of entropy if possible(using the CYTO_TLS_SEED env).
* Note that we ignore the error code. */
seed = getenv("CYTO_TLS_SEED");
jordan commented 2024-09-15 22:24:43 +00:00
Owner
Copy link

Forgive me for not understanding MbedTLS that well, but why does this need to be sourced from the environment? Can't we randomly generate a seed? What's the use case for using the same seed more than once?

Forgive me for not understanding MbedTLS that well, but why does this need to be sourced from the environment? Can't we randomly generate a seed? What's the use case for using the same seed more than once?
lda commented 2024-09-16 07:49:11 +00:00
Author
Contributor
Copy link

It doesn't have to in most cases(MbedTLS will try to use things like /dev/random if possible) but considering how POSIX doesn't really mandate such sources to exist in the first place (and that I really wouldn't use something like the Rand API for cryptographic tasks), and I think having an optional source of extra randomness, if possible, may be worth it.

What's the use case for using the same seed more than once?
MbdeTLS tends to change the seed anyways with whatever other sources it can grab too.

It *doesn't* have to in most cases(MbedTLS will try to use things like `/dev/random` if possible) but considering how POSIX doesn't really mandate such sources to exist in the first place (and that I really *wouldn't* use something like the `Rand` API for cryptographic tasks), and I think having an optional source of extra randomness, if possible, may be worth it. > What's the use case for using the same seed more than once? MbdeTLS tends to change the seed anyways with whatever other sources it can grab too.
jordan commented 2024-09-16 17:11:42 +00:00
Owner
Copy link

(and that I really wouldn't use something like the Rand API for cryptographic tasks)

I agree, and that's the real problem here. Ideally, we should use the Rand API to generate the MbedTLS seed. The fact that we can't because Rand isn't cryptographically secure is a problem of its own that, in my opinion, should be fixed first before we start relying on environment variables.

I'd like Cytoplasm to very much be link-and-forget. It shouldn't have any special runtime dependencies like this. I don't even know how we'd trust the user to set the seed properly. If the system doesn't have some source of entropy that we can draw from, then I don't know how the user could generate a seed. Because if they can do it on the machine we're running on, then so can we, and so should we. Otherwise, they'd have to generate it on a different machine and then copy it over to the machine executing this Cytoplasm code? That seems pretty impractical, particularly because every time Cytoplasm is started, (or every time this function is called?) the seed needs to be freshly generated.

> (and that I really wouldn't use something like the Rand API for cryptographic tasks) I agree, and that's the real problem here. Ideally, we *should* use the Rand API to generate the MbedTLS seed. The fact that we can't because Rand isn't cryptographically secure is a problem of its own that, in my opinion, should be fixed first before we start relying on environment variables. I'd like Cytoplasm to very much be link-and-forget. It shouldn't have any special runtime dependencies like this. I don't even know how we'd trust the user to set the seed properly. If the system doesn't have some source of entropy that we can draw from, then I don't know how the user *could* generate a seed. Because if they can do it on the machine we're running on, then so can we, and so should we. Otherwise, they'd have to generate it on a different machine and then copy it over to the machine executing this Cytoplasm code? That seems pretty impractical, particularly because every time Cytoplasm is started, (or every time this function is called?) the seed needs to be freshly generated.
jordan reviewed 2024-09-15 22:25:34 +00:00
src/Tls/TlsMbedTLS.c Outdated
@ -0,0 +117,4 @@
}
/* Setup key verification */
cafile = getenv("CYTO_TLS_CA");
jordan commented 2024-09-15 22:25:34 +00:00
Owner
Copy link

Again, why should this come from the environment? Is there a way we can somehow just use the system's CA file?

Again, why should this come from the environment? Is there a way we can somehow just use the system's CA file?
lda commented 2024-09-16 21:28:22 +00:00
Author
Contributor
Copy link

Just made it check for known certificate paths(though I've kept the environment, in case any users may still want to use a custom directory/are really unlucky in their OS setups)

Just made it check for known certificate paths(though I've kept the environment, in case any users may still want to use a custom directory/are really unlucky in their OS setups)
jordan commented 2024-09-21 00:14:52 +00:00
Owner
Copy link

Okay, I think I'm good with what you've done here. I do have a question though: Can you load multiple PEMs? Like, why bother with return true on success, when you could just keep going and load all the CA stores you can find? If the answer is no, you can only load one CA store, then that makes sense. I'm just curious if we could be even more flexible and load multiple.

However, I don't think this behavior should be specific to Mbed. If we're going to use CYTO_TLS_CA to modify Cytoplasm's runtime behavior in loading certificate stores, shouldn't all of the other TLS implementations honor this environment variable too?

For example, say I'm a sysadmin at a large corporation with tons of self-signed certificates for internal services. Instead of installing the certificate authority to the system, I want to just set CYTO_TLS_CA to my CA store. I'm on a Linux system so I can use OpenSSL, but the TLS implementation really shouldn't matter. Though I guess it might determine what format the CA store must be in. But the point is, the behavior should be consistent across all implementations.

Okay, I think I'm good with what you've done here. I do have a question though: Can you load *multiple* PEMs? Like, why bother with `return true` on success, when you could just keep going and load all the CA stores you can find? If the answer is no, you can only load one CA store, then that makes sense. I'm just curious if we could be *even more* flexible and load multiple. **However**, I don't think this behavior should be specific to Mbed. If we're going to use `CYTO_TLS_CA` to modify Cytoplasm's runtime behavior in loading certificate stores, shouldn't all of the *other* TLS implementations honor this environment variable too? For example, say I'm a sysadmin at a large corporation with tons of self-signed certificates for internal services. Instead of installing the certificate authority to the system, I want to just set `CYTO_TLS_CA` to my CA store. I'm on a Linux system so I can use OpenSSL, but the TLS implementation really shouldn't matter. Though I guess it might determine what format the CA store must be in. But the point is, the behavior should be consistent across all implementations.
lda commented 2024-09-21 07:52:47 +00:00
Author
Contributor
Copy link

Can you load multiple PEMs?

You actually can(if you look under the MbedTLS hood, loading a directory of PEM files is essentially just loading every PEM directly), so I just added that.

As for the CYTO_TLS_CA, I'm not sure I can exactly replicate that behavior around LibreSSL, as the functions used to replace certs with it seem to overwrite the current option, rather than just adding it alongside. We could just also do that on the MbedTLS side, but I don't know if that'd be a good thing...

> Can you load *multiple* PEMs? You actually *can*(if you look under the MbedTLS hood, loading a directory of PEM files is essentially just loading every PEM directly), so I just added that. As for the `CYTO_TLS_CA`, I'm not sure I can exactly replicate that behavior around LibreSSL, as the functions used to replace certs with it seem to *overwrite* the current option, rather than just adding it alongside. We *could* just also do that on the MbedTLS side, but I don't know if that'd be a good thing...
jordan commented 2024-09-21 18:29:54 +00:00
Owner
Copy link

Ah, that's a bummer. I suppose maybe it would suffice to have CYTO_TLS_CA replace the system's CA store for all implementations. In other words, have Libre and Open check CYTO_TLS_CA and load the store given by that variable if it is set, otherwise do nothing and assume that they load the system store. For Mbed, only check and load the other certificate stores if CYTO_TLS_CA is unset, so that the behavior is consistent across all three implementations: if CYTO_TLS_CA is set, use only that store, otherwise use the system stores, and in the case of Mbed, try to determine what those stores are.

Doe that make sense? Does that sound like a good idea?

Ah, that's a bummer. I suppose maybe it would suffice to have `CYTO_TLS_CA` replace the system's CA store for all implementations. In other words, have Libre and Open check `CYTO_TLS_CA` and load the store given by that variable if it is set, otherwise do nothing and assume that they load the system store. For Mbed, only check and load the other certificate stores if `CYTO_TLS_CA` is unset, so that the behavior is consistent across all three implementations: if `CYTO_TLS_CA` is set, use *only* that store, otherwise use the system stores, and in the case of Mbed, try to determine what those stores are. Doe that make sense? Does that sound like a good idea?
jordan commented 2024-09-15 22:28:08 +00:00
Owner
Copy link

We should definitely try to use the system's CA store when possible. I don't know if there's a standard location for this, but on all the systems I've used, it's been /etc/ssl/cert.pem.

Then again, if the system doesn't have a CA store, I'm not really sue what we'll do there.

Either way, this is otherwise a nice looking pull request. I think our design for supporting multiple TLS implementations is great, it's super easy to add new implementations.

We should definitely try to use the system's CA store when possible. I don't know if there's a standard location for this, but on all the systems I've used, it's been `/etc/ssl/cert.pem`. Then again, if the system doesn't *have* a CA store, I'm not really sue what we'll do there. Either way, this is otherwise a nice looking pull request. I think our design for supporting multiple TLS implementations is great, it's super easy to add new implementations.
lda added 2 commits 2024-09-16 21:26:35 +00:00
lda changed title from Adds MbedTLS support to Cytoplasm to WIP: Adds MbedTLS support to Cytoplasm 2024-09-19 03:38:06 +00:00
lda added 1 commit 2024-09-21 07:53:28 +00:00
lda added 1 commit 2024-09-24 05:17:00 +00:00
Merge branch 'master' into add-mbed
Some checks failed
Compile Cytoplasm / Compile Cytoplasm (aarch64, alpine) (pull_request) Has been cancelled
Details
da7199f03a
Some checks failed
Compile Cytoplasm / Compile Cytoplasm (aarch64, alpine) (pull_request) Has been cancelled
Details
This pull request is marked as a work in progress.
This branch is out-of-date with the base branch
View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u add-mbed:lda-add-mbed
git checkout lda-add-mbed

Merge

Merge the changes and update on Forgejo.

Warning: The "Autodetect manual merge" setting is not enabled for this repository, you will have to mark this pull request as manually merged afterwards.

git checkout master
git merge --no-ff lda-add-mbed
git checkout lda-add-mbed
git rebase master
git checkout master
git merge --ff-only lda-add-mbed
git checkout lda-add-mbed
git rebase master
git checkout master
git merge --no-ff lda-add-mbed
git checkout master
git merge --squash lda-add-mbed
git checkout master
git merge --ff-only lda-add-mbed
git checkout master
git merge lda-add-mbed
git push origin master
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
bug

Something is not working

  
duplicate

This issue or pull request already exists

  
enhancement

New feature

  
help wanted

Need some help

  
invalid

Something is wrong

  
question

More information is needed

  
wontfix

This won't be fixed

No labels
bug
duplicate
enhancement
help wanted
invalid
question
wontfix
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: Telodendria/Cytoplasm#54
No description provided.
Delete branch "lda/Cytoplasm:add-mbed"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?