Subject: [acvp] ACVP draft comments

From: Yaron Sheffer <[email-masked]>
Date: Thu, 28 Mar 2019 11:17:39 +0100

Hi,

I reviewed the spec in some depth, but only skimmed the other
major document. So here are a few comments. Thank you for this
important work!

Yaron

* FCS: expand the acronym: in the Terminology section?

* Why support HTTP (as opposed to HTTPS) at all? IMO TLS should be
REQUIRED, not RECOMMENDED.

* What is the difference between validation testing and crypto
assessment testing?

* The notion of "verifiable artifacts" is not well defined here or
possibly even bogus (any crypto module can be subverted to produce
fake results), so I wouldn't use it.

* Authentication should actually be in scope, at least to enable a
minimum interoperable implementation.

* "Mutual authentication", you probably mean TLS with mutual
certificate-based auth, please clarify.

* DRBG: expand.

* Figure 1: I would think the DUT is only the crypto module, and
does not include the ACV client which is not actually being tested.

* 4.1: please clarify the notion of real-time vs. non-real time
operation. It's not clear to me where real-time testing can be
useful. (This is explained later, so maybe add a forward reference
to the Terminology).

* I am sure you're already used to it, but first-timers will be
highly confused by the sense of request+response which is opposite
the usual: server to client. Can we rename them?

* 5.1, please use an example.com URI (RFC 6761).

* 5.1, version numbers are later included in the URI, but not here.
Why?

* 6.1, as noted above, IMO a single baseline authentication method
should be mandated, though implementations may choose to add others.

* 7: the word "extensions" should be quoted. Better yet, give an
example. It is also very common to have an IANA registry for
extensions names, in order to avoid conflicts, and/or to give a
naming convention such as using a DNS name of the organization
(similar to Java packages).

* 8: this is not well-specified. What if a new port is NOT required?
Also, what is the syntax for the secondary port? How is it
authenticated? And in fact, why is it ever needed, i.e. why would a
server agree to accept on an auxiliary port what it refuses to
accept on the main port?

* 9. This section mentions "major/minor" but the examples all have
"v1". In general this section needs a lot more clarity.

* 10. Maybe start the section with a quick overview of the message
exchanges.

* 10.1. Primary contact? I'm not sure it is useful to configure
people's names on the servers.

* 10.2. It is implied that you cannot have multiple outstanding
requests in parallel for the same client. I suggest to make it
explicit.

* 10.3. Alg:none should never be used in production, and also, never
in a standard.

* 10.3. "Key agreement would follow RFC7518." I'm not sure what you
mean. I suspect you want to say, Keys are pre-provisioned to the
client and server according to RFC 7518.

* 10.3. More work is needed on the JWT: you want integrity
protection, not encryption. Unless you're passing the database key,
in which case you do need encryption. HS256 is obviously only
integrity protection. Also, you'd want a public key (RS256) rather
than a shared HMAC key, for better security.

* 10.3.2. Why use the expired token in the exchange? This means the
server should have a special case where it accepts expired tokens -
not very good.

* 10.5. Please avoid Markdown "bold", it doesn't render correctly in
the text version.

