Skip to content

Commit

Permalink
fix #310 + revert other "fixes"
Browse files Browse the repository at this point in the history 
Turns out the testbench failures are due to the testbench
being out of date. OpenSK is correct, the tests are wrong.

See discussions here:

google/OpenSK#586
google/OpenSK#584
google/OpenSK#582

We'll await updates to the test tool and fold those into CI,
and leave the current failures standing.
  • Loading branch information
@bunnie
bunnie committed Feb 7, 2023
1 parent 279b2f7 commit 4fa4bec
Show file tree
Hide file tree
Showing 2 changed files with 5 additions and 60 deletions.
9 changes: 1 addition & 8 deletions apps/vault/src/ctap/data_formats.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -926,15 +926,8 @@ impl TryFrom<cbor::Value> for PinUvAuthProtocol {
match extract_unsigned(cbor_value)? {
1 => Ok(PinUvAuthProtocol::V1),
2 => Ok(PinUvAuthProtocol::V2),
/* from this test:
https://github.com/google/CTAP2-test-tool/blob/1afd50bd1b700dc86d6dffeca70a331771d24e45/src/tests/make_credential.cc#L503-L515
Test successful: Tests the response on an empty PIN auth without a PIN in MakeCredential.
Test successful: Tests if the PIN protocol parameter is checked in MakeCredential.
Expected error code `CTAP2_ERR_PIN_AUTH_INVALID`, got `CTAP1_ERR_INVALID_PARAMETER`.
*/
_ => {
Err(Ctap2StatusCode::CTAP2_ERR_PIN_AUTH_INVALID)
Err(Ctap2StatusCode::CTAP1_ERR_INVALID_PARAMETER)
},
}
}
Expand Down
56 changes: 4 additions & 52 deletions apps/vault/src/ctap/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -822,31 +822,7 @@ impl CtapState {
if storage::has_always_uv(env)? {
return Err(Ctap2StatusCode::CTAP2_ERR_PUAT_REQUIRED);
}
// Corresponds to makeCredUvNotRqd set to true.
/*
Should fail regardless of options.rk setting, because of MakeCredentialPinAuthMissingParameterTest()
https://github.com/google/CTAP2-test-tool/blob/1afd50bd1b700dc86d6dffeca70a331771d24e45/src/tests/make_credential.cc#L593-L609
Testbench sends the following options:
AuthenticatorMakeCredential(
AuthenticatorMakeCredentialParameters {
client_data_hash: [205, 205, 205, 205, 205, 205, 205, 205, 205, 205, 205, 205, 205, 205, 205, 205, 205, 205, 205, 205, 205, 205, 205, 205, 205, 205, 205, 205, 205, 205, 205, 205],
rp: PublicKeyCredentialRpEntity { rp_id: "make_credential_pin_auth_missing_parameter.example.com", rp_name: None, rp_icon: None },
user: PublicKeyCredentialUserEntity { user_id: [29, 29, 29, 29, 29, 29, 29, 29, 29, 29, 29, 29, 29, 29, 29, 29, 29, 29, 29, 29, 29, 29, 29, 29, 29, 29, 29, 29, 29, 29, 29, 29],
user_name: Some("Adam"),
user_display_name: None,
user_icon: None
},
pub_key_cred_params: [PublicKeyCredentialParameter { cred_type: PublicKey, alg: Es256 }],
exclude_list: None,
extensions: MakeCredentialExtensions { hmac_secret: false, cred_protect: None, min_pin_length: false, cred_blob: None, large_blob_key: None },
options: MakeCredentialOptions { rk: false, uv: false },
pin_uv_auth_param: None, pin_uv_auth_protocol: None, enterprise_attestation: None }
)
Note that options.rk = false.
(Original code had `if options.rk && storage::pin_hash(env)?.is_some() {` in the line below)
*/
if storage::pin_hash(env)?.is_some() {
if options.rk && storage::pin_hash(env)?.is_some() {
return Err(Ctap2StatusCode::CTAP2_ERR_PUAT_REQUIRED);
}
0x00
Expand Down Expand Up @@ -1212,36 +1188,12 @@ impl CtapState {
&pin_uv_auth_param,
pin_uv_auth_protocol.ok_or(Ctap2StatusCode::CTAP2_ERR_MISSING_PARAMETER)?,
)?;
// Failing test: https://github.com/google/CTAP2-test-tool/blob/1afd50bd1b700dc86d6dffeca70a331771d24e45/src/tests/get_assertion.cc#L253-L280
// This check can "never" pass, because the `permission` is consumed after a user presence is
// requested (`self.client_pin.clear_token_flags()` right after the permission is requested).
//
// The specific sequence that is observed in the failing test is:
// - SetPinToken is called to set a PIN
// - GetPinToken is called, which sets a default permission allowing the next operation
// - A test credential is created, requiring user presence. This consumes the permission
// previously granted by GetPinToken
// - GetAssertion is requested with `GetAssertionOptions { up: true, uv: true }` and
// pin_uv_auth_param: Some()..., which triggers the commented out tests below.
//
// Thus, either it is the case that the checks below are incorrect, or, it is mandatory
// to do a GetPinToken immediately prior to getting an assertion. Alternatively, it is
// "incorrect" to consume permissions after the user presence test. Looking back at the `stable`
// branch for OpenSK, it seems that perhaps the reason this test passed before is because the
// permissions are not consumed by the user presence test (there is no `self.client_pin.clear_token_flags();`
// after the user presence test). Thus an alternative hypothesis is that there is a bug
// in the clear_token_flags() code that clears too many permissions.
//
// For now, we're going to go with skipping this test; removing it does not cause any other test to fail,
// and I don't understand the life cycle of the permission state machine well enough to determine
// if the clear_token_flags() should be modified.
//
// self.client_pin
// .has_permission(PinPermission::GetAssertion)?;
self.client_pin
.has_permission(PinPermission::GetAssertion)?;
// Checking for the UV flag is specified earlier for GetAssertion.
// Error codes are identical though, so the implementation can be identical with
// MakeCredential.
// self.client_pin.check_user_verified_flag()?;
self.client_pin.check_user_verified_flag()?;
self.client_pin.ensure_rp_id_permission(&rp_id)?;
UV_FLAG
}
Expand Down

0 comments on commit 4fa4bec

Please sign in to comment.