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 http message creation detail & images #383

Closed
wants to merge 8 commits into from
Closed

Conversation

tselit
Copy link

@tselit tselit commented Nov 28, 2023

Changes proposed in this pull request

Describes the HTTP signature creation process, including diagrams.

Context

Explains, in fairly simple terms, how the OpenPaymets API has implemented part of the HTTP message signatures draft spec.

Copy link

changeset-bot bot commented Nov 28, 2023

⚠️ No Changeset found

Latest commit: f723c50

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link

netlify bot commented Nov 28, 2023

Deploy Preview for openpayments-preview ready!

Name Link
🔨 Latest commit f723c50
🔍 Latest deploy log https://app.netlify.com/sites/openpayments-preview/deploys/659d52170e4d820008d64a89
😎 Deploy Preview https://deploy-preview-383--openpayments-preview.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Member

@sabineschaller sabineschaller left a comment

Choose a reason for hiding this comment

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

Do the graphics work on black and white background? I think the written part is good, but the technical writers may be able to make it even better 😉.

Copy link
Member

Choose a reason for hiding this comment

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

The Content-Digest header is not standard but part of the signature headers. I also believe that the Date header is not standard. We never use it.

I think we should also add the Authorization header.

Copy link
Author

Choose a reason for hiding this comment

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

  1. Updated header:
  • removed Content-Digest and Date
  • added Authorization
  1. Replaced the diagram with a codeblock titled 'Example: message before signature'.

Copy link
Member

Choose a reason for hiding this comment

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

Date is not part of it. I also cannot find it in the docs.

Also, the first line (POST ...), that is split into @method and @target-uri. The HTTP version is irrelevant. I think @method is missing from your list.

And again, I'd add the Authorization header.

"content-digest" "content-length" "content-type" are also part of the signature params.

I think it's confusing to use creation_time, signing_algorithm, and key_id instead of created, alg, and keyid respectively.

Copy link
Author

Choose a reason for hiding this comment

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

  • Replaced image with codeblock Example: signature base.
  • Removed Date from header
  • Removed HTTP version
  • Included Authorization header
  • Added to signature params: @method; content-digest, content-length, and content-type
  • Renamed signature params: creation_time, signing_algorithm, and key_id

Copy link
Member

Choose a reason for hiding this comment

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

The content headers are missing. Drop date but add authorization.

Copy link
Author

Choose a reason for hiding this comment

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

  • Replaced image with codeblock Example: signature base
  • Removed Date from header
  • Included Authorization header

:::caution
This section is WIP
:::
The [RFC8032](https://www.rfc-editor.org/rfc/rfc8032) specification permits the use of different components of an HTTP message to generate a uniquely labelled message signature. There can be multiple signatures within each HTTP message, each one generated using a different signature algorithm.
Copy link
Member

Choose a reason for hiding this comment

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

Should we link that spec or the part about signatures in the GNAP spec?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, done, thank you!

Copy link
Member

Choose a reason for hiding this comment

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

I like that one!

Copy link
Author

Choose a reason for hiding this comment

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

Update: image is now created in light mode with solid white background.

@melissahenderson
Copy link
Contributor

If possible, please change the font used in the images. Code examples should use a monospace font (Starlight uses a system monospace font by default). For "regular" text and diagrams we should use a sans-serif font that is easier to read and doesn't look handwritten (Starlight uses a system sans-serif font by default). I believe Mermaid uses Trebuchet for its sequence diagrams, but it's not my favorite. Regular Arial would work.

Copy link
Contributor

Choose a reason for hiding this comment

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

Could this image be replaced w/ the code block component and a title? https://interledger.tech/shared/codeblock/

Copy link
Author

Choose a reason for hiding this comment

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

Yes indeed.
Replaced the diagram with a codeblock titled 'Example: message before signature'.

Copy link
Contributor

@melissahenderson melissahenderson Nov 29, 2023

Choose a reason for hiding this comment

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

Could this image also be replaced w/ code block and title? https://interledger.tech/shared/codeblock/

Edit: Maybe not. I see now there's a light gray arrow and text referencing a line in the code.

Copy link
Author

Choose a reason for hiding this comment

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

I think you're right. I replaced the image with the Example: signature base codeblock, and then used accompanying text to add detail.

@melissahenderson
Copy link
Contributor

Related #321

(1) added a reference to the GNAP spec
(2) removed all but 1 diagram, in favour of using code blocks
(3) corrected the original HTTP message header to exclude a Date, and to include Authorization
(4) updated the signature params to include: Authorization; "content-digest" "content-length" "content-type"
@tselit
Copy link
Author

tselit commented Jan 9, 2024

If possible, please change the font used in the images. Code examples should use a monospace font (Starlight uses a system monospace font by default). For "regular" text and diagrams we should use a sans-serif font that is easier to read and doesn't look handwritten (Starlight uses a system sans-serif font by default). I believe Mermaid uses Trebuchet for its sequence diagrams, but it's not my favorite. Regular Arial would work.

Cool and noted, thanks for the thoughtful points @melissahenderson.
I wanted to use Excalidraw exactly for the handwritten 'look' for images.
But if this feels like it creates inconsistency or if it's not inline with the doc style-guide, then we can definitely change it.
From what I see in the Excalidraw docs, there is currently just one alternative font available: 'Helvetica'.
For more detail, check out the Excalidraw docs here

@tselit
Copy link
Author

tselit commented Jan 9, 2024

Do the graphics work on black and white background? I think the written part is good, but the technical writers may be able to make it even better 😉.

Thanks @sabineschaller - image updated to work with white or black backgrounds.
😆 I am with you about the written part.

@tselit tselit self-assigned this Jan 9, 2024
Copy link
Contributor

@melissahenderson melissahenderson left a comment

Choose a reason for hiding this comment

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

Can you update the diagram so that it doesn't use a handwritten font? Excalidraw allows you to use what looks like Arial (in the Excalidraw UI, under font family, it's the button with an "A").


### Signed HTTP Message

The original message gets signed by adding uniqeuly labelled signature headers to the original message: `Signature-Input` and `Signature`.
Copy link
Contributor

Choose a reason for hiding this comment

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

There's a typo here :) "uniqeuly"

:::

## Use the HTTP signature Lambda function
{/* prettier-ignore */}
Copy link
Contributor

Choose a reason for hiding this comment

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

If everything from here down is no longer necessary, it can be deleted.


</CodeBlock>

:::note
Copy link
Contributor

Choose a reason for hiding this comment

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

When I look at this note in the preview, there's a line break between the two sentences. Can you either add a space between the lines or include them together in a single paragraph? Minor thing, I know.

melissahenderson added a commit that referenced this pull request Feb 13, 2024
Updates to this doc have been in progress for a while (see #383 ). For the sake of simplicity, I've taken the changes and am merging them using this PR instead of the original.
melissahenderson added a commit that referenced this pull request Feb 13, 2024
Updates to this doc have been in progress for a while (see #383 ). For the sake of simplicity, I've taken the changes and am merging them using this PR instead of the original.
@melissahenderson
Copy link
Contributor

Updates merged with #424

@melissahenderson melissahenderson deleted the mt-http-sig-creation branch February 13, 2024 14:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants