8000 feat(privval)!: Allow Privval to sign arbitrary bytes. by itsdevbear · Pull Request #2692 · cometbft/cometbft · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

feat(privval)!: Allow Privval to sign arbitrary bytes. #2692

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
Merged
merged 11 commits into from
Apr 10, 2024
Merged

feat(privval)!: Allow Privval to sign arbitrary bytes. #2692

merged 11 commits into from
Apr 10, 2024

Conversation

itsdevbear
Copy link
@itsdevbear itsdevbear commented Mar 29, 2024 •
edited by cason
Loading

Closes #2517

PR checklist

  • Tests written/updated
  • Changelog entry added in .changelog (we use unclog to manage our changelog)
  • Updated relevant documentation (docs/ or spec/) and code comments
  • Title follows the Conventional Commits spec

@itsdevbear itsdevbear requested review from a team as code owners March 29, 2024 04:55
@faddat
Copy link
Contributor
faddat commented Mar 29, 2024

saucy!

@cason
Copy link
Contributor
cason commented Apr 2, 2024

Do you mind adding some test units covering the added feature?

@itsdevbear
Copy link
Author

@cason no problem
8000

@itsdevbear
Copy link
Author

@cason done

cason
cason approved these changes Apr 2, 2024
View reviewed changes
Copy link
Contributor
@cason cason left a comment

Choose a reason for hiding this comment

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

Conceptually and implementation-wise legit.

But I wonder whether we should somehow limit the maximum size of the array of bytes that can be signed. As this can constitute an attack vector.

Devon Bear and others added 5 commits April 2, 2024 12:40
@cason
Update privval/file.go
5a1ce2d 
Co-authored-by: Daniel <daniel.cason@informal.systems>
@cason
Update proto/cometbft/privval/v1/types.proto
6c80481 
Co-authored-by: Daniel <daniel.cason@informal.systems>
@cason
Update types/priv_validator.go
890e97a 
Co-authored-by: Daniel <daniel.cason@informal.systems>
@cason
Update proto/cometbft/privval/v1/types.proto
7d7bd45 
Co-authored-by: Daniel <daniel.cason@informal.systems>
@cason
Update privval/file.go
6cdec0c 
Co-authored-by: Daniel <daniel.cason@informal.systems>
@itsdevbear
Copy link
Author

@cason good with all those changes,

re: max length, I mean I suppose? but feels overkill imo.

@cason
Copy link
Contributor
cason commented Apr 2, 2024

Maybe is too much establishing a maximum size for the data that can be send to the remote signer.

But I would like to have a second opinion from the team.

@cason cason added enhancement New feature or request privval labels Apr 2, 2024
@cason cason added this to the 2024-Q2 milestone Apr 2, 2024
@cason
Copy link
Contributor
cason commented Apr 2, 2024

I assume this closes #2517?

@itsdevbear
Copy link
Author

ya

@cason cason linked an issue Apr 2, 2024 that may be closed by this pull request
privval: support arbitrary message signing #2517
Closed
@adizere
Copy link
Member
adizere commented Apr 3, 2024

@itsdevbear is this needed on v0.38, or is it enough to backport to v1 ?

@itsdevbear
Copy link
Author

@adizere both would be great, or get the sdk onto v1 :P

@adizere adizere added backport-to-v1.x Tell Mergify to backport the PR to v1.x backport-to-v0.38.x Tell Mergify to backport the PR to v0.38.x labels Apr 4, 2024
@adizere adizere removed this from the 2024-Q2 milestone Apr 4, 2024
melekes
melekes approved these changes Apr 8, 2024
View reviewed changes
Copy link
Contributor
@melekes melekes left a comment

Choose a reason for hiding this comment

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

Thanks @itsdevbear ❤️

@melekes melekes changed the title feat(privval): Allow Privval to sign arbitrary bytes. feat(privval)!: Allow Privval to sign arbitrary bytes. Apr 8, 2024
@melekes
Copy link
Contributor
melekes commented Apr 8, 2024

