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

Update to hyper dependency to version 1 #50

Merged
merged 15 commits into from
Jun 27, 2024

Conversation

kellpossible
Copy link
Contributor

No description provided.

src/types.rs Outdated Show resolved Hide resolved
Copy link
Owner

@djc djc left a comment

Choose a reason for hiding this comment

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

Thanks for working on this! I'm afraid this will need some effort to get to a mergeable state.

Cargo.toml Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
@kellpossible
Copy link
Contributor Author

@djc thanks for the review!

Just wondering there's a failed audit check due to license of a subdependency:

https://github.com/instant-labs/instant-acme/actions/runs/9630966408/job/26563174365#step:4:92

This is due to the enabled webpki-roots feature on hyper-rustls which is being relied upon to allow an infallible Default implementation for DefaultClient

What would you like to do about this? We could switch to a fallible replacement for Default perhaps instead? (Like a try_new() method)

src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
@djc
Copy link
Owner

djc commented Jun 24, 2024

What would you like to do about this? We could switch to a fallible replacement for Default perhaps instead? (Like a try_new() method)

Hmm, we should probably start using the rustls-platform-verifier crate instead.

@djc
Copy link
Owner

djc commented Jun 24, 2024

It's okay to bump the MSRV to what we need here. If you rebase on top of #52 that should fix the audit issue.

@kellpossible
Copy link
Contributor Author

It's okay to bump the MSRV to what we need here. If you rebase on top of #52 that should fix the audit issue.

I've merged in that change (anticipating this MR will be merged in squash anyway?)

If the license check with webpki-roots included now passes due to these changes, can we consider this #50 (comment) resolved too, without needing to use rustls-platform-verifier yet? Perhaps can be done in next MR?

@kellpossible
Copy link
Contributor Author

It's okay to bump the MSRV to what we need here. If you rebase on top of #52 that should fix the audit issue.

@djc I bumped the MSRV to 1.64

src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/types.rs Outdated Show resolved Hide resolved
src/types.rs Outdated Show resolved Hide resolved
src/types.rs Outdated Show resolved Hide resolved
src/types.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
@djc
Copy link
Owner

djc commented Jun 25, 2024

If the license check with webpki-roots included now passes due to these changes, can we consider this #50 (comment) resolved too, without needing to use rustls-platform-verifier yet? Perhaps can be done in next MR?

No, I don't want to merge a dependency on webpki-roots. Instead you can make the HttpClient constructor fallible.

@kellpossible
Copy link
Contributor Author

@djc thanks again! I think I may have addressed all the outstanding comments?

Copy link
Owner

@djc djc left a comment

Choose a reason for hiding this comment

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

This is looking good, a few more small things to address.

Can you also bump the version to 0.6.0?

src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/types.rs Show resolved Hide resolved
src/types.rs Show resolved Hide resolved
@djc
Copy link
Owner

djc commented Jun 26, 2024

Also need to address the CI issues:

  • Bump the MSRV in the CI workflow to 1.64
  • Add BSD-3-Clause to the allowed licenses in deny.toml
  • Fix the clippy lint

@kellpossible
Copy link
Contributor Author

@djc done!

@djc djc merged commit efb83b4 into djc:main Jun 27, 2024
7 checks passed
@djc
Copy link
Owner

djc commented Jun 27, 2024

Thanks for your patience working through my feedback!

  • Published instant-acme v0.6.0 at registry crates-io
  • [new tag] 0.6.0 -> 0.6.0

@gahooa
Copy link

gahooa commented Jun 27, 2024

@djc and @kellpossible, just wanted to thank you both for getting this done. We are in the middle of building management features for around 10,000 certificates, and being able to work with great maintainers and developers is so crucial. Thank you again!

@djc
Copy link
Owner

djc commented Jun 27, 2024

Always curious to get a bit more context about how/where you're using instant-acme!

@gahooa
Copy link

gahooa commented Jun 28, 2024

@djc I can give a brief overview here, and if you'd like more details, reach out to me at appcove.

We have several clients who operate saas type systems for thousands and thousands of businesses. These business oftentimes bring their own domains, or pick a subdomain, and some of them generated a multitude of new sites on a daily basis.

There are some great tools out there for automatic TLS issuance, like the caddy webserver. The issue with such tools is that while they fully automate the process, they provide little central insight or control into how it's going. Also, we do not like the idea of giving each web-server potentially destructive DNS API access in order to respond to DNS challenges. This calls for something more centralized.

We are building a central infrastructure management solution with TLS certificate audits/issuance/distribution being one aspect. It will control when and how certificates are renewed, be able to audit that, report on issues, and so on and so forth. Then on the webserver side, we are working to ensure that the ALPN verification challenges can be successfully answered in real-time by communicating with the central management system.

This is the area that instant-acme helps us -- to be the workhorse of issuance and renewals across a number of providers, both with DNS and ALPN, and to provide strongly typed rust APIs for accessing both the challenge information and the issued certificates, so they can both be appropriately distributed to the respective web server clusters.

That's it in a nutshell. We appreciate your efforts to make this possible.

@djc djc mentioned this pull request Jul 16, 2024
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.

3 participants