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

fix: make password as non required field #1023

Closed
wants to merge 1 commit into from

Conversation

shrimalmadhur
Copy link
Contributor

Why are these changes needed?

Fixes #1021

  • Makes password as not required since folks do testing with no password encrypted keys.

Checks

  • I've made sure the tests are passing. Note that there might be a few flaky tests, in that case, please comment that they are not relevant.
  • I've checked the new test coverage and the coverage percentage didn't drop.
  • Testing Strategy
    • Unit tests
    • Integration tests
    • This PR is not tested :(

@@ -165,8 +165,8 @@ func NewConfig(ctx *cli.Context) (*Config, error) {
if blsRemoteSignerEnabled && (ctx.GlobalString(flags.BLSRemoteSignerUrlFlag.Name) == "" || ctx.GlobalString(flags.BLSPublicKeyHexFlag.Name) == "") {
return nil, fmt.Errorf("BLS remote signer URL and Public Key Hex is required if BLS remote signer is enabled")
}
if !blsRemoteSignerEnabled && (ctx.GlobalString(flags.BlsKeyFileFlag.Name) == "" || ctx.GlobalString(flags.BlsKeyPasswordFlag.Name) == "") {
return nil, fmt.Errorf("BLS key file and password is required if BLS remote signer is disabled")
if !blsRemoteSignerEnabled && ctx.GlobalString(flags.BlsKeyFileFlag.Name) == "" {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe add a warn message?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

warn message about not providing a password?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it will anyway fail since the keys won't decrypt and server won't start. so I am not sure if warn is helpful for that.

Copy link
Contributor

@ian-shim ian-shim left a comment

Choose a reason for hiding this comment

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

Hmm I think we need an input from @anupsv
I think an argument can be made that we can be more opinionated here and require password even if they just want to test it (or only allow no password if testMode?). So that there cannot be plain keys in production

@shrimalmadhur
Copy link
Contributor Author

cannot be plain keys in production

hmm I think in testMode - we use plaintext pk.
In testnet though - which is not testMode - they still use a file but what the issue says is they don't encrypt the file with password (which is kinda useless). But I am wondering if folks want to quickly test with no password on testnet. We can try to encore password on testnet too if that makes more sense.

@anupsv
Copy link
Contributor

anupsv commented Dec 18, 2024

cannot be plain keys in production

hmm I think in testMode - we use plaintext pk. In testnet though - which is not testMode - they still use a file but what the issue says is they don't encrypt the file with password (which is kinda useless). But I am wondering if folks want to quickly test with no password on testnet. We can try to encore password on testnet too if that makes more sense.

For testnet, i'd say it's good to keep the behavior close to as what they'd need to do for production. If it's in testmode, then sure we can have no password. Since the effort is to increase the security posture for operators, it'll be good to have more secure defaults.

@shrimalmadhur
Copy link
Contributor Author

Closing this. As Anup said we want to have similar behavior in testnet and mainnet.

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.

[Bug]: Empty NODE_BLS_KEY_PASSWORD error
4 participants