@itsdevbear would you mind adding a changelog entry?

But I wonder whether we should somehow limit the maximum size of the array of bytes that can be signed. As this can constitute an attack vector.

The remote signer would be responsible for having a hard limit, after which it would refuse to sign bytes.

is this needed on v0.38, or is it enough to backport to v1 ?

How could we backport this if it's a breaking change?

@melekes melekes removed the backport-to-v0.38.x Tell Mergify to backport the PR to v0.38.x label Apr 9, 2024
@cason
Copy link
Contributor
cason commented Apr 9, 2024

Can we merge this PR?

Could the discussion about backporting it to v0.38 proceed on the associated issue #2517?
8000

@cason
Copy link
Contributor
cason commented Apr 9, 2024

@itsdevbear would you mind adding a changelog entry?

Ah, we are also missing this.

@itsdevbear
Copy link
Author

@cason done

@melekes
Copy link
Contributor
melekes commented Apr 10, 2024 •
edited
Loading

The changelog entry must be added to .changelog, not CHANGELOG.md. NOTE: I can't fix it because maintainers don't have push rights to berachain/cometbft.

@melekes melekes enabled auto-merge April 10, 2024 06:11
@melekes melekes disabled auto-merge April 10, 2024 06:15
@melekes melekes added this pull request to the merge queue Apr 10, 2024
Merged via the queue into cometbft:main with commit 3815d41 Apr 10, 2024
37 checks passed
mergify bot pushed a commit that referenced this pull request Apr 10, 2024
@mergify
feat(privval)!: Allow Privval to sign arbitrary bytes. (#2692)
0c00d48 
Closes #2517

---

#### PR checklist

- [x] Tests written/updated
- [ ] Changelog entry added in `.changelog` (we use
[unclog](https://github.com/informalsystems/unclog) to manage our
changelog)
- [ ] Updated relevant documentation (`docs/` or `spec/`) and code
comments
- [x] Title follows the [Conventional
Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec

---------

Co-authored-by: Daniel <daniel.cason@informal.systems>
(cherry picked from commit 3815d41)

# Conflicts:
#	CHANGELOG.md
@mergify mergify bot mentioned this pull request Apr 10, 2024
Merged
4 tasks
melekes added a commit that referenced this pull request Apr 10, 2024
@melekes
chore: move changelog entry
15ac69d 
Follow-up to #2692
@melekes melekes mentioned this pull request Apr 10, 2024
Merged
melekes added a commit that referenced this pull request Apr 10, 2024
@mergify @melekes
feat(privval)!: Allow Privval to sign arbitrary bytes. (backport #2692)…
4f4faec 
… (#2762)

Closes #2517

---

#### PR checklist

- [x] Tests written/updated
- [ ] Changelog entry added in `.changelog` (we use
[unclog](https://github.com/informalsystems/unclog) to manage our
changelog)
- [ ] Updated relevant documentation (`docs/` or `spec/`) and code
comments
- [x] Title follows the [Conventional
Commits](https://www.conventionalcommits.org/en/v1.0.0/) spec
<hr>This is an automatic backport of pull request #2692 done by
[Mergify](https://mergify.com).

---------

Co-authored-by: Devon Bear <itsdevbear@berachain.com>
Co-authored-by: Anton Kaliaev <anton.kalyaev@gmail.com>
github-merge-queue bot pushed a commit that referenced this pull request Apr 10, 2024
@melekes
chore: move changelog entry (#2763)
040fe03 
Follow-up to #2692
@zrbecker zrbecker mentioned this pull request May 20, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cason cason cason approved these changes

@melekes melekes melekes approved these changes

Assignees
No one assigned
Labels
backport-to-v1.x Tell Mergify to backport the PR to v1.x enhancement New feature or request privval
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

privval: support arbitrary message signing
5 participants
@itsdevbear @faddat @cason @adizere @melekes
0