Closed woodruffw closed 1 year ago
haydentherapper
commented
1 year ago https://github.com/sigstore/cosign/issues/2143#issuecomment-1209463170 - my thoughts on this previously
I view it as a verification implementation error to trust the last certificate in the chain without comparing it to a root provided out of band. Happy to revisit this if folks think otherwise!
woodruffw
commented
1 year ago Yep -- I agree that it's an implementation error to trust the last certificate in the chain, but I also think we can nudge people away from that implementation error by defining the problem away 🙂
From your comment:
TLS does the same, returning a complete chain.
I might be wrong, but I don't think this is true: here's what I get from the TLS handshake for www.google.com:
CONNECTED(00000005)
depth=2 C = US, O = Google Trust Services LLC, CN = GTS Root R1
verify return:1
depth=1 C = US, O = Google Trust Services LLC, CN = GTS CA 1C3
verify return:1
depth=0 CN = www.google.com
verify return:1
write W BLOCK
---
Certificate chain
0 s:/CN=www.google.com
i:/C=US/O=Google Trust Services LLC/CN=GTS CA 1C3
-----BEGIN CERTIFICATE-----
MIIEiDCCA3CgAwIBAgIQdRpHZluxJPIKXzgYCivsdzANBgkqhkiG9w0BAQsFADBG
MQswCQYDVQQGEwJVUzEiMCAGA1UEChMZR29vZ2xlIFRydXN0IFNlcnZpY2VzIExM
QzETMBEGA1UEAxMKR1RTIENBIDFDMzAeFw0yMzAzMjgxNjU0NThaFw0yMzA2MjAx
NjU0NTdaMBkxFzAVBgNVBAMTDnd3dy5nb29nbGUuY29tMFkwEwYHKoZIzj0CAQYI
KoZIzj0DAQcDQgAE6kUnCX9wl4HT8yKdCpbrW4tFeoKS9lX53ZKskCFwqQiMDiSq
sS96H8y9QqCeKB3unHGY9Ulj/uVhfE/nDi5iAKOCAmgwggJkMA4GA1UdDwEB/wQE
AwIHgDATBgNVHSUEDDAKBggrBgEFBQcDATAMBgNVHRMBAf8EAjAAMB0GA1UdDgQW
BBR7H/f4Obli7c4lJ89UzABm/2VLFTAfBgNVHSMEGDAWgBSKdH+vhc3ulc09nNDi
RhTzcTUdJzBqBggrBgEFBQcBAQReMFwwJwYIKwYBBQUHMAGGG2h0dHA6Ly9vY3Nw
LnBraS5nb29nL2d0czFjMzAxBggrBgEFBQcwAoYlaHR0cDovL3BraS5nb29nL3Jl
cG8vY2VydHMvZ3RzMWMzLmRlcjAZBgNVHREEEjAQgg53d3cuZ29vZ2xlLmNvbTAh
BgNVHSAEGjAYMAgGBmeBDAECATAMBgorBgEEAdZ5AgUDMDwGA1UdHwQ1MDMwMaAv
oC2GK2h0dHA6Ly9jcmxzLnBraS5nb29nL2d0czFjMy9mVkp4YlYtS3Rtay5jcmww
ggEFBgorBgEEAdZ5AgQCBIH2BIHzAPEAdgDoPtDaPvUGNTLnVyi8iWvJA9PL0RFr
7Otp4Xd9bQa9bgAAAYcpW+C7AAAEAwBHMEUCIQDDaZBvW5LdpaxpZuxj4JhVo2Jz
9y1SCBuXqr74f3UWbgIgaBfwz+cLZ7WZjmBUXSpiRAAVwAc/wc12V+SDxxH+AK0A
dwB6MoxU2LcttiDqOOBSHumEFnAyE4VNO9IrwTpXo1LrUgAAAYcpW+ECAAAEAwBI
MEYCIQChIGffyBpm9AaI3x3vIr+aru09qseHIJVfPNWTuO9u+wIhANfvVjMetsSU
/6h2swcmKYvrekoT1mfH2PUXjJ6/j98gMA0GCSqGSIb3DQEBCwUAA4IBAQC1ZzJW
LcT22gys/bV0qgv7/F5/JVZvELGs3htiWpvUTX5MQG9Jm+w6Bu7yflAGdh2B1iCD
qpOHjlRi8NFk15+Q+9i9KNv0et0HZD3o5FXgb8JUxwJov2VbBOBudoHP44VynoHV
9k37j8YNyLjT1M9NuQTZsGA9ohQSSym5ygASZJNDEiuL4kf/fVx7L/9BcWXY3R8z
Bs8UyaY4gbdGBcz0GPutsJTgqqt3lio/qwqjl5y0ZrXQgy5m+g6Nepkwyg83JW+D
c77wcU9cGsJWai9GAJLu6gLGFia+8dGGsyAbmxSdxYPWO+80tf1V/kt6LMnRIyr2
gf3L6kmpMtgnogff
-----END CERTIFICATE-----
1 s:/C=US/O=Google Trust Services LLC/CN=GTS CA 1C3
i:/C=US/O=Google Trust Services LLC/CN=GTS Root R1
-----BEGIN CERTIFICATE-----
MIIFljCCA36gAwIBAgINAgO8U1lrNMcY9QFQZjANBgkqhkiG9w0BAQsFADBHMQsw
CQYDVQQGEwJVUzEiMCAGA1UEChMZR29vZ2xlIFRydXN0IFNlcnZpY2VzIExMQzEU
MBIGA1UEAxMLR1RTIFJvb3QgUjEwHhcNMjAwODEzMDAwMDQyWhcNMjcwOTMwMDAw
MDQyWjBGMQswCQYDVQQGEwJVUzEiMCAGA1UEChMZR29vZ2xlIFRydXN0IFNlcnZp
Y2VzIExMQzETMBEGA1UEAxMKR1RTIENBIDFDMzCCASIwDQYJKoZIhvcNAQEBBQAD
ggEPADCCAQoCggEBAPWI3+dijB43+DdCkH9sh9D7ZYIl/ejLa6T/belaI+KZ9hzp
kgOZE3wJCor6QtZeViSqejOEH9Hpabu5dOxXTGZok3c3VVP+ORBNtzS7XyV3NzsX
lOo85Z3VvMO0Q+sup0fvsEQRY9i0QYXdQTBIkxu/t/bgRQIh4JZCF8/ZK2VWNAcm
BA2o/X3KLu/qSHw3TT8An4Pf73WELnlXXPxXbhqW//yMmqaZviXZf5YsBvcRKgKA
gOtjGDxQSYflispfGStZloEAoPtR28p3CwvJlk/vcEnHXG0g/Zm0tOLKLnf9LdwL
tmsTDIwZKxeWmLnwi/agJ7u2441Rj72ux5uxiZ0CAwEAAaOCAYAwggF8MA4GA1Ud
DwEB/wQEAwIBhjAdBgNVHSUEFjAUBggrBgEFBQcDAQYIKwYBBQUHAwIwEgYDVR0T
AQH/BAgwBgEB/wIBADAdBgNVHQ4EFgQUinR/r4XN7pXNPZzQ4kYU83E1HScwHwYD
VR0jBBgwFoAU5K8rJnEaK0gnhS9SZizv8IkTcT4waAYIKwYBBQUHAQEEXDBaMCYG
CCsGAQUFBzABhhpodHRwOi8vb2NzcC5wa2kuZ29vZy9ndHNyMTAwBggrBgEFBQcw
AoYkaHR0cDovL3BraS5nb29nL3JlcG8vY2VydHMvZ3RzcjEuZGVyMDQGA1UdHwQt
MCswKaAnoCWGI2h0dHA6Ly9jcmwucGtpLmdvb2cvZ3RzcjEvZ3RzcjEuY3JsMFcG
A1UdIARQME4wOAYKKwYBBAHWeQIFAzAqMCgGCCsGAQUFBwIBFhxodHRwczovL3Br
aS5nb29nL3JlcG9zaXRvcnkvMAgGBmeBDAECATAIBgZngQwBAgIwDQYJKoZIhvcN
AQELBQADggIBAIl9rCBcDDy+mqhXlRu0rvqrpXJxtDaV/d9AEQNMwkYUuxQkq/BQ
cSLbrcRuf8/xam/IgxvYzolfh2yHuKkMo5uhYpSTld9brmYZCwKWnvy15xBpPnrL
RklfRuFBsdeYTWU0AIAaP0+fbH9JAIFTQaSSIYKCGvGjRFsqUBITTcFTNvNCCK9U
+o53UxtkOCcXCb1YyRt8OS1b887U7ZfbFAO/CVMkH8IMBHmYJvJh8VNS/UKMG2Yr
PxWhu//2m+OBmgEGcYk1KCTd4b3rGS3hSMs9WYNRtHTGnXzGsYZbr8w0xNPM1IER
lQCh9BIiAfq0g3GvjLeMcySsN1PCAJA/Ef5c7TaUEDu9Ka7ixzpiO2xj2YC/WXGs
Yye5TBeg2vZzFb8q3o/zpWwygTMD0IZRcZk0upONXbVRWPeyk+gB9lm+cZv9TSjO
z23HFtz30dZGm6fKa+l3D/2gthsjgx0QGtkJAITgRNOidSOzNIb2ILCkXhAd4FJG
AJ2xDx8hcFH1mt0G/FX0Kw4zd8NLQsLxdxP8c4CU6x+7Nz/OAipmsHMdMqUybDKw
juDEI/9bfU1lcKwrmz3O2+BtjjKAvpafkmO8l7tdufThcV4q5O8DIrGKZTqPwJNl
1IXNDw9bg1kWRxYtnCQ6yICmJhSFm/Y3m6xv+cXDBlHz4n/FsRC6UfTd
-----END CERTIFICATE-----
2 s:/C=US/O=Google Trust Services LLC/CN=GTS Root R1
i:/C=BE/O=GlobalSign nv-sa/OU=Root CA/CN=GlobalSign Root CA
-----BEGIN CERTIFICATE-----
MIIFYjCCBEqgAwIBAgIQd70NbNs2+RrqIQ/E8FjTDTANBgkqhkiG9w0BAQsFADBX
MQswCQYDVQQGEwJCRTEZMBcGA1UEChMQR2xvYmFsU2lnbiBudi1zYTEQMA4GA1UE
CxMHUm9vdCBDQTEbMBkGA1UEAxMSR2xvYmFsU2lnbiBSb290IENBMB4XDTIwMDYx
OTAwMDA0MloXDTI4MDEyODAwMDA0MlowRzELMAkGA1UEBhMCVVMxIjAgBgNVBAoT
GUdvb2dsZSBUcnVzdCBTZXJ2aWNlcyBMTEMxFDASBgNVBAMTC0dUUyBSb290IFIx
MIICIjANBgkqhkiG9w0BAQEFAAOCAg8AMIICCgKCAgEAthECix7joXebO9y/lD63
ladAPKH9gvl9MgaCcfb2jH/76Nu8ai6Xl6OMS/kr9rH5zoQdsfnFl97vufKj6bwS
iV6nqlKr+CMny6SxnGPb15l+8Ape62im9MZaRw1NEDPjTrETo8gYbEvs/AmQ351k
KSUjB6G00j0uYODP0gmHu81I8E3CwnqIiru6z1kZ1q+PsAewnjHxgsHA3y6mbWwZ
DrXYfiYaRQM9sHmklCitD38m5agI/pboPGiUU+6DOogrFZYJsuB6jC511pzrp1Zk
j5ZPaK49l8KEj8C8QMALXL32h7M1bKwYUH+E4EzNktMg6TO8UpmvMrUpsyUqtEj5
cuHKZPfmghCN6J3Cioj6OGaK/GP5Afl4/Xtcd/p2h/rs37EOeZVXtL0m79YB0esW
CruOC7XFxYpVq9Os6pFLKcwZpDIlTirxZUTQAs6qzkm06p98g7BAe+dDq6dso499
iYH6TKX/1Y7DzkvgtdizjkXPdsDtQCv9Uw+wp9U7DbGKogPeMa3Md+pvez7W35Ei
Eua++tgy/BBjFFFy3l3WFpO9KWgz7zpm7AeKJt8T11dleCfeXkkUAKIAf5qoIbap
sZWwpbkNFhHax2xIPEDgfg1azVY80ZcFuctL7TlLnMQ/0lUTbiSw1nH69MG6zO0b
9f6BQdgAmD06yK56mDcYBZUCAwEAAaOCATgwggE0MA4GA1UdDwEB/wQEAwIBhjAP
BgNVHRMBAf8EBTADAQH/MB0GA1UdDgQWBBTkrysmcRorSCeFL1JmLO/wiRNxPjAf
BgNVHSMEGDAWgBRge2YaRQ2XyolQL30EzTSo//z9SzBgBggrBgEFBQcBAQRUMFIw
JQYIKwYBBQUHMAGGGWh0dHA6Ly9vY3NwLnBraS5nb29nL2dzcjEwKQYIKwYBBQUH
MAKGHWh0dHA6Ly9wa2kuZ29vZy9nc3IxL2dzcjEuY3J0MDIGA1UdHwQrMCkwJ6Al
oCOGIWh0dHA6Ly9jcmwucGtpLmdvb2cvZ3NyMS9nc3IxLmNybDA7BgNVHSAENDAy
MAgGBmeBDAECATAIBgZngQwBAgIwDQYLKwYBBAHWeQIFAwIwDQYLKwYBBAHWeQIF
AwMwDQYJKoZIhvcNAQELBQADggEBADSkHrEoo9C0dhemMXoh6dFSPsjbdBZBiLg9
NR3t5P+T4Vxfq7vqfM/b5A3Ri1fyJm9bvhdGaJQ3b2t6yMAYN/olUazsaL+yyEn9
WprKASOshIArAoyZl+tJaox118fessmXn1hIVw41oeQa1v1vg4Fv74zPl6/AhSrw
9U5pCZEt4Wi4wStz6dTZ/CLANx8LZh1J7QJVj2fhMtfTJr9w4z30Z209fOU0iOMy
+qduBmpvvYuR7hZL6Dupszfnw0Skfths18dG9ZKb59UhvmaSGZRVbNQpsg3BZlvi
d0lIKO2d1xozclOzgjXPYovJJIultzkMu34qQb9Sz/yilrbCgj8=
-----END CERTIFICATE-----
---
Server certificate
subject=/CN=www.google.com
issuer=/C=US/O=Google Trust Services LLC/CN=GTS CA 1C3
---
No client certificate CA names sent
Server Temp Key: ECDH, X25519, 253 bits
---
SSL handshake has read 4279 bytes and written 374 bytes
---
New, TLSv1/SSLv3, Cipher is AEAD-CHACHA20-POLY1305-SHA256
Server public key is 256 bit
Secure Renegotiation IS NOT supported
Compression: NONE
Expansion: NONE
No ALPN negotiated
SSL-Session:
Protocol : TLSv1.3
Cipher : AEAD-CHACHA20-POLY1305-SHA256
Session-ID:
Session-ID-ctx:
Master-Key:
Start Time: 1681263370
Timeout : 7200 (sec)
Verify return code: 0 (ok)
---(via openssl s_client -connect www.google.com:443 -servername www.google.com -showcerts)
i.e. it sends the leaf, then an intermediate, and then a cross-sign for the intermediate, but no actual root CA. Chrome shows the root CA in its cert viewer, but that's only because of a successful out-of-band chain construction 🙂
woodruffw
commented
1 year ago I also talked a bit with @reaperhulk about the RFC language referenced in https://github.com/sigstore/cosign/issues/2143#issuecomment-1212075858, and we think that this part:
Because certificate validation requires that trust anchors be distributed independently, a certificate that specifies a trust anchor MAY be omitted from the chain, provided that supported peers are known to possess any omitted certificates.
Refers specifically to intermediates and possible client-side intermediate caching, given that the first part refers explicitly to the trust anchors (i.e. root CAs) being distributed independently. And that would comport with that www.google.com actually sends over TLS 🙂
haydentherapper
commented
1 year ago Couple thoughts: is this a breaking change? Possibly, because clients would expect a root cert in the final spot? But if the clients are just python and JavaScript now, and I think you both load in presented certs into cert pools, maybe we’re good.
do we need an identifier, the subject key ID of the root, if the root is omitted? The intermediate should contain the authority key ID for chain building, but in byo pki cases, it may not. Either we consider that a hard requirement or provide the SKID of the root, like how we provide the log ID for the tlogs.
kommendorkapten
commented
1 year ago Just for some context. We are aware that shipping the entire cert chain can be seen as problematic. The client spec clarifies how to verify an artifact via the bundle:
Perform certification path validation (RFC 5280 §6) of the returned certificate chain with the pre-distributed Fulcio root certificate(s) as a trust anchor.
But if we feel that there is a risk of creating faulty implementations, we may revisit the bundle format. We decided to not allow any public keys to be stored in the bundle for exactly this reason, it could lead to footguns. As the spec mandates that anchoring trust to an OOB delivered root, it was deemed safe to ship the entire chain.
edit: Added link to the Sigstore client spec
woodruffw
commented
1 year ago Couple thoughts: is this a breaking change? Possibly, because clients would expect a root cert in the final spot? But if the clients are just python and JavaScript now, and I think you both load in presented certs into cert pools, maybe we’re good.
I think this is arguably a breaking change, but I think it's also a safe change to make for the reasons you've mentioned: both the Python and JS clients (thankfully) do the correct thing already and don't need/access the other chain members at all. We're also technically below 1.0 for the protobuf specs, so this kind of "breaking only in spirit, not in practice" change is arguably okay 😉
do we need an identifier, the subject key ID of the root, if the root is omitted? The intermediate should contain the authority key ID for chain building, but in byo pki cases, it may not.
I might be underthinking it, but I think we'd be okay without any additional key ID -- the intermediate (or even just the leaf really, since for the public deployments the intermediates are also shipped via TUF) contains sufficient context for chain building, since it tells us the next step in the chain via its AKI extension.
(This is actually an argument IMO for dropping the entire chain, and stipulating that even the intermediate(s) must be delivered over TUF or a similar trusted mechanism. But I think that's completely separate from this, and doesn't represent a footgun the way this does.)
But if we feel that there is a risk of creating faulty implementations, we may revisit the bundle format. We decided to not allow any public keys to be stored in the bundle for exactly this reason, it could lead to footguns. As the spec mandates that anchoring trust to an OOB delivered root, it was deemed safe to ship the entire chain.
This reasoning makes sense to me, but I personally fall on the side of thinking that this is a footgun! I had to double (and then triple-check) sigstore-python to make sure I wasn't accidentally doing this, and I think the only reason I didn't do it inadvertently is because I shoehorned the bundle's deserialization into our pre-existing data models (which only take the leaf, and not any other parts of the chain). That's purely coincidental though; it I had written the bundle support from scratch I would likely have skipped that part of the client spec and accidentally shoved everything into the chain building context!
kommendorkapten
commented
1 year ago That's purely coincidental though; it I had written the bundle support from scratch I would likely have skipped that part of the client spec and accidentally shoved everything into the chain building context!
I'm not gonna say this is a solution, as it's not (you can't get high quality or security via only testing). But when we get more traction on the conformance testing, I would envision that we have test-cases for scenarios like this -- bundle with roots that does not exist in the trust root and other negative test cases.
Shipping only the leaf certificate makes the most sense from ease of client implementation, and keeps the bundle smaller. I'm wondering if we know the reasoning for the inital Sigstore design to ship the entire chain? As we did replicate that behaviour when creating the bundle. @haydentherapper @znewman01 maybe have more historical context?
woodruffw
commented
1 year ago I'm not gonna say this is a solution, as it's not (you can't get high quality or security via only testing). But when we get more traction on the conformance testing, I would envision that we have test-cases for scenarios like this -- bundle with roots that does not exist in the trust root and other negative test cases.
Agreed! This is a another strong argument for us pushing forward on bundle test cases in sigstore-conformance (cc @tetsuo-cpp).
I'd also be curious to understand the context/historical reasons for including anything besides the leaf -- I can imagine in making some BYO PKI setups a little easier, but I'm a little hazy there 🙂
haydentherapper
commented
1 year ago Correct, the context was BYO PKI, for those that only provide their root out of band and view intermediates as just chain builders. FWIW, this has been a little messy in Cosign, because we have logic where we first check the TUF root for the intermediates and if it's not present, then we check the bundle/OCI manifest. Sigstore's TUF root does ship the entire chain, and it would be my preference to mandate this because TUF becomes the revocation mechanism rather than deal with CRLs. However we did get some feedback for Cosign where someone said they wanted to use CRLs - While I'd prefer to not support that, if there's enough interest for BYO PKI, we might do so.
One way we could do it is having a required field for the leaf and an optional field for the intermediate chain. We could also do it is a one-off, either setting just the leaf cert or the chain (leaf + intermediates). For the public deployment of sigstore, we'll set only the leaf cert in the bundle, and rely on the TUF trusted root for providing the chain.
This shouldn't complicate client logic any more than it already is. So it becomes:
Thoughts @woodruffw @kommendorkapten ?
the intermediate (or even just the leaf really, since for the public deployments the intermediates are also shipped via TUF) contains sufficient context for chain building, since it tells us the next step in the chain via its AKI extension.
Not necessarily for BYO PKI cases, unless we mandate that intermediates contain the AKI. They should, and something like RFC5280 mandates it iirc. I'm fine saying that chain building isn't possible without the AKI so it should be mandated.
woodruffw
commented
1 year ago Thoughts @woodruffw @kommendorkapten ?
Making sure I understand: does this mean that in one scenario the bundle would still include the full chain, including the root, or that the chain would only be the leaf + intermediate(s)? If it's the latter, that sounds great to me!
Not necessarily for BYO PKI cases, unless we mandate that intermediates contain the AKI. They should, and something like RFC5280 mandates it iirc. I'm fine saying that chain building isn't possible without the AKI so it should be mandated.
Yeah, I think it would be nice for us to ratchet down on RFC 5280 here and mandate the AKI. @reaperhulk pointed out to me that AKIs aren't technically necessary for chain building (OpenSSL will also compare DNs, apparently), but we have the opportunity to do everything right from scratch 🙂
kommendorkapten
commented
1 year ago Making sure I understand: does this mean that in one scenario the bundle would still include the full chain, including the root, or that the chain would only be the leaf + intermediate(s)? If it's the latter, that sounds great to me!
My preference is if we could make this via a patch update, i.e only update the comment to state:
// The chain of certificates, with indices 0 to n. // The first certificate in the array must be the leaf // certificate used for signing. Optional intermediate certificates // must be stored as offset 1 to n, and the root certificate // MUST not be present.
This would not break the message format, and clients that ignores ~leaf~ intermediate certificates can just grab the first cert.
edit: s/leaf/intermediate/
kommendorkapten
commented
1 year ago Requiring SKI and AKI sounds like a good plan 👍
woodruffw
commented
1 year ago This would not break the message format, and clients that ignores leaf certificates can just grab the first cert.
Oh, that's a good point -- yeah, preserving the wire format would be nice here.
haydentherapper
commented
1 year ago +1, I'm good with that as a plan (That's effectively what Cosign now does anyways, if the intermediates exist in the trust root)
haydentherapper
commented
1 year ago Just realized that this is reused in the trustroot proto too, the chains will contain a root and optionally contain a leaf (the leaf is included for the TSA, but not for Fulcio obviously). So we might need to rethink this. Maybe just update the comment in https://github.com/sigstore/protobuf-specs/blob/main/protos/sigstore_bundle.proto#L54?
woodruffw
commented
1 year ago WDYT about two identical (in terms of wire layout) types, but with different names to distinguish their purposes? The trust root could continue to use X509CertificateChain, while the trust bundle could use X509PartialChain or similar.
(If you think that's too complex or not worth the additional codegen, then I'm good with just updating the comment + adding additional client checks during signing and verification that ensure a root isn't present!)
kommendorkapten
commented
1 year ago I would vote on what @haydentherapper proposes, update the comment in the bundle that the root must not be present. This is a non-breaking change.
My first gut feeling was to require a client to abort if the bundle contains a root cert, but that could cause problems if a new client verifies an old bundle, so that would be a breaking change.
haydentherapper
commented
1 year ago Do we then need to instruct clients to inspect all certificates in the bundle and ignore any roots if present? So then the comment becomes "The bundle certificate chain SHOULD contain a leaf and MAY contain a set of intermediates. The bundle SHOULD NOT contain the root, but if present, the client MUST ignore the root for verification purposes. The client MUST confirm if the last certificate in the chain is a root or not, by comparing the certificate's Subject and Issuer fields"
Thoughts?
loosebazooka
commented
1 year ago FWIW, I would prefer to keep the full cert chain in the bundle. The java logic we have now expects a full chain, and it verifies that the chain concludes in a known trust anchor. But maybe the Java logic just worked out conveniently that way and I'm being lazy asking for no changes :shrug:
Removing the chain seems like I now have a more complex lookup for the intermediate/root, and do a reconstruction. Is there enough information in the bundle to make that easy?
We already should be doing a few checks here:
I think we can still check that clients are behaving via conformance?
woodruffw
commented
1 year ago My first gut feeling was to require a client to abort if the bundle contains a root cert, but that could cause problems if a new client verifies an old bundle, so that would be a breaking change.
I think we can have our cake and eat it too: if we set an "epoch" after which clients must not include the root in their bundles, then any previously generated bundle (i.e., based on leaf issuance time) can be given a warning and any subsequently generated bundle can produce a hard error.
(This will also make inclusion in the conformance suite easier, since we'll be able to confirm that verification actually fails with a "dangerous" bundle.)
The other option there (which is also reasonable, IMO) is to specify that the bundle MUST NOT contain a root, but that for legacy purposes clients MUST be prepared to handle bundles that contain roots, and that clients MUST handle that root by explicitly ignoring it during chain building. This would be consistent with what RFC 5280 and similar X.509/PKI RFCs do around existing public certificates that don't conform to specs: they specify that conforming implementations MUST NOT do a thing, but that conforming implementations MUST also be prepared to handle well-known non-conforming situations.
FWIW, I would prefer to keep the full cert chain in the bundle. The java logic we have now expects a full chain, and it verifies that the chain concludes in a known trust anchor. But maybe the Java logic just worked out conveniently that way and I'm being lazy asking for no changes 🤷
Yeah, I think it's purely convenience (and a little dangerous) that it worked out that way 🙂. TLS and other ecosystems that I'm aware of all discourage sending full chains, because "accept the chain and check" is much easier to accidentally mess up than "sort into trusted and untrusted and build the chain."
Removing the chain seems like I now have a more complex lookup for the intermediate/root, and do a reconstruction. Is there enough information in the bundle to make that easy?
Yep! Depending on what you're doing (I know very little about Java cryptography), there are two routes I can think of:
You can either use a "store-style" API for chain-building, which your cryptographic library may (should?) already provide: your trusted certificates (from TUF) go in, followed by your untrusted leaf and intermediates (checked explicitly for not containing any root CAs), and then as the store to perform the chain building. That's what sigstore-python does, with OpenSSL's X509_STORE performing all of the logic internally.
You can do the above manually, by starting with your trusted set (from TUF), building then chain up using each untrusted member's AKIs/SKIs, and verifying each signature. This also requires you to verify pathlen and other CA constraints, for strict compliance. I believe this is what sigstore-js does.
kommendorkapten
commented
1 year ago You can either use a "store-style" API for chain-building, which your cryptographic library may (should?) already provide: your trusted certificates (from TUF) go in, followed by your untrusted leaf and intermediates (checked explicitly for not containing any root CAs), and then as the store to perform the chain building. That's what sigstore-python does, with OpenSSL's X509_STORE performing all of the logic internally.
This is true for the sigstore go library too (go has a method for chain building). Sigstore-js implements this)
znewman01
commented
1 year ago Sorry, just catching up on this.
Overall I'm a big +1 on taking the root out of the bundle, very carefully. I think it removes a big footgun. Given that:
I don't actually think a "flag day" is necessary: in terms of backwards compatibility, I don't see a big deal with continuing to accept chains that have the root in them indefinitely. The footgun that we're looking to avoid is depending on this root and accidentally accepting it even if it doesn't come from a trusted source. IIRC a lot of "store-style" APIs will handle this nicely out of the box—does that make sense, esp. @woodruffw ?
loosebazooka
commented
1 year ago signers SHOULD NOT include the root certs.
so only leafs? or everything but root?
kommendorkapten
commented
1 year ago I think @znewman01 proposes makes sense (which is very similar to what @woodruffw proposed). It follows the robust principle, be liberate in what you accept (i.e the root may be present but ignored), and strict in what you produce (no roots in the bundle). As we don't have any mass adoption yet, and the clients I'm aware of would not break if we start to remove the root certificates from the bundle we can roll this out "easily". (sigstore-js would not break, and my understanding is that the the Python client would still continue to work.)
For intermediates, we would need to specify the expectations. There is a desire to support the BYO PKI scenario, where the intermediates may not be available via the OOB method. As this is mostly for the non public-good instance, I think we should support it to make it easier to adopt the bundle. Unless there is a strong will against, I would think it simplifies that if we always require the intermediates to be present in the bundle. Thoughts?
loosebazooka
commented
1 year ago Yeah I'm okay with anything as long as we have very strong guidance around expectations.
znewman01
commented
1 year ago signers SHOULD NOT include the root certs.
so only leafs? or everything but root?
This is getting complicated enough that it warrants discussion at the next sig-clients meeting, so the below should just be considered my opinion.
Yeah I'm okay with anything as long as we have very strong guidance around expectations.
+1, we'll add this in both the client spec and the protobuf-spec docs.
tl;dr: I think we want to recommend that signers only include things below the intermediate (given that the public good instance sets pathlen: 0 at the intermediate, this means just the leaf cert). But we need to do some extra checking to avoid "secret" intermediates that don't come from TUF.
This is a little wonky because we distribute the intermediates using TUF as well (CC @asraa, @haydentherapper). I guess strictly speaking, the verification procedure for the public good instance should be:
Get the raw key material from TUF. This includes the following artifacts:
These artifacts also have associated metadata, describing which ones were valid when.
Resolve the raw key material and metadata into a "trust root." This should include older certificates as well so we can do Log Verification with Expired Targets. Rather than combining these into one big certificate pool, I'd probably implement this by keeping a dict from
This is actually pretty subtle and we should make sure the root-signing docs point to a spec for doing it.
Verify against the "trust root" using the "store-style APIs" mentioned above, but "ignore" the timestamp:
Additional check: all intermediates should be in the "trust" root. The way I'd do this in the Golang API is:
RootsIntermediates.You could also just do this as a post-regular-validation extra step too.
woodruffw
commented
1 year ago IIRC a lot of "store-style" APIs will handle this nicely out of the box—does that make sense, esp. @woodruffw ?
I'm not sure about Java and other ecosystems, but this unfortunately isn't true of OpenSSL (or other cryptographic libraries that have inherited OpenSSL's baggage): store construction isn't divided into trusted and untrusted components, because the act of adding a cert to the store is considered equivalent to trusting it.
That's one of the footguns I'd like us to define away: clients who do something naive like using an OpenSSL (or LibreSSL, etc.)-style X509_STORE without adequate pre-checks should not be able to (easily) put themselves in an exploitable state using nothing but a well-formed Sigstore bundle.
To that end: what do you think about my other proposal (i.e., the non-"flag day" one)? I think making this into a MUST NOT rather than a SHOULD NOT on the bundle generation side will make the conformance testing story a little easier, while still preserving backwards compatibility with bundles that have already been generated.
Edit: Also, 👍 to discussing this at the clients meeting on May 2nd!
znewman01
commented
1 year ago I'm not sure about Java and other ecosystems, but this unfortunately isn't true of OpenSSL (or other cryptographic libraries that have inherited OpenSSL's baggage): store construction isn't divided into trusted and untrusted components, because the act of adding a cert to the store is considered equivalent to trusting it.
That's one of the footguns I'd like us to define away: clients who do something naive like using an OpenSSL (or LibreSSL, etc.)-style X509_STORE without adequate pre-checks should not be able to (easily) put themselves in an exploitable state using nothing but a well-formed Sigstore bundle.
I guess my concern is that a syntactically-correct but logically-incorrect Sigstore bundle can exploit this as well: we can't rely on clients not to do something. So then we have to add the check anyway.
I think this still mostly works with openssl-style APIs: you should add only the root as a trusted element of the store. Then:
kommendorkapten
commented
1 year ago Some of the questions overlaps quite much with the client spec update I worked on end of last year, related to verification against the trusted root. I feel like it's time to revive that content and add it to the client spec for discussion, the larger topic at that time not being resolved was related to polices (which I think we can punt on for now).
The major topic discussed here (certificate chain building) was not covered in detail (it relied on the existing way cosign does it, i.e a but underspecified 😄) . But as we are seeing more interest in the trust bundle now, and the bundle is also in production TUF root I think it's time to hash out the details. To make sure we have enough written documentation before the next client spec meeting, I can extract the previous details and the primary topic in this PR and suggest it into the client spec.
haydentherapper
commented
1 year ago The split seems to be between verification with the public good instance (where intermediates will always be present in TUF) and private instances. Would it make sense that environment be encoded in the bundle? At the very least, we can mark if we're verifying something from PGI vs elsewhere and have stricter verification logic for PGI.
znewman01
commented
1 year ago In terms of the bundle, we expect the inputs to a verification to match the Input message: https://github.com/sigstore/protobuf-specs/blob/b6d25769ec1d565ba45fd121310daf4ab963da9b/protos/sigstore_verification.proto#L119-L136
This includes the environment, encoded in a TrustedRoot: https://github.com/sigstore/protobuf-specs/blob/b6d25769ec1d565ba45fd121310daf4ab963da9b/protos/sigstore_trustroot.proto#L59-L88
as well as general verification options:
It would make sense to me to make this a property of the TrustedRoot; something like CertificateAuthority.allow_other_intermediates: https://github.com/sigstore/protobuf-specs/blob/b6d25769ec1d565ba45fd121310daf4ab963da9b/protos/sigstore_trustroot.proto#L43-L57
kommendorkapten
commented
1 year ago Would it make sense that environment be encoded in the bundle? At the very least, we can mark if we're verifying something from PGI vs elsewhere and have stricter verification logic for PGI.
I'm not sure 🤔 This value is unauthenticated and so could be misused. The verifier would still require to make a decision based on a policy on how to verify, so I think @znewman01 's proposal on adding a parameter to the CertificateAuthority is easier to understand and work with. And we put all the decisions to the verifier where they should be.
edit: s/your/znewman01/
woodruffw
commented
1 year ago I guess my concern is that a syntactically-correct but logically-incorrect Sigstore bundle can exploit this as well: we can't rely on clients not to do something. So then we have to add the check anyway.
Yep, agreed that the check needs to happen anyways. I see this more as a way to introduce additional test cases, with explicit success and failure states -- with a MUST NOT, we can add a conformance test case that produces a failure if a client produces a bundle containing a root, whereas SHOULD NOT would mean that the conformance suite is enforcing a superset of restrictions 🙂
I think this still mostly works with openssl-style APIs: you should add only the root as a trusted element of the store.
I might have misled you here: there are no "trusted" or "untrusted" members of an OpenSSL-style store. All members are considered trusted, since the act of adding a member is considered equivalent to trusting it. That's why the "leaf + intermediates + root" case is potentially dangerous with OpenSSL/LibreSSL/BoringSSL/etc.: a client might think that they're only adding untrusted chain-building components, when what they're really telling the API to do is to trust everything 🙃
kommendorkapten
commented
1 year ago That's why the "leaf + intermediates + root" case is potentially dangerous with OpenSSL/LibreSSL/BoringSSL/etc.: a client might think that they're only adding untrusted chain-building components, when what they're really telling the API to do is to trust everything 🙃
What sigstore-js does is that it verifies each certificate in the created chain, that it is included in the trusted store provided before using the chain to do path verification.
Approx algorithm is:
So I feel like a similar algorithm should be fairly easy to implement with an "OpenSSL-style store".
znewman01
commented
1 year ago Yep, agreed that the check needs to happen anyways. I see this more as a way to introduce additional test cases, with explicit success and failure states -- with a MUST NOT, we can add a conformance test case that produces a failure if a client produces a bundle containing a root, whereas SHOULD NOT would mean that the conformance suite is enforcing a superset of restrictions 🙂
ACK, makes sense.
I think this still mostly works with openssl-style APIs: you should add only the root as a trusted element of the store.
I might have misled you here: there are no "trusted" or "untrusted" members of an OpenSSL-style store. All members are considered trusted, since the act of adding a member is considered equivalent to trusting it. That's why the "leaf + intermediates + root" case is potentially dangerous with OpenSSL/LibreSSL/BoringSSL/etc.: a client might think that they're only adding untrusted chain-building components, when what they're really telling the API to do is to trust everything 🙃
No, I think I was just a little unclear. I still think this works, it's just a little awkward. We should probably add an "implementation note" in the client spec.
Approx algorithm is:
- Combine trusted roots and intermediates with the root and intermediates from the bundle.
- Deduplicate the new set of certs
- Find all paths for the leaf certificate
- Filter and keep paths that are anchored to the provided trusted root certs
- Use the shortest path from the filtered ones
So I feel like a similar algorithm should be fairly easy to implement with an "OpenSSL-style store".
If I'm reading you correctly, this means that a CA could use an intermediate that's not from the trust root, which I think is the whole point of embedded intermediates in the trust root. This point could definitely use clarification in the spec. We can also consider adding "implementation notes" or so.
kommendorkapten
commented
1 year ago If I'm reading you correctly, this means that a CA could use an intermediate that's not from the trust root, which I think is the whole point of embedded intermediates in the trust root. This point could definitely use clarification in the spec. We can also consider adding "implementation notes" or so.
Yes, that may be the case. I'll have to dig more into this, I'm not very used to JS so I might have misunderstood something. But agreed, the entire point of having the intermediates in the trusted root is to make sure that we can revoke them if needed, and so we should not accept unknowns intermediates in the bundle.
woodruffw
commented
1 year ago and so we should not accept unknowns intermediates in the bundle.
This sounds like another good SHOULD restriction: except for the BYO PKI case, bundle should really only ever contain the leaf cert (since everything else will be supplied by the trust root).
(I believe sigstore-python doesn't use any intermediates from the bundle currently, so we shouldn't have any incorrect potential chains here.)
woodruffw
commented
1 year ago Taking a step back for a second: the lack of clarity (including on my part!) about what each client currently does indicates that we need to start with tests here -- this is exactly the kind of thing the conformance suite is intended for, and we should take advantage of it.
So, as an actionable proposal: we should create some test vectors ensuring that current implementations do not trust random intermediates or roots that don't also appear in the trust root, and that they can gracefully handle bundles that only contain leaves.
CC @tetsuo-cpp, since I know you were working on bundle support in the test suite.
znewman01
commented
1 year ago Okay, I've been thinking about this a bit more. I think it's possibly clearer to view verification as a two-step process. Imagine the following structure:
- root (in TUF)
- int1 (in TUF, pathlen=0)
- leaf1
- int2 (in TUF, pathlen=1)
- int2a
- leaf2To verify:
int1, int2) as the roots for verification.Then, the guidance for clients: clients SHOULD include the leaf up to (but not including) the Fulcio certificate in the bundle. So: leaf2->int2a, leaf1.
This has a few advantages. First, the client spec becomes much simpler: just use the provided Fulcio certs. We avoid departing from RFC 5280 by requiring any extra checks. We don't need to filter the certificates from the chain or anything (I worry a client might forget).
Second, all of the complexities about which intermediates to trust can get moved into the "policy for distributing roots." This makes sense, as that's a peculiarity of our deployment, not a fundamental part of the model. If you want to avoid surprise intermediates, you should set pathLen: 0 in the Fulcio intermediates. We should still test these complexities, but they can be in a test vector for resolving the intermediates.
EDIT: there is one potential issue. If the root CA expiration is before the intermediate's expiration, we need to propagate that information.
kommendorkapten
commented
1 year ago Okay, I've been thinking about this a bit more. I think it's possibly clearer to view verification as a two-step process. Imagine the following structure: ... This has a few advantages. First, the client spec becomes much simpler: just use the provide Fulcio certs. We avoid departing from RFC 5280 by requiring any extra checks. We don't need to filter the certificates from the chain or anything (I worry a client might forget).
I think this makes sense, but that would be for public good Sigstore, right? As in the BYO PKI scenario Fulcio may not even be used. Will this be clear enough in the specification?
But comparing this with the other alternative proposed in this discussion. Having a parameter (either on the CertificateAuthority or on the ArtifactVerificationOptions ) that dictates how chain building works (if we allow any intermediates to be used from the bundle or not). My understanding is that this would still be RFC 5280 style chain building, it's a question based on which material we build the chain. This is what I think you refer to here:
Second, all of the complexities about which intermediates to trust can get moved into the "policy for distributing roots."
So that I understand, your comment is mostly around how the spec clarifies what certificates goes into the bundle? For public good instance we can for sure describe what you propose, but I'm afraid that we may end up underspecified for the BYO PKI. I'm thinking if we can describe the "default policy" for the public good instance (i.e not trust anything except the leaf cert in the bundle), and provide recommendation for a generic use case and clearly describe that each verifier MAY have their own policy on chain building/certificate verification, and those expectations should be communicated OOB before artifacts are signed. I don't think that's too much to ask that you understand how signatures are being verified before you sign them.
znewman01
commented
1 year ago I wrote a bunch below but it's pretty rambly, hopefully this clears some things up but if not we can discuss.
I think this makes sense, but that would be for public good Sigstore, right? As in the BYO PKI scenario Fulcio may not even be used. Will this be clear enough in the specification?
I don't understand why you'd need the Sigstore specification for BYO PKI. It's up to you in that circumstance to figure out how to validate. If you have BYO PKI, are your certs even short-lived? We don't know that, and can't write a one-size fits all spec for you. I'm generally opposed to over-optimizing for this use case in the specifications/bundle format.
I'm thinking if we can describe the "default policy" for the public good instance (i.e not trust anything except the leaf cert in the bundle), and provide recommendation for a generic use case and clearly describe that each verifier MAY have their own policy on chain building/certificate verification, and those expectations should be communicated OOB before artifacts are signed.
I guess my point is that, for the "Sigstore Client Spec," describing it as "get some roots and some intermediates, then do some extra checks that are deployment-specific to build a path from a leaf up to the roots" is really complicated both to explain and implement. Whereas "(1) figure out which Fulcio certs you trust, and use those in the trusted certificate store; (2) you MUST validate like you'd validate any other certificate" is friendlier to openssl-style APIs (which are quite common) and moves all the complexity about figuring out what to trust over to the deployment ("Public Deployment Spec"), which is good because it's a deployment-dependent process.
Then we don't need a configuration option for how to validate, we just have to construct the trust root correctly.
So that I understand, your comment is mostly around how the spec clarifies what certificates goes into the bundle? For public good instance we can for sure describe what you propose, but I'm afraid that we may end up underspecified for the BYO PKI.
My comment is mostly about how the "Client Spec" describes the certificate validation process (section Verification -> Certificates).
The concern is that if we say "use the roots and the intermediates from the trust root, and also you can take some intermediates from the certificate chain but how you do that is totally up to you but you do have to be careful about it" is confusing (I think @woodruffw points out that this is really confusing for openssl-style verification APIs).
BUT it's pretty easy for us to say in the client spec "just validate like normal, chaining up to the Fulcio cert that you got securely out of band."
Then, if we want to enforce the rule "only use intermediates that come from TUF," we do that when describing certificate distribution (which is a property of the Public Good Deployment):
I don't think that's too much to ask that you understand how signatures are being verified before you sign them.
That's why a simple rule like "always chain up to the Fulcio cert, ignore everything above that" is nice. That should always be true. Then "how signatures are being verified" is part of "get the keys securely, how you do that is out of scope."
I don't know if we're disagreeing really, I just really want the "Client Spec" to be simple and prescriptive about this question and the quirks of how this actually works in Sigstore can go in the "Public Deployment Spec."
kommendorkapten
commented
1 year ago I don't know if we're disagreeing really, I just really want the "Client Spec" to be simple and prescriptive about this question and the quirks of how this actually works in Sigstore can go in the "Public Deployment Spec."
No, I believe we are trying to achieve the same thing :)
It's the BYO PKI adds confusion IMHO. And I think that can be left outside of the "Client Spec", i.e if you have BYO PKI you are on your own. The question as I see it is if we have a language in the comments in this repo that makes BYO PKI conflicting/confusing, would we update that, or keep the language in this repo compliant with the "Client Spec"? If the language is more general in this repo we can "tighten/clarify" in the "Client spec".
znewman01
commented
1 year ago It's the BYO PKI adds confusion IMHO. And I think that can be left outside of the "Client Spec", i.e if you have BYO PKI you are on your own.
👍
The question as I see it is if we have a language in the comments in this repo that makes BYO PKI conflicting/confusing, would we update that, or keep the language in this repo compliant with the "Client Spec"? If the language is more general in this repo we can "tighten/clarify" in the "Client spec".
IMO, we should prioritize the full Sigstore flow in this repo.
Then, we can have guidance about BYO PKI which basically says "hey, use this Client Spec flow with modifications X and Y and you might want to use the protobuf-specs bundle, disregarding Z." We don't want to leave BYO PKI folks in the dark, but at the same time I don't think we can anticipate every need they might have.
kommendorkapten
commented
1 year ago IMO, we should prioritize the full Sigstore flow in this repo.
Then, we can have guidance about BYO PKI which basically says "hey, use this Client Spec flow with modifications X and Y and you might want to use the protobuf-specs bundle, disregarding Z." We don't want to leave BYO PKI folks in the dark, but at the same time I don't think we can anticipate every need they might have.
💯
haydentherapper
commented
1 year ago LGTM on that, let’s make that explicit in the documentation.
woodruffw
commented
1 year ago Okay, so to summarize the expected doc changes here:
MUST NOT indicating that the cert chain must not contain a root CASHOULD NOT indicating that the cert chain should not contain intermediate CAs that should appear in an independent root of trustMUST indicating that verifying clients must validate the chain carefully to ensure that it upholds the invariants above, but that they SHOULD be prepared to handle old or non-complying bundles that do include the root and/or intermediates (maybe this belongs somewhere else?)If that sounds good, then I can create a PR and close this out!
haydentherapper
commented
1 year ago LGTM, (3) seems necessary to avoid breaking backwards compatibility. Should it be MUST be prepared to handle old bundles? I think our intention is "new clients SHOULD, existing clients with significant usage MUST"?
woodruffw
commented
1 year ago LGTM, (3) seems necessary to avoid breaking backwards compatibility. Should it be
MUST be prepared to handle old bundles? I think our intention is "new clients SHOULD, existing clients with significant usage MUST"?
Yes, good point!
I was thinking about this some more:
https://github.com/sigstore/protobuf-specs/blob/4dbf10bc287d76f1bfa68c05a78f3f5add5f56fe/protos/sigstore_common.proto#L167-L175
...and I think, as specified, it presents a sharp edge towards Sigstore clients: the naive thing to do would be to shove the entire cert chain into the verifying context (i.e. a thing that looks roughly like an OpenSSL
X509_STORE). However, if a client does this, then Sigstore signatures effectively become self-signed: any malicious user can insert a cert chain that includes a root CA that isn't one of the trusted ones, and chain validation will resolve against that CA instead.Clients still need to handle this their own (they should verify the expected contents of the bundle's cert chain), but as a risk reduction strategy: what does everyone think about specifying the root certificate out of the bundle's cert chain? That would turn this from a silent "gotcha" into a spec violation, one that we could write conformance tests against.