YANG Data Types and Groupings for Cryptography
draft-ietf-netconf-crypto-types-34

Thank you to Valery Smyslov for the SECDIR review.

I support the Paul's DISCUSS position.

Thanks for addressing my DISCUSS feedback and most of my COMMENT feedback.

** Section 3.5.
   When accessing key values, it is desireable that implementations
   ensure that the strength of the keys being accessed is not greater
   than the strength of the underlying secure transport connection over
   which the keys are conveyed.  However, comparing key strengths can be
   complicated and difficult to implement in practice. 

I don’t understand the guidance in this section.  I would have benefited from clarity in the following areas.

-- Explain the impact of using keys whose strength exceeds the underlying transport connection (i.e., it doesn’t offer more security)

-- The verb “accessing” is confusing.  Let’s say that an implementation notices a discrepancy between key strength, what is it supposed to do?

-- The last sentence (“However, comparing ...) seems to acknowledge (correctly) that this advice might not be practical.  Is the WG sure the text is needed?

** Section 3.6
   Implementations SHOULD only use secure transport protocols meeting
   local policy.  A reasonable policy may, e.g., state that only
   ciphersuites listed as "recommended" by the IETF be used (e.g.,
   [RFC7525] for TLS).

-- Would there be instances where implementation would use secure transport that _doesn’t_ meet local policy?

-- RFC7525 has been obsoleted.  s/RFC7525/RFC9325/

Thank you for the work on this document.

Section 1.4:
> value for binary data has has been base64 encoded. This placeholder 

The document is missing a reference to RFC 4648 (and specify which encoding, Section 4 or 5). I assume that this is the same as for RFC 7950 which states:

   Binary values are encoded with the base64 encoding scheme (see
   Section 4 in [RFC4648]).

Even if this is meant to use and extend 7950 (as specified later in 2.1.3), it would be good to repeat the above and reference 4648 early on, rather than relying on the reader investigating capabilities. If you don't want to do it in Section 1.4, I suggest the reference to 4648 and explicitly stating the base64 encoding should be at least added to 2.1.3.

RFC 3447 has been obsoleted by 8017. I assume this is an error, given that the doc uses "PKCS #1: RSA Cryptography Specifications Version 2.2" which is actually 8017, rather than version 2.1.

# GEN AD review of draft-ietf-netconf-crypto-types-29

CC @larseggert

Thanks to Dale R. Worley for the General Area Review Team (Gen-ART) review
(https://mailarchive.ietf.org/arch/msg/gen-art/AmEmW-zWeMAEWMxu8-Czt4ZB_cE).

## Nits

All comments below are about very minor potential issues that you may choose to
address in some way - or ignore - as you see fit. Some were flagged by
automated tools (via https://github.com/larseggert/ietf-reviewtool), so there
will likely be some false positives. There is no need to let me know what you
did with these suggestions.

### Typos

#### Section 2.1.2, paragraph 1
```
-    The following diagram illustrates the hierarchal relationship amongst
+    The following diagram illustrates the hierarchical relationship amongst
+                                                  ++
```

### Outdated references

Reference `[RFC3447]` to `RFC3447`, which was obsoleted by `RFC8017` (this may
be on purpose).

Reference `[RFC7525]` to `RFC7525`, which was obsoleted by `RFC9325` (this may
be on purpose).

Reference `[RFC6125]` to `RFC6125`, which was obsoleted by `RFC9525` (this may
be on purpose).

### Grammar/style

#### Section 1.1, paragraph 5
```
 placeholder value for binary data has has been base64 encoded. This placeho
                                   ^^^^^^^
```
Possible typo: you repeated a word.

#### Section 2.3, paragraph 59
```
ed-value-format' based identity MUST by set (e.g., cms-encrypted-data-format
                                     ^^
```
Did you maybe mean "buy" or "be"?

#### Section 2.3, paragraph 59
```
ed-value-format' based identity MUST by set (e.g., cms-enveloped-data-format
                                     ^^
```
Did you maybe mean "buy" or "be"?

## Notes

This review is in the ["IETF Comments" Markdown format][ICMF], You can use the
[`ietf-comments` tool][ICT] to automatically convert this review into
individual GitHub issues. Review generated by the [`ietf-reviewtool`][IRT].

[ICMF]: https://github.com/mnot/ietf-comments/blob/main/format.md
[ICT]: https://github.com/mnot/ietf-comments
[IRT]: https://github.com/larseggert/ietf-reviewtool

Similar to what Paul's DISCUSS says:

Section 3.5:

        That said, expert Security opinion suggests that already it is
        infeasible to break a 128-bit symmetric key using a classical 
        computer, and thus the concern for conveying higher-strength
        keys begins to lose its allure.

Can we make a reference to said opinion?  Without that, this looks like argument from authority (and an anonymous one at that).

===

Additional comments from incoming ART AD, Orie Steele:

Section 1.4

Prefer to see the exact base64 format cited (not base64url, with or without padding , etc...)

...

I assume normative terminology in the model itself, does not contradict the RFCs from which it is derived?


> "A private key and, optionally, its associated public key.
>  Implementations SHOULD ensure that the two keys, when both
>  are specified, are a matching pair.";
       
Why not MUST?

> "A private/public key pair and an associated certificate.
>  Implementations SHOULD assert that the certificate contains
>  the matching public key.";

Why not MUST?

> "A private/public key pair and a list of associated
> certificates.  Implementations SHOULD assert that
> certificates contain the matching public key.";

Why not MUST?

> 3.3. Unconstrained Public Key Usage

This seems not great. later...

> whereby associated certificates may constrain the usage of the public key according to local policy.
...
> whereby configured certificates (e.g., identity certificates) may constrain the use of the public key according to local policy.

Why is this not a SHOULD / MUST ?

> 3.5. Strength of Keys Conveyed

it might be better to convert this to a recommendation, with MUST NOT / SHOULD NOT, etc...

> Implementations SHOULD only use secure transport protocols meeting local policy.

Why not MUST ?

> This module defines storage for cleartext key values that SHOULD be zeroized when deleted, so as to prevent the remnants of their persisted storage locations from being analyzed in any meaningful way.

Nice to see this recommendation.

I couldn't really decide between No Objection and Discuss. I ended up deciding on No Obj.

I'm **assuming** that the "hidden" is conceptually similar to a write-only variable (or like some device vendors who elide the password/keys/similar in their equivalent of 'show config'), but it's really really unclear -- this section could do with some more words to explain this...

Thanks for working on this specification. I have no objection from transport protocol point of view.

I was wondering if the following description is sufficiently clear to the implementers -

leaf public-key-format {
      nacm:default-deny-write;
      type identityref {
        base public-key-format;
      }
      mandatory true;
      description
        "Identifies the public key's format. Implementations SHOULD
         ensure that the incoming public key value is encoded in the
         specified format.";
    }


Which format is specified here ? and how the implementation can ensure the compliance to the specific format for the incoming public key value?

I am missing some reference/example/direction here.