[Merged by Bors] - Pass EL JWT secret key via cli flag #3568

mariuspod · 2022-09-12T22:15:17Z

Proposed Changes

In this change I've added a new beacon_node cli flag --execution-jwt-secret-key for passing the JWT secret directly as string.

Without this flag, it was non-trivial to pass a secrets file containing a JWT secret key without compromising its contents into some management repo or fiddling around with manual file mounts for cloud-based deployments.

When used in combination with environment variables, the secret can be injected into container-based systems like docker & friends quite easily.

It's both possible to either specify the file_path to the JWT secret or pass the JWT secret directly.

I've modified the docs and attached a test as well.

Additional Info

The logic has been adapted a bit so that either one of --execution-jwt or --execution-jwt-secret-key must be set when specifying --execution-endpoint so that it's still compatible with the semantics before this change and there's at least one secret provided.

…cret directly.

CLAassistant · 2022-09-12T22:15:22Z

All committers have signed the CLA.

paulhauner · 2022-09-13T07:56:53Z

Thanks for the contribution @mariuspod. It looks like cargo fmt is unhappy, you could fix this by running cargo fmt (ensuring you're on the latest Rust version).

mariuspod · 2022-09-13T08:29:23Z

thanks for the hint, I've just pushed another commit after running the formatter.

mariuspod · 2022-09-26T08:09:50Z

@paulhauner if there's anything I could change for this PR, please let me know.

paulhauner

Heyo, apologies for the delay here. I think this got swept up in all the merge excitement.

Thanks for this contribution, especially for implementing a test and updating the documentation ❤️ I have a few suggestions, please let me know what you think :)

beacon_node/src/config.rs

beacon_node/src/cli.rs

book/src/merge-migration.md

…cret-key and --execution-jwt as conflicting.

mariuspod · 2022-09-28T10:08:50Z

@paulhauner Thanks for your review! I agree to all of your recommendations and have updated the code.
I was unsure about the .to_string() on the error message string itself but I kept it for consistency.

paulhauner

Thanks for the changes, just a couple more along the same lines 🙏

paulhauner · 2022-09-29T05:16:56Z

beacon_node/src/config.rs

+            use std::io::Write;
+            secret_file = client_config.data_dir.join(DEFAULT_JWT_FILE);
+            let mut jwt_secret_key_file = File::create(secret_file.clone())
+                .expect("Error while creating jwt_secret_key file!");


I missed this on the last review sorry, but this expect will panic. Something like this would be more suitable:

Suggested change

.expect("Error while creating jwt_secret_key file!");

.map_err(|e| "Error while creating jwt_secret_key file: {:?}")?;

That's nice! So this handles the error and exits early without panicking, very cool. Will implement it like you proposed.

paulhauner · 2022-09-29T05:19:06Z

beacon_node/src/config.rs

+                .expect("Error while creating jwt_secret_key file!");
+            jwt_secret_key_file
+                .write_all(jwt_secret_key.as_bytes())
+                .expect("Error occured while writing to jwt_secret_key file!");


As above 🙏

mariuspod · 2022-09-29T15:21:48Z

I've added a call to format! inside the map_err block to interpolate the error object into the message. Your suggestion didn't include this, and I was wondering if there was some rust magic that defaults to format! inside the map_err if nothing else is specified 🤔

Anyways the tests are passing on my local machine with this 🎉

mariuspod · 2022-09-29T15:31:07Z

nvm, just tried without the format! and then the compiler gives a warning about the unused var so I guess no magic here 😅

warning: unused variable: `e`
311 |                 .map_err(|e| "Error while creating jwt_secret_key file: {:?}")?;
    |                           ^ help: if this is intentional, prefix it with an underscore: `_e`

mariuspod · 2022-10-03T21:08:49Z

for some reason this test failed only on windows. It seems unrelated to my changes: subnet_service::tests::attestation_service::test_same_subnet_unsubscription
What do you think ?

michaelsproul · 2022-10-03T21:23:38Z

@mariuspod Yeah that's a spurious failure. A flaky test that sometimes fails on slow hardware (we'll make it less flaky... soon).

paulhauner

Looks good, thanks for the contribution ❤️

This is ready to be merged with the next bors batch!

paulhauner · 2022-10-04T06:42:17Z

bors r+

## Proposed Changes In this change I've added a new beacon_node cli flag `--execution-jwt-secret-key` for passing the JWT secret directly as string. Without this flag, it was non-trivial to pass a secrets file containing a JWT secret key without compromising its contents into some management repo or fiddling around with manual file mounts for cloud-based deployments. When used in combination with environment variables, the secret can be injected into container-based systems like docker & friends quite easily. It's both possible to either specify the file_path to the JWT secret or pass the JWT secret directly. I've modified the docs and attached a test as well. ## Additional Info The logic has been adapted a bit so that either one of `--execution-jwt` or `--execution-jwt-secret-key` must be set when specifying `--execution-endpoint` so that it's still compatible with the semantics before this change and there's at least one secret provided.

bors · 2022-10-04T08:33:24Z

Build failed (retrying...):

release-tests-windows

## Proposed Changes In this change I've added a new beacon_node cli flag `--execution-jwt-secret-key` for passing the JWT secret directly as string. Without this flag, it was non-trivial to pass a secrets file containing a JWT secret key without compromising its contents into some management repo or fiddling around with manual file mounts for cloud-based deployments. When used in combination with environment variables, the secret can be injected into container-based systems like docker & friends quite easily. It's both possible to either specify the file_path to the JWT secret or pass the JWT secret directly. I've modified the docs and attached a test as well. ## Additional Info The logic has been adapted a bit so that either one of `--execution-jwt` or `--execution-jwt-secret-key` must be set when specifying `--execution-endpoint` so that it's still compatible with the semantics before this change and there's at least one secret provided.

