Skip to content

feat:tofu certificate validation#193

Draft
lucasdbr05 wants to merge 1 commit intobitcoindevkit:masterfrom
lucasdbr05:feat/TOFU-certificate-validation
Draft

feat:tofu certificate validation#193
lucasdbr05 wants to merge 1 commit intobitcoindevkit:masterfrom
lucasdbr05:feat/TOFU-certificate-validation

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.

@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
Collaborator

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

@lucasdbr05 lucasdbr05 force-pushed the feat/TOFU-certificate-validation branch from 9178ff9 to 42639ae Compare January 21, 2026 04:06
pub use config::{Config, ConfigBuilder, Socks5Config};
pub use types::*;

mod tofu;
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 we want to feature-gate the whole module?

Copy link
Author

Choose a reason for hiding this comment

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

I have not created the tofu feature.
I tried to add, but imho the code become too messy when doing so. Adding a feature-gate for it introduces many cases to handle with using [cfg(feature = "tofu")] and [cfg(not(feature = "tofu"))], and since the param tofu_store is optional, I'd rather keep without it. Could you give some feedback about it?

_now: UnixTime,
) -> Result<ServerCertVerified, rustls::Error> {
// Verify using TOFU
self.verify_tofu(end_entity.as_ref())
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we also offer a mode where the TofuVerifier just wraps the rustls verifier, i.e., we'd still also validate the certificate as usual if validate_domain is set? Or do we think that wouldn't ever be used, and doing only TOFU would suffice?

Copy link
Author

Choose a reason for hiding this comment

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

When I implemented it, I thought that when using TOFU, it implicitly validates the domain as well, so TOFU alone would be enough in this case. Does that make sense, or do you think this assumption is wrong and that we also need to use the "regular" validation in these cases?

@oleonardolima oleonardolima moved this from Discussion to Needs Review in BDK Chain Feb 10, 2026
@oleonardolima oleonardolima added the api API breaking change label Feb 10, 2026
…ections

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.

This commit introduces the following features:

- `TofuStore` trait to manage certificates
- Implement TOFU validations both for OpenSSL and Rustls configurations
- Add custom certificate verifier for Rustls
- Support TOFU validation in proxy SSL connections
- Extend enum with error variants specifics to TOFU
- New method to initialize a client with TOFU from config
- Update existing constructors to support the revised SSL backend signatures
- Add some unit tests related to TOFU. The tests cover the following cases: first-use storage and certificate matching/replacement
- Add a TOFU certificates validation example, based on a in-memory store implementation for demonstration purposes

NOTE: Unit tests, supporting mock implementation, and the custom verifier for Rustls were created with AI assistance.
@lucasdbr05 lucasdbr05 force-pushed the feat/TOFU-certificate-validation branch from 42639ae to 3adefc6 Compare February 19, 2026 15:47
@lucasdbr05
Copy link
Author

lucasdbr05 commented Feb 19, 2026

@tnull I've squashed the previous commits into a single after the rebase to avoid having changes in the same files spread across diferent commits. However, if you prefer, I can split it in more than one commit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api API breaking change new feature New feature or request

Projects

Status: Needs Review

Development

Successfully merging this pull request may close these issues.

3 participants