Reviewed 1 of 1 files at r1.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @karampok, @oncilla, and @scrye)

a discussion (no related file):
In general I wonder whether it would make sense to have this in the existing CPPKI document. Or what was the specific reason to have it separate?

doc/CertificateRenewal.md, line 96 at r1 (raw file):

### Request Payload

The proof-of-possessions are signatures of the request info using the JWS

Maybe its obvious but I would state for what the proof-of-possessions are.
Maybe something like
To proof the possession of the signing and revocation key in the request info, the request payload contains signatures of the request info using ...

doc/CertificateRenewal.md, line 142 at r1 (raw file):

keys

key

doc/CertificateRenewal.md, line 144 at r1 (raw file):

AS's

I think it's just AS'

doc/CertificateRenewal.md, line 160 at r1 (raw file):

- The request is signed with a signing key that is authenticated by a currently
  active AS certificate of the subject.

Is there a way to efficiently find the set of active AS certs? Or does the requestor indicate the version?

doc/CertificateRenewal.md, line 173 at r1 (raw file):

- Compromise recovery needs additional logic, i.e., there needs a way to disable
  automatic renewal. It can only be activated again after all the compromised

I guess an easy way would be to delete the AS from the customer table. Then delete all active certs, and insert the new out-of-band created one. Then re-add the AS to the customer table.

doc/CertificateRenewal.md, line 182 at r1 (raw file):

reues

request

doc/CertificateRenewal.md, line 265 at r1 (raw file):

renew 

renewed

Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @oncilla and @scrye)

a discussion (no related file):
I am not very able to understand the process of cert renewal.
I am missing the components of the system, who talks to whom (e.g. op person to cert-server through HTTP API)
Ideally I would like to have a diagram with the steps clearly defined. And ideally one diagram per SCION control-plane certificates.

Also the fact that we do not use CSR, and be able to explain the process using tools like openssl would add extra challenge.

From my previous experience, I can only suggest how kubernetes does the cert certificate rotation (used in mTLS).
Here are some links:
https://kubernetes.io/docs/tasks/tls/managing-tls-in-a-cluster/
https://www.ibm.com/support/knowledgecenter/SSCKRH_1.1.0/platform/t_certificate_renewal.html
https://kubernetes.io/docs/tasks/tls/certificate-rotation/
https://kubernetes.io/docs/reference/command-line-tools-reference/kubelet-tls-bootstrapping/#certificate-rotation

Reviewable status: 0 of 1 files reviewed, 11 unresolved discussions (waiting on @karampok, @lukedirtwalker, and @oncilla)

a discussion (no related file):

We can move this to the CPPKI doc once we have better documentation indexing support, otherwise it's a bit difficult to iterate on the spec.

a discussion (no related file):

Your comment points out something very important, and it's definitely something we'll need to do as a follow-up to this.

However, this document describes the interaction at a protocol level, so the diagram would include only two parties (Alice and Bob style): the requester and issuer. The goal here is to get a rigorous description of how the parties interact on w.r.t. to cryptographic protocol messages.

Management of keys, synchronization, tooling, server structure, and extent of operator intervention are out of scope, for now.

We'll need a separate document detailing how to run this protocol (e.g., how an can implement the periodic requests; which service sends them out; how are keys derived and managed inside the AS; how can an issuer be implemented; how should an issues interact with an authoritative AS; how does an issuer roll over its issuing keys; what commands must an operator run to recover from a compromise; etc).

a discussion (no related file):
Bit-lengths are hard to express when talking about JSON, because JSON has a weird way of working with them (JSON itself does not define what a number is, so a fixed-width integer is an undefined concept).

The spec should instead state that compliant applications MUST support unsigned integers up to x bytes in size, and MUST NOT create messages certificate renewal replies with values that need more than x bytes. Implementations MUST error out if an integer in the message would require more than x bytes.

The X509 draft RFC does so via the means of an INTEGER type. Maybe we can also define our own keyword and attach the semantics in the previous paragraph to it.