bors · 2022-10-04T10:37:34Z

Build failed (retrying...):

merge-transition-ubuntu

## Proposed Changes In this change I've added a new beacon_node cli flag `--execution-jwt-secret-key` for passing the JWT secret directly as string. Without this flag, it was non-trivial to pass a secrets file containing a JWT secret key without compromising its contents into some management repo or fiddling around with manual file mounts for cloud-based deployments. When used in combination with environment variables, the secret can be injected into container-based systems like docker & friends quite easily. It's both possible to either specify the file_path to the JWT secret or pass the JWT secret directly. I've modified the docs and attached a test as well. ## Additional Info The logic has been adapted a bit so that either one of `--execution-jwt` or `--execution-jwt-secret-key` must be set when specifying `--execution-endpoint` so that it's still compatible with the semantics before this change and there's at least one secret provided.

bors · 2022-10-04T14:46:23Z

Pull request successfully merged into unstable.

Build succeeded:

## Proposed Changes In this change I've added a new beacon_node cli flag `--execution-jwt-secret-key` for passing the JWT secret directly as string. Without this flag, it was non-trivial to pass a secrets file containing a JWT secret key without compromising its contents into some management repo or fiddling around with manual file mounts for cloud-based deployments. When used in combination with environment variables, the secret can be injected into container-based systems like docker & friends quite easily. It's both possible to either specify the file_path to the JWT secret or pass the JWT secret directly. I've modified the docs and attached a test as well. ## Additional Info The logic has been adapted a bit so that either one of `--execution-jwt` or `--execution-jwt-secret-key` must be set when specifying `--execution-endpoint` so that it's still compatible with the semantics before this change and there's at least one secret provided.

feat: added --execution-jwt-secret-key cli flag for specifying the se…

30621d2

…cret directly.

paulhauner added the ready-for-review The code is ready for review label Sep 13, 2022

fix: cargo fmt

dcddf32

paulhauner requested changes Sep 28, 2022

View reviewed changes

beacon_node/src/config.rs Outdated Show resolved Hide resolved

beacon_node/src/cli.rs Show resolved Hide resolved

book/src/merge-migration.md Outdated Show resolved Hide resolved

paulhauner added waiting-on-author The reviewer has suggested changes and awaits thier implementation. and removed ready-for-review The code is ready for review labels Sep 28, 2022

paulhauner self-assigned this Sep 28, 2022

fix: return Err instead of panic and set cli flags --execution-jwt-se…

ec1c17a

…cret-key and --execution-jwt as conflicting.

paulhauner added ready-for-review The code is ready for review and removed waiting-on-author The reviewer has suggested changes and awaits thier implementation. labels Sep 29, 2022

paulhauner requested changes Sep 29, 2022

View reviewed changes

paulhauner added waiting-on-author The reviewer has suggested changes and awaits thier implementation. and removed ready-for-review The code is ready for review labels Sep 29, 2022

fix: replace #except by #map_error combinator and use '?' operator.

ee28bde

michaelsproul added ready-for-review The code is ready for review and removed waiting-on-author The reviewer has suggested changes and awaits thier implementation. labels Oct 3, 2022

paulhauner approved these changes Oct 4, 2022

View reviewed changes

paulhauner added ready-for-merge This PR is ready to merge. and removed ready-for-review The code is ready for review labels Oct 4, 2022

bors bot changed the title ~~Pass EL JWT secret key via cli flag~~ [Merged by Bors] - Pass EL JWT secret key via cli flag Oct 4, 2022

bors bot closed this Oct 4, 2022

	.expect("Error while creating jwt_secret_key file!");
	.map_err(\|e\| "Error while creating jwt_secret_key file: {:?}")?;

[Merged by Bors] - Pass EL JWT secret key via cli flag #3568

[Merged by Bors] - Pass EL JWT secret key via cli flag #3568

Uh oh!

Conversation

mariuspod commented Sep 12, 2022

Proposed Changes

Additional Info

Uh oh!

CLAassistant commented Sep 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

paulhauner commented Sep 13, 2022

Uh oh!

mariuspod commented Sep 13, 2022

Uh oh!

mariuspod commented Sep 26, 2022

Uh oh!

paulhauner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mariuspod commented Sep 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

paulhauner left a comment

Choose a reason for hiding this comment

Uh oh!

paulhauner Sep 29, 2022

Choose a reason for hiding this comment

Uh oh!

mariuspod Sep 29, 2022

Choose a reason for hiding this comment

Uh oh!

paulhauner Sep 29, 2022

Choose a reason for hiding this comment

Uh oh!

mariuspod commented Sep 29, 2022

Uh oh!

mariuspod commented Sep 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mariuspod commented Oct 3, 2022

Uh oh!

michaelsproul commented Oct 3, 2022

Uh oh!

paulhauner left a comment

Choose a reason for hiding this comment

Uh oh!

paulhauner commented Oct 4, 2022

Uh oh!

bors bot commented Oct 4, 2022

Uh oh!

bors bot commented Oct 4, 2022

Uh oh!

bors bot commented Oct 4, 2022

Uh oh!

Uh oh!

CLAassistant commented Sep 12, 2022 •

edited

Loading

mariuspod commented Sep 28, 2022 •

edited

Loading

mariuspod commented Sep 29, 2022 •

edited

Loading