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

Code before:
    fn from(entity: PublicKeyCredentialUserEntity) -> Self {
        cbor_map_options! {
            "id" => entity.user_id,
            "name" => entity.user_name,
            "displayName" => entity.user_display_name,
            "icon" => entity.user_icon,
        }
    }
}

// https://www.w3.org/TR/webauthn/#enumdef-publickeycredentialtype
#[derive(Clone, PartialEq)]
#[cfg_attr(any(test, feature = "debug_ctap"), derive(Debug))]
pub enum PublicKeyCredentialType {
    PublicKey,
    // This is the default for all strings not covered above.
    // Unknown types should be ignored, instead of returning errors.
    Unknown,
}

impl From<PublicKeyCredentialType> for cbor::Value {
    fn from(cred_type: PublicKeyCredentialType) -> Self {
        match cred_type {
            PublicKeyCredentialType::PublicKey => "public-key",
            // We should never create this credential type.
            PublicKeyCredentialType::Unknown => unreachable!(),
        }
        .into()
    }
}

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

    fn try_from(cbor_value: &cbor::Value) -> Result<Self, Ctap2StatusCode> {
        let cred_type_string = read_text_string(cbor_value)?;
        match &cred_type_string[..] {
            "public-key" => Ok(PublicKeyCredentialType::PublicKey),
            _ => Ok(PublicKeyCredentialType::Unknown),
        }
    }
}

// https://www.w3.org/TR/webauthn/#dictdef-publickeycredentialparameters
#[derive(PartialEq)]
#[cfg_attr(any(test, feature = "debug_ctap"), derive(Debug))]
pub struct PublicKeyCredentialParameter {
    pub cred_type: PublicKeyCredentialType,
    pub alg: SignatureAlgorithm,
}


Review comment (security):
Even though we indeed shouldn't create such unknown credential types, using `unreachable` here sounds like a code smell. It's only unreachable as long as no caller of this function passes the `Unknown` variant. Usually, `unreachable` can be checked locally within the current function.

So we should probably remove the `Unknown` variant of `PublicKeyCredentialType`, and either provide `impl TryFrom<&cbor::Value> for Option<PublicKeyCredentialType>`, or have another enum to layer that:

```rust
enum CredentialType {
    PublicKeyCredentialType,
    Unknown,
}
```

I'm actually surprised that the `PublicKeyCredentialType` enum contains `PublicKey` only. Shouldn't it be named `CredentialType` if it could in principle contain something else than `PublicKey`?

Code after:
    fn from(entity: PublicKeyCredentialUserEntity) -> Self {
        cbor_map_options! {
            "id" => entity.user_id,
            "name" => entity.user_name,
            "displayName" => entity.user_display_name,
            "icon" => entity.user_icon,
        }
    }
}

// https://www.w3.org/TR/webauthn/#enumdef-publickeycredentialtype
#[derive(Clone, PartialEq)]
#[cfg_attr(any(test, feature = "debug_ctap"), derive(Debug))]
pub enum PublicKeyCredentialType {
    PublicKey,
    // This is the default for all strings not covered above.
    // Unknown types should be ignored, instead of returning errors.
    Unknown,
}

impl From<PublicKeyCredentialType> for cbor::Value {
    fn from(cred_type: PublicKeyCredentialType) -> Self {
        match cred_type {
            PublicKeyCredentialType::PublicKey => "public-key",
            // We should never create this credential type.
            PublicKeyCredentialType::Unknown => "unknown",
        }
        .into()
    }
}

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

    fn try_from(cbor_value: &cbor::Value) -> Result<Self, Ctap2StatusCode> {
        let cred_type_string = read_text_string(cbor_value)?;
        match &cred_type_string[..] {
            "public-key" => Ok(PublicKeyCredentialType::PublicKey),
            _ => Ok(PublicKeyCredentialType::Unknown),
        }
    }
}

// https://www.w3.org/TR/webauthn/#dictdef-publickeycredentialparameters
#[derive(PartialEq)]
#[cfg_attr(any(test, feature = "debug_ctap"), derive(Debug))]
pub struct PublicKeyCredentialParameter {
    pub cred_type: PublicKeyCredentialType,
    pub alg: SignatureAlgorithm,
}