Skip to content

feat: Add alternative verification API and expand MessageDetails #14

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

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

Conversation

jvatic
Copy link

@jvatic jvatic commented Jul 9, 2025

Thanks for creating & maintaining this library @yaronf!

This PR resolves a need we have at $employer to do some additional verification on the message artifacts (and removes having to build an intermediate http.Request object since we're using this in an rpc context).

  • Add Message struct with a Verify method that can be used in place of VerifyRequest + RequestDetails, and VerifyResponse + ResponseDetails.
  • MessageDetails now contains created, expires, nonce, and tag params.

I'm happy to iterate on this a bit if there's any changes you'd like to see before accepting.

- Add `Message` struct with a `Verify` method that can be used in place of `VerifyRequest` + `RequestDetails`, and `VerifyResponse` + `ResponseDetails`.
- `MessageDetails` now contains created, expires, nonce, and tag params.
@@ -81,7 +82,11 @@ func testHTTP(t *testing.T, proto string) {
if err != nil {
t.Errorf("could not create verifier")
}
sigInput, err := verifyRequestDebug("sig1", *verifier, r)
Copy link
Owner

Choose a reason for hiding this comment

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

Why change the old test instead of creating a new one?

@@ -1406,15 +1407,19 @@ func TestMultipleSignatures(t *testing.T) {
SetVerifyCreated(false).SetKeyID("test-key-ecc-p256"), Headers("@method", "@authority", "@path", "content-digest",
"content-type", "content-length"))
assert.NoError(t, err, "cannot create verifier1")
_, err = verifyRequestDebug("sig1", *verifier1, req)
Copy link
Owner

Choose a reason for hiding this comment

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

Please add new tests rather than changing existing ones, since we're not deprecating the existing library's API.

@yaronf
Copy link
Owner

yaronf commented Jul 10, 2025

Hi @jvatic, thank you for the pull request. This is a reasonably large PR so it might take a bit of iteration. Here are some initial comments:

  • I don't think the Message structure belongs in signatures.go. Please refactor it to its own file. Possibly along with MessageDetails.
  • This is essentially a "quality of life" code change, so please include a worked example that'll go into the package documentation, see https://pkg.go.dev/github.com/yaronf/httpsign#pkg-examples
  • I have not (yet) tested that the new fuzz tests are working, or that test coverage has not dropped.

@jvatic
Copy link
Author

jvatic commented Jul 10, 2025

Thanks for the review @yaronf!

  • I've moved Message/MessageConfig and MessageDetails into message.go.
  • I've added an example that does the same thing as the existing VerifyRequest example but using the new API.

RE code deletion/test changes: My thinking was that it felt cleaner to for MessageConfig to build decomposed req/resp parts vs composing them, and being able to re-use existing tests could provide more confidence in the new API & simplify testing. Would you prefer if the Message.Verify called VerifyRequest/VerifyResponse vs the other way around? That would reduce the scope of the changes.

While code coverage remains about the same overall, there are a few error paths not covered when calling VerifyRequest/VerifyResponse which reduces coverage on the func level a bit, but these appear to be covered by NewMessage tests (I can include the coverage output if that'd be helpful).

@yaronf yaronf self-assigned this Jul 12, 2025
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.

2 participants