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

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

Closed
the-over-ape opened this issue Aug 29, 2024 · 8 comments
Milestone

Comments

@the-over-ape
Copy link

environment: Demo
Note: Copy of usnistgov/ACVP#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
Copy link

woodbe commented Aug 30, 2024

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
Copy link
Collaborator

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

@the-over-ape
Copy link
Author

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

@livebe01
Copy link
Collaborator

livebe01 commented Sep 6, 2024

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
Copy link
Author

the-over-ape commented Sep 6, 2024 via email

@livebe01
Copy link
Collaborator

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.

@livebe01 livebe01 added this to the v1.1.0.36 milestone Oct 3, 2024
@livebe01
Copy link
Collaborator

livebe01 commented Oct 7, 2024

The fix for this issue has been deployed to ACVTS Demo as part of the v1.1.0.36 release.

@livebe01
Copy link
Collaborator

The fix for this issue has been deployed to ACVTS Prod as part of the v1.1.0.36 release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants