usnistgov / ACVP-Server

A repository tracking releases of NIST's ACVP server. See www.github.com/usnistgov/ACVP for the protocol.
51 stars 18 forks source link

`UsesHybridSharedSecret: false` is treated as if UsesHybridSharedSecret was NULL #353

Closed the-over-ape closed 1 month ago

the-over-ape commented 1 month ago

environment: Demo Note: Copy of https://github.com/usnistgov/ACVP/issues/1536 which was at the wrong place.

We have the following error when submitting the below registration.json:

Failed to create test session: acvp: HTTP error [\r\n {\r\n \"acvVersion\": \"1.0\"\r\n },\r\n {\r\n \"error\": \"Validation error(s) on JSON payload.\",\r\n \"context\": [\r\n \"KDA-TwoStep-Sp800-56Cr2: The UsesHybridSharedSecret registration property is required for algo/mode/revision KDA_TwoStep_Sp800_56Cr2 testing, but was not provided.\"\r\n ]\r\n }\r\n]\n"}

This is the error we expect to get when we do not have the UsesHybridSharedSecret key in the file at all, where it is expected based on this code and this documentation

We used very similar files last year for another device and UsesHybridSharedSecret was not yet required, probably the fix for this issue had not made it to public facing servers.


registration.json:

[
    {
        "acvVersion": "1.0"
    },
    {
        "isSample": false,
        "publishable": true,
        "algorithms": [
            {
                "algorithm": "KDA",
                "capabilities": [
                    {
                        "counterLength": [
                            8
                        ],
                        "encoding": [
                            "concatenation"
                        ],
                        "fixedDataOrder": [
                            "after fixed data"
                        ],
                        "fixedInfoPattern": "uPartyInfo||vPartyInfo",
                        "kdfMode": "feedback",
                        "macMode": [
                            "HMAC-SHA2-256"
                        ],
                        "macSaltMethods": [
                            "random",
                            "default"
                        ],
                        "requiresEmptyIv": true,
                        "supportedLengths": [
                            {
                                "increment": 64,
                                "max": 1024,
                                "min": 128
                            }
                        ],
                        "supportsEmptyIv": true
                    }
                ],
                "l": 1024,
                "mode": "TwoStep",
                "UsesHybridSharedSecret": false,
                "revision": "Sp800-56Cr2",
                "z": [
                    256,
                    384
                ]
            }
        ]
    }
]
woodbe commented 1 month ago

I am also seeing this issue with a component that successfully passed the Sp800-56Cr2 last year when the UsesHybridSharedSecret was not required in the registration file. As the UsesHybridSharedSecret setting is boolean, it should accept a "false" statement, yet only gives error messages when attempting to submit to the demo server with this value set. Specifying true does work, but then asks for more settings, and since this value is not used in the module, those settings are not relevant, but it does show it working.

If we back down to Sp800-56Cr1 without it, that will be accepted, but that isn't the functionality of the module.

livebe01 commented 1 month ago

Thanks for reporting this. Agreed, this should not be happening. We'll look to see what's amiss.

the-over-ape commented 1 month ago

The documentation has usesHybridSharedSecret with lowecase 'u' With lowercase it works... closing...

livebe01 commented 1 month ago

Thanks for the update and for letting us know you were able to get past this. It would be better if the error message that's provided used the correct case for the property name. I.e., usesHybridSharedSecret vs UsesHybridSharedSecret

the-over-ape commented 1 month ago

Also it could say 'unrecognized property' which would make people check spelling and case...

:)

On Fri, Sep 6, 2024, 16:43 livebe01 @.***> wrote:

Thanks for the update and for letting us know you were able to get past this. It would be better if the error message that's provided used the correct case for the property name. I.e., usesHybridSharedSecret vs UsesHybridSharedSecret

— Reply to this email directly, view it on GitHub https://github.com/usnistgov/ACVP-Server/issues/353#issuecomment-2334217440, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABRWP5BWFQKGOAHFOLI5CMLZVG5PVAVCNFSM6AAAAABNLAWJ2WVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDGMZUGIYTONBUGA . You are receiving this because you modified the open/close state.Message ID: @.***>

livebe01 commented 3 weeks ago

Ah, yes. That would be better, but wouldn't work well for how we've implemented ACVTS.

ACVTS is setup such that extra/irrelevant properties contained within algorithm registrations are ignored vs rejected. I remember some conversations w/in the team where we discussed the virtues of each option, but I can't remember the complete back story for why we chose to ignore vs reject.