[secdir] secdir review of draft-ietf-sidr-rpki-manifests-10

[Nico Williams <[e-mail]>]

[Resend, from the correct address.]

I have reviewed this document as part of the security directorate's
ongoing effort to review all IETF documents being processed by the IESG.
These comments were written primarily for the benefit of the security
area directors. Document editors and WG chairs should treat these
comments just like any other last call comments.

This document is almost ready for publication.  My comments are below.
A summary of my comments is:

 - Text on versioning of the manifest ASN.1 would be useful.
 - Handling of boundary conditions on manifest validation could use some
 improvement, such as regarding clock skew.  See specific comments
 below.
 - Some "SHOULDs" could use some additional commentary regarding when
 an implementation might act otherwise.
 - Nits (typos, out of date references).

Nico


> 2.  Manifest Scope
>
>    [...]
>    Where an EE certificate is placed in the Cryptographic Message Syntax
>    (CMS) wrapper of a published RPKI signed object
>    [ID.sidr-signed-object] there is no requirement to separately publish
>    the EE certificate in the CA's repository publication point.
>    [...]

Any guidance on when to place EE certs in CMS wrappers and when not to?

BTW, the reference to ID.sidr-signed-object needs to be updated (the
current version is -03).

> 3.  Manifest Signing
>
>    A CA's manifest is verified using an EE certificate The
                                                      ^
Missing period.
s/EE certificate The/EE certificate.  The/

>    SubjectInfoAccess (SIA) field of this EE certificate contains the
>    access method OID of id-ad-signedObject.
>    [...]
>
> 4.2.  eContent
>
>    The content of a Manifest is defined as follows:
>
>      Manifest ::= SEQUENCE {
>       version     [0] INTEGER DEFAULT 0,
>       manifestNumber  INTEGER (0..MAX),
>       thisUpdate      GeneralizedTime,
>       nextUpdate      GeneralizedTime,
>       fileHashAlg     OBJECT IDENTIFIER,
>       fileList        SEQUENCE SIZE (0..MAX) OF FileAndHash
>       }
>
>     FileAndHash ::=     SEQUENCE {
>       file            IA5String,
>       hash            BIT STRING
>       }

IA5String?  Not UTF8String?  What goes into file naming?

> 4.2.1.  Manifest
>
>    The data elements of the Manifest structure are defined as follows:
>
>    version:
>       The version number of this version of the manifest specification
>       MUST be 0.

Some text on how versioning is intended to be used would be nice.
Specifically, how might extensions be added?  Or perhaps extensibility
here is seen as unnecessary?

If extensibility is inteded to be done by turning the version field of
the above SEQUENCE into a CHOICE then say so -- implementors with
sufficiently capable ASN.1 compilers and runtimes may prefer to modify
the above to use an extensible CHOICE.

In general I would much prefer that we make use of ASN.1's explicit
extensibility features (namely, the extensibility marker in SEQUENCEs,
SETs, and CHOICEs) and/or typed-holes as appropriate.

>    manifestNumber:
>       This field is an integer that is incremented each time a new
>       manifest is issued for a given publication point.  This field
>       allows an RP to detect gaps in a sequence of published manifests.
>
>       As the manifest is modeled on the CRL specification, the
>       ManifestNumber is analogous to the CRLNumber, and the guidance in
>       [RFC5280] for CRLNumber values is appropriate as to the range of
>       number values that can be used for the manifestNumber.  Manifest
>       numbers can be expected to contain long integers.  Manifest
>       verifiers MUST be able to handle number values up to 20 octets.
>       Conforming Manifest issuers MUST NOT use number values longer than
>       20 octets

