approver-policy icon indicating copy to clipboard operation
approver-policy copied to clipboard

Improve CRD fields for specifying key requirements

Open SgtCoDFish opened this issue 2 years ago • 0 comments

We discussed curve parameters and checking for curves in standup earlier, and I said I'd look into approver-policy because checking curves is an important part of that. This issue documents what I found.

approver-policy uses just the field size when checking ECDSA curves, which isn't ideal since it opens the possibility of a cert using a bizarre non-standard curve over a field of the same size yet being accepted.

As a user of approver-policy, 99.999999% of the time I don't want to allow this. I want to ensure that certs use one of the standard curves.

More than that, though, the whole API for policy around key types isn't ideal.

It makes sense to allow "minSize" and "maxSize" for RSA, but those are largely meaningless for the other 2 key types.

For Ed25519 there's no meaningful other parameter to check, and for ECDSA it would be better to check for exact named curves for reasons given above.

Checking named curves obviously means that if a new curve is added we'd need to update approver-policy to support it, but that seems a worthwhile tradeoff.

This section of the CRD might be better if it had different properties for each curve type, e.g.:

RSA

    privateKey:
      algorithm: RSA # unchanged, nothing wrong with this
      minSize: 2048
      maxSize: 4096

ECDSA

    privateKey:
      algorithm: ECDSA
      allowedCurves: ["P-256", "P-384"] # don't allow P521 or any possible future curves

EdDSA

    privateKey:
      algorithm: Ed25519
      # nothing else makes sense here

SgtCoDFish avatar Feb 20 '23 13:02 SgtCoDFish