Skip to content

Require domain separation for SignerDecrypter signature payloads using the StandardCapabilityAccount#1578

Merged
cedric-cordenier merged 8 commits intomainfrom
PRIV-198-domain-sep
Oct 14, 2025
Merged

Require domain separation for SignerDecrypter signature payloads using the StandardCapabilityAccount#1578
cedric-cordenier merged 8 commits intomainfrom
PRIV-198-domain-sep

Conversation

@vreff
Copy link
Copy Markdown
Contributor

@vreff vreff commented Oct 2, 2025

This ensures that users of the StandardCapabilityAccount account for the SignerDecrypter component domain separate their payloads using the recommended MakePeerIDSignatureDomainSeparatedPayload helper function from libocr.

Also, bumps libocr.

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Oct 2, 2025

✅ API Diff Results - No breaking changes


📄 View full apidiff report | 📚 Learn about apidiff

@vreff vreff changed the title Require domain separation for SignerDecrypter signature payloads Require domain separation for SignerDecrypter signature payloads using the StandardCapabilityAccount Oct 8, 2025
@vreff vreff marked this pull request as ready for review October 8, 2025 22:27
@vreff vreff requested a review from a team as a code owner October 8, 2025 22:27
Comment thread pkg/types/core/keystore.go Outdated
Co-authored-by: Jordan Krage <jmank88@gmail.com>
Copy link
Copy Markdown

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

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

⚠️ Performance Alert ⚠️

Possible performance regression was detected for benchmark.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 2.

Benchmark suite Current: 3d2884e Previous: 60798d1 Ratio
BenchmarkKeystore_Sign/nop/in-process 731.8 ns/op 352.2 ns/op 2.08

This comment was automatically generated by workflow using github-action-benchmark.

@vreff
Copy link
Copy Markdown
Contributor Author

vreff commented Oct 9, 2025

@jmank88 interesting to see: #1578 (review).

I guess bytes.hasPrefix is a bit less performant than the manual check. I don't foresee this being a problem for our use case, LMK if there's any reason we should care about the change in signing speed.

@jmank88
Copy link
Copy Markdown
Contributor

jmank88 commented Oct 9, 2025

@jmank88 interesting to see: #1578 (review).

I guess bytes.hasPrefix is a bit less performant than the manual check. I don't foresee this being a problem for our use case, LMK if there's any reason we should care about the change in signing speed.

Oh, wasn't that there with the last version too? The internals look equivalent.

@vreff
Copy link
Copy Markdown
Contributor Author

vreff commented Oct 9, 2025

Oh, wasn't that there with the last version too? The internals look equivalent.

The benchmarks test passed on the commit before the hasPrefix change 🤷

@cedric-cordenier cedric-cordenier enabled auto-merge (squash) October 14, 2025 13:20
@cedric-cordenier cedric-cordenier merged commit 5662ca1 into main Oct 14, 2025
23 of 25 checks passed
@cedric-cordenier cedric-cordenier deleted the PRIV-198-domain-sep branch October 14, 2025 13:26
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.

4 participants