support EdDSA signing key

[xshadowlegendx]

hello will this project support EdDSA signing key? I can help contribute as well If anyone could guide me on how to contribute to the project, which file to edit and stuff

[ramosbugs]

Hey @xshadowlegendx,

I assume we're talking about the EdDSA alg value defined in RFC 8037? Everything in this crate should be based on a published standard.

To add support for verifying EdDSA signatures, the changes needed are roughly:

1. Add an algorithm variant for EdDSA to CoreJwsSigningAlgorithm. This is a #[non_exhaustive] struct, so adding a new variant is not a breaking change. I'd suggest naming the variant Eddsa to follow the current naming convention, but the serde value should be EdDSA as defined in the RFC.
2. Add an OKP variant to CoreJsonWebKeyType. I'd suggest naming this OctetKeyPair with OKP as the serde value defined in the RFC.
3. Add Ed25519 and/or Ed448 variants to CoreJsonCurveType. This will support the JWK's crv field.
4. Handle the new CoreJwsSigningAlgorithm::EdDSA variant in CoreJsonWebKey's implementation of JsonWebKey::verify_signature.
5. Add tests similar to test_core_jwk_deserialization_ec, test_ecdsa_verification.
6. Optionally add a CoreJsonWebKey::new_okp method similar to new_ec that accepts x, crv, and kid parameters.

If you also want to add support for generating EdDSA signatures (optional, currently implemented for RSA and HMAC but not ECDSA), you'll want to follow the example of CoreRsaPrivateSigningKey.

Hopefully that helps!

[xshadowlegendx]

hello @ramosbugs thanks, yes it is about EdDSA alg value, I will go check out the code and make a pr when adding the support

[xshadowlegendx]

hello @ramosbugs If I add Ed25519 and/or Ed448 variants to CoreJsonCurveType, I will have to update ec_public_key and verify_ec_signature to extract the public key part x of Ed25519 and crv and verify the message without adding new functions correct?

[ramosbugs]

I would add separate functions since the parameters are different (i.e., ECDSA keys use both x and y parameters)

[xshadowlegendx]

hello @ramosbugs, btw is this expected? I think the signature supposed to base64 decoded first? the test ran fine but the error should not be what the error test is expected

[xshadowlegendx]

i also added the CoreEdDsaPrivateSigningKey and all tests have passed but have not yet push the commit, below is the code, however there is unused code warning which I noted below using comment, so what do you think about this

pub enum EdDsaVariant {
    Ed25519, # unused warning
}

struct EdDsaSigningKey {
    variant: EdDsaVariant,
    ed25519: Option<ed25519_dalek::SigningKey>,
}

impl EdDsaSigningKey {
    # this method unused warning
    fn from_pem(pem: &str, variant: EdDsaVariant) -> Result<Self, String> {
        match variant {
            EdDsaVariant::Ed25519 => Ok(Self {
                variant,
                ed25519: Some(ed25519_dalek::SigningKey::from_pkcs8_pem(pem).map_err(|err| err.to_string())?),
            })
        }
    }

    fn public_part(&self) -> Vec<u8> {
        match self.variant {
            EdDsaVariant::Ed25519 => {
                self.ed25519
                    .as_ref()
                    .expect("fail to refer to Ed25519 signing key")
                    .verifying_key()
                    .as_bytes()
                    .to_vec()
            }
        }
    }

    fn sign(&self, message: &[u8]) -> Vec<u8> {
        match self.variant {
            EdDsaVariant::Ed25519 => {
                let key = self.ed25519.as_ref().expect("fail to refer to Ed25519 signing key");

                let signature = key.sign(message);

                signature.to_vec()
            }
        }
    }
}

pub struct CoreEdDsaPrivateSigningKey {
    kid: Option<JsonWebKeyId>,
    key_pair: EdDsaSigningKey,
}
impl CoreEdDsaPrivateSigningKey {
    # this method unused warning
    pub fn from_pem(pem: &str, kid: Option<JsonWebKeyId>, variant: EdDsaVariant) -> Result<Self, String> {
        match variant {
            EdDsaVariant::Ed25519 => Ok(Self {
                kid,
                key_pair: EdDsaSigningKey::from_pem(pem, variant)?,
            })
        }
    }
}
impl
    PrivateSigningKey<
    CoreJwsSigningAlgorithm,
    CoreJsonWebKeyType,
    CoreJsonWebKeyUse,
    CoreJsonWebKey,
    > for CoreEdDsaPrivateSigningKey
{
    fn sign(&self, signature_alg: &CoreJwsSigningAlgorithm, message: &[u8]) -> Result<Vec<u8>, SigningError> {
        match *signature_alg {
            CoreJwsSigningAlgorithm::EdDsaEd25519 => {
                Ok(self.key_pair.sign(message))
            },
            ref other => Err(SigningError::UnsupportedAlg(
                serde_plain::to_string(other).unwrap_or_else(|err| {
                    panic!(
                        "signature alg {:?} failed to serialize to a string: {}",
                        other, err
                    )
                }),
            )),
        }
    }

    fn as_verification_key(&self) -> CoreJsonWebKey {
        CoreJsonWebKey {
            kty: CoreJsonWebKeyType::OctetKeyPair,
            use_: Some(CoreJsonWebKeyUse::Signature),
            kid: self.kid.clone(),
            n: None,
            e: None,
            crv: Some(CoreJsonCurveType::Ed25519),
            x: Some(Base64UrlEncodedBytes::new(self.key_pair.public_part())),
            y: None,
            d: None,
            k: None,
        }
    }
}

