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

Version bumps #5

Merged
merged 5 commits into from
Oct 24, 2023
Merged

Version bumps #5

merged 5 commits into from
Oct 24, 2023

Conversation

okjodom
Copy link
Collaborator

@okjodom okjodom commented Sep 21, 2023

@okjodom okjodom changed the title Version bump Version bumps Sep 21, 2023
@okjodom

This comment was marked as resolved.

@Kixunil
Copy link

Kixunil commented Oct 13, 2023

Oh, crap, removing config is terrible.

@okjodom okjodom requested a review from Kixunil October 21, 2023 00:16
@okjodom
Copy link
Collaborator Author

okjodom commented Oct 21, 2023

@sr-gi I believe we cracked it! Please take a look at the latest iteration in PR 🥳

@okjodom
Copy link
Collaborator Author

okjodom commented Oct 21, 2023

Additional concerns with this version bump:

  • Regarding cert verification, I went with a simple check to see if provided cert is an end entity, but I feel like we should have more robust verification.
  • Should we redefine msrv?

cc @Kixunil

@okjodom okjodom marked this pull request as ready for review October 23, 2023 16:51
Copy link

@sr-gi sr-gi left a comment

Choose a reason for hiding this comment

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

tACK 0a13823

I cannot believe that just packing the Client and Interceptor in an InterceptedService service did the trick. So happy :DD

src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Show resolved Hide resolved
Copy link

@sr-gi sr-gi left a comment

Choose a reason for hiding this comment

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

reACK 90637b1

src/lib.rs Outdated
}
}

if let Some(last) = self.certs.last() {
if *last != end_entity.0 {
Copy link

Choose a reason for hiding this comment

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

This is the only part I'm not 100% sure about, it would be good to double-check for a cert that is signed by letsencrypt

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've dropped this custom validation after checking against letsencrypt certs and seeing failures. My tests were inconclusive though. Some of the failures seemed expected, i.e when I passed a certificate that was not provided by server to client

Maybe it's best to consider the whole custom validation changes as follow up improvement

Copy link

Choose a reason for hiding this comment

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

How did you manage to create the letsencytp cert for LND? I'll give it a go tomorrow and if we cannot get far we can just leave it as it was and tackle it down the line

Copy link
Collaborator Author

@okjodom okjodom Oct 24, 2023

Choose a reason for hiding this comment

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

Let's test it together tomorrow

@dpc
Copy link
Collaborator

dpc commented Oct 23, 2023

I'm a bit out of context here, but on my level of understanding LGTM. Please ping me once you want to land and there are no outstanding issues, if you need someone to approve.

Copy link
Collaborator

@douglaz douglaz left a comment

Choose a reason for hiding this comment

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

Perhaps we need to review the cert verification in future but LGTM for now

Tonic v0.7 introduced a breaking change removing direct tonic transport configuration with rustls::ClientConfig, reason being that tonic needs to avoid version lock with rustls, i.e changes in rustls APIs should not break tonic.
The recommended workaround is to directly use hyper and tonic, avoiding tonic::transport:Channel within the InterceptedService, as applied in this commit
tonic-build v0.10.0 codegen produces types that use TryInto trait. To avail this in scope, we bump rust edition to 2021, thus including TryInto trait in rust prelude
@okjodom okjodom requested review from sr-gi, douglaz and dpc October 23, 2023 23:26
@justinmoon justinmoon merged commit f8b1889 into fedimint:master Oct 24, 2023
@sr-gi
Copy link

sr-gi commented Oct 24, 2023

For context, I think we could create our own custom certificate verifier based on: briansmith/webpki#127, which allows self-signed certs, and then pass it to rustls via with_custom_certificate_verifier

@sr-gi
Copy link

sr-gi commented Oct 24, 2023

For context, I think we could create our own custom certificate verifier based on: briansmith/webpki#127, which allows self-signed certs, and then pass it to rustls via with_custom_certificate_verifier

I think this may actually be harder than I though. It requires copying a lot of functionality from webpky given most of it is not public. I think we should, at least, verify that the end_entity cert is within the ones presented.

okjodom added a commit to okjodom/tonic_lnd that referenced this pull request Oct 24, 2023
- release includes changes introduced by fedimint#5
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.

6 participants