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

Redact endpoint secrets in logs #3989

Merged
merged 3 commits into from
Sep 14, 2023
Merged

Redact endpoint secrets in logs #3989

merged 3 commits into from
Sep 14, 2023

Conversation

j4m1ef0rd
Copy link
Contributor

Pull Request

Closes: PRO-733

Checklist

Please conduct a thorough self-review before opening the PR.

  • I am confident that the code works.
  • I have updated documentation where appropriate.

Summary

  • Moved the existing (but unused) redact code to the utilities crate.
  • Made a new type SecretUrl that is a wrapper around a string so I can guarantee it is displayed using the redact function.
    • The new type will not redact if you print the type using debug and have debug_assersion enabled.
  • Used the new type in the Eth,Dot & Btc witnessers.
  • Added the SecretUrl to some errors in setting.rs, for better error feedback.

Note: The redact code was specific for Eth, but should work for the other endpoints because it will just redact almost the whole url if no eth secret is found. But let me know if you have an example of a Dot or Btc endpoint that we can use to create specific redaction code.

@j4m1ef0rd j4m1ef0rd added the CFE label Sep 12, 2023
@j4m1ef0rd j4m1ef0rd requested a review from msgmaxim September 12, 2023 06:23
@j4m1ef0rd j4m1ef0rd self-assigned this Sep 12, 2023
@linear
Copy link

linear bot commented Sep 12, 2023

@codecov
Copy link

codecov bot commented Sep 12, 2023

Codecov Report

Merging #3989 (d16de5e) into main (a9a25eb) will decrease coverage by 0%.
Report is 5 commits behind head on main.
The diff coverage is 82%.

@@          Coverage Diff           @@
##            main   #3989    +/-   ##
======================================
- Coverage     72%     72%    -0%     
======================================
  Files        366     366            
  Lines      57082   57340   +258     
  Branches   57082   57340   +258     
======================================
+ Hits       41210   41385   +175     
- Misses     13786   13854    +68     
- Partials    2086    2101    +15     
Files Changed Coverage Δ
engine/src/btc/rpc.rs 0% <0%> (ø)
engine/src/dot/http_rpc.rs 0% <0%> (ø)
engine/src/dot/retry_rpc.rs 7% <0%> (ø)
engine/src/dot/rpc.rs 0% <0%> (ø)
engine/src/eth/mod.rs 0% <ø> (ø)
engine/src/eth/rpc.rs 0% <0%> (ø)
engine/src/witness/eth/key_manager.rs 0% <0%> (ø)
utilities/src/with_std.rs 50% <ø> (ø)
utilities/src/with_std/redact_endpoint_secret.rs 94% <94%> (ø)
engine/src/settings.rs 85% <98%> (+<1%) ⬆️

... and 22 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@j4m1ef0rd j4m1ef0rd force-pushed the feat/redact_endpoint_secret branch from a0cb104 to 1676b99 Compare September 12, 2023 06:43
@kylezs
Copy link
Contributor

kylezs commented Sep 12, 2023

Here's an example of a bitcoin one from getblock.io (not valid, but a valid format): https://btc.getblock.io/de76678e-a489-4503-2ba2-81156c471220/mainnet/

Copy link
Contributor

@kylezs kylezs left a comment

Choose a reason for hiding this comment

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

Nice, thanks. Maybe take a look at the bitcoin case I gave before merging

/// Partially redacts the secret in the url of the node endpoint.
/// eg: `wss://cdcd639308194d3f977a1a5a7ff0d545.rinkeby.ws.rivet.cloud/` ->
/// `wss://cdc****.rinkeby.ws.rivet.cloud/`
#[allow(unused)]
Copy link
Contributor

Choose a reason for hiding this comment

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

it's used now

utilities/src/with_std/redact_endpoint_secret.rs Outdated Show resolved Hide resolved
utilities/src/with_std/redact_endpoint_secret.rs Outdated Show resolved Hide resolved
@j4m1ef0rd j4m1ef0rd force-pushed the feat/redact_endpoint_secret branch from 7e254d0 to d16de5e Compare September 13, 2023 04:20
@msgmaxim msgmaxim merged commit 52551a0 into main Sep 14, 2023
@msgmaxim msgmaxim deleted the feat/redact_endpoint_secret branch September 14, 2023 00:39
dandanlen pushed a commit that referenced this pull request Sep 18, 2023
* chore: moved redact code to utils

* feat: use SecretUrl type for endpoints

* refactor: Addressed PR comments
dandanlen pushed a commit that referenced this pull request Sep 28, 2023
* chore: moved redact code to utils

* feat: use SecretUrl type for endpoints

* refactor: Addressed PR comments
dandanlen pushed a commit that referenced this pull request Oct 9, 2023
* chore: moved redact code to utils

* feat: use SecretUrl type for endpoints

* refactor: Addressed PR comments
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.

3 participants