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

test(libs): use shared signatures #23

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

test(libs): use shared signatures #23

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
41 changes: 41 additions & 0 deletions libraries/go/webhook_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,12 @@
package standardwebhooks_test

import (
"encoding/json"
"fmt"
"net/http"
"os"
"path/filepath"
"strconv"
"strings"
"testing"
"time"
Expand Down Expand Up @@ -211,3 +215,40 @@ func TestWebhookSign(t *testing.T) {
}

}

type sharedWebhookTestData struct {
ID string `json:"id"`
Timestamp int `json:"timestamp"`
Secret string `json:"secret"`
Payload string `json:"payload"`
Signature string `json:"signature"`
}

func TestWebhookShared(t *testing.T) {
b, err := os.ReadFile(filepath.Join("..", "testdata", "signatures.json"))
if err != nil {
t.Fatal(err)
}
testdatas := []sharedWebhookTestData{}
if err = json.Unmarshal(b, &testdatas); err != nil {
t.Fatal(err)
}

for i := 0; i < len(testdatas); i++ {
w, err := standardwebhooks.NewWebhook(testdatas[i].Secret)
if err != nil {
t.Fatal(err)
}
ts, err := strconv.ParseInt(fmt.Sprint(testdatas[i].Timestamp), 10, 64)
if err != nil {
t.Fatal(err)
}
signature, err := w.Sign(testdatas[i].ID, time.Unix(ts, 0), []byte(testdatas[i].Payload))
if err != nil {
t.Fatal(err)
}
if signature != testdatas[i].Signature {
t.Errorf("%s is not equal to signature: %s", signature, testdatas[i].Signature)
}
}
}
2 changes: 2 additions & 0 deletions libraries/rust/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,3 +17,5 @@ base64 = "0.21.3"
time = "0.3"
thiserror = ">1.0.30"
http = "0.2"
serde = { version = "1.0", features = ["derive"] }
serde_json = "1.0"
29 changes: 29 additions & 0 deletions libraries/rust/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,9 @@ impl Webhook {
#[cfg(test)]
mod tests {

use serde::{Deserialize, Serialize};
use std::{fs, path::Path};

use super::*;
use http::HeaderMap;

Expand Down Expand Up @@ -282,4 +285,30 @@ mod tests {
}
}
}

#[derive(Serialize, Deserialize)]
struct SignatureTestData {
id: String,
timestamp: i64,
secret: String,
payload: String,
signature: String,
}

#[test]
fn test_shared_signatures() {
let data_path = Path::new(env!("CARGO_MANIFEST_DIR"))
.join("..")
.join("testdata")
.join("signatures.json");
let data: String = fs::read_to_string(data_path).unwrap();
let test_data: Vec<SignatureTestData> = serde_json::from_str(&data).unwrap();
for test in test_data {
let wh = Webhook::new(&test.secret).unwrap();
let signature = wh
.sign(&test.id, test.timestamp, test.payload.as_bytes())
.unwrap();
assert_eq!(signature, test.signature);
}
}
}
9 changes: 9 additions & 0 deletions libraries/testdata/signatures.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
[
{
"id": "msg_p5jXN8AQM9LWM0D4loKWxJek",
"timestamp": 1696200454,
Copy link
Contributor

@tasn tasn Oct 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

The problem with this is that the verification function also verifies the timestamp is right.

I think we should either (or both?):

  1. Expose functions that allow verification while ignoring the timestamp (e.g. we have it in Go) just for the tests.
  2. Make it possible to create a test Webhook instance with a fake current time (or some other way to mock the current time).

Copy link
Member Author

Choose a reason for hiding this comment

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

Currently the test only verifies the signature is properly done and asserts its format. We're not doing the verification of this signature. Indeed we can do it, however mocking time.Now can be tedious in some languages which my knowledge is a bit too shallow.

Every lib test the verification of the signature with their test suites.

Not sure which path we want to go for.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh right, if we just compare the signatures it should be deterministic. In my head I was also thinking about the asymmetric-signature variants because those don't have deterministic signatures, so this won't work there.

Yeah, I think mocking time.Now is probably too big of a pain, I more meant: Webhook::new_with_mocked_time(key, fake_timestamp. Though maybe it's better to just expose a verification that doesn't depend on the timestamp. I think this can probably be useful when building some utilities. E.g. we used it in https://github.com/svix/svix-cli I think.

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 that highlights the fact that libs should implement the same interface which is not the case currently. Eg:

standard-webhooks/libraries/go/webhook.go

Line 71 in 1e928f4

func (wh *Webhook) VerifyIgnoringTimestamp(payload []byte, headers http.Header) error {

standard-webhooks/libraries/rust/src/lib.rs

Line 62 in 1e928f4

pub fn verify(&self, payload: &[u8], headers: &HeaderMap) -> Result<(), WebhookError> {

Copy link
Contributor

Choose a reason for hiding this comment

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

Go also has a verify without ignoring (normal verify), but yeah Rust (and all the rest) are missing the verify with ignoring.

I updated #21 accordingly.

Copy link
Member Author

Choose a reason for hiding this comment

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

Restarting the conversation on this. Shouldn't this shared signature verify only the Sign function accross every lib? with inputs and expected outputs? We might also introduce algorithm in the future as we might want to support ecdsa for example.

wdyt?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that's fair. We can cross the asymmetric bridge once we get to it, but given that it's deterministic at the moment, this is probably a good idea. So yeah, let's just verify sign.

We just need to also make sure we have tests that fail verification when the timestamp is outside of the allowed tolerance, but this need not be static.

"secret": "MfKQ9r8GKYqrTwjUPD8ILPZIo2LaLaSw",
"payload": "{\"type\": \"example.created\", \"timestamp\":\"2023-09-28T19:20:22+00:00\", \"data\":{\"str\":\"string\",\"bool\":true,\"int\":42}}",
"signature":"v1,GgjvufBlIj71MBIVQotYGb+xEC3LNXCsIZp0HsDC8qs="
}
]
Loading