Address reviewer feedback on httpsig#391
Conversation
Apparently the Date header can be rewritten in transit, so it should not be covered by the signature.
The /.well-known/jwks.json path is not registered with IANA nor defined by any spec. Advertise the JWK Set location in a new OPTIONAL jwksUri discovery field instead, which MUST be present when the http-sig capability is advertised.
Determine the HTTP Message Signature algorithm from the alg value of the JWK selected by keyid, applied per RFC 9421 Section 3.3.7. This removes the restriction to the HTTP Signature Algorithms registry, so new (e.g. post-quantum) JOSE algorithms can be adopted without spec changes. JWK alg values should be fully-specified per RFC 9864; Ed25519 is recommended. The alg signature parameter is optional and must be consistent with the key when present.
The keyid parameter is now mandatory alongside created, and its value must equal the kid of the corresponding key in the signer's JWK Set. Verifiers reject signatures without keyid or without a matching key.
Move the JWKS publication requirement and the payload consistency check into the normative HTTP Message Signatures section, and strip RFC 2119 keywords from the examples appendix.
The example signed the content-digest component but showed an RFC 3230 Digest header. Use the RFC 9530 Content-Digest field with the actual digest and length of the example body.
Replace ad-hoc "line breaks for display purposes only" notes with standard RFC 8792 single-backslash folding, and add RFC 8792 as an informative reference.
Update the Trusted Server definition, Discovery introduction, and Decision to Discard to speak of JWK Sets discovered via jwksUri and the keyid/kid match, instead of a single discoverable keypair. Cover content-length in the Token Request example as required by Signing Requirements, use a real Content-Digest and Content-Length in the Appendix B example, derive the verification algorithm from the JWK in Appendix B, and record the Date-header and example changes in the Version 07 changelog.
MahdiBaghbani
left a comment
There was a problem hiding this comment.
Thanks for this, I think the direction is really good.
Requiring keyid, deriving the algorithm from the JWK, and dropping Date from the covered components all make the signing model a lot easier to reason about. I also like moving the JWKS publication and payload-consistency requirements into the normative section and marking Appendix B as informative.
I have included suggested changes inline for each comment, so you can apply them quickly from the GitHub UI if you agree with them.
I left a few inline comments. Most of them are editorial, but three are worth sorting out before RFC last call:
jwksUrihas no https requirement (spec.yaml + I-D), and the I-D text does not say the JWK Set must be fetched over https.- There is no
kiduniqueness MUST within a JWK Set, so a verifier that hits two keys with the samekidhas no defined behavior. - The cross-registry
algmapping is only shown for Ed25519, so "MUST denote the same algorithm" is not really interoperable for RSA-PSS or ECDSA yet.
A couple of non-inline notes:
- The Version 07 changelog looks like it mixes in a few items that I think actually landed in #390 (the same-string note, the Appendix D label fix, the Signing Direction Index, Appendix E, the diagrams). Could you please double-check those belong here?
- Removing the fixed
/.well-known/jwks.jsonpath is a real wire change for peers that hardcoded it, and it adds a mandatory Discovery round-trip. I think a short migration note would help implementers. - The diagrams under
diagrams/(02-discovery-receiver-publish, 07-signing-directions) still hardcode/.well-known/jwks.json. I added those diagrams myself, so if you like I can fix the diagram paths in this PR too, to keep them consistent with the newjwksUrimodel.
A few more small ones I could not attach inline because the lines fall outside this PR's diff:
- spec.yaml line 32: the 200 description still says "public key" (singular); now it is a JWK Set via
jwksUri. - IETF-OCM.md line 1348 and line 1999: "HTTP Signature" should read "HTTP Message Signature(s)" to match the section title and RFC 9421.
The examples check out as far as I can tell: the Content-Digest values and content-lengths match the example bodies, and the RFC 8792 folding looks mechanically correct.
Some of my suggestions are a bit more normative than editorial, so if I misunderstood the intended model, feel free to push back and we can narrow it down.
- Require https for jwksUri in spec.yaml and I-D text - Add explicit discard case for http-sig without jwksUri - Drop "or otherwise" from JWK Set fetch rule - Add reject step in verification procedure for missing jwksUri - Use "signature parameters" per RFC 9421 terminology - Rename "JWKS Endpoint" section to "Published JWK Set" - Expand RFC 9864 title to full published form - Align "as specified under" with the SHOULD in normative text - Reference RFC 9110 for Date header in verification procedure Co-authored-by: Mahdi Baghbani <mahdi-baghbani@azadehafzar.io>
|
I think I addressed your feedback now @MahdiBaghbani |
- Require https for jwksUri in spec.yaml and I-D text - Add explicit discard case for http-sig without jwksUri - Drop "or otherwise" from JWK Set fetch rule - Add reject step in verification procedure for missing jwksUri - Use "signature parameters" per RFC 9421 terminology - Rename "JWKS Endpoint" section to "Published JWK Set" - Expand RFC 9864 title to full published form - Align "as specified under" with the SHOULD in normative text - Reference RFC 9110 for Date header in verification procedure - Actually do a signature in the example with a complete payload Co-authored-by: Mahdi Baghbani <mahdi-baghbani@azadehafzar.io>
bd3dbe2 to
c3cbba3
Compare
- Require https for jwksUri in spec.yaml and I-D text - Add explicit discard case for http-sig without jwksUri - Drop "or otherwise" from JWK Set fetch rule - Add reject step in verification procedure for missing jwksUri - Use "signature parameters" per RFC 9421 terminology - Rename "JWKS Endpoint" section to "Published JWK Set" - Expand RFC 9864 title to full published form - Align "as specified under" with the SHOULD in normative text - Reference RFC 9110 for Date header in verification procedure - Actually do a signature in the example with a complete payload Co-authored-by: Mahdi Baghbani <mahdi-baghbani@azadehafzar.io>
c3cbba3 to
7b5e2b9
Compare
glpatcern
left a comment
There was a problem hiding this comment.
This looks good now, Mahdi you have the last word following your detailed review
Implements the changes from cs3org/OCM-API#391 and closes the remaining gaps with the HTTP Message Signatures section: - discover peer JWK Sets via the new `jwksUri` discovery field instead of the fixed /.well-known/jwks.json path; advertise our own jwksUri and require it (https) from peers advertising the `http-sig` capability - stop covering the Date header; freshness is anchored on the `created` signature parameter - mark and select the OCM signature by its integrity-protected tag="ocm" parameter instead of the dictionary label - require the JWK `alg` parameter and derive the signature algorithm from it, accepting fully-specified RFC 9864 names - reject signed requests whose verification key cannot be resolved instead of treating them as unsigned - derive the signer origin from the spec-canonical fqdn#id keyid form Signed-off-by: Micke Nordin <kano@sunet.se>
I'll review this soon |
|
Maybe we shouldn't mandate HTTPS for fetching the key material @MahdiBaghbani ? It breaks real deployments in Nextcloud, as well as a lot of testing setups: nextcloud/server#62543 I would be content with RECOMMENDING https. |
Address HTTPSig review feedback
This PR addresses the review feedback from Justin Richer on the
HTTP Message Signatures section of the draft.
Changes
Date header
The
Dateheader is no longer covered by signatures: intermediaries commonly rewrite it, and thecreatedsignature parameter already conveys creation time (RFC 9421 §7.2.4).Algorithm derivation
The signature algorithm is now derived from the key material instead of being restricted to the IANA "HTTP Signature Algorithms" registry. Each JWK MUST carry
kidandalg; the JWKalgMUST be an asymmetric JOSE-registered algorithm and SHOULD be fully-specified per RFC 9864 (Ed25519RECOMMENDED, which also replaces the RFC 8037 reference). The algorithm is applied per RFC 9421 §3.3.7, so new (e.g. post-quantum) JOSE algorithms work without updating this document.noneand MACs are forbidden. The HTTPSigalgsignature parameter is OPTIONAL; if present it MUST denote the same algorithm as the JWK, and is omitted when no registry counterpart exists. A new "Keys and Algorithms" subsection holds this text.JWKS discovery
The unregistered
/.well-known/jwks.jsonendpoint is replaced by ajwksUrifield in the Discovery response (also added tospec.yaml). Servers advertising thehttp-sigcapability MUST provide it.keyid
The
keyidsignature parameter is now REQUIRED and MUST equal thekidof the verification key in the signer's JWK Set; verifiers reject signatures with no matching key.Informative examples
Appendix B is now explicitly informative; its normative statements were moved into the HTTP Message Signatures section. All signature examples use RFC 8792 line wrapping, and the
Token Request example uses
Content-Digest(RFC 9530) instead of the legacyDigestheader, with real digest values.Consistency pass
The Trusted Server definition, Discovery introduction, and Decision to Discard now speak of JWK Sets discovered via
jwksUriand thekeyid/kidmatch; the Token Request example coverscontent-lengthas required; the Appendix B example carries a realContent-DigestandContent-Length.The Version 07 changelog records all of the above.