Repository: google/OpenSK
File: src/ctap/data_formats.rs (Rust)

Code before:
    // Relying parties do not need to provide the credential ID in an allow_list if true.
    pub fn is_discoverable(&self) -> bool {
        self.cred_protect_policy.is_none()
            || self.cred_protect_policy
                == Some(CredentialProtectionPolicy::UserVerificationOptional)
    }
}

// TODO(kaczmarczyck) we could decide to split this data type up
// It depends on the algorithm though, I think.
// So before creating a mess, this is my workaround.
#[derive(Clone)]
#[cfg_attr(any(test, feature = "debug_ctap"), derive(Debug, PartialEq))]
pub struct CoseKey(pub BTreeMap<cbor::KeyType, cbor::Value>);

// This is the algorithm specifier that is supposed to be used in a COSE key
// map. The CTAP specification says -25 which represents ECDH-ES + HKDF-256
// here: https://www.iana.org/assignments/cose/cose.xhtml#algorithms
// In fact, this is just used for compatibility with older specification versions.
const ECDH_ALGORITHM: i64 = -25;
// This is the identifier used by OpenSSH. To be compatible, we accept both.
const ES256_ALGORITHM: i64 = -7;
const EC2_KEY_TYPE: i64 = 2;
const P_256_CURVE: i64 = 1;

impl TryFrom<cbor::Value> for CoseKey {
    type Error = Ctap2StatusCode;

    fn try_from(cbor_value: cbor::Value) -> Result<Self, Ctap2StatusCode> {
        if let cbor::Value::Map(cose_map) = cbor_value {
            Ok(CoseKey(cose_map))
        } else {
            Err(Ctap2StatusCode::CTAP2_ERR_VENDOR_INTERNAL_ERROR)
        }
    }
}

fn cose_key_from_bytes(x_bytes: [u8; ecdh::NBYTES], y_bytes: [u8; ecdh::NBYTES]) -> CoseKey {
    let x_byte_cbor: cbor::Value = cbor_bytes_lit!(&x_bytes);
    let y_byte_cbor: cbor::Value = cbor_bytes_lit!(&y_bytes);
    // TODO(kaczmarczyck) do not write optional parameters, spec is unclear
    let cose_cbor_value = cbor_map_options! {
        1 => EC2_KEY_TYPE,
        3 => ECDH_ALGORITHM,
        -1 => P_256_CURVE,
        -2 => x_byte_cbor,
        -3 => y_byte_cbor,
    };
    // Unwrap is safe here since we know it's a map.
    cose_cbor_value.try_into().unwrap()
}

Review comment (suggestion):
I don't think that having the `CoseKey` type publicly expose the underlying `BTreeMap` is a robust design, because anyone can construct a `CoseKey` from a `BTreeMap` which contains something else. Likewise, publicly exposing this `TryFrom` conversion allows anyone to construct a `CoseKey` from an arbitrary `BTreeMap` without any validation of the input.

All we want to expose publicly is the `impl From<ecdh::PubKey> for CoseKey`, as well as a read-only API to convert the `CoseKey` into a `cbor::Value` (without giving access to the underlying `BTreeMap`), i.e. an API that does the `cbor::Value::Map(CoseKey::from(pk).0)` conversion done elsewhere.

For now, I only see this `TryFrom` being used in `cose_key_from_bytes`, so It'd be better to rather have this code inlined there.

Code after:
}

impl PublicKeyCredentialSource {
    // Relying parties do not need to provide the credential ID in an allow_list if true.
    pub fn is_discoverable(&self) -> bool {
        self.cred_protect_policy.is_none()
            || self.cred_protect_policy
                == Some(CredentialProtectionPolicy::UserVerificationOptional)
    }
}

// The COSE key is used for both ECDH and ECDSA public keys for transmission.
#[derive(Clone)]
#[cfg_attr(any(test, feature = "debug_ctap"), derive(Debug, PartialEq))]
pub struct CoseKey {
    x_bytes: [u8; ecdh::NBYTES],
    y_bytes: [u8; ecdh::NBYTES],
    algorithm: i64,
}

impl CoseKey {
    // This is the algorithm specifier for ECDH.
    // CTAP requests -25 which represents ECDH-ES + HKDF-256 here:
    // https://www.iana.org/assignments/cose/cose.xhtml#algorithms
    const ECDH_ALGORITHM: i64 = -25;
    // The parameter behind map key 1.
    const EC2_KEY_TYPE: i64 = 2;
    // The parameter behind map key -1.
    const P_256_CURVE: i64 = 1;
}

// This conversion accepts both ECDH and ECDSA.
impl TryFrom<cbor::Value> for CoseKey {
    type Error = Ctap2StatusCode;

    fn try_from(cbor_value: cbor::Value) -> Result<Self, Ctap2StatusCode> {
        destructure_cbor_map! {
            let {
                // This is sorted correctly, negative encoding is bigger.
                1 => key_type,
                3 => algorithm,
                -1 => curve,
                -2 => x_bytes,
                -3 => y_bytes,
            } = extract_map(cbor_value)?;
        }

        let x_bytes = extract_byte_string(ok_or_missing(x_bytes)?)?;
        if x_bytes.len() != ecdh::NBYTES {
            return Err(Ctap2StatusCode::CTAP1_ERR_INVALID_PARAMETER);
        }