Test item MakeCredentialCredParamsTest

[nuno0529]

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

client platforms MUST ignore any PublicKeyCredentialDescriptor with an unknown type.

ctap2 spec has description for pubKeyCredParams(0x04) of authenticatorMakeCredential's command

PublicKeyCredentialParameters' algorithm identifiers are values that SHOULD be registered in the IANA COSE Algorithms registry

So authenticator should not accept type="non-existing type" with the alg value of COSE.

[ve7jtb]

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.

[ve7jtb]

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.

[equalsJeffH]

It is the unknown type that is causing errors not the unknown alg.

How do you know? In the test, the invalid PublicKeyCredentialParameters sequence element contains both an invalid alg value and an invalid type value, and the authenticator returns an error that does not indicate which of the PublicKeyCredentialParameters instance's members caused the error.

If we changed the test code's L359 to have cbor::Value("public-key") rather than cbor::Value("non-existing type") (leaving the invalid alg value of cbor::Value(-1) unaltered), we could see whether the invalid alg value causes a misbehaving authnr to return the error, which it should not do because the test's PublicKeyCredentialParameters sequence does contain one valid PublicKeyCredentialParameters instance.

See also: https://github.com/fido-alliance/fido-2-specs/issues/1113#issuecomment-731836851

[kaczmarczyck]

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.

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?

[equalsJeffH]

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 PublicKeyCredentialParameters instance containing a valid COSEAlgorithmIdentifier value and valid type value ("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 PublicKeyCredentialParameters instance as invalid if it contains an alg member 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 alg value 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 pick 0 since it is denoted as "reserved" and perhaps is less likely to be assigned someday?

[ve7jtb]

It is the unknown type that is causing errors not the unknown alg.

How do you know? In the test, the invalid PublicKeyCredentialParameters sequence element contains both an invalid alg value and an invalid type value, and the authenticator returns an error that does not indicate which of the PublicKeyCredentialParameters instance'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 than cbor::Value("non-existing type") (leaving the invalid alg value of cbor::Value(-1) unaltered), we could see whether the invalid alg value causes a misbehaving authnr to return the error, which it should not do because the test's PublicKeyCredentialParameters sequence does contain one valid PublicKeyCredentialParameters instance.

See also: fido-alliance/fido-2-specs#1113 (comment)

[ve7jtb]

0 is probably a safe alg for testing.

[nuno0529]

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.

[kaczmarczyck]

@nuno0529 So your point is that a type different from public-key should not be ignored? The point against this behaviour is what I meant with

Why 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 type is 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.

[nuno0529]

@kaczmarczyck I support your point that a type different from public-key should 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.