Add support for x509 certificates in DSSE #50

asraa · 2022-04-27T14:33:53Z

Signed-off-by: Asra Ali [email protected]

Adds an optional field for specifying an x509 certificate to a signature. This is unathenticated content used to additionally verify that the certificate for the public key used for the signature chains back to a trusted root.

Fixes #42

Open questions:

IF the certificate is defined, MUST the clients verify it against a root? I think yes. (But what if clients don't know or care about the root?)
An alternative here would be to define a way to collect the PKI/verification material in a layer/envelope around DSSE and use the keyID to hint for the associated cert.

Also, see sigstore/cosign#1743

Related:
https://github.com/in-toto/ITE/tree/master/ITE/7#metadata-signtaures

@laurentsimon @MarkLodato @dlorenc

Signed-off-by: Asra Ali <[email protected]>

protocol.md

Signed-off-by: Asra Ali <[email protected]>

mnm678 · 2022-04-27T17:50:54Z

Open questions:

* IF the certificate is defined, MUST the clients verify it against a root? I think yes. (But what if clients don't know or care about the root?)

Yes, they must verify it. If they don't know about the root, they should probably reject the signature as they don't know whether to trust it.

* An alternative here would be to define a way to collect the PKI/verification material in a layer/envelope around DSSE and use the keyID to hint for the associated cert.

Is there anything other than the cert that would/could be added to this envelope? If there are other fields that would be helpful specifically for PKI, I'd suggest we move this there to keep DSSE simple. If it's just the cert I don't think the wrapper is needed.

trishankatdatadog · 2022-04-27T18:51:23Z

Hmm, interesting. Could someone explain the use cases here for distributing these public keys / certs in such a manner? For example, is it for Fulcio? Because I'm not sure whether we ever arrived at a consensus for #42.

adityasaky · 2022-04-27T19:17:21Z

use cases

ITE-7 for one would eventually need this. That in turn enables SPIFFE / SPIRE support in in-toto. Also, the proposal here closely mirrors the changes ITE-7 recommends to the old signature wrapper!

MarkLodato

Is there a use case for non-X509 certificates? If so, should the name be "x509cert" or similar?

IF the certificate is defined, MUST the clients verify it against a root? I think yes. (But what if clients don't know or care about the root?)

Yes, or more accurately, if and only if it is used. The client MUST authenticate the certificate before using it, but they are free to ignore the field altogether if they are verifying the payload against some other root of trust.

MarkLodato · 2022-04-29T13:32:20Z

envelope.md

@@ -33,6 +34,8 @@ Base64() is [Base64 encoding](https://tools.ietf.org/html/rfc4648), transforming
 a byte sequence to a unicode string. Either standard or URL-safe encoding is
 allowed.

+PEM() is a [PEM encoding](), transforming a DER (binary) encoded X.509 certificate to a base64 encoding with a one-line header and footer.


Need to fix the link to PEM encoding.

MarkLodato · 2022-04-29T13:34:05Z

implementation/signing_spec.py

@@ -56,6 +56,9 @@ def keyid(self) -> Optional[str]:
        """Returns the ID of this key, or None if not supported."""
        ...

+    def certificate(self) -> Optional[str]:
+        """Returns the cert chain of the key, or None if not supported."""


in PEM format

MarkLodato · 2022-04-29T13:42:38Z

implementation/signing_spec.py

@@ -66,10 +69,16 @@ def keyid(self) -> Optional[str]:
        """Returns the ID of this key, or None if not supported."""
        ...

+class RootPool(Protocol):


I don't think this should be part of the interface. Instead:

Verifier.verify ought to take a cert: str (which may be empty) argument

If the implementation of Verifier does not accept certificates, it MUST ignore the cert field.

If the implementation of Verifier does accept certificates, it MUST verify cert using a known root pool before verifying that signature was signed by cert.

protocol.md

MarkLodato · 2022-04-29T14:25:46Z

protocol.md

@@ -77,7 +84,7 @@ Functions:
 Out of band:

 -   Agree on a PAYLOAD_TYPE and cryptographic details, optionally including
-    KEYID.
+    KEYID and trusted root certificates.


I worry that this small amount of detail will lead to insecure implementations, in particular just verifying that it came from a trusted root but not verifying the actual chain properties (e.g. common name). Previously, "cryptographic details" implied roots of trust because we said nothing about the public key. Now root certs are called out explicitly, but without sufficient detail.

I feel like we need to either say less (so that it's obvious that there is missing detail) or more (so that it's clear how to implement correctly.)

in particular just verifying that it came from a trusted root but not verifying the actual chain properties (e.g. common name).

Hmmm I had to think about this. I would definitely like to call this out explicitly because leaving it out would lead to people potentially not verifying the chain and using the public key inside to verify the sig.

I agree that this does not call out details on path validation: I could link https://www.rfc-editor.org/rfc/rfc5280.html#section-6, and rephrase to say and trusted root certificates and constraints. WDYT?

I'm not sure RFC5280 is sufficient. That RFC is super complex, and it sounds like TLS doesn't even use it directly. It is more strict and requires a SEQUENCE of certificates that leads directly from the root CA to the server's certificate (or the other way around?) and it does something with the subject name to match against the domain name. I feel like we need to call that out explicitly, or else it will get dropped.

Alternatively, is there some implementation we can point to to make things more concrete?

There are also caveats around Common Name vs Subject Alternative Name. I think the former used to be the one to verify, but now it's the latter in TLS

I'm not sure RFC5280 is sufficient. That RFC is super complex, and it sounds like TLS doesn't even use it directly. It is more strict and requires a SEQUENCE of certificates that leads directly from the root CA to the server's certificate (or the other way around?) and it does something with the subject name to match against the domain name. I feel like we need to call that out explicitly, or else it will get dropped.

Alternatively, is there some implementation we can point to to make things more concrete?

I just had to deal with this myself, and ended up using https://github.com/wbond/certvalidator which does complete path validation back to trusted roots

MarkLodato · 2022-04-29T14:35:10Z

protocol.md

 -   Optionally, filter acceptable public keys by KEYID.
 -   Verify SIGNATURE against PAE(UTF8(PAYLOAD_TYPE), SERIALIZED_BODY). Reject if
    the verification fails.
+-   Optionally, verify the signing key's CERTIFICATE chains back to a trusted root.


This sounds too optional and too insecure. I suggest folding it into the line above, maybe:

Verify SIGNATURE against PAE(UTF8(PAYLOAD_TYPE), SERIALIZED_BODY) using the predefined roots of trust and optionally CERTIFICATE. If CERTIFICATE is used, it MUST be verified against a trusted root certificate. Reject if the verification fails.

protocol.md

asraa · 2022-05-03T16:51:12Z

Is there anything other than the cert that would/could be added to this envelope? If there are other fields that would be helpful specifically for PKI, I'd suggest we move this there to keep DSSE simple.

Yes -- for any certificate that's shortlived, you would also want to specify a signed timestamp (e.g. an RFC 3161) from a timestamp authority. So we would also need a generic timestamp field (and maybe a type field for that? or the client can support specific ones decided on out of band).

Could someone explain the use cases here for distributing these public keys / certs in such a manner?

Yep, it's for short lived certs, and can also be used to identify identity of a public key as normal x509 certificate usage. And yes! Exactly, ITE-7 would support this.

Is there a use case for non-X509 certificates? If so, should the name be "x509cert" or similar?

There are some other certificate formats, like SPKI, but that's in draft form, or openpgp's certificate formats, or CVC's. I think we should keep a general name, although 99% of use-cases are probably x.509.

Signed-off-by: Asra Ali <[email protected]>

MarkLodato · 2022-05-04T15:05:38Z

envelope.md

@@ -33,6 +34,8 @@ Base64() is [Base64 encoding](https://tools.ietf.org/html/rfc4648), transforming
 a byte sequence to a unicode string. Either standard or URL-safe encoding is
 allowed.

+PEM() is a [PEM encoding](https://datatracker.ietf.org/doc/html/rfc1421), transforming a DER (binary) encoded X.509 certificate to a base64 encoding with a one-line header and footer.


nits:

This is a certificate chain, so it is a series of PEM-encoded certificates, concatenated with newlines?

wrap at 80 columns for consistency with rest of file

MarkLodato · 2022-05-04T15:20:43Z

protocol.md

@@ -52,6 +53,12 @@ KEYID           | string | No       | No
    decisions; it may only be used to narrow the selection of possible keys to
    try.

+*   CERTIFICATE: Optional, unauthenticated PEM encoded X.509 certificate chain for 


nit: Should we also allow ASN.1 encoding in the protocol? I can see why we'd want to specify PEM in the JSON envelope because that would need base64 encoding otherwise, but a different envelope format (say CBOR or protobuf) might want a binary format encoding of the certificate.

MarkLodato · 2022-05-04T15:24:12Z

protocol.md

+-   Verify SIGNATURE against PAE(UTF8(PAYLOAD_TYPE), SERIALIZED_BODY) using 
+    the predefined roots of trust and constraints optionally CERTIFICATE. If 
+    CERTIFICATE is specified, it MUST be verified against a trusted root 
+    certificate. Reject if the verification fails.


"trusted root certificate and path validation" or similar

same on line 134 below

MarkLodato · 2022-05-04T15:24:31Z

protocol.md

-   Verify SIGNATURE against PAE(UTF8(PAYLOAD_TYPE), SERIALIZED_BODY). Reject if
-    the verification fails.
+-   Verify SIGNATURE against PAE(UTF8(PAYLOAD_TYPE), SERIALIZED_BODY) using 
+    the predefined roots of trust and constraints optionally CERTIFICATE. If 


typo: remove "constraints"

same on line 132 below

MarkLodato · 2022-05-04T15:25:40Z

protocol.md

+    -   Verify SIGNATURE against PAE(UTF8(PAYLOAD_TYPE), SERIALIZED_BODY) using 
+        the predefined roots of trust and constraints optionally CERTIFICATE. If 
+        CERTIFICATE is specified, it MUST be verified against a trusted root 
+        certificate. Reject if the verification fails.
    -   Add the accepted public key to the set ACCEPTED_KEYS.


nit: ACCEPTED_KEYS is no longer correct. What should this be instead?

MarkLodato · 2022-05-04T15:59:25Z

protocol.md

@@ -77,7 +84,7 @@ Functions:
 Out of band:

 -   Agree on a PAYLOAD_TYPE and cryptographic details, optionally including
-    KEYID.
+    KEYID and trusted root certificates.


I'm not sure RFC5280 is sufficient. That RFC is super complex, and it sounds like TLS doesn't even use it directly. It is more strict and requires a SEQUENCE of certificates that leads directly from the root CA to the server's certificate (or the other way around?) and it does something with the subject name to match against the domain name. I feel like we need to call that out explicitly, or else it will get dropped.

Alternatively, is there some implementation we can point to to make things more concrete?

asraa · 2022-09-27T12:45:49Z

Just a quick ping: will be addressing comments and sending at least one update today.

adityasaky · 2023-02-13T15:44:14Z

I see this PR has been dormant for a while. @asraa are you still working on this change? 😄

colek42 · 2023-02-18T15:01:42Z

We would love to see this merged in. We have extended DSSE in our implementation of Witness, and we would like to bring it in spec.

https://github.com/testifysec/go-witness/blob/main/dsse/dsse.go#L56

Add support for x509 certificates in DSSE

b4f07d3

Signed-off-by: Asra Ali <[email protected]>

TomHennen reviewed Apr 27, 2022

View reviewed changes

protocol.md Outdated Show resolved Hide resolved

update certificate wording to include chain

18f72d4

Signed-off-by: Asra Ali <[email protected]>

MarkLodato requested changes Apr 29, 2022

View reviewed changes

update

1085437

Signed-off-by: Asra Ali <[email protected]>

MarkLodato reviewed May 4, 2022

View reviewed changes

adityasaky mentioned this pull request May 30, 2023

Extending DSSE Signatures #59

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for x509 certificates in DSSE #50

Add support for x509 certificates in DSSE #50

asraa commented Apr 27, 2022

mnm678 commented Apr 27, 2022

trishankatdatadog commented Apr 27, 2022 •

edited

Loading

adityasaky commented Apr 27, 2022 •

edited

Loading

MarkLodato left a comment

MarkLodato Apr 29, 2022

MarkLodato Apr 29, 2022

MarkLodato Apr 29, 2022

MarkLodato Apr 29, 2022

asraa May 3, 2022

MarkLodato May 4, 2022

laurentsimon May 4, 2022 •

edited

Loading

shaunmlowry Sep 23, 2022

MarkLodato Apr 29, 2022

asraa commented May 3, 2022

MarkLodato May 4, 2022

MarkLodato May 4, 2022

MarkLodato May 4, 2022

MarkLodato May 4, 2022

MarkLodato May 4, 2022

MarkLodato May 4, 2022

asraa commented Sep 27, 2022

adityasaky commented Feb 13, 2023

colek42 commented Feb 18, 2023

Add support for x509 certificates in DSSE #50

Are you sure you want to change the base?

Add support for x509 certificates in DSSE #50

Conversation

asraa commented Apr 27, 2022

mnm678 commented Apr 27, 2022

trishankatdatadog commented Apr 27, 2022 • edited Loading

adityasaky commented Apr 27, 2022 • edited Loading

MarkLodato left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laurentsimon May 4, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asraa commented May 3, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asraa commented Sep 27, 2022

adityasaky commented Feb 13, 2023

colek42 commented Feb 18, 2023

trishankatdatadog commented Apr 27, 2022 •

edited

Loading

adityasaky commented Apr 27, 2022 •

edited

Loading

laurentsimon May 4, 2022 •

edited

Loading