Skip to content

Server check clients use only hardware keys for fido2 factor - #3878

Open
filipslezaklab wants to merge 10 commits into
devfrom
fido2-update
Open

filipslezaklab wants to merge 10 commits into
devfrom
fido2-update

Conversation

@filipslezaklab

@filipslezaklab filipslezaklab commented Oct 16, 2026 •

Copy link
Copy Markdown
Contributor

Related: #2131
Related: #3773

@filipslezaklab filipslezaklab self-assigned this Oct 16, 2026
@filipslezaklab
filipslezaklab marked this pull request as ready for review October 16, 2026 10:23
fn deserialize_passkeys(bin_keys: &[Vec<u8>]) -> Vec<Passkey> {
bin_keys
.iter()
.map(|bin| serde_cbor::from_slice(bin).expect("Can't deserialize Passkey"))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't panic :)

/// Every root is scoped to its own AAGUIDs, never blanket allowed.
#[must_use]
pub fn trusted_attestation_cas() -> AttestationCaList {
trusted_cas().into_owned()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure this call warrants a dedicated function

..Default::default()
};
if verify_registered_fido2_assertion(
let verified = verify_client_fido2_assertion(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure what's gained by adding this variable

// Changes the process-wide cwd, so under plain `cargo test` it must not run alongside other
// tests that use relative paths.
#[test]
fn rotated_log_symlink_resolves_for_relative_path() {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
fn rotated_log_symlink_resolves_for_relative_path() {
fn test_rotated_log_symlink_resolves_for_relative_path() {

use super::*;

#[test]
fn bundled_list_parses() {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
fn bundled_list_parses() {
fn test_bundled_list_parses() {

use tonic::Status;

/// How enabling a factor treats the user's recovery codes.
pub(super) enum RecoveryCodes {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RecoveryCodesHandling or RecoveryCodesPolicy?
This does not actually contain any codes :)

}

/// Fetch the user's passkeys that passed hardware attestation at registration.
pub async fn hardware_passkeys_for_user(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is almost the same query as passkeys_for_user, so they could use a shared method with an optional kind argument.

}

/// Check whether a user has at least one security key that passed hardware attestation.
pub async fn hardware_exists_for_user<'e, E>(executor: E, user_id: Id) -> sqlx::Result<bool>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same situation as above with the duplicate query

}

let appender = RollingFileAppender::builder()
.rotation(Rotation::DAILY)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we are doing this then let's make rotation configurable and allow for setting limited retention: https://docs.rs/tracing-appender/latest/tracing_appender/rolling/struct.Builder.html#method.max_log_files

fs::remove_file(path)
.with_context(|| format!("Failed to remove log file symlink {path}"))?;
}
let file = OpenOptions::new()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verify the default permissions set by this are safe (preferrably 0600)

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants