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

warn for not using local stacks node with the signer #4782

Merged
merged 8 commits into from
May 22, 2024

Conversation

ASuciuX
Copy link
Contributor

@ASuciuX ASuciuX commented May 13, 2024

Current Status: Waiting for FAQ section to add link to.

Example display of message. Do we want to place it on only one line? Maybe make the message shorter.

image

@ASuciuX ASuciuX requested a review from jferrant May 13, 2024 13:28
@ASuciuX ASuciuX self-assigned this May 13, 2024
@ASuciuX ASuciuX marked this pull request as draft May 13, 2024 13:28
stacks-signer/src/main.rs Outdated Show resolved Hide resolved
@hstove
Copy link
Contributor

hstove commented May 13, 2024

I think it could potentially cause more harm (or at least worry) than good if we're being strict about this warning with non-localhost setups. IMO it's probably best practice to have these two services on separate colocated machines, or at most using something like docker-compose, but we wouldn't really be able to check that. The main risk is that the signer isn't a registered event_observer, and that's easy to mess up if you're using an external node, but we don't have a good way of verifying that the signer is an event observer.

@jferrant
Copy link
Collaborator

jferrant commented May 13, 2024

I think it could potentially cause more harm (or at least worry) than good if we're being strict about this warning with non-localhost setups. IMO it's probably best practice to have these two services on separate colocated machines, or at most using something like docker-compose, but we wouldn't really be able to check that. The main risk is that the signer isn't a registered event_observer, and that's easy to mess up if you're using an external node, but we don't have a good way of verifying that the signer is an event observer.

Well the warning is mostly in response to the audit as we are susecptible to man in the middle type situations so perhaps we should just always print it then.

@ASuciuX
Copy link
Contributor Author

ASuciuX commented May 13, 2024

Updated message displayed after having all of it as 1 line

image

stacks-signer/src/main.rs Outdated Show resolved Hide resolved
@ASuciuX ASuciuX marked this pull request as ready for review May 17, 2024 14:17
@ASuciuX ASuciuX requested a review from a team as a code owner May 17, 2024 14:17
@ASuciuX ASuciuX requested a review from hstove May 22, 2024 14:34
@ASuciuX ASuciuX requested a review from jferrant May 22, 2024 15:28
@ASuciuX ASuciuX added this pull request to the merge queue May 22, 2024
Merged via the queue into develop with commit c06473b May 22, 2024
94 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[stacks-signer] Add a log message to signer on bootup to warn that signer should only be run with a LOCAL node
3 participants