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.

Sign up for GitHub

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

Jump to bottom

Add aws_lc_rs feature and set it as default #56

Merged
merged 9 commits into from
Jul 16, 2024
Merged

Add aws_lc_rs feature and set it as default #56

merged 9 commits into from
Jul 16, 2024

Conversation

SajjadPourali
Copy link
Contributor

This PR adds the aws_lc_rs feature to the instant-acme library, sets it as the default feature, and updates the dependencies.

@SajjadPourali
Copy link
Contributor Author

SajjadPourali commented Jul 14, 2024 •
edited
Loading

cargo test --all-features --all-targets causes enabling both ring and aws_lc_rs.
To fix the test error, I disabled the test when both ring and aws_lc_rs features are enabled. The TLS client cannot choose a default provider when both are enabled.

See https://github.com/rustls/rustls/blob/main/rustls/src/crypto/mod.rs#L270-L283

@djc
Copy link
Owner

djc commented Jul 14, 2024

I'm happy to add a feature for using aws-lc-rs, but I want to stick with ring as the default for now and please avoid any changes unrelated to this (such as TOML reformatting and version bumps to other dependencies).

djc
djc reviewed Jul 15, 2024
View reviewed changes
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.

That looks a lot better. There are still a few small unrelated changes that I'd prefer to avoid.

src/lib.rs Outdated
@@ -13,14 +17,13 @@ use http_body_util::{BodyExt, Full};
use hyper::body::{Bytes, Incoming};
use hyper::header::{CONTENT_TYPE, LOCATION};
use hyper::{Method, Request, Response, StatusCode};
use hyper_util::client::legacy::{connect::Connect, Client as HyperClient};
Copy link
Owner

Choose a reason for hiding this comment

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

I prefer to stick to one line per module (so keeping the separate lines for Connect and Client, as before).

src/types.rs Outdated
use rustls_pki_types::CertificateDer;
use serde::de::DeserializeOwned;
use serde::ser::SerializeMap;
use serde::{Deserialize, Serialize};
use std::fmt;
Copy link
Owner

Choose a reason for hiding this comment

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

This should remain in a separate block of imports by itself, as before.

Cargo.toml Outdated
@@ -29,7 +33,7 @@ thiserror = "1.0.30"
[dev-dependencies]
anyhow = "1.0.66"
clap = { version = "4.0.29", features = ["derive"] }
rcgen = "0.12"
rcgen = { version = "0.12", features = ["pem"], default-features = false }
Copy link
Owner

Choose a reason for hiding this comment

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

As elsewhere, please keep default-features = false before features = [].

djc
djc reviewed Jul 15, 2024
View reviewed changes
Cargo.toml Outdated
default = ["hyper-rustls"]
default = ["hyper-rustls", "ring"]
ring = ["dep:ring", "hyper-rustls?/ring", "rcgen/ring"]
aws_lc_rs = ["dep:aws-lc-rs", "hyper-rustls?/aws-lc-rs", "rcgen/aws_lc_rs"]
Copy link
Owner

Choose a reason for hiding this comment

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

Would prefer to name the feature with hyphens instead of underscores.

@djc
Copy link
Owner

djc commented Jul 15, 2024

CI is failing because --all-features now enables fips, which only works on Linux. Probably makes sense to change CI to do the following:

  • check --no-default-features
  • test --features hyper-rustls,aws-lc-rs
  • test --features hyper-rustls,ring

@SajjadPourali
Copy link
Contributor Author

CI is failing because --all-features now enables fips, which only works on Linux. Probably makes sense to change CI to do the following:

* `check --no-default-features`

* `test --features hyper-rustls,aws-lc-rs`

* `test --features hyper-rustls,ring`

The suggested commands don’t work because ring is in the default feature.
What about the following?

  • cargo check
  • cargo test
  • cargo test --no-default-features --features hyper-rustls,aws_lc_rs

@djc
Copy link
Owner

djc commented Jul 15, 2024

Ah, sorry, that sounds good!

@SajjadPourali
Copy link
Contributor Author

SajjadPourali commented Jul 15, 2024 •
edited
Loading

@djc Apparently, on Windows, NASM is a dependency for aws_lc_rs. Any idea?
I just added AWS_LC_SYS_NO_ASM: 1 env variable to fix it.

@SajjadPourali
Copy link
Contributor Author

SajjadPourali commented Jul 15, 2024 •
edited
Loading

@djc Done. Is there anything else that should be applied?
Out of curiosity, is there any particular reason to maintain the current version of the dependencies, as they don’t introduce any breaking changes?

djc
djc approved these changes Jul 16, 2024
View reviewed changes
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 a good start. Going to merge this and follow up in a separate PR.

@djc djc merged commit a9c8fc4 into djc:main Jul 16, 2024
7 checks passed
@djc djc mentioned this pull request Jul 16, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@djc djc djc approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@SajjadPourali @djc