fulcio: remove detached SCT support #1236
Conversation
This fully removes detached SCT support, meaning that any Fulcio instance must use embedded SCTs. Closes #850. Signed-off-by: William Woodruff <[email protected]>
woodruffw
added
component:signing
Signed-off-by: William Woodruff <[email protected]>
| except KeyError: | ||
| raise FulcioClientError("Fulcio response missing certificate chain") | ||
| else: | ||
| sct_embedded = False |
There was a problem hiding this comment.
For instances that don’t use a CT log, the certificate chain is written to signedCertificateDetachedSct. To continue to support these local deployments, you’d need to still read the chain from either field, but only check the SCT if embedded.
Cc @codysoyland who added that for Sigstore-go
There was a problem hiding this comment.
Is it not a problem if the server can just "downgrade" to detached sct like that?
Or are we supposed to always require embedded SCT if the trust root contains at least one CT log, but otherwise be ok with detached SCT?
There was a problem hiding this comment.
Right now sigstore-python doesn't allow non-CT BYO uses at all, so allowing signedCertificateDetachedSct while removing detached SCT support would mean a larger refactor 😅
Given that, I think we're OK to remove this outright, since signedCertificateDetachedSct-without-CT is a use case we already didn't support. But I'm curious if this matches @jku's understanding 🙂
There was a problem hiding this comment.
Is it not a problem if the server can just "downgrade" to detached sct like that?
Or are we supposed to always require embedded SCT if the trust root contains at least one CT log, but otherwise be ok with detached SCT?
For Sigstore-go, it’s a threshold configured by the user. For cosign, SCTs are required unless opted out explicitly by the user.
If you require CT now, then agreed this would not be a breaking change.
| try: | ||
| # The SignedCertificateTimestamp should be accessed by the index 0 | ||
| sct = _get_precertificate_signed_certificate_timestamps(cert)[0] | ||
|
|
||
| except UnexpectedSctCountException as ex: | ||
| raise FulcioClientError(ex) |
There was a problem hiding this comment.
itching to refactor these methods a bit, using them is overly complicated and it's still incomplete (you could get ValueError in addition to this unnecessary internal error)... but that's not really related to this PR
There was a problem hiding this comment.
Yeah, I'd love to have these cleaned up. Let's do a follow-up for that 🙂
This fully removes detached SCT support, meaning
that any Fulcio instance must use embedded SCTs.
CC @haydentherapper for sanity-checking here, since this follows your original RFC on Fulcio.
Closes #850.
xref: sigstore/fulcio#1499