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

add support for client certs #2596

Merged
merged 28 commits into from
Jul 3, 2024
Merged

Conversation

rajatjindal
Copy link
Collaborator

@rajatjindal rajatjindal commented Jun 24, 2024

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

Copy link
Collaborator

@rylev rylev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Outdated Show resolved Hide resolved
crates/trigger-http/src/lib.rs Outdated Show resolved Hide resolved
crates/trigger-http/src/tls.rs Outdated Show resolved Hide resolved
crates/trigger/src/runtime_config.rs Outdated Show resolved Hide resolved
crates/trigger/src/runtime_config/client_tls.rs Outdated Show resolved Hide resolved
@rajatjindal rajatjindal requested a review from lann June 24, 2024 11:52
crates/trigger-http/src/lib.rs Outdated Show resolved Hide resolved
crates/trigger-http/src/lib.rs Outdated Show resolved Hide resolved
crates/trigger/src/runtime_config/client_tls.rs Outdated Show resolved Hide resolved
crates/trigger/src/runtime_config/client_tls.rs Outdated Show resolved Hide resolved
crates/trigger/src/runtime_config/client_tls.rs Outdated Show resolved Hide resolved
crates/trigger-http/src/tls.rs Outdated Show resolved Hide resolved
crates/trigger-http/src/lib.rs Outdated Show resolved Hide resolved
crates/trigger/src/runtime_config/client_tls.rs Outdated Show resolved Hide resolved
crates/trigger-http/src/lib.rs Outdated Show resolved Hide resolved
@@ -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>,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@bacongobbler bacongobbler Jun 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Collaborator

@lann lann Jun 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

})?;

let (mut sender, worker) = if use_tls {
#[cfg(any(target_arch = "riscv64", target_arch = "s390x"))]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

crates/trigger-http/src/lib.rs Outdated Show resolved Hide resolved
#[derive(Debug, Clone)]
pub struct ParsedClientTlsOpts {
pub components: Vec<String>,
pub hosts: Vec<String>,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

@lann lann Jun 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

(Though ime this is rarely used)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@rajatjindal
Copy link
Collaborator Author

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
Copy link
Collaborator Author

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 <[email protected]>
Signed-off-by: Rajat Jindal <[email protected]>
Signed-off-by: Rajat Jindal <[email protected]>
Copy link
Collaborator

@rylev rylev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

crates/trigger-http/src/tls.rs Outdated Show resolved Hide resolved
crates/trigger-http/src/tls.rs Outdated Show resolved Hide resolved
crates/trigger/src/lib.rs Show resolved Hide resolved
crates/trigger/src/runtime_config.rs Outdated Show resolved Hide resolved
crates/trigger/src/runtime_config.rs Outdated Show resolved Hide resolved
crates/trigger/src/runtime_config.rs Outdated Show resolved Hide resolved
crates/trigger/src/runtime_config.rs Outdated Show resolved Hide resolved
crates/trigger/src/runtime_config.rs Outdated Show resolved Hide resolved
Signed-off-by: Rajat Jindal <[email protected]>
@rajatjindal
Copy link
Collaborator Author

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
Copy link
Collaborator

rylev commented Jul 2, 2024

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

@rajatjindal rajatjindal merged commit 436ad58 into fermyon:main Jul 3, 2024
17 checks passed
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

Successfully merging this pull request may close these issues.

5 participants