* 10.5. We should discuss the use of JSON Schema
(https://json-schema.org/) to avoid lengthy an ill-defined examples.

* Are you sure you want the complexity of PUT, with per-property
updates? This breaks once you have arrays. For example, you cannot
add or remove a single phone numbers, you have to specify them all.

* Having the DELETE methods as "optional" makes it very hard to test
this API. I would suggest to have them as mandatory, but *allow*
servers to refuse them based on policy (and then return an error
code).

* 10.8.3. Dependency properties seem to introduce too much
complexity. I would assume they can be hardcoded or maybe specified
here, but not available for listing/parsing at run-time.

* Algorithms: in many cases you have a cross-product of name X
key-length X mode. Maybe for these cases you want an array of
available key-lengths and an array of available modes, provided all
combinations are supported.

* What does "encrypt at rest" mean, and why is it a parameter at
all?

* When listing sessions, I would expect a status field: not-started,
in-progress, complete-passed, complete-failed.

* Why is there a per-session access token, why not reuse the
on-going login based access token?

* "Expiry" field, please use standard ISO8601 time, as usual.
Besides, the current format lacks a time zone.

* 13. Returned errors: please use RFC 7807 rather than creating a
new format.

* IANA section: better incorporate it into the main document.

* draft-celi-block-ciph: is there any support for negative testing,
specifically cases where AES-GCM decryption should fail because the
auth tag is incorrect?

---

From: "Celi, Christopher T. (Fed)" <[email-masked]>
Date: Thu, 28 Mar 2019 14:44:39 +0000

Hi,

In the symmetric block cipher draft, there is negative testing for GCM (and GCM-SIV) decryption operations.

Thank you for the feedback.

Chris Celi

________________________________
From: acvp <[email-masked]> on behalf of Yaron Sheffer <[email-masked]>
Sent: Thursday, March 28, 2019 6:17:39 AM
To: [email-masked]
Subject: [acvp] ACVP draft comments

Hi,

I reviewed the spec in some depth, but only skimmed the other major document. So here are a few comments. Thank you for this important work!

    Yaron

* Why support HTTP (as opposed to HTTPS) at all? IMO TLS should be REQUIRED, not RECOMMENDED.
* What is the difference between validation testing and crypto assessment testing?
* The notion of "verifiable artifacts" is not well defined here or possibly even bogus (any crypto module can be subverted to produce fake results), so I wouldn't use it.
* Authentication should actually be in scope, at least to enable a minimum interoperable implementation.
* "Mutual authentication", you probably mean TLS with mutual certificate-based auth, please clarify.
* DRBG: expand.
* Figure 1: I would think the DUT is only the crypto module, and does not include the ACV client which is not actually being tested.
* 4.1: please clarify the notion of real-time vs. non-real time operation. It's not clear to me where real-time testing can be useful. (This is explained later, so maybe add a forward reference to the Terminology).
* I am sure you're already used to it, but first-timers will be highly confused by the sense of request+response which is opposite the usual: server to client. Can we rename them?
* 5.1, please use an example.com URI (RFC 6761).
* 5.1, version numbers are later included in the URI, but not here. Why?
* 6.1, as noted above, IMO a single baseline authentication method should be mandated, though implementations may choose to add others.
* 7: the word "extensions" should be quoted. Better yet, give an example. It is also very common to have an IANA registry for extensions names, in order to avoid conflicts, and/or to give a naming convention such as using a DNS name of the organization (similar to Java packages).
* 8: this is not well-specified. What if a new port is NOT required? Also, what is the syntax for the secondary port? How is it authenticated? And in fact, why is it ever needed, i.e. why would a server agree to accept on an auxiliary port what it refuses to accept on the main port?
* 9. This section mentions "major/minor" but the examples all have "v1". In general this section needs a lot more clarity.
* 10. Maybe start the section with a quick overview of the message exchanges.
* 10.1. Primary contact? I'm not sure it is useful to configure people's names on the servers.
* 10.2. It is implied that you cannot have multiple outstanding requests in parallel for the same client. I suggest to make it explicit.
* 10.3. Alg:none should never be used in production, and also, never in a standard.
* 10.3. "Key agreement would follow RFC7518." I'm not sure what you mean. I suspect you want to say, Keys are pre-provisioned to the client and server according to RFC 7518.
* 10.3. More work is needed on the JWT: you want integrity protection, not encryption. Unless you're passing the database key, in which case you do need encryption. HS256 is obviously only integrity protection. Also, you'd want a public key (RS256) rather than a shared HMAC key, for better security.
* 10.3.2. Why use the expired token in the exchange? This means the server should have a special case where it accepts expired tokens - not very good.
* 10.5. Please avoid Markdown "bold", it doesn't render correctly in the text version.
* 10.5. We should discuss the use of JSON Schema (https://json-schema.org/<https://gcc01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fjson-schema.org%2F&data=02%7C01%7Cchristopher.celi%40nist.gov%7Cbe9138758ab342dcaa8808d6b366b487%7C2ab5d82fd8fa4797a93e054655c61dec%7C1%7C0%7C636893651028812930&sdata=0mnEIPMOjEgXire%2BsAb0WSHNcgDKYQCvaLFNxjVOu5Q%3D&reserved=0>) to avoid lengthy an ill-defined examples.
* Are you sure you want the complexity of PUT, with per-property updates? This breaks once you have arrays. For example, you cannot add or remove a single phone numbers, you have to specify them all.
* Having the DELETE methods as "optional" makes it very hard to test this API. I would suggest to have them as mandatory, but *allow* servers to refuse them based on policy (and then return an error code).
* 10.8.3. Dependency properties seem to introduce too much complexity. I would assume they can be hardcoded or maybe specified here, but not available for listing/parsing at run-time.
* Algorithms: in many cases you have a cross-product of name X key-length X mode. Maybe for these cases you want an array of available key-lengths and an array of available modes, provided all combinations are supported.
* What does "encrypt at rest" mean, and why is it a parameter at all?
* When listing sessions, I would expect a status field: not-started, in-progress, complete-passed, complete-failed.
* Why is there a per-session access token, why not reuse the on-going login based access token?
* "Expiry" field, please use standard ISO8601 time, as usual. Besides, the current format lacks a time zone.
* 13. Returned errors: please use RFC 7807 rather than creating a new format.
* IANA section: better incorporate it into the main document.
* draft-celi-block-ciph: is there any support for negative testing, specifically cases where AES-GCM decryption should fail because the auth tag is incorrect?