tests

const TEST_ED25519_KEY: &str =
      "\
      -----BEGIN PRIVATE KEY-----\n\
      MC4CAQAwBQYDK2VwBCIEICWeYPLxoZKHZlQ6rkBi11E9JwchynXtljATLqym/XS9\n\
      -----END PRIVATE KEY-----\
      ";

fn expect_ed_sig(
    private_key: &CoreEdDsaPrivateSigningKey,
    message: &[u8],
    alg: &CoreJwsSigningAlgorithm,
    expected_sig_base64: &str
) {
    let sig = private_key.sign(alg, message).unwrap();
    assert_eq!(expected_sig_base64, base64::encode(&sig));

    let public_key = private_key.as_verification_key();
    public_key.verify_signature(alg, message, &sig).unwrap();
}

#[test]
fn test_ed_signing() {
    let private_key = CoreEdDsaPrivateSigningKey::from_pem(
        TEST_ED25519_KEY,
        Some(JsonWebKeyId::new("test_key".to_string())),
        EdDsaVariant::Ed25519,
    )
    .unwrap();

    let public_key_jwk = private_key.as_verification_key();
    let public_key_jwk_str = serde_json::to_string(&public_key_jwk).unwrap();

    assert_eq!(
        "{\
        \"kty\":\"OKP\",\
        \"use\":\"sig\",\
        \"kid\":\"test_key\",\
        \"crv\":\"Ed25519\",\
        \"x\":\"E6lXdyel1n9C1lcr3FK8OsfsfO2ZgcWhPflJ6yIf7e8\"\
        }",
        public_key_jwk_str
    );

    let message = "hello EdDsa".as_ref();
    expect_ed_sig(
        &private_key,
        message,
        &CoreJwsSigningAlgorithm::EdDsaEd25519,
        "XqP8sXaPrQa37+2lw+aiXv+6pegjioYUgo1/ShcX6kRhD2Vxh8DrQUbQlaGbljLJTNNc453E2Axp+Mxm+4OVAQ=="
    );

    assert_eq!(
        private_key.sign(&CoreJwsSigningAlgorithm::HmacSha256, message),
        Err(SigningError::UnsupportedAlg("HS256".to_string())),
    );

    assert_eq!(
        private_key.sign(&CoreJwsSigningAlgorithm::None, message),
        Err(SigningError::UnsupportedAlg("none".to_string())),
    );
}

[ramosbugs]

hello @ramosbugs, btw is this expected? I think the signature supposed to base64 decoded first? the test ran fine but the error should not be what the error test is expected

wow nice catch! I'll push a fix.

[ramosbugs]

i also added the CoreEdDsaPrivateSigningKey and all tests have passed but have not yet push the commit, below is the code, however there is unused code warning which I noted below using comment, so what do you think about this

That's surprising because you're calling CoreEdDsaPrivateSigningKey::from_pem in the test, so it shouldn't be unused. Try pushing the commit to the PR, and I'll take a closer look if the issue persists.

[xshadowlegendx]

ok got it

[ramosbugs]

I'm still not sure why the warning is happening despite it being used in a test (possibly due to pub without being re-exported from the private jwt module?), but it should go away once you add a pub use for EdDsaVariant and CoreEdDsaPrivateSigningKey here: https://github.com/ramosbugs/openidconnect-rs/blob/54dc7ca14c08db7595f60533215cc4694bd8c3e7/src/core/mod.rs#L35

I haven't done a full review yet since it's still in draft state, but please let me know when it's ready for comments.

[ramosbugs]

hello @ramosbugs, btw is this expected? I think the signature supposed to base64 decoded first? the test ran fine but the error should not be what the error test is expected

wow nice catch! I'll push a fix.

Fixed in 7b6fdd06ee4f728d7e4d40d256cccc5002178cb1

[xshadowlegendx]

hello @ramosbugs, the warning went away now when add pub use, it will be marked ready when I push the commit

[ramosbugs]

Implemented in #130