Why not write that MAX value explicitly in the constraint for this
field?  (Because it's a fairly long number?)

> 5.1.  Manifest Generation Procedure
>
>    6.  In the case of a key pair that is to be used only once, in
>        conjunction with a "one-time-use" EE certificate, the private key
>        associated with this key pair SHOULD now be destroyed.

Any reason not to make this SHOULD a MUST?  Any guidance as to when one
might not heed this SHOULD?

> 5.2.  Considerations for Manifest Generation
>
>    A new manifest MUST be issued on or before the nextUpdate time.

Well, a new manifest must be published on or before the nextUpdate time.
Since RPs clocks will have some skew, new manifests should really be
published some time ahead of the nextUpdate time.  A few seconds or
minutes will do.  See comments on section 6.2.

What happens if an authority fails to publish a new manifest in a timely
fashion?  This would surely be an important operational consideration...

>    When a CA entity is performing a key rollover, the entity MAY chose

s/chose/choose/

>    to have two CAs instances simultaneously publishing into the same

s/CAs instances/CA instances/

>    repository publication point.  In this case there will be one
>    manifest associated with each active CA instance that is publishing
>    into the common repository publication point (directory).

> 6.1.  Tests for Determining Manifest State
>
>    For a given publication point, the RP SHOULD perform the following
>    tests to determine the manifest state of the publication point:
>
>    1.  For each CA using this publication point, select the CA's current
>        manifest (The "current" manifest is the manifest issued by this
>        CA having highest manifestNumber among all valid manifests, and
>        where manifest validity is defined in Section 4.4).
>
>        If the publication point does not contain a valid manifest, see
>        Section 6.2.  Lacking a valid manifest, the following tests
>        cannot be performed.
>
>    2.  To verify completeness, an RP MAY check that every file at each
>        publication point appears in one and only one current manifest,
>        and that every file listed in a current manifest that is
>        published at the same publication point as the manifest.

See comment on (4) below.

>    3.  Check that the current time (translated to UTC) is between
>        thisUpdate and nextUpdate.
>
>        If the current time does not lie within this interval then see
>        Section 6.4, but still continue with the following tests.

This appears to be in conflict with (1) above.  The manifest can't be
valid if the current time does not fall in the manifest's validity
period, so what's the point in continuing?  I suppose I'll find out when
I get to section 6.4!

>    4.  Verify that listed hash value of every file listed in each
>        manifest matches the value obtained by hashing the file at the
>        publication point.
>
>        If the computed hash value of a file listed on the manifest does
>        not match the hash value contained in the manifest, then see
>        Section 6.6.

Will the RP need to check every file?  Why not just those that are of
interest?

>    For each signed object, if all of the following conditions hold:
>
>        [...]
>
>    then the RP can conclude that no attack against the repository system
>    has compromised the given signed object, and the signed object MUST
>    be treated as valid.

No scope for local policy exemptions to the above MUST?

> 6.2.  Missing Manifests
>
>    The absence of a current manifest at a publication point could occur
>    due to an error by the publisher or due to (malicious or accidental)
>    deletion or corruption of all valid manifests.
>
>    When no valid manifest is available, there is no protection against
>    attacks that delete signed objects or replay old versions of signed
>    objects.  All signed objects at the publication point, and all
>    descendant objects that are validated using a certificate at this
>    publication point SHOULD be viewed as suspect, but MAY be used by the
>    RP, as per local policy.

I wonder if we shouldn't have a latestNextUpdate field specifying a time
past which RPs MUST NOT accept expired manifests.  Alternatively, local
policy ought to specify how old an expired manifest may be accepted,
with RECOMMENDED guidance as to what that maximum age should be.

Additionally, CA operators should get guidance to publish new manifests
somewhat sooner than the expiration of current manifests being replaced
so as to have some time to cope with operations failures during manifest
generation and publication.

>    The primary risk in using signed objects at this publication point is
>    that a superseded (but not stale) CRL would cause an RP to improperly
>    accept a revoked certificate as valid (and thus rely upon signed
>    objects that are validated using that certificate).  This risk is
>    somewhat mitigated if the CRL for this publication point has a short
>    time between thisUpdate and nextUpdate (and the current time is
>    within this interval).  The risk in discarding signed objects at this
>    publication point is that an RP may incorrectly discard a large
>    number of valid objects.  This gives significant power to an
>    adversary that is able to delete a manifest at the publication point.

I.e., there's a trade-off between DoS and more severe attacks.  However,
we can't protect against DoS attacks here anyways, so might as well give
guidance in preference of protecting against the other attacks.

Additionally, guidance to interleave new manifest publication such that
there's enough time to cope with operations failures and DoS attacks
should help.

>    Regardless of whether signed objects from this publication are deemed
>    fit for use by an RP, this situation SHOULD result in a warning to
>    the effect that: "No manifest is available for <pub point name>, and
>    thus there may have been undetected deletions or replay substitutions
>    from the publication point."

I imagine this isn't a MUST because of log squelching considerations.
Right?

>    In the case where an RP has access to a local cache of previously
>    issued manifests that are valid, the RP MAY use the most recently
>    previously issued valid manifests for this RPKI repository
>    publication collection in this case for each entity that publishes at
>    his publication point.

Subject to the same considerations, surely.

Any advice as to when to poll for new manifests ahead the current,
cached manifest's nextUpdate?

> 6.4.  Stale Manifests

My comments on section 6.2 apply here as well.

>    Note that there is also the potential for the current time to be
>    before the thisUpdate time for the manifest.  This case could be due
>    to publisher error, or a local clock error, and in such a case this
>    situation SHOULD result in a warning to the effect that: "A manifest
>    found at <pub point name> has an incorrect thisUpdate field.  This
>    could be due to publisher error, or a local clock error, and
>    processing for this publication point will continue using this
>    otherwise valid manifest."

This can also happen due to having a slow clock on the RP or a fast
clock at the CA, or both.  Clock skew is hardly an error, since there
will always be some skew, even if only on the order of nanoseconds in
the case of good hardware setup with good time distribution mechanisms
and good hardware time sources.  A bit more text (any!) regarding clock
skew would be useful.

> 8.  Security Considerations
>
...
>    the manifest structure .

s/ \././