Support ECDSA_P521_SHA512 when using aws_lc_rs feature

[Alvenix]

Hope I didn't miss anything.

[djc]

Looks good to me!

[cpu]

I think the KeyPairAlgorithm enum in rustls-cert-gen could also use an update for the new alg.

[Alvenix]

I think the KeyPairAlgorithm enum in rustls-cert-gen could also use an update for the new alg.

Should I add aws_lc_rs feature to rustls-cert-gen crate?

[djc]

I think the KeyPairAlgorithm enum in rustls-cert-gen could also use an update for the new alg.

Should I add aws_lc_rs feature to rustls-cert-gen crate?

If necessary, yes!

[cpu]

rustls-cert-gen was extended with RSA key gen support, which is already tied to using aws-lc-rs. Wouldn't that have required piping through the feature or did I miss something?

[est31]

one doesn't have to pipe features through, one can also depend on rcgen directly, or do the cargo install --features rcgen/... trick. This can download additional dependencies not present in Cargo.toml. scary, right?

[est31]

note that in this instance, I think it makes sense to add features to rustls-cert-gen.

[Alvenix]

I think the KeyPairAlgorithm enum in rustls-cert-gen could also use an update for the new alg.

I added it in a separate commit to ease the review, I will squash it after the review is done.

[djc]

Looks okay to me modulo some nits.

[cpu]

Thanks! Looks good :-)

[cpu]

For what it's worth I was able to use the code in this branch over in rustls/rustls#1852 👍

[est31]

LGTM

[Alvenix]

I performed the requested changes, rebased to main, and squashed the two commits into a single commit.

[Alvenix]

I added additional commit for review to fix the CICD.

Mainly the failures are due rustls-cert-gen now requiring either ring or aws_lc_rs features which causes these failures.

I am not sure about the remaining failures. (if they are related to this PR and how to fix them)

[djc]

IIRC aws-lc-rs with FIPS is only supported on Linux for now.

[cpu]

IIRC aws-lc-rs with FIPS is only supported on Linux for now.

Agreed. There are additional FIPS build reqs documented here that include "Linux".

ci / Clippy (-p rcgen --no-default-features) (pull_request) Failing after 4m

I think this is a true-positive finding for this branch:

error: unused import: `super::*`
    --> rcgen/src/certificate.rs:1146:6
     |
1146 |     use super::*;
     |         ^^^^^^^^
     |
     = note: `-D unused-imports` implied by `-D warnings`
     = help: to override `-D warnings` add `#[allow(unused_imports)]`

[Alvenix]

I think this is a true-positive finding for this branch:

error: unused import: `super::*`
    --> rcgen/src/certificate.rs:1146:6
     |
1146 |     use super::*;
     |         ^^^^^^^^
     |
     = note: `-D unused-imports` implied by `-D warnings`
     = help: to override `-D warnings` add `#[allow(unused_imports)]`

I got the same warning on the main branch of this repo. That is why I think it is unrelated. Should I fix this warning in this pull request?

[djc]

I got the same warning on the main branch of this repo. That is why I think it is unrelated. Should I fix this warning in this pull request?

Either a separate commit in this PR or a separate PR would be great!

[cpu]

I got the same warning on the main branch of this repo. That is why I think it is unrelated.

Oh! My mistake, sorry about the confusion.

[cpu]

Is there anything holding back fixing the CI and rebasing so this could merge? I think we just need the non-Linux CI tasks to avoid the FIPS feature.

[Alvenix]

Is there anything holding back fixing the CI and rebasing so this could merge? I think we just need the non-Linux CI tasks to avoid the FIPS feature.

I removed FIPS step from non-Linux CI tasks.

In addition I rebased to main and squashed the two commits.

For quicker review, here is the second commit before it was squashed. These changes were made to fix the CICD.

[cpu]

Thanks!

[djc]

LGTM!

[djc]

Thanks for getting this in good shape!