Skip to content

Conversation

@lucasdbr05
Copy link

What does this merge request do?

This feature (see the reference issue #176) adds SSL certificate validation based on Trust On First Use (TOFU), storing the certificate on the first connection and verifying its consistency on subsequent connections.
It is implemented via the TofuStore trait, which allows customizable certificate persistence.

  • Introduces a refactored Trust On First Use (TOFU) implementation for SSL certificate verification.
  • Adds the TofuStore trait, allowing library consumers to provide their own certificate persistence mechanism.
  • Updates the Client configuration SSL client initialization flow to accept a TofuStore via ConfigBuilder.
  • Adds examples in examples directory and scripts to demonstrate how TOFU can be integrated and used in practice.

@lucasdbr05 lucasdbr05 changed the title Feat/tofu certificate validation feat:tofu certificate validation Jan 14, 2026
Copy link
Contributor

@tnull tnull left a comment

Choose a reason for hiding this comment

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

Took an initial quick look and added some comments (though have yet to do a full review). Though before I proceed, please rewrite the commit history to avoid changing the approach mid-history. Basically, you should try to avoid to touch the same code paths in following commits as far as possible.

Given the code structure and verbosity I'm also suspecting that some form of AI agent was involved here. If this is indeed the case, please note that it is best practice to disclose such use in the PR and commit descriptions.


/// A trait for storing and retrieving TOFU (Trust On First Use) certificate data.
/// Implementors of this trait are responsible for persisting certificate data and retrieving it based on the host.
pub trait TofuStore: Send + Sync + Debug {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does this need to be Send + Sync?

Copy link
Author

Choose a reason for hiding this comment

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

I did it this way to ensure that the Client remains thread-safe for concurrent applications, so I designed it to allow the store to be shared and safely accessed across multiple threads via an Arc.

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess? Though the Rust compiler should usually infer the bounds when needed.

src/config.rs Outdated
/// when ssl, validate the domain, default true
validate_domain: bool,
/// TOFU store for certificate validation
tofu_store: Option<Arc<dyn TofuStore>>,
Copy link
Contributor

Choose a reason for hiding this comment

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

As mentioned over at the issue, it would likely be preferable to add this via a separate constructor. Having an Arc<dyn ..> in a config is rather odd, as it mixes code and data, basically.

.connect(&domain, stream)
.map_err(Error::SslHandshakeError)?;

if !validate_domain {
Copy link
Contributor

Choose a reason for hiding this comment

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

Not quite sure why we here error out? Care to explain?

builder.set_verify(SslVerifyMode::NONE);
let connector = builder.build();

let domain = socket_addrs.domain().unwrap_or("NONE").to_string();
Copy link
Contributor

Choose a reason for hiding this comment

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

No, if we don't have a domain we should just abort rather than try to connect to NONE?

Copy link
Author

Choose a reason for hiding this comment

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

That makes sense, I’ll update it.

@lucasdbr05
Copy link
Author

Took an initial quick look and added some comments (though have yet to do a full review). Though before I proceed, please rewrite the commit history to avoid changing the approach mid-history. Basically, you should try to avoid to touch the same code paths in following commits as far as possible.

Given the code structure and verbosity I'm also suspecting that some form of AI agent was involved here. If this is indeed the case, please note that it is best practice to disclose such use in the PR and commit descriptions.

First of all, thanks for the feedback. When rewriting the history, the best approach would be to avoid keeping commits that record my refactor from persistence-based usage to the trait-based approach, right?

@tnull
Copy link
Contributor

tnull commented Jan 15, 2026

When rewriting the history, the best approach would be to avoid keeping commits that record my refactor from persistence-based usage to the trait-based approach, right?

Yes, this would be preferable. Basically, you could just do a git reset and then commit the individual pieces again.

@luisschwab luisschwab self-requested a review January 17, 2026 20:32
@oleonardolima oleonardolima self-requested a review January 19, 2026 17:54
@oleonardolima oleonardolima added the new feature New feature or request label Jan 19, 2026
@oleonardolima oleonardolima moved this to Discussion in BDK Chain Jan 19, 2026
Copy link
Contributor

@oleonardolima oleonardolima left a comment

Choose a reason for hiding this comment

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

I agree with tnull's comments above, you should narrow it down to few commits, for example: feat(tofu): add tofu store mod; feat(client): add new tofu method/feature, docs(example): add new tofu example.

Also, it's best if it's added under a new feature, and by a separate constructor. I don't think many changes to already-existing methods are needed, maybe it's something remaining from previous changes you did.

It's failing in CI, please make sure that everything is building successfully and passing CI too.


let builder = ClientConfig::builder();

let domain = socket_addr.domain().unwrap_or("NONE").to_string();
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as tnull's previous comment, it should error-out and it's been handled below.

Comment on lines 616 to 623
let error_msg = format!("{}", e);
if error_msg.contains("TLS certificate changed") {
Error::TlsCertificateChanged(domain.clone())
} else if error_msg.contains("TOFU") {
Error::TofuPersistError(error_msg)
} else {
Error::CouldNotCreateConnection(e)
}
Copy link
Contributor

@oleonardolima oleonardolima Jan 19, 2026

Choose a reason for hiding this comment

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

IMHO it's not ideal to handle the error variants by checking if it contains certain strings on it, you should properly return an map a new error variant where needed.

Trust On First Use (TOFU) is a security model where a client trusts a certificate upon the first connection and subsequent connections are verified against that initially stored record.

- Introduce the  trait to manage  certificates
- Add  module and export the trait in the library root
- Update  signature to accept an optional
- Implement  for both OpenSSL and Rustls
- Add  custom certificate verifier for Rustls (with AI help)
- Extend  enum with error variants specifics to TOFU
- Support TOFU validation in proxy SSL connections
Implemented a dedicated constructor in the  to handle Trust On First Use (TOFU) certificate validation.

- New  method to initialize a client with TOFU from config
- Update existing constructors to support the revised SSL backend signatures
- Implement  for testing purposes
- Add comprehensive tests covering first-use storage, certificate matching/replacement, and large payloads

NOTE: Unit tests and supporting mock implementation were created with AI assistance.
- Provide an usage example for the  trait
- Demonstrate how to initialize a  using
- Include a sample in-memory store implementation for demonstration purposes
@lucasdbr05 lucasdbr05 force-pushed the feat/TOFU-certificate-validation branch from 9178ff9 to 42639ae Compare January 21, 2026 04:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

new feature New feature or request

Projects

Status: Discussion

Development

Successfully merging this pull request may close these issues.

3 participants