add mTLS and TLS verification options#55
Conversation
| /// | ||
| pub fn verify_tls(config: Configuration, which: Bool) -> Configuration { | ||
| Builder(..config, verify_tls: which) | ||
| pub fn verify_tls( |
There was a problem hiding this comment.
It seems advantageous to do this without a breaking change if possible. Right now this method flags a single keyword to ssl, which is sort of orthongal to the rest of what is here.
There was a problem hiding this comment.
this is indeed a breaking change :/
i tried to get my head around what was discussed here: #36 (comment):
I don't think it would make sense to say "do not verify TLS" and "here's a cert to verify TLS with" at the same time, so instead we make a custom type with variants for not verifying, verifying with system certs, and verifying with custom certs. Do you think that would cover all the possibilities?
@lpil can we get your input on this?
There was a problem hiding this comment.
There are many options in a full-featured TLS stack that are not included in gleam_httpc today. The reason you see stacks split them is that there is potentially interest in configuring with this CA, but only verifying the chain, not the hostname. The axes become "how much to verify in this connection" and "how to verify the connection." On further thought, there is more benefit to making it harder to misuse.
src/gleam/httpc.gleam
Outdated
| ssl_verify_host_options(True) | ||
| |> list.map(fn(opt) { | ||
| case opt { | ||
| Cacerts(_) -> Cacertfile(charlist.from_string(path)) |
There was a problem hiding this comment.
This seems like a footgun because, while {cacerts, CACerts} is in the default options, if for some reason it wasn't or that ever changed, this wouldn't get added. It should probably be two steps: first, get the default options; if {cacertfile, Path} is needed, the cacerts key is removed and replaced.
There was a problem hiding this comment.
done. i transformed this part by first filtering the list, then prepending the option. still need to traverse the list, but i don't see another solution
jtdowney
left a comment
jtdowney
left a comment
There was a problem hiding this comment.
I am not affiliated with the Gleam project in any way; I am just a Gleam user with 15 years of reviewing TLS-related code. So feel free to ignore me.
The other thing I'd add is that there isn't much in the way of tests for the client certificate code, though it is difficult/tedious to test these things. You basically have to generate a mini-PKI and stand up a test server.
| let verify_opts = verification_options(config.tls_verification) | ||
|
|
||
| case config.tls_verification, cert_opts { | ||
| VerifyWithSystemCerts, [] -> option.None |
There was a problem hiding this comment.
Seems odd to resolve verify_opts outside of the case if it is only needed in one branch.
2 participants
Fixes #54