Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pre-flight and AppID improvements #297

Merged
merged 5 commits into from
Aug 24, 2023
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 34 additions & 19 deletions examples/ctap2.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,13 +10,13 @@ use authenticator::{
crypto::COSEAlgorithm,
ctap2::server::{
PublicKeyCredentialDescriptor, PublicKeyCredentialParameters, RelyingParty,
ResidentKeyRequirement, Transport, User, UserVerificationRequirement,
RelyingPartyWrapper, ResidentKeyRequirement, Transport, User, UserVerificationRequirement,
},
statecallback::StateCallback,
Pin, StatusPinUv, StatusUpdate,
};
use getopts::Options;
use sha2::{Digest, Sha256};
use rand::{thread_rng, RngCore};
use std::sync::mpsc::{channel, RecvError};
use std::{env, thread};

Expand All @@ -31,6 +31,9 @@ fn main() {
let args: Vec<String> = env::args().collect();
let program = args[0].clone();

let rp_id = "example.com".to_string();
let app_id = "https://fido.example.com/myApp".to_string();

let mut opts = Options::new();
opts.optflag("x", "no-u2f-usb-hid", "do not enable u2f-usb-hid platforms");
opts.optflag("h", "help", "print this help menu").optopt(
Expand All @@ -39,6 +42,11 @@ fn main() {
"timeout in seconds",
"SEC",
);
opts.optflag(
"a",
"app_id",
&format!("Using App ID {app_id} from origin 'https://{rp_id}'"),
);
opts.optflag("s", "hmac_secret", "With hmac-secret");
opts.optflag("h", "help", "print this help menu");
opts.optflag("f", "fallback", "Use CTAP1 fallback implementation");
Expand Down Expand Up @@ -73,14 +81,8 @@ fn main() {
};

println!("Asking a security key to register now...");
let challenge_str = format!(
"{}{}",
r#"{"challenge": "1vQ9mxionq0ngCnjD-wTsv1zUSrGRtFqG2xP09SbZ70","#,
r#" "version": "U2F_V2", "appId": "http://example.com"}"#
);
let mut challenge = Sha256::new();
challenge.update(challenge_str.as_bytes());
let chall_bytes: [u8; 32] = challenge.finalize().into();
let mut chall_bytes = [0u8; 32];
thread_rng().fill_bytes(&mut chall_bytes);

let (status_tx, status_rx) = channel::<StatusUpdate>();
thread::spawn(move || loop {
Expand Down Expand Up @@ -146,14 +148,19 @@ fn main() {
name: Some("A. User".to_string()),
display_name: None,
};
let origin = "https://example.com".to_string();
// If we're testing AppID support, then register with an RP ID that isn't valid for the origin.
let relying_party = RelyingParty {
id: if matches.opt_present("app_id") {
app_id.clone()
} else {
rp_id.clone()
},
name: None,
};
let ctap_args = RegisterArgs {
client_data_hash: chall_bytes,
relying_party: RelyingParty {
id: "example.com".to_string(),
name: None,
},
origin: origin.clone(),
relying_party,
origin: format!("https://{rp_id}"),
user,
pub_cred_params: vec![
PublicKeyCredentialParameters {
Expand Down Expand Up @@ -226,10 +233,11 @@ fn main() {
allow_list = Vec::new();
}

let alternate_rp_id = matches.opt_present("app_id").then(|| app_id.clone());
let ctap_args = SignArgs {
client_data_hash: chall_bytes,
origin,
relying_party_id: "example.com".to_string(),
origin: format!("https://{rp_id}"),
relying_party_id: rp_id,
allow_list,
user_verification_req: UserVerificationRequirement::Preferred,
user_presence_req: true,
Expand All @@ -248,7 +256,7 @@ fn main() {
},
},
pin: None,
alternate_rp_id: None,
alternate_rp_id: alternate_rp_id.clone(),
use_ctap1_fallback: fallback,
};

Expand All @@ -270,6 +278,13 @@ fn main() {
match sign_result {
Ok(assertion_object) => {
println!("Assertion Object: {assertion_object:?}");
if let Some(alt_rp_id) = alternate_rp_id {
assert_eq!(
assertion_object.0[0].auth_data.rp_id_hash,
RelyingPartyWrapper::from(alt_rp_id.as_str()).hash()
);
println!("Used AppID");
}
println!("Done.");
break;
}
Expand Down
2 changes: 1 addition & 1 deletion examples/interactive_management.rs
Original file line number Diff line number Diff line change
Expand Up @@ -472,7 +472,7 @@ fn ask_user_bio_options(
.expect("error: unable to read user input");
let name = input.trim().to_string();
tx.send(InteractiveRequest::BioEnrollment(
BioEnrollmentCmd::ChangeName(chosen_id, name.clone()),
BioEnrollmentCmd::ChangeName(chosen_id, name),
puat,
))
.expect("Failed to send GetEnrollments request.");
Expand Down
23 changes: 4 additions & 19 deletions src/ctap2/commands/get_assertion.rs
Original file line number Diff line number Diff line change
Expand Up @@ -168,9 +168,6 @@ pub struct GetAssertion {
pub extensions: GetAssertionExtensions,
pub options: GetAssertionOptions,
pub pin_uv_auth_param: Option<PinUvAuthParam>,

// This is used to implement the FIDO AppID extension.
pub alternate_rp_id: Option<String>,
}

impl GetAssertion {
Expand All @@ -180,7 +177,6 @@ impl GetAssertion {
allow_list: Vec<PublicKeyCredentialDescriptor>,
options: GetAssertionOptions,
extensions: GetAssertionExtensions,
alternate_rp_id: Option<String>,
) -> Self {
Self {
client_data_hash,
Expand All @@ -189,7 +185,6 @@ impl GetAssertion {
extensions,
options,
pin_uv_auth_param: None,
alternate_rp_id,
}
}
}
Expand Down Expand Up @@ -636,7 +631,6 @@ pub mod test {
user_verification: None,
},
Default::default(),
None,
);
let mut device = Device::new("commands/get_assertion").unwrap();
assert_eq!(device.get_protocol(), FidoProtocol::CTAP2);
Expand Down Expand Up @@ -835,7 +829,6 @@ pub mod test {
user_verification: None,
},
Default::default(),
None,
);
let mut device = Device::new("commands/get_assertion").unwrap(); // not really used (all functions ignore it)
// channel id
Expand Down Expand Up @@ -925,7 +918,6 @@ pub mod test {
user_verification: None,
},
Default::default(),
None,
);

let mut device = Device::new("commands/get_assertion").unwrap(); // not really used (all functions ignore it)
Expand Down Expand Up @@ -1106,7 +1098,6 @@ pub mod test {
user_verification: None,
},
Default::default(),
None,
);
let mut device = Device::new("commands/get_assertion").unwrap();
assert_eq!(device.get_protocol(), FidoProtocol::CTAP2);
Expand Down Expand Up @@ -1155,7 +1146,7 @@ pub mod test {

// Sending first GetAssertion with first allow_list-entry, that will return an error
let mut msg = cid.to_vec();
msg.extend(vec![HIDCmd::Cbor.into(), 0x00, 0x94]);
msg.extend(vec![HIDCmd::Cbor.into(), 0x00, 0x90]);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not a huge fan of "this is the magic length byte (I assume?) that you have to know to modify if you change anything below", but I think that's out of scope for this change.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Agreed, I filed #298.

msg.extend(vec![0x2]); // u2f command
msg.extend(vec![
0xa4, // map(4)
Expand Down Expand Up @@ -1191,10 +1182,7 @@ pub mod test {
0x6a, // text(10)
0x70, 0x75, 0x62, 0x6C, 0x69, 0x63, 0x2D, 0x6B, 0x65, 0x79, // public-key
0x5, // options
0xa2, // map(2)
0x62, // text(2)
0x75, 0x76, // uv
0xf4, // false
0xa1, // map(1)
0x62, // text(2)
0x75, 0x70, // up
0xf4, // false
Expand All @@ -1210,7 +1198,7 @@ pub mod test {

// Sending second GetAssertion with first allow_list-entry, that will return a success
let mut msg = cid.to_vec();
msg.extend(vec![HIDCmd::Cbor.into(), 0x00, 0x94]);
msg.extend(vec![HIDCmd::Cbor.into(), 0x00, 0x90]);
msg.extend(vec![0x2]); // u2f command
msg.extend(vec![
0xa4, // map(4)
Expand Down Expand Up @@ -1246,10 +1234,7 @@ pub mod test {
0x6a, // text(10)
0x70, 0x75, 0x62, 0x6C, 0x69, 0x63, 0x2D, 0x6B, 0x65, 0x79, // public-key
0x5, // options
0xa2, // map(2)
0x62, // text(2)
0x75, 0x76, // uv
0xf4, // false
0xa1, // map(1)
0x62, // text(2)
0x75, 0x70, // up
0xf4, // false
Expand Down
47 changes: 26 additions & 21 deletions src/ctap2/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -32,9 +32,10 @@ use crate::ctap2::commands::{
};
use crate::ctap2::preflight::{
do_credential_list_filtering_ctap1, do_credential_list_filtering_ctap2,
silently_discover_credentials,
};
use crate::ctap2::server::{
RelyingParty, RelyingPartyWrapper, ResidentKeyRequirement, UserVerificationRequirement,
RelyingPartyWrapper, ResidentKeyRequirement, UserVerificationRequirement,
};
use crate::errors::{AuthenticatorError, UnsupportedOption};
use crate::statecallback::StateCallback;
Expand Down Expand Up @@ -531,19 +532,35 @@ pub fn sign<Dev: FidoDevice>(
}
}

let mut allow_list = args.allow_list;
let mut rp_id = args.relying_party_id;
let client_data_hash = ClientDataHash(args.client_data_hash);
if let Some(alt_rp_id) = args.alternate_rp_id {
if !allow_list.is_empty() {
// Try to silently discover U2F credentials that require the FIDO App ID extension. If
// any are found, we should use the alternate RP ID instead of the provided RP ID.
let silent_creds = silently_discover_credentials(
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we do this here again? Didn't we already do this in StateMachine::sign()?
Couldn't we reuse the silent_creds-list we acquired in StateMachine, and simply overwrite the args.allow_list and args.relying_party_id there, before handing it in?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

StateMachine is only used by USB HID devices, and I intend to write virtual device tests for this.

dev,
&allow_list,
&RelyingPartyWrapper::from(alt_rp_id.as_str()),
&client_data_hash,
);
if !silent_creds.is_empty() {
allow_list = silent_creds;
rp_id = alt_rp_id;
}
}
}

let mut get_assertion = GetAssertion::new(
ClientDataHash(args.client_data_hash),
RelyingPartyWrapper::Data(RelyingParty {
id: args.relying_party_id,
name: None,
}),
args.allow_list,
client_data_hash,
RelyingPartyWrapper::from(rp_id.as_str()),
allow_list,
GetAssertionOptions {
user_presence: Some(args.user_presence_req),
user_verification: None,
},
args.extensions,
args.alternate_rp_id,
);

let mut skip_uv = false;
Expand Down Expand Up @@ -622,19 +639,7 @@ pub fn sign<Dev: FidoDevice>(
debug!("{get_assertion:?} using {pin_uv_auth_result:?}");
debug!("------------------------------------------------------------------");
send_status(&status, crate::StatusUpdate::PresenceRequired);
let mut resp = dev.send_msg_cancellable(&get_assertion, alive);
if resp.is_err() {
// Retry with a different RP ID if one was supplied. This is intended to be
// used with the AppID provided in the WebAuthn FIDO AppID extension.
if let Some(alternate_rp_id) = get_assertion.alternate_rp_id {
get_assertion.rp = RelyingPartyWrapper::Data(RelyingParty {
id: alternate_rp_id,
..Default::default()
});
get_assertion.alternate_rp_id = None;
resp = dev.send_msg_cancellable(&get_assertion, alive);
}
}
let resp = dev.send_msg_cancellable(&get_assertion, alive);
match resp {
Ok(result) => {
callback.call(Ok(result));
Expand Down
32 changes: 20 additions & 12 deletions src/ctap2/preflight.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ use crate::crypto::PinUvAuthToken;
use crate::ctap2::server::{PublicKeyCredentialDescriptor, RelyingPartyWrapper};
use crate::errors::AuthenticatorError;
use crate::transport::errors::{ApduErrorStatus, HIDError};
use crate::transport::{FidoDevice, VirtualFidoDevice};
use crate::transport::{FidoDevice, FidoProtocol, VirtualFidoDevice};
use crate::u2ftypes::CTAP1RequestAPDU;
use sha2::{Digest, Sha256};

Expand Down Expand Up @@ -144,11 +144,6 @@ pub(crate) fn do_credential_list_filtering_ctap2<Dev: FidoDevice>(
}
}

// Step 1.2: Return early, if we only have one chunk anyways
if cred_list.len() <= chunk_size {
return Ok(cred_list);
}

let chunked_list = cred_list.chunks(chunk_size);

// Step 2: If we have more than one chunk: Loop over all, doing GetAssertion
Expand All @@ -160,15 +155,10 @@ pub(crate) fn do_credential_list_filtering_ctap2<Dev: FidoDevice>(
rp.clone(),
chunk.to_vec(),
GetAssertionOptions {
user_verification: if pin_uv_auth_token.is_some() {
None
} else {
Some(false)
},
user_verification: None, // defaults to Some(false) if puap is absent
user_presence: Some(false),
},
GetAssertionExtensions::default(),
None,
);
silent_assert.set_pin_uv_auth_param(pin_uv_auth_token.clone())?;
let res = dev.send_msg(&silent_assert);
Expand Down Expand Up @@ -200,3 +190,21 @@ pub(crate) fn do_credential_list_filtering_ctap2<Dev: FidoDevice>(
// MakeCredential or a Success in case of GetAssertion
Ok(final_list)
}

pub(crate) fn silently_discover_credentials<Dev: FidoDevice>(
dev: &mut Dev,
cred_list: &[PublicKeyCredentialDescriptor],
rp: &RelyingPartyWrapper,
client_data_hash: &ClientDataHash,
) -> Vec<PublicKeyCredentialDescriptor> {
if dev.get_protocol() == FidoProtocol::CTAP2 {
if let Ok(cred_list) = do_credential_list_filtering_ctap2(dev, cred_list, rp, None) {
return cred_list;
}
} else if let Some(key_handle) =
do_credential_list_filtering_ctap1(dev, cred_list, rp, client_data_hash)
{
return vec![key_handle];
}
vec![]
}
9 changes: 9 additions & 0 deletions src/ctap2/server.rs
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,15 @@ pub enum RelyingPartyWrapper {
Hash(RpIdHash),
}

impl From<&str> for RelyingPartyWrapper {
fn from(rp_id: &str) -> Self {
Self::Data(RelyingParty {
id: rp_id.to_string(),
name: None,
})
}
}

impl RelyingPartyWrapper {
pub fn hash(&self) -> RpIdHash {
match *self {
Expand Down
2 changes: 1 addition & 1 deletion src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -46,8 +46,8 @@ pub use ctap2::commands::client_pin::{Pin, PinError};
pub use ctap2::commands::credential_management::CredentialManagementResult;
pub use ctap2::commands::get_assertion::{Assertion, GetAssertionResult};
pub use ctap2::commands::get_info::AuthenticatorInfo;
use serde::Serialize;
pub use ctap2::commands::make_credentials::MakeCredentialsResult;
use serde::Serialize;
pub use statemachine::StateMachine;
pub use status_update::{
BioEnrollmentCmd, CredManagementCmd, InteractiveRequest, InteractiveUpdate, StatusPinUv,
Expand Down
Loading