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

support dynamic issuer in tls cert issuer logic #354

Open
GlenDC opened this issue Nov 13, 2024 · 2 comments
Open

support dynamic issuer in tls cert issuer logic #354

GlenDC opened this issue Nov 13, 2024 · 2 comments
Assignees
Labels
enhancement New feature or request mentor available A mentor is available to help you through the issue.
Milestone

Comments

@GlenDC
Copy link
Member

GlenDC commented Nov 13, 2024

At the time of writing this issue there is already the following support for tls acceptor auth config:

pub enum ServerAuth {
    SelfSigned(SelfSignedData),
    Single(ServerAuthData),
    CertIssuer(ServerCertIssuerData),
}

Where ServerCertIssuerData is:

pub struct ServerCertIssuerData {
    pub kind: ServerCertIssuerKind,
    pub max_cache_size: u64,
}

and ServerCertIssuerKind is:

pub enum ServerCertIssuerKind {
    SelfSigned(SelfSignedData),
    Single(ServerAuthData),
}

The goal of this issue is to provide a third variant to ServerCertIssuerKind such that it becomes:

pub enum ServerCertIssuerKind {
    SelfSigned(SelfSignedData),
    Single(ServerAuthData),
    Dynamic(???),
}

The dynamic "thing" would support any kind of custom logic to support getting certs. The thing could receive some kind of CertIssueInput, for example:

struct CertIssuerInput {
     pub sni: Host,
}

Proposed steps to resolve this issue:

  1. choose an approach, or create your own, work out on paper and align here in the issue in the comment section with me.
  2. implement the approach and open a PR as soon as you feel ready, want early feedback, are stuck, have questions, or want to collaborate.

Some approaches we can make this happen:

  1. provide a trait ServerCertIssuer and work with a Box`; This approach is the most obvious but also not one I like as it means we have to box by default

  2. provide a channel based approach such that the variant would become something like (pseudo code):

  // ...
  Dynamic(ServerCertInputSender),
}

struct ServerCertInputSender {
     sender: Sender<ServerCertInputReceiver>,
}

struct ServerCertInputReceiver {
    sender: oneshot<Result<ServerAuthData, BoxError>>,
}

// the internal details of `ServerCertInputSender` and `ServerCertInputReceiver` would be hidden behind an api that simply uses
// the `send` methods. E.g. `send(input)` and `send(result)`. Wthout having to know the internal implementation
// or that a channel is even used
  1. something else. Feel free to propose something other than the above proposals

Generics is something we do want to avoid here as it would make the entire type and using it a lot more cumbersome / complicated. Even when not using this dynamic approach.


This kind of feature would allow the implementation of an external cert issuer, as to not have to keep the CA or physical proximity of the proxy. It could also allow dynamic cert selection. And many other use cases.

@GlenDC GlenDC added enhancement New feature or request low prio Low priority item. mentor available A mentor is available to help you through the issue. labels Nov 13, 2024
@GlenDC GlenDC added this to the v0.3 milestone Nov 13, 2024
@plabayo plabayo deleted a comment Nov 13, 2024
@soundofspace soundofspace self-assigned this Nov 13, 2024
@GlenDC GlenDC modified the milestones: v0.3, v0.2 Nov 13, 2024
@GlenDC GlenDC removed the low prio Low priority item. label Nov 13, 2024
@soundofspace
Copy link
Collaborator

As discussed in private we will implement this with a Box/Arc approach. The reason to prefer a channel over a box is to prevent the usage of fat pointers to improve performance, but currently we have no metrics to prove that a channel is indeed better performance wise then a box. Once everything is implemented and we do see performance issues, or someone can provide benchmarks that a channel is faster/more efficient we can always switch to a channel. We will hide this implementation detail from external users so switching implementations should only be an internal detail.

Currently I was thinking of providing the entire ClientHello as input to this function/trait so it has all the info it needs to make this decision. This is similar to how rustls does it in ResolvesServerCert. Just to be safe I also plan to wrap this in an input type so we can always add additional inputs if needed without introducing breaking changes.

Any other feedback or things we need to discuss before I start implementing this?

@GlenDC
Copy link
Member Author

GlenDC commented Nov 18, 2024

Just go for dynamic traits, I don't think there's gonna be a performance issue. I have a hard time believing that channels are faster than it.

We will hide this implementation detail from external users so switching implementations should only be an internal detail.

How would you plan on doing so? Don't think that is hidden if you have to expose the trait?

Currently I was thinking of providing the entire ClientHello as input to this function/trait so it has all the info it needs to make this decision. This is similar to how rustls does it in ResolvesServerCert

https://ramaproxy.org/docs/rama/net/tls/client/struct.ClientHello.html, do you mean this ClientHello? If so, fine for me. It can be both converted from both Rustl's version as well as boring's. No need to wrap it btw, purposes like this is why in rama's tls types there is the ClientHello.

The only issue is that I'm not sure how boring exposes this ClientHello directly in this callback...

https://docs.rs/boring/latest/boring/ssl/struct.SslRef.html <- that's the type you get as part of the callback that we can use to set certificates on the fly (e.g. with an issuer): https://docs.rs/boring/latest/boring/ssl/struct.SslContextBuilder.html#method.set_servername_callback

We might need to switch over to using https://docs.rs/boring/latest/boring/ssl/struct.SslContextBuilder.html#method.set_async_select_certificate_callback instead. That one does give access to the client hello. Does require some trickery such as setting the waker etc... will need to be careful with that one.

For rustls I'm not sure what there is needed. But In case that latter is too much, feel free to leave rustls unsupported for now, it's not supporting the current cert issue live in main neither.

Do hit me up here or on Discord if there's something not clear or right.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request mentor available A mentor is available to help you through the issue.
Projects
None yet
Development

No branches or pull requests

2 participants