a discussion (no related file):
We should define a maximum value for NotAfter - NotBefore. Otherwise in the event of AS key compromise, an attacker can create certificates of infinite lifetime, thus poisoning external crypto caches, and then we're in revocation

doc/CertificateRenewal.md, line 96 at r1 (raw file):

Done.

doc/CertificateRenewal.md, line 142 at r1 (raw file):

Done.

doc/CertificateRenewal.md, line 144 at r1 (raw file):

Done.

doc/CertificateRenewal.md, line 160 at r1 (raw file):

There is a key version in the metadata/protected area. I think this the issuer needs to search for an active certificate that vouches for that key.

doc/CertificateRenewal.md, line 182 at r1 (raw file):

Done.

doc/CertificateRenewal.md, line 265 at r1 (raw file):

Done.

Reviewed 1 of 1 files at r2.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @karampok, @oncilla, and @scrye)

a discussion (no related file):

in Go it's easy to verify that an integer in a JSON has a certain size.

a discussion (no related file):

+1

doc/CertificateRenewal.md, line 41 at r2 (raw file):

and the signature of that payload.

For the field definitions in this document, we use the **integer** type to

maybe this should also be subsection ### Types ?

doc/CertificateRenewal.md, line 52 at r2 (raw file):

https://github.com/scionproto/scion/blob/master/doc/ControlPlanePKI.md

this can just be ControlPlanePKI.md#types

doc/CertificateRenewal.md, line 68 at r2 (raw file):

Signature verification using the certificate ...

Does that make sense in the request?

doc/CertificateRenewal.md, line 288 at r2 (raw file):

            "algorithm": "Ed25519",
            "key": "RUHOtezvoir6DWVCBBZjf3M_4giLbWgE0o3f4oJQu18=",
            "key_version": 29

We should remove the duplicated key_version. Not yet sure where we should leave it.

doc/CertificateRenewal.md, line 339 at r2 (raw file):

(if we go with Option 1)

delete, since you deleted the options above.

Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @karampok and @lukedirtwalker)

a discussion (no related file):

Yes, but the spec itself is language agnostic, so it should include these aspects.

a discussion (no related file):

forgot to finish the sentence, "we're in revocation lists land, which gets trickier."

I've added a limit in the validation section of the document.

doc/CertificateRenewal.md, line 41 at r2 (raw file):

Done.

doc/CertificateRenewal.md, line 52 at r2 (raw file):

Done.

doc/CertificateRenewal.md, line 68 at r2 (raw file):

Ah, it was a generic definition of the field, but it's misleading. Removed.

doc/CertificateRenewal.md, line 288 at r2 (raw file):

Let's remove this one. There's a Metadata section on the top-level that can share the format of the Metadata sections on the Request Info level if that is the case.

Removed.

doc/CertificateRenewal.md, line 339 at r2 (raw file):

Done.

Reviewed 1 of 1 files at r4.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @karampok and @scrye)

a discussion (no related file):

I like it

a discussion (no related file):

I feel the spec right now is clear in what integer type is. Maybe I don't understand the other proposal you're having.

doc/CertificateRenewal.md, line 52 at r2 (raw file):

Well I meant you don't need the full link, relative works.

Reviewed 1 of 1 files at r4.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @scrye)

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @oncilla and @scrye)

doc/CertificateRenewal.md, line 94 at r4 (raw file):

    "keys": {
        "signing": {
            "algorithm": "Ed25519",

this is also duplicate

Reviewable status: 0 of 1 files reviewed, 2 unresolved discussions (waiting on @karampok, @lukedirtwalker, and @oncilla)

a discussion (no related file):

Ah, I see. I wrote this comment before writing the update; so this was kind of a "I think this is needed, and here's the reasons" for why I did it in the update.

Yes, the specification is clear now, and we don't need to change anything right now (hopefully).

doc/CertificateRenewal.md, line 94 at r4 (raw file):

Done.

Reviewed 1 of 1 files at r5.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @oncilla)

Reviewable status: complete! all files reviewed, all discussions resolved