add support for client certs #2596

rajatjindal · 2024-06-24T11:06:14Z

this PR attempts to add support for client certs based auth. As a summary following are the changes done in this PR:

copies the default_send_request_handler function from wasmtime::wasi_http crate to spin and modifies it to support client-cert-auth and custom-root-ca.
adds client_tls as a new runtime_config option
parses the runtime config and store it indexed by component-id -> host:port (TBD: do we consider default 443 port here if none provided)
uses the client tls config when making outbound request

Known Limitation:

As of now, it will only work when the Outbound request is made from with-in an http-trigger. The other triggers (e.g. redis-trigger) uses a different code path and that has not been changed in this PR).

Example Runtime config:

[[client_tls]]
hosts = ["localhost:6551"]
component_ids = ["app"]
custom_root_ca_file = "certs/custom_root_ca"
cert_chain_file = "certs/cert_chain"
private_key_file = "certs/private_key"

I will attempt to add some runtime tests for these changes, but I wanted to submit the PR to be able to collect some initial feedback.

Thank you

rylev

From a code quality perspective, this looks good to me (just a few nits). I don't have much opinion on whether this is the best way to expose SSL configuration options, but it looks reasonable to me.

crates/trigger-http/src/lib.rs

crates/trigger-http/src/tls.rs

crates/trigger/src/runtime_config.rs

crates/trigger/src/runtime_config/client_tls.rs

crates/trigger-http/src/lib.rs

crates/trigger/src/runtime_config/client_tls.rs

crates/trigger-http/src/tls.rs

crates/trigger-http/src/lib.rs

crates/trigger/src/runtime_config/client_tls.rs

crates/trigger-http/src/lib.rs

endocrimes · 2024-06-24T13:49:17Z

crates/trigger/src/runtime_config.rs

