Handle the case of missing EKU in _is_preissuer

[ccordoui]

Summary

Previously, TBSCertificate signed with the same CA than the final certificate was failing if no ExtendedKeyUsage were present

Release Note

Fix a case where signing an artifact against a private sigstore instance was failing if no ExtendedKeyUsage was present in the Sigining CA

Documentation

NONE

Resolves: #658

[woodruffw]

/gcbrun

[woodruffw]

Thanks @ccordoui! For maximum pedantry: we probably also need the CA:true and similar RFC 5280 profile checks for the CA, right?

(Given that we don't build the chain anyways, I'm okay with skipping this for the purposes of this PR. But I want to make sure I'm not missing anything.)

[woodruffw]

LGTM; needs a "fixed" CHANGELOG.md entry 🙂

[ccordoui]

we probably also need the CA:true and similar RFC 5280 profile checks for the CA, right?

Yes, will do.

LGTM; needs a "fixed" CHANGELOG.md entry 🙂

I'll add that too.

Thanks @woodruffw!

[woodruffw]

No problem, thanks to you as well! You may be able to reuse one of the cert utility functions that I added under utils 🙂

[ccordoui]

I have to check why I have an expired client certificate using the signing unit tests, but outside of that, it seems that using cert_is_ca from utils like I did is breaking the sigstore sign 😞 (at least for the moment)

[ccordoui]

@woodruffw this version doesn't pass the unit tests but it is expected:

================================================================================================== short test summary info ==================================================================================================
...
FAILED test/unit/test_sign.py::test_sign_rekor_entry_consistent[staging] - sigstore._internal.sct.InvalidSCTError: Invalid issuer pubkey basicConstraint (not a CA): <cryptography.hazmat.backends.openssl.ec._EllipticCurvePublicKey object at 0x10ae4e990>
....

With debug logs:

ERROR    sigstore._internal.sct:sct.py:153 Found <Certificate(subject=<Name(O=sigstore.dev,CN=sigstore-intermediate)>, ...)> as issuer, verifying if it is a ca
ERROR    sigstore._internal.sct:sct.py:157 Invalid <Certificate(subject=<Name(O=sigstore.dev,CN=sigstore-intermediate)>, ...)>: failed to validate ca constrain: invalid certificate states: KeyUsage.digitalSignature=False, KeyUsage.keyCertSign=True, BasicConstraints.ca=True

So it fail this check in cert_is_ca:

      # If all three states are set, this is a CA.
      if ca and key_cert_sign and digital_signature:
          return True

And it make sense to me, in the case of sigstore public instance, they are using a Precertificate Signing Certificate and its only purpose is to sign Precertificate hence keyCertSign KU, digitalSignature doesn't make sense (to me) here.

From what I could gather from rfc5280 on KU:

• keyCertSign:

The keyCertSign bit is asserted when the subject public key is
used for verifying signatures on public key certificates. If the
keyCertSign bit is asserted, then the cA bit in the basic
constraints extension MUST also be asserted.

• digitalSignature:

The digitalSignature bit is asserted when the subject public key
is used for verifying digital signatures, other than signatures on
certificates (bit 5) and CRLs (bit 6), such as those used in an
entity authentication service, a data origin authentication
service, and/or an integrity service.

I couldn't find a RFC specifying that we need the digitalSignature KU to be a CA, what did I miss?

[woodruffw]

I couldn't find a RFC specifying that we need the digitalSignature KU to be a CA, what did I miss?

Nothing 🙂 -- I added that check based on observed behaviors for the Sigstore CAs, but it's not actually mandated in RFC 5280 for CAs.

I don't think there's any harm in removing that check on digitalSignature entirely, but I'll check the current PKI profile draft for Sigstore and confirm.

[woodruffw]

/gcbrun

[woodruffw]

/gcbrun

[woodruffw]

@ccordoui mind updating the branch here? If I do it, we'll need to do another hop of reviewers because of the merge restrictions 🙃

[ccordoui]

@ccordoui mind updating the branch here? If I do it, we'll need to do another hop of reviewers because of the merge restrictions 🙃

@woodruffw a rebase will do?

[woodruffw]

@woodruffw a rebase will do?

Yes, that should be fine -- anything that updates the HEAD to make the ownership not mine.

[woodruffw]

/gcbrun