Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add support for x509 certificates in DSSE #50

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Add support for x509 certificates in DSSE #50

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b4f07d3
Add support for x509 certificates in DSSE
asraa Apr 27, 2022
18f72d4
update certificate wording to include chain
asraa Apr 27, 2022
1085437
update
asraa May 3, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 9 additions & 4 deletions envelope.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,8 @@ the following form, called the "JSON envelope":
"payloadType": "<PAYLOAD_TYPE>",
"signatures": [{
"keyid": "<KEYID>",
"sig": "<Base64(SIGNATURE)>"
"sig": "<Base64(SIGNATURE)>",
"cert": "<PEM(CERTIFICATE_CHAIN)>"
}]
}
```
Expand All @@ -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.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to fix the link to PEM encoding.


### Multiple signatures

An envelope MAY have more than one signature, which is equivalent to separate
Expand All @@ -44,10 +47,12 @@ envelopes with individual signatures.
"payloadType": "<PAYLOAD_TYPE>",
"signatures": [{
"keyid": "<KEYID_1>",
"sig": "<SIG_1>"
"sig": "<SIG_1>",
"cert": "<CERT_1>"
}, {
"keyid": "<KEYID_2>",
"sig": "<SIG_2>"
"sig": "<SIG_2>",
"cert": "<CERT_2>"
}]
}
```
Expand All @@ -56,7 +61,7 @@ envelopes with individual signatures.

* The following fields are REQUIRED and MUST be set, even if empty: `payload`,
`payloadType`, `signature`, `signature.sig`.
* The following fields are OPTIONAL and MAY be unset: `signature.keyid`.
* The following fields are OPTIONAL and MAY be unset: `signature.keyid`, `signature.cert`
An unset field MUST be treated the same as set-but-empty.
* Producers, or future versions of the spec, MAY add additional fields.
Consumers MUST ignore unrecognized fields.
Expand Down
4 changes: 4 additions & 0 deletions envelope.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,4 +32,8 @@ message Signature {
// *Unauthenticated* hint identifying which public key was used.
// OPTIONAL.
string keyid = 2;

// *Unauthenticated* PEM encoded X.509 certificate chain corresponding to the public key.
// OPTIONAL.
string cert = 3;
}
19 changes: 16 additions & 3 deletions implementation/signing_spec.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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."""
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in PEM format



class Verifier(Protocol):
def verify(self, message: bytes, signature: bytes) -> bool:
Expand All @@ -66,10 +69,16 @@ def keyid(self) -> Optional[str]:
"""Returns the ID of this key, or None if not supported."""
...

class RootPool(Protocol):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

def verify(self, certificate: bytes) -> bool:
"""Returns true if `certificate` chains back to the Root pool."""

# Collection of verifiers, each of which is associated with a name.
VerifierList = Iterable[Tuple[str, Verifier]]

# A root certificate pool.
Root = RootPool


@dataclasses.dataclass
class VerifiedPayload:
Expand Down Expand Up @@ -100,17 +109,20 @@ def Sign(payloadType: str, payload: bytes, signer: Signer) -> str:
signature = {
'keyid': signer.keyid(),
'sig': b64enc(signer.sign(PAE(payloadType, payload))),
'cert': signer.cert(),
}
if not signature['keyid']:
del signature['keyid']
if not signature['cert']:
del signature['cert']
return json.dumps({
'payload': b64enc(payload),
'payloadType': payloadType,
'signatures': [signature],
})


def Verify(json_signature: str, verifiers: VerifierList) -> VerifiedPayload:
def Verify(json_signature: str, verifiers: VerifierList, root: RootPool) -> VerifiedPayload:
wrapper = json.loads(json_signature)
payloadType = wrapper['payloadType']
payload = b64dec(wrapper['payload'])
Expand All @@ -122,8 +134,9 @@ def Verify(json_signature: str, verifiers: VerifierList) -> VerifiedPayload:
verifier.keyid() is not None and
signature.get('keyid') != verifier.keyid()):
continue
if verifier.verify(pae, b64dec(signature['sig'])):
recognizedSigners.append(name)
if (verifier.verify(pae, b64dec(signature['sig'])) and
root.verify(signature['cert'])):
recognizedSigners.append(name)
if not recognizedSigners:
raise ValueError('No valid signature found')
return VerifiedPayload(payloadType, payload, recognizedSigners)
Expand Down
16 changes: 12 additions & 4 deletions protocol.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ Name | Type | Required | Authenticated
SERIALIZED_BODY | bytes | Yes | Yes
PAYLOAD_TYPE | string | Yes | Yes
KEYID | string | No | No
CERTIFICATE | string | No | No

* SERIALIZED_BODY: Arbitrary byte sequence to be signed.

Expand Down Expand Up @@ -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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

the key used to sign the message. As with Sign(), details on the trusted root
certificates are agreed upon out-of-band by the signer and verifier. This
ensures the necessary information to verify the signature remains alongside
the metadata.

Functions:

* PAE() is the "Pre-Authentication Encoding", where parameters `type` and
Expand All @@ -77,7 +84,7 @@ Functions:
Out of band:

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link

@laurentsimon laurentsimon May 4, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


To sign:

Expand All @@ -90,12 +97,13 @@ To sign:

asraa marked this conversation as resolved.
Show resolved Hide resolved
To verify:

- Receive and decode SERIALIZED_BODY, PAYLOAD_TYPE, SIGNATURE, and KEYID, such
as from the recommended [JSON envelope](envelope.md). Reject if decoding
fails.
- Receive and decode SERIALIZED_BODY, PAYLOAD_TYPE, SIGNATURE, KEYID, and
CERTIFICATE such as from the recommended [JSON envelope](envelope.md).
Reject if decoding fails.
- 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.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

- Reject if PAYLOAD_TYPE is not a supported type.
- Parse SERIALIZED_BODY according to PAYLOAD_TYPE. Reject if the parsing
fails.
asraa marked this conversation as resolved.
Show resolved Hide resolved
Expand Down