@@ -216,6 +240,9 @@ pub struct RuntimeConfigOpts {

    #[serde(skip)]
    pub file_path: Option<PathBuf>,
+
+    #[serde(rename = "client_tls", default)]
+    pub client_tls_opts: Vec<ClientTlsOpts>,


It's fairly common to need to change client certs at runtime based on some other logic (e.g presenting certs that represent a particular tenant in a system) - placing them in runtime config means that to use Spin, you'd need N different Spin apps running, which may be subpar

To be clear, this is intended for cases where the client runtime cannot load client certificates. The Spin Javascript SDK v2 uses the browser's fetch API is a clear example of this. The browser API cannot load certificates presented by the client - it needs the user to add those certificates into the root certificate store in order for that endpoint to be trusted.

I am hopeful that StarlingMonkey or Spin's SDK will eventually provide ways to load TLS certs via guest code similar to Deno, but we're not at that point yet.

(and yes, this is far from ideal)

I think ideally we could define a standard API in terms of wasi:http, possibly using proposed request metadata (TLS config was one of my use cases for that proposal).

I just filed fermyon/spin-js-sdk#249 to track this feature request.

endocrimes · 2024-06-24T15:58:01Z

crates/trigger-http/src/lib.rs

+        })?;
+
+    let (mut sender, worker) = if use_tls {
+        #[cfg(any(target_arch = "riscv64", target_arch = "s390x"))]


riscv64 should work for TLS? do we need to update a dependency instead? (tokio-rustls >= 0.25.0 at least worked last i looked)

This impl is copied from wasmtime-wasi-http upstream.

ah i guess they haven't fixed it since upstream got fixed

crates/trigger-http/src/lib.rs

endocrimes · 2024-06-24T16:02:51Z

crates/trigger/src/runtime_config.rs

+#[derive(Debug, Clone)]
+pub struct ParsedClientTlsOpts {
+    pub components: Vec<String>,
+    pub hosts: Vec<String>,


Tests and documentation for what happens when multiple TLS blocks contain overlapping hosts would be pretty useful

great point. I added some testcases for it. right now the last block wins.

is that ok? or we want it to error out if different tls-config found for same component-id/host combination. what do you suggest.

also cc @lann

Technically we should allow multiple certs to be configured; the tls negotiation can select from multiple.

(Though ime this is rarely used)

in that case maybe we keep it simple and let the last block win (the current behavior) or should we fail it for now?

I might suggest first-wins here since that is how variable provider configs already work.

👍 , I have now changed the logic to make first entry win.

crates/trigger/src/runtime_config/client_tls.rs

crates/trigger-http/src/lib.rs

rajatjindal · 2024-06-26T07:57:22Z

I've addressed most of the review comments (except a few unwrap comments, and I do plan to address them before merge). if there are any additional comments, kindly let me know. thanks

I will also continue to add more testcases to it until we are ready to merge the PR.

rajatjindal · 2024-06-28T09:07:36Z

I think I've addressed most of the review feedback. I can spend some more time to add more tests, but otherwise it looks ok for re-review.

thanks

Signed-off-by: Rajat Jindal <rajatjindal83@gmail.com>

rylev

Looks good - I have some nits, but you can choose whether to address them or not.

crates/trigger-http/src/tls.rs

crates/trigger/src/lib.rs

crates/trigger/src/runtime_config.rs

Signed-off-by: Rajat Jindal <rajatjindal83@gmail.com>

rajatjindal · 2024-07-02T16:00:03Z

Looks good - I have some nits, but you can choose whether to address them or not.

I have addressed most of the nits you suggested.

rylev · 2024-07-02T16:17:11Z

@rajatjindal feel free to merge this whenever you're ready

rylev reviewed Jun 24, 2024

View reviewed changes

rajatjindal requested a review from lann June 24, 2024 11:52

lann reviewed Jun 24, 2024

View reviewed changes

endocrimes reviewed Jun 24, 2024

View reviewed changes

bacongobbler mentioned this pull request Jun 24, 2024

[sdk-v2]: ability to create a custom HttpClient with custom TLS certificates/connect via proxy fermyon/spin-js-sdk#249

Open

lann reviewed Jun 25, 2024

View reviewed changes

crates/trigger/src/runtime_config/client_tls.rs Outdated Show resolved Hide resolved

lann reviewed Jun 25, 2024

View reviewed changes

crates/trigger-http/src/lib.rs Outdated Show resolved Hide resolved

rajatjindal added 21 commits June 28, 2024 19:15

update tls - reword commit msg

c0065bb

Signed-off-by: Rajat Jindal <rajatjindal83@gmail.com>

add client_tls option to runtime config

52d9e01

Signed-off-by: Rajat Jindal <rajatjindal83@gmail.com>

add function to parse client tls opts

4aa175f

Signed-off-by: Rajat Jindal <rajatjindal83@gmail.com>

add client tls options to TriggerAppEngine

8477d37

Signed-off-by: Rajat Jindal <rajatjindal83@gmail.com>

copy wasmtime default send request handler to spin

b8402af

Signed-off-by: Rajat Jindal <rajatjindal83@gmail.com>

move configuring client_tls_config to a separate fn

5d96bdd

Signed-off-by: Rajat Jindal <rajatjindal83@gmail.com>

add client_tls_opts to HttpRuntimeData

d64ea76

Signed-off-by: Rajat Jindal <rajatjindal83@gmail.com>

modify default_send_request_handler to allow use of client_tls_opts

3a97ef1

Signed-off-by: Rajat Jindal <rajatjindal83@gmail.com>

use spin's version of default_send_request_handler with client_tls_opts

419848d

Signed-off-by: Rajat Jindal <rajatjindal83@gmail.com>

address review comments

19e4dff

Signed-off-by: Rajat Jindal <rajatjindal83@gmail.com>

parse hosts as Authority and use that as key for client tls opts

1b74452

Signed-off-by: Rajat Jindal <rajatjindal83@gmail.com>

remove into_iter to fix lint issue

dc78e99

Signed-off-by: Rajat Jindal <rajatjindal83@gmail.com>

add couple of tests for parse_client_tls_opts

108e2a3

Signed-off-by: Rajat Jindal <rajatjindal83@gmail.com>

use iterator for creating host -> client opts map

71ea773

Signed-off-by: Rajat Jindal <rajatjindal83@gmail.com>

throw error if adding cert to trust store fails

1527ca1

Signed-off-by: Rajat Jindal <rajatjindal83@gmail.com>

add knob to use ca_webpki_roots and defaults

e4094d6

Signed-off-by: Rajat Jindal <rajatjindal83@gmail.com>

add testcases for client tls options in runtime config

58a3595

Signed-off-by: Rajat Jindal <rajatjindal83@gmail.com>

store parsed value of ca_webpki_roots in ParsedClientTlsOpts

3cad9dd

Signed-off-by: Rajat Jindal <rajatjindal83@gmail.com>

parse component-id as spin_serde::KebabId to catch invalid ids early

ed1077f

Signed-off-by: Rajat Jindal <rajatjindal83@gmail.com>

add an exception for lint false positive mutable_key_type

b97f09c

Signed-off-by: Rajat Jindal <rajatjindal83@gmail.com>

fix lint warnings

bb3e1ee

Signed-off-by: Rajat Jindal <rajatjindal83@gmail.com>

rajatjindal added 4 commits June 28, 2024 19:15

fix lint warnings

9bef9d2

Signed-off-by: Rajat Jindal <rajatjindal83@gmail.com>

remove use of unwrap from load tls certs/key fns

857e39a

Signed-off-by: Rajat Jindal <rajatjindal83@gmail.com>

return error instead of unwrap

b64c582

Signed-off-by: Rajat Jindal <rajatjindal83@gmail.com>

ran make update-cargo-locks

cd2c096

Signed-off-by: Rajat Jindal <rajatjindal83@gmail.com>

rajatjindal force-pushed the client-certs branch from efae441 to cd2c096 Compare June 28, 2024 13:45

change tls opts logic to ensure first entry wins

8f66682

Signed-off-by: Rajat Jindal <rajatjindal83@gmail.com>

rajatjindal force-pushed the client-certs branch from 3c5af70 to 8f66682 Compare July 1, 2024 08:51

remove duplicate into_iter()

881c3bb

Signed-off-by: Rajat Jindal <rajatjindal83@gmail.com>

rylev approved these changes Jul 2, 2024

View reviewed changes

address final review nits

8604bb2

Signed-off-by: Rajat Jindal <rajatjindal83@gmail.com>

rajatjindal merged commit 436ad58 into fermyon:main Jul 3, 2024
17 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add support for client certs #2596

add support for client certs #2596

rajatjindal commented Jun 24, 2024 •

edited

Loading

rylev left a comment

endocrimes Jun 24, 2024

bacongobbler Jun 24, 2024 •

edited

Loading

lann Jun 24, 2024 •

edited

Loading

bacongobbler Jun 24, 2024

endocrimes Jun 24, 2024

lann Jun 24, 2024

endocrimes Jun 24, 2024

endocrimes Jun 24, 2024

rajatjindal Jun 26, 2024

lann Jun 26, 2024 •

edited

Loading

rajatjindal Jun 28, 2024

lann Jun 28, 2024

rajatjindal Jul 1, 2024

rajatjindal commented Jun 26, 2024

rajatjindal commented Jun 28, 2024

rylev left a comment

rajatjindal commented Jul 2, 2024

rylev commented Jul 2, 2024

add support for client certs #2596

add support for client certs #2596

Conversation

rajatjindal commented Jun 24, 2024 • edited Loading

rylev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bacongobbler Jun 24, 2024 • edited Loading

Choose a reason for hiding this comment

lann Jun 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lann Jun 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rajatjindal commented Jun 26, 2024

rajatjindal commented Jun 28, 2024

rylev left a comment

Choose a reason for hiding this comment

rajatjindal commented Jul 2, 2024

rylev commented Jul 2, 2024

rajatjindal commented Jun 24, 2024 •

edited

Loading

bacongobbler Jun 24, 2024 •

edited

Loading

lann Jun 24, 2024 •

edited

Loading

lann Jun 26, 2024 •

edited

Loading