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.

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

Add RFC for JWS-signing PGXN releases #5

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open

Conversation

theory
Copy link
Member

@theory theory commented Sep 18, 2024

No description provided.

@theory theory added the enhancement New feature or request label Sep 18, 2024
@theory theory self-assigned this Sep 18, 2024
text/0000-release-meta-spec-v2.md Outdated Show resolved Hide resolved
text/0000-release-meta-spec-v2.md Outdated Show resolved Hide resolved
text/0000-release-meta-spec-v2.md Outdated Show resolved Hide resolved
text/0000-release-meta-spec-v2.md Outdated Show resolved Hide resolved
@theory theory added rfc New RFC and removed enhancement New feature or request labels Sep 19, 2024
Also add some items to the "Unresolved questions" section and a "Future
possibilities" item for author signing.
@theory theory force-pushed the release-meta-spec-v2 branch from 8a4ad64 to fa66f7a Compare September 19, 2024 19:00
@theory theory requested a review from pgguru September 19, 2024 19:02
text/0000-release-meta-spec-v2.md Outdated Show resolved Hide resolved
text/0000-release-meta-spec-v2.md Outdated Show resolved Hide resolved
text/0000-release-meta-spec-v2.md Outdated Show resolved Hide resolved
Comment on lines 77 to 78
"protected":"eyJhbGciOiJSUzI1NiJ9",
"header": {"kid": "2024-12-29" },

Choose a reason for hiding this comment

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

Normally a JWS is like headerB64.payloadB64.signatureB64, and therefore I am wondering about why the payload is separate from the header and signature. I believe it is like that in order to support multiple signatures.

Questions to consider are...

  • is it necessary to support multiple signatures?
  • For 'protected' and 'header', both of these parts typically included in the "header" as described in RFC 7515, so why not combine that into just 'header' as b64 encoded.
  • Wondering because if you take the payload, the header (as suggested in previous comment), signature, and concatenate them with "." joined (RFC7515 section 3.3), then would that make it exactly like a "normal" signature? i.e. easier to use with existing clients that do signature validation maybe.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's an official RFC 7515 format called JWS JSON Serialization. We don't need URL-safe serialization, so this is just a little friendlier for clients. And yes, it allows multiple signatures, which could be useful for author signing or gradual key rotation.

  1. I think it's useful to support multiple signatures because, unlike ephemeral HTTP requests, there's are records at rest and will be around for many years.
  2. protected and header are not combined because the JWS JSON Serialization format separates them.
  3. Clients should also support this format, as it's part of the standard. But even if they don't, conversion will be trivial, as you point out.

Really I wanted the payload not to be Base64 URL-encoded, as specified in JWS-JS, but I believe that spec was abandoned in favor of RFC 7515 JWS JSON Serialization, so going with that approved standard, instead.

@theory theory requested a review from sjmiller609 September 23, 2024 15:52
Copy link

@sjmiller609 sjmiller609 left a comment

Choose a reason for hiding this comment

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

Embedding the public key in clients seems questionable to me, but looks good otherwise.

@theory
Copy link
Member Author

theory commented Sep 23, 2024

Embedding the public key in clients seems questionable to me, but looks good otherwise.

Switched to talking about downloading and using a JWK Set in 01140f3.

@theory theory linked an issue Oct 4, 2024 that may be closed by this pull request
theory added a commit to pgxn/meta that referenced this pull request Oct 8, 2024
Following [RFC 5], add new JSON schemas for a `certs` property
containing JWS [JSON Serialization], supporting both the general and
flattened syntaxes. The schemas are:

*   `certs.schema.json`: One or more certifications, with the `pgxn`
    property required.
*   `jws.schema.json`: JWS general and flattened [JSON Serialization]
*   `jws-header.schema.json`: JWS headers
*   `jwk.schema.json`: [RFC 7517] JSON Web Key (JWK) format, required by
    the `jwk` property of `jws-header.schema.json`
*   `payload.schema.json`: The PGXN release payload

Include tests for each of these schemas.

  [RFC 5]: pgxn/rfcs#5
  [JSON Serialization]: https://datatracker.ietf.org/doc/html/rfc7515#section-7
  [RFC 7517]: https://datatracker.ietf.org/doc/html/rfc7517
theory added a commit to pgxn/meta that referenced this pull request Oct 8, 2024
Following [RFC 5], add new JSON schemas for a `certs` property
containing JWS [JSON Serialization], supporting both the general and
flattened syntaxes. The schemas are:

*   `certs.schema.json`: One or more certifications, with the `pgxn`
    property required.
*   `jws.schema.json`: JWS general and flattened [JSON Serialization]
*   `jws-header.schema.json`: JWS headers
*   `jwk.schema.json`: [RFC 7517] JSON Web Key (JWK) format, required by
    the `jwk` property of `jws-header.schema.json`
*   `payload.schema.json`: The PGXN release payload

Include tests for each of these schemas, and fix comments for existing
schema tests.

  [RFC 5]: pgxn/rfcs#5
  [JSON Serialization]: https://datatracker.ietf.org/doc/html/rfc7515#section-7
  [RFC 7517]: https://datatracker.ietf.org/doc/html/rfc7517
theory added a commit to pgxn/meta that referenced this pull request Oct 8, 2024
With the switch from the old `release` field to the new `certs` and
`release` fields as defined in [RFC 5] and implemented in 0773ad7 and
acb0d22, the previously-defined JSON schemas and data structure are no
longer used. So remove them and their tests.

  [RFC 5]: pgxn/rfcs#5
theory added 4 commits October 8, 2024 20:24
Still not sure about the name, but I think it's better.
Make another edit pass, tweaking some language and structure, but mostly
highlight the support for both the "general" and "flattened" JWS JSON
Serialization syntaxes. Hide lines beginning with `#` in JSON code
fences.
Copy link

@pgguru pgguru left a comment

Choose a reason for hiding this comment

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

Mainly nits, a couple q's.

distribution `META.json` file:

``` json
#{
Copy link

Choose a reason for hiding this comment

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

What does this mean? Just an extract from a larger JSON object? (The syntax hilighter in GH is complaining about this...)

Copy link
Member Author

Choose a reason for hiding this comment

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

This book.toml config

[output.html.code.hidelines]
json = "#"

Means that json fences will not show lines that begin with #. It kinda-sorta works in other parsers, but properly omits them from the rendered view that'll be published on rfcs.pgxn.org.

text/0005-release-certification.md Outdated Show resolved Hide resolved
text/0005-release-certification.md Outdated Show resolved Hide resolved
fails.
4. Decode the payload and use its `uri` field to download the release zip
file.
5. Compare one of the digests from the payload to a digest generated from the
Copy link

Choose a reason for hiding this comment

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

This just means there could be multiple types included, like sha-256 or others, we consider a single validation to be sufficient for our purposes here?

Copy link
Member Author

Choose a reason for hiding this comment

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

A single digest, yes. I suspect there will usually be only one, but wanted to support multiple to simplify migrating SHA-1s. More details in the "Unanswered questions" section.

text/0005-release-certification.md Outdated Show resolved Hide resolved
4. PGXN Manager uses the private release key to sign releases as described
above. The most important property in the signed payload is the list of
digests.
5. Clients **MUST** regularly fetch the [RFC 7517 JWK Set] of public keys,
Copy link

Choose a reason for hiding this comment

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

Would a recommended schedule be useful here; weekly? Each invocation of the tool?

Copy link
Member Author

Choose a reason for hiding this comment

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

Given @sjmiller609's feedback, I'm thinking each invocation, yeah.

> Header Parameter values are integrity protected.
>
> * **header**: The "header" member **MUST** be present and contain the
> value JWS Unprotected Header when the JWS Unprotected Header value is
Copy link

@pgguru pgguru Oct 9, 2024

Choose a reason for hiding this comment

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

Suggested change
> value JWS Unprotected Header when the JWS Unprotected Header value is
> value "JWS Unprotected Header" when the JWS Unprotected Header value is

or maybe

Suggested change
> value JWS Unprotected Header when the JWS Unprotected Header value is
> value `JWS Unprotected Header` when the JWS Unprotected Header value is

also the second non-quoted form here also looks a bit off to me.

Copy link
Member Author

Choose a reason for hiding this comment

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

These are quoted from the source, which does not quote those strings.

text/0005-release-certification.md Outdated Show resolved Hide resolved
> conveyed.
>
> Additional members can be present in both the JSON objects defined above; if
> not understood by implementations encountering them, they MUST be ignored.
Copy link

Choose a reason for hiding this comment

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

Suggested change
> not understood by implementations encountering them, they MUST be ignored.
> not understood by implementations encountering them, they **MUST** be ignored.

Copy link
Member Author

Choose a reason for hiding this comment

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

Also quoted from the source.

* `receipts` Cute, but a bit opaque
* `attestations` Abstract and too JWT-y
* `jws` Dissuades other formats
* `coupons`, `authentication`, `authenticity`, `credentials`,
Copy link

Choose a reason for hiding this comment

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

I personally think certs -> certificates; authenticity seems nice to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rfc New RFC
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support Release metadata in pgxn_spec
3 participants