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

P-521 curves in WebCrypto #13449

Open
littledivy opened this issue Jan 21, 2022 · 7 comments
Open

P-521 curves in WebCrypto #13449

littledivy opened this issue Jan 21, 2022 · 7 comments
Assignees
Labels
ext/crypto related to the ext/crypto crate feat new feature (which has been agreed to/accepted) upstream Changes in upstream are required to solve these issues

Comments

@littledivy
Copy link
Member

littledivy commented Jan 21, 2022

Support for p521 curves for ECDSA and ECDH on all relevant operations. Ideally when its part of RustCrypto https://github.com/RustCrypto/elliptic-curves

@littledivy littledivy added upstream Changes in upstream are required to solve these issues ext/crypto related to the ext/crypto crate feat new feature (which has been agreed to/accepted) labels Jan 21, 2022
@littledivy littledivy changed the title P-512 curves in WebCrypto P-521 curves in WebCrypto Jun 1, 2022
@diachedelic
Copy link
Contributor

It appears that P-521 is implemented by Mundane, a Rust cryptography library by Google: https://github.com/google/mundane. Would that be considered as an alternative to RustCrypto?

@diachedelic
Copy link
Contributor

And this is the relevant issue for RustCrypto: RustCrypto/elliptic-curves#114.

@bartlomieju
Copy link
Member

It appears that P-521 is implemented by Mundane, a Rust cryptography library by Google: https://github.com/google/mundane. Would that be considered as an alternative to RustCrypto?

At this point it's unlikely that we'll switch to another crypto library.

@mrmikardo
Copy link

Has there been any update on this issue since last year? It looks like P-521 curves are now supported by the Rust crypto library: https://github.com/RustCrypto/elliptic-curves#crates

Although it does look like some of these are slightly experimental, at this point: RustCrypto/elliptic-curves#114 (comment)

@dojyorin
Copy link

dojyorin commented Jan 5, 2024

P-521 has been implemented upstream (v0.13.3~), I think WebCrypto P-521 implementation has become possible.

@littledivy littledivy self-assigned this Jan 5, 2024
@dojyorin
Copy link

dojyorin commented Jan 5, 2024

I'm not familiar with Rust, so it's incomplete, but I feel like it would work if I specifically added code like this.

  • /ext/crypto/shared.rs
// L132
pub fn as_ec_public_key_p521(&self) -> Result<p521::EncodedPoint, AnyError> {
  match self {
    V8RawKeyData::Public(data) => {
      // public_key is a serialized EncodedPoint
      p521::EncodedPoint::from_bytes(data)
        .map_err(|_| type_error("expected valid public EC key"))
    }
    V8RawKeyData::Private(data) => {
      let signing_key = p521::SecretKey::from_pkcs8_der(data)
        .map_err(|_| type_error("expected valid private EC key"))?;
      Ok(signing_key.public_key().to_encoded_point(false))
    }
    // Should never reach here.
    V8RawKeyData::Secret(_) => unreachable!(),
  }
}
  • /ext/crypto/lib.rs
// L522
CryptoNamedCurve::P521 => {
  let secret_key = p521::SecretKey::from_pkcs8_der(&args.key.data)
    .map_err(|_| type_error("Unexpected error decoding private key"))?;

  let public_key = match public_key.r#type {
    KeyType::Private => {
      p521::SecretKey::from_pkcs8_der(&public_key.data)
        .map_err(|_| {
          type_error("Unexpected error decoding private key")
        })?
        .public_key()
    }
    KeyType::Public => {
      let point = p521::EncodedPoint::from_bytes(public_key.data)
        .map_err(|_| {
          type_error("Unexpected error decoding private key")
        })?;

      let pk = p521::PublicKey::from_encoded_point(&point);
      // pk is a constant time Option.
      if pk.is_some().into() {
        pk.unwrap()
      } else {
        return Err(type_error(
          "Unexpected error decoding private key",
        ));
      }
    }
    _ => unreachable!(),
  };

  let shared_secret = p521::elliptic_curve::ecdh::diffie_hellman(
    secret_key.to_nonzero_scalar(),
    public_key.as_affine(),
  );

  // raw serialized x-coordinate of the computed point
  Ok(shared_secret.raw_secret_bytes().to_vec().into())
}
  • /ext/crypto/key.rs
// L38
#[serde(rename = "P-521")]
P521,
// L45
CryptoNamedCurve::P521 => &ring::agreement::ECDH_P521,
// L58
CryptoNamedCurve::P521 => {
  &ring::signature::ECDSA_P521_SHA512_FIXED_SIGNING
}
// L67
CryptoNamedCurve::P521 => &ring::signature::ECDSA_P521_SHA512_FIXED,
  • /ext/crypto/import_key.rs
// L523
EcNamedCurve::P521 => {
  let x = decode_b64url_to_field_bytes::<p521::NistP521>(&x)?;
  let y = decode_b64url_to_field_bytes::<p521::NistP521>(&y)?;

  p521::EncodedPoint::from_affine_coordinates(&x, &y, false).to_bytes()
}
// L559
EcNamedCurve::P521 => {
  let d = decode_b64url_to_field_bytes::<p521::NistP521>(&d)?;
  let pk = p521::SecretKey::from_bytes(&d)?;

  pk.to_pkcs8_der()
    .map_err(|_| data_error("invalid JWK private key"))?
}
// L568
EcNamedCurve::P521 => CryptoNamedCurve::P521.into(),
// L632
EcNamedCurve::P521 => {
  // 1-2.
  let point = p521::EncodedPoint::from_bytes(&data)
    .map_err(|_| data_error("invalid P-521 elliptic curve point"))?;
  // 3.
  if point.is_identity() {
    return Err(data_error("invalid P-521 elliptic curve point"));
  }
}
// L642
EcNamedCurve::P256 | EcNamedCurve::P384 | EcNamedCurve::P521 => {
// L672
EcNamedCurve::P521 => CryptoNamedCurve::P521.into(),
// L758
EcNamedCurve::P521 => {
  let point =
    p521::EncodedPoint::from_bytes(&*encoded_key).map_err(|_| {
      data_error("invalid P-521 elliptic curve SPKI data")
    })?;

  if point.is_identity() {
    return Err(data_error("invalid P-521 elliptic curve point"));
  }

  point.as_bytes().len()
}
  • /ext/crypto/generate_key.rs
// L91
EcNamedCurve::P521 => &ring::signature::ECDSA_P521_SHA512_FIXED_SIGNING,
  • /ext/crypto/export_key.rs
// L256
EcNamedCurve::P521 => {
  let point = key_data.as_ec_public_key_p521()?;

  point.as_ref().to_vec()
}
// L274
EcNamedCurve::P521 => {
  let point = key_data.as_ec_public_key_p521()?;

  point.as_ref().to_vec()
}
// L288
EcNamedCurve::P521 => AlgorithmIdentifierOwned {
  oid: elliptic_curve::ALGORITHM_OID,
  parameters: Some((&p521::NistP521::OID).into()),
}
// L354
EcNamedCurve::P521 => {
  let point = key_data.as_ec_public_key_p521()?;
  let coords = point.coordinates();

  if let p521::elliptic_curve::sec1::Coordinates::Uncompressed { x, y } =
    coords
  {
    Ok(ExportKeyResult::JwkPublicEc {
      x: bytes_to_b64(x),
      y: bytes_to_b64(y),
    })
  } else {
    Err(custom_error(
      "DOMExceptionOperationError",
      "failed to decode public key",
    ))
  }
}
// L405
EcNamedCurve::P521 => {
  let ec_key =
    p521::SecretKey::from_pkcs8_der(private_key).map_err(|_| {
      custom_error(
        "DOMExceptionOperationError",
        "failed to decode private key",
      )
    })?;

  let point = ec_key.public_key().to_encoded_point(false);
  if let elliptic_curve::sec1::Coordinates::Uncompressed { x, y } =
    point.coordinates()
  {
    Ok(ExportKeyResult::JwkPrivateEc {
      x: bytes_to_b64(x),
      y: bytes_to_b64(y),
      d: bytes_to_b64(&ec_key.to_bytes()),
    })
  } else {
    Err(data_error("expected valid public EC key"))
  }
}
  • /ext/crypto/00_crypto.js
// L67
const supportedNamedCurves = ["P-256", "P-384", "P-521"];
// L861
||
(key[_algorithm].namedCurve === "P-521" &&
  hashAlgorithm !== "SHA-512")
// L1349
||
(key[_algorithm].namedCurve === "P-521" && hash !== "SHA-512")
  • /ext/crypto/Cargo.toml
# L33
p521 = "0.13.8"

@littledivy
Copy link
Member Author

Thanks @dojyorin

Some parts of the WebCrypto implementation use ring which does not yet support P521 IIRC. (In the code snippet for generate_key.rs: ring::signature::ECDSA_P521_SHA512_FIXED_SIGNING does not exist)

I've currently started working on moving some parts to the p521 crate and eventually plan to move everything to the RustCrypto group of crates.

littledivy added a commit that referenced this issue Jan 6, 2024
…rtKey` (#21815)

Part 1 of a potential 3 part series. Ref #13449 

The current implementation passes key material back and forth RustCrypto
group of crates and ring. ring does not implement p521 yet.

This PR adds support for P521 named curve in `generateKey` and
`importKey` where we use RustCrypto. Other parts should be moved over to
the RustGroup group of crates for consistency.
bartlomieju pushed a commit that referenced this issue Jan 12, 2024
…rtKey` (#21815)

Part 1 of a potential 3 part series. Ref #13449 

The current implementation passes key material back and forth RustCrypto
group of crates and ring. ring does not implement p521 yet.

This PR adds support for P521 named curve in `generateKey` and
`importKey` where we use RustCrypto. Other parts should be moved over to
the RustGroup group of crates for consistency.
wiktor-k added a commit to wiktor-k/ssh-sig that referenced this issue Mar 6, 2024
Sadly Deno doesn't support it.

See: denoland/deno#13449 (comment)
wiktor-k added a commit to wiktor-k/ssh-sig that referenced this issue Mar 7, 2024
Sadly Deno doesn't support it.

See: denoland/deno#13449 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ext/crypto related to the ext/crypto crate feat new feature (which has been agreed to/accepted) upstream Changes in upstream are required to solve these issues
Projects
None yet
Development

No branches or pull requests

5 participants