Repository navigation
Test item MakeCredentialCredParamsTest #88
Description
Activity
make_credential_cred_params needs to be looked at.
In principle, for algorithm agility, we don't want authenticators blowing up when seeing an unknown /unsupported alg.
YK ignores unsupported algs so we need to look at why this test is failing.
We may need to clarify the test in CTAP2.1 as I can see that the "PublicKeyCredentialParameters' algorithm identifiers are values that SHOULD be registered in the IANA COSE Algorithms registry" that was intended for RP might be misinterpreted by authenticators as a reason to reject unknown values.
From what I can make of it I think the intent of the test is correct.
Looking at the test https://github.com/google/CTAP2-test-tool/blob/master/src/tests/make_credential.cc#L359
It is the unknown type that is causing errors not the unknown alg.
I am guessing most authenticators will reject type values that "public-key".
We should clarify in CTAP 2.1 if unknown type and unknown algs MUST both be ignored when processing pubKeyCredParams.
It is the unknown type that is causing errors not the unknown alg.
How do you know? In the test, the invalid
PublicKeyCredentialParameterssequence element contains both an invalidalgvalue and an invalidtypevalue, and the authenticator returns an error that does not indicate which of thePublicKeyCredentialParametersinstance's members caused the error.If we changed the test code's L359 to have
cbor::Value("public-key")rather thancbor::Value("non-existing type")(leaving the invalidalgvalue ofcbor::Value(-1)unaltered), we could see whether the invalidalgvalue causes a misbehaving authnr to return the error, which it should not do because the test'sPublicKeyCredentialParameterssequence does contain one validPublicKeyCredentialParametersinstance.See also: https://github.com/fido-alliance/fido-2-specs/issues/1113#issuecomment-731836851
For the sake of debugging, I could split this test into 2. Both with 1 valid entry, and an invalid entry with only
typeoralgbeing unknown.However, the question still stands if we want both. Since an authenticator's behaviour shouldn't depend on knowing the current IANA COSE Algorithms registry, I don't see why it should be different.
I think this test makes authenticators more compatible with future versions. Or, to put it differently: Why even have an enum with 1 variant if we can't extend it?
Why even have an enum with 1 variant if we can't extend it?
Well, IIUC, the relevant CTAP makeCred step is implicitly saying to ignore invalid values, i.e., if there's at least one
PublicKeyCredentialParametersinstance containing a valid COSEAlgorithmIdentifier value and validtypevalue ("public-key"). Please see https://github.com/fido-alliance/fido-2-specs/issues/1113#issuecomment-731836851 for a suggestion regarding how we might clarify this in the CTAP2.1 spec.For the sake of debugging, I could split this test into 2. Both with 1 valid entry, and an invalid entry with only type or alg being unknown.
Sure, that'd aid in determining what in particular an improperly-operating authenticator is getting wrong.
Since an authenticator's behaviour shouldn't depend on knowing the current IANA COSE Algorithms registry, I don't see why it should be different.
IIUC, what you mean is that an authnr ought to be considering a given
PublicKeyCredentialParametersinstance as invalid if it contains analgmember whose value is an alg that the authenticator does not implement, rather than whether the alg is a valid alg as defined by the IANA COSE Algorithms registry ...?If so, I agree.
Though, for the purposes picking an
algvalue that presumably an authnr would not support, picking one from the IANA COSE Algorithms registry (as you have done) that is "unassigned" seems nominally reasonable, although it could become assigned and a future authnr could conceivably implement it. Perhaps it would be slightly better to pick0since it is denoted as "reserved" and perhaps is less likely to be assigned someday?It is the unknown type that is causing errors not the unknown alg.
How do you know? In the test, the invalid
PublicKeyCredentialParameterssequence element contains both an invalidalgvalue and an invalidtypevalue, and the authenticator returns an error that does not indicate which of thePublicKeyCredentialParametersinstance's members caused the error.I checked our code.
We get the RSA alg which is unknown for the YK CTAP2.1_Pre keys from the MS RP and we have successfully ignored it.
I know the YK will error on any type value that is not"public-key".If we changed the test code's L359 to have
cbor::Value("public-key")rather thancbor::Value("non-existing type")(leaving the invalidalgvalue ofcbor::Value(-1)unaltered), we could see whether the invalidalgvalue causes a misbehaving authnr to return the error, which it should not do because the test'sPublicKeyCredentialParameterssequence does contain one validPublicKeyCredentialParametersinstance.0 is probably a safe alg for testing.
My point is webauthn spec says "Currently one credential type is defined, namely "public-key", link.
And I agree authenticator must ignore unknown IANA COSE Algorithms registry and I think this is a common way most authenticators implement.Supplement:
I prefer the way to have the positive response (if it's still valid) than to have error end/response. I think this behavior on some authenticators is to comply fido2-conformance-module test item to not allow non-"public-key" case.@nuno0529 So your point is that a
typedifferent frompublic-keyshould not be ignored? The point against this behaviour is what I meant withWhy even have an enum with 1 variant if we can't extend it?
If later protocol versions introduce a different type and make that valid, your security key would stop working whenever this new
typeis in the list?I prefer the way to have the positive response (if it's still valid) than to have error end/response. I think this behavior on some authenticators is to comply fido2-conformance-module test item to not allow non-"public-key" case.
My general approach is to try to constrain allowed behaviour more tightly. I think this has a long-term benefit. And you can pass both tools with one implementation.
@kaczmarczyck I support your point that a
typedifferent frompublic-keyshould be ignored for more compatible with future versions. So I think the fido-2-specs, PR#1124 is a good change and we should also push this change into fido2-conformance-tool.
test item: MakeCredentialCredParamsTest
{
"description": "Tests entries in the credential parameters list.",
"error_message": "Falsely rejected cred params list with 1 good and 1 bad element.",
"id": "make_credential_cred_params",
"observations": [
"A prompt was expected, but not performed. Sometimes it is just not recognized if performed too fast.",
"The failing error code is
CTAP2_ERR_UNSUPPORTED_ALGORITHM."],
"result": "fail",
"tags": []
},
Webauthn specs says below in https://www.w3.org/TR/webauthn-2/#dom-publickeycredentialdescriptor-type
ctap2 spec has description for pubKeyCredParams(0x04) of authenticatorMakeCredential's command
So authenticator should not accept type="non-existing type" with the alg value of COSE.