20231129 attestation finalisation (kanidm#396)

* Changes for kanidm attestation

attestation-ca/src/lib.rs

@@ -6,7 +6,7 @@ use std::collections::BTreeMap;
 
 use uuid::Uuid;
 
-#[derive(Debug, Clone, Serialize, Deserialize)]
+#[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)]
 pub struct DeviceDescription {
     pub(crate) en: String,
     pub(crate) localised: BTreeMap<String, String>,
@@ -31,13 +31,14 @@ impl DeviceDescription {
 pub struct SerialisableAttestationCa {
     pub(crate) ca: Base64UrlSafeData,
     pub(crate) aaguids: BTreeMap<Uuid, DeviceDescription>,
+    pub(crate) blanket_allow: bool,
 }
 
 /// A structure representing an Attestation CA and other options associated to this CA.
 ///
 /// Generally depending on the Attestation CA in use, this can help determine properties
 /// of the authenticator that is in use.
-#[derive(Debug, Clone, Serialize, Deserialize)]
+#[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)]
 #[serde(
     try_from = "SerialisableAttestationCa",
     into = "SerialisableAttestationCa"
@@ -49,6 +50,7 @@ pub struct AttestationCa {
     /// attested as trusted by this CA. AAGUIDS that are not in this set, but signed by
     /// this CA will NOT be trusted.
     aaguids: BTreeMap<Uuid, DeviceDescription>,
+    blanket_allow: bool,
 }
 
 #[allow(clippy::from_over_into)]
@@ -57,6 +59,7 @@ impl Into<SerialisableAttestationCa> for AttestationCa {
         SerialisableAttestationCa {
             ca: Base64UrlSafeData(self.ca.to_der().expect("Invalid DER")),
             aaguids: self.aaguids,
+            blanket_allow: self.blanket_allow,
         }
     }
 }
@@ -68,6 +71,7 @@ impl TryFrom<SerialisableAttestationCa> for AttestationCa {
         Ok(AttestationCa {
             ca: x509::X509::from_der(&data.ca.0)?,
             aaguids: data.aaguids,
+            blanket_allow: data.blanket_allow,
         })
     }
 }
@@ -81,6 +85,10 @@ impl AttestationCa {
         &self.aaguids
     }
 
+    pub fn blanket_allow(&self) -> bool {
+        self.blanket_allow
+    }
+
     /// Retrieve the Key Identifier for this Attestation Ca
     pub fn get_kid(&self) -> Result<Vec<u8>, OpenSSLErrorStack> {
         self.ca
@@ -94,6 +102,7 @@ impl AttestationCa {
         desc_english: String,
         desc_localised: BTreeMap<String, String>,
     ) {
+        self.blanket_allow = false;
         self.aaguids.insert(
             aaguid,
             DeviceDescription {
@@ -107,15 +116,55 @@ impl AttestationCa {
         Ok(AttestationCa {
             ca: x509::X509::from_pem(data)?,
             aaguids: BTreeMap::default(),
+            blanket_allow: true,
         })
     }
+
+    fn union(&mut self, other: &Self) {
+        // if either is a blanket allow, we just do that.
+        if self.blanket_allow || other.blanket_allow {
+            self.blanket_allow = true;
+            self.aaguids.clear();
+            return;
+        } else {
+            self.blanket_allow = false;
+            for (o_aaguid, o_device) in other.aaguids.iter() {
+                // We can use the entry api here since o_aaguid is copy.
+                self.aaguids
+                    .entry(*o_aaguid)
+                    .or_insert_with(|| o_device.clone());
+            }
+        }
+    }
+
+    fn intersection(&mut self, other: &Self) {
+        // If they are a blanket allow, do nothing, we are already
+        // more restrictive, or we also are a blanket allow
+        if other.blanket_allow() {
+            // Do nothing
+            return;
+        } else if self.blanket_allow {
+            // Just set our aaguids to other, and remove our blanket allow.
+            self.blanket_allow = false;
+            self.aaguids = other.aaguids.clone();
+        } else {
+            // Only keep what is also in other.
+            self.aaguids
+                .retain(|s_aaguid, _| other.aaguids.contains_key(s_aaguid))
+        }
+    }
+
+    fn can_retain(&self) -> bool {
+        // Only retain a CA if it's a blanket allow, or has aaguids remaining.
+        self.blanket_allow || !self.aaguids.is_empty()
+    }
 }
 
 /// A list of AttestationCas and associated options.
-#[derive(Debug, Default, Clone, Serialize, Deserialize)]
+#[derive(Debug, Default, Clone, Serialize, Deserialize, PartialEq, Eq)]
 pub struct AttestationCaList {
     /// The set of CA's that we trust in this Operation
-    pub cas: BTreeMap<Base64UrlSafeData, AttestationCa>,
+    cas: BTreeMap<Base64UrlSafeData, AttestationCa>,
 }
 
 impl TryFrom<&[u8]> for AttestationCaList {
@@ -130,6 +179,18 @@ impl TryFrom<&[u8]> for AttestationCaList {
 }
 
 impl AttestationCaList {
+    pub fn cas(&self) -> &BTreeMap<Base64UrlSafeData, AttestationCa> {
+        &self.cas
+    }
+
+    pub fn clear(&mut self) {
+        self.cas.clear()
+    }
+
+    pub fn len(&self) -> usize {
+        self.cas.len()
+    }
+
     /// Determine if this attestation list contains any members.
     pub fn is_empty(&self) -> bool {
         self.cas.is_empty()
@@ -144,6 +205,38 @@ impl AttestationCaList {
         let att_ca_dgst = att_ca.get_kid()?;
         Ok(self.cas.insert(att_ca_dgst.into(), att_ca))
     }
+
+    /// Join two CA lists into one, taking all elements from both.
+    pub fn union(&mut self, other: &Self) {
+        for (o_kid, o_att_ca) in other.cas.iter() {
+            if let Some(s_att_ca) = self.cas.get_mut(o_kid) {
+                s_att_ca.union(o_att_ca)
+            } else {
+                self.cas.insert(o_kid.clone(), o_att_ca.clone());
+            }
+        }
+    }
+
+    /// Retain only the CA's and devices that exist in self and other.
+    pub fn intersection(&mut self, other: &Self) {
+        self.cas.retain(|s_kid, s_att_ca| {
+            // First, does this exist in our partner?
+            if let Some(o_att_ca) = other.cas.get(s_kid) {
+                // Now, intersect.
+                s_att_ca.intersection(o_att_ca);
+                if s_att_ca.can_retain() {
+                    // Still as elements, retain.
+                    true
+                } else {
+                    // Nothing remains, remove.
+                    false
+                }
+            } else {
+                // Not in other, remove.
+                false
+            }
+        })
+    }
 }
 
 #[derive(Default)]
@@ -173,6 +266,7 @@ impl AttestationCaListBuilder {
             AttestationCa {
                 ca,
                 aaguids: BTreeMap::default(),
+                blanket_allow: false,
             }
         };
 

webauthn-authenticator-rs/src/softtoken.rs

@@ -27,8 +27,8 @@ use base64urlsafedata::Base64UrlSafeData;
 
 use webauthn_rs_proto::{
     AllowCredentials, AuthenticationExtensionsClientOutputs, AuthenticatorAssertionResponseRaw,
-    AuthenticatorAttachment, AuthenticatorAttestationResponseRaw, PublicKeyCredential,
-    PublicKeyCredentialCreationOptions, PublicKeyCredentialRequestOptions,
+    AuthenticatorAttachment, AuthenticatorAttestationResponseRaw, AuthenticatorTransport,
+    PublicKeyCredential, PublicKeyCredentialCreationOptions, PublicKeyCredentialRequestOptions,
     RegisterPublicKeyCredential, RegistrationExtensionsClientOutputs, UserVerificationPolicy,
 };
 
@@ -39,7 +39,7 @@ pub struct SoftToken {
     #[serde(with = "PKeyPrivateDef")]
     _ca_key: pkey::PKey<pkey::Private>,
     #[serde(with = "X509Def")]
-    _ca_cert: X509,
+    ca_cert: X509,
     #[serde(with = "PKeyPrivateDef")]
     intermediate_key: pkey::PKey<pkey::Private>,
     #[serde(with = "X509Def")]
@@ -246,7 +246,7 @@ impl SoftToken {
             SoftToken {
                 // We could consider throwing these away?
                 _ca_key: ca_key,
-                _ca_cert: ca_cert,
+                ca_cert,
                 intermediate_key,
                 intermediate_cert,
                 tokens: HashMap::new(),
@@ -596,7 +596,7 @@ impl AuthenticatorBackendHashedClientData for SoftToken {
             response: AuthenticatorAttestationResponseRaw {
                 attestation_object: Base64UrlSafeData(ao_bytes),
                 client_data_json: Base64UrlSafeData(vec![]),
-                transports: None,
+                transports: Some(vec![AuthenticatorTransport::Internal]),
             },
             type_: "public-key".to_string(),
             extensions: RegistrationExtensionsClientOutputs::default(),

webauthn-rs-core/src/attestation.rs

@@ -362,6 +362,16 @@ pub enum AttestationFormat {
     None,
 }
 
+impl AttestationFormat {
+    /// Only a small number of devices correctly report their transports. These are
+    /// limited to attested devices, and exclusively packed (fido2) and tpms. Most
+    /// other devices/browsers will get this wrong, meaning that authentication will
+    /// fail or not offer the correct transports to the user.
+    pub(crate) fn transports_valid(&self) -> bool {
+        matches!(self, AttestationFormat::Packed | AttestationFormat::Tpm)
+    }
+}
+
 impl TryFrom<&str> for AttestationFormat {
     type Error = WebauthnError;
 
@@ -1242,7 +1252,7 @@ pub fn verify_attestation_ca_chain<'a>(
     danger_disable_certificate_time_checks: bool,
 ) -> Result<Option<&'a AttestationCa>, WebauthnError> {
     // If the ca_list is empty, Immediately fail since no valid attestation can be created.
-    if ca_list.cas.is_empty() {
+    if ca_list.cas().is_empty() {
         return Err(WebauthnError::AttestationCertificateTrustStoreEmpty);
     }
 
@@ -1288,7 +1298,7 @@ pub fn verify_attestation_ca_chain<'a>(
             .map_err(WebauthnError::OpenSSLError)?;
     }
 
-    for ca_crt in ca_list.cas.values() {
+    for ca_crt in ca_list.cas().values() {
         ca_store
             .add_cert(ca_crt.ca().clone())
             .map_err(WebauthnError::OpenSSLError)?;
@@ -1342,7 +1352,7 @@ pub fn verify_attestation_ca_chain<'a>(
     // attestation CA.
     res.and_then(|dgst| {
         ca_list
-            .cas
+            .cas()
             .get(dgst.as_ref())
             .ok_or_else(|| {
                 WebauthnError::AttestationChainNotTrusted("Invalid CA digest maps".to_string())

webauthn-rs-core/src/core.rs

@@ -602,10 +602,10 @@ impl WebauthnCore {
         debug!("attested_ca_crt = {:?}", attested_ca_crt);
 
         // Assert that the aaguid of the device, is within the authority of this CA (if
-        // a list of aaguids was provided).
+        // a list of aaguids was provided, and the ca blanket allows verification).
         if let Some(att_ca_crt) = attested_ca_crt {
-            if att_ca_crt.aaguids().is_empty() {
-                trace!("No aaguids set present, allowing all associated keys.");
+            if att_ca_crt.blanket_allow() {
+                trace!("CA allows all associated keys.");
             } else {
                 match &credential.attestation.metadata {
                     AttestationMetadata::Packed { aaguid }

webauthn-rs-core/src/interface.rs

@@ -284,20 +284,13 @@ impl Credential {
     /// be useful if you want to re-assert your credentials match an updated or changed
     /// ca_list from the time that registration occured. This can also be useful to
     /// re-determine certain properties of your device that may exist.
-    ///
-    /// # Safety
-    /// Due to the design of CA infrastructure by certain providers, it is NOT possible
-    /// to verify the CA expiry time. Certain vendors use CA intermediates that have
-    /// expiries that are only valid for approximately 10 minutes, meaning that if we
-    /// enforced time validity, these would false negative for their validity.
     pub fn verify_attestation<'a>(
         &'_ self,
         ca_list: &'a AttestationCaList,
     ) -> Result<Option<&'a AttestationCa>, WebauthnError> {
-        // Why do we disable this? Because of Apple. They issue dynamic short lived
-        // attestation certs, that last for about 5 minutes. This means that
-        // post-registration validation will always fail if we validate time.
-        let danger_disable_certificate_time_checks = true;
+        // Formerly we disabled this due to apple, but they no longer provide
+        // meaningful attesation so we can re-enable it.
+        let danger_disable_certificate_time_checks = false;
         verify_attestation_ca_chain(
             &self.attestation.data,
             ca_list,

webauthn-rs-core/src/internals.rs

@@ -126,7 +126,7 @@ impl Credential {
         req_extn: &RequestRegistrationExtensions,
         client_extn: &RegistrationExtensionsClientOutputs,
         attestation_format: AttestationFormat,
-        _transports: &Option<Vec<AuthenticatorTransport>>,
+        transports: &Option<Vec<AuthenticatorTransport>>,
     ) -> Self {
         let cred_protect = match (
             auth_data.extensions.cred_protect.as_ref(),
@@ -182,8 +182,11 @@ impl Credential {
         let backup_eligible = auth_data.backup_eligible;
         let backup_state = auth_data.backup_state;
 
-        // due to bugs in chrome, we have to disable transports because lol.
-        let transports = None;
+        let transports = if attestation_format.transports_valid() {
+            transports.clone()
+        } else {
+            None
+        };
 
         Credential {
             cred_id: acd.credential_id.clone(),