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

feat(transport): added support for EC keys #1145

Merged
merged 2 commits into from
Feb 13, 2023

Conversation

YuJuncen
Copy link
Contributor

@YuJuncen YuJuncen commented Nov 9, 2022

Signed-off-by: Yu Juncen [email protected]

Motivation

This PR tries to fix #1143. Which caused tonic reports error when using EC keys as client keys.

Solution

The solution has been described in the issue. Copy & paste it here:

By reading the code, I have noticed that we are going to extract key via applying pkcs8_private_keys and rsa_private_keys. It seems rustls-pemfile supports SEC1 EC keys however doesn't provide something like ec_private_keys (!).
Perhaps we can use read_once direcly, so we only need to iterate the key file once and can extract the EC keys:

fn load_rustls_private_key(
    mut cursor: std::io::Cursor<&[u8]>,
) -> Result<PrivateKey, crate::Error> {
    while let Ok(Some(item)) = rustls_pemfile::read_one(&mut cursor) {
        match item {
            rustls_pemfile::Item::RSAKey(key)
            | rustls_pemfile::Item::PKCS8Key(key)
            | rustls_pemfile::Item::ECKey(key) => return Ok(PrivateKey(key)),
            _ => continue,
        }
    }

    // Otherwise we have a Private Key parsing problem
    Err(Box::new(TlsError::PrivateKeyParseError))
}

@YuJuncen YuJuncen changed the title fix: added support to EC keys fix: added support for EC keys Nov 9, 2022
@@ -188,21 +188,15 @@ mod rustls_keys {
use crate::transport::service::tls::TlsError;
use crate::transport::Identity;

fn load_rustls_private_key(
pub(crate) fn load_rustls_private_key(
Copy link
Member

@LucioFranco LucioFranco Nov 9, 2022

Choose a reason for hiding this comment

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

Why pub(crate), I don't see this used outside of the crate or am I going crazy?

Copy link
Contributor Author

@YuJuncen YuJuncen Nov 10, 2022

Choose a reason for hiding this comment

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

I want to use it in the tests module, if I remove this rustc would blame me load_rustls_private_key is a private function... Are there some better solutions?

Well, I guess pub(super) is what we need?

@LucioFranco LucioFranco changed the title fix: added support for EC keys feat(transport): added support for EC keys Feb 13, 2023
@LucioFranco LucioFranco added this pull request to the merge queue Feb 13, 2023
Merged via the queue into hyperium:master with commit 17d6a4b Feb 13, 2023
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.

bug?: cannot extract client key from pem file if it encoded by SEC1 EC
2 participants