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.

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

fix: sanitize URL to prevent leaking user credentials in logs #6175

Merged
merged 4 commits into from
Dec 12, 2023
+20 −11
Merged

fix: sanitize URL to prevent leaking user credentials in logs #6175

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e1dd46e
fix: sanitize URL to prevent leaking user credentials in logs
nflaig Dec 9, 2023
876063f
Best effort to sanitize if an invalid URL is provided
nflaig Dec 11, 2023
71fef20
Only log used builder URL
nflaig Dec 11, 2023
d64cdca
Ensure URLs are logged after validation
nflaig Dec 11, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Ensure URLs are logged after validation
@nflaig
nflaig committed Dec 11, 2023

Verified

This commit was signed with the committer’s verified signature.
nflaig Nico Flaig
GPG key ID: 0DA302A48F59A62C
Verified
Learn about vigilant mode
commit d64cdca951bce6a2c2cae4b8570c29e6cfba2d45
2 changes: 1 addition & 1 deletion packages/beacon-node/src/eth1/provider/eth1Provider.ts
View file
Original file line number Diff line number Diff line change
@@ -75,7 +75,6 @@ export class Eth1Provider implements IEth1Provider {
this.depositContractAddress = toHexString(config.DEPOSIT_CONTRACT_ADDRESS);

const providerUrls = opts.providerUrls ?? DEFAULT_PROVIDER_URLS;
this.logger?.info("Eth1 provider", {urls: providerUrls.map(toSafePrintableUrl).toString()});
this.rpc = new JsonRpcHttpClient(providerUrls, {
signal,
// Don't fallback with is truncated error. Throw early and let the retry on this class handle it
@@ -85,6 +84,7 @@ export class Eth1Provider implements IEth1Provider {
jwtVersion: opts.jwtVersion,
metrics: metrics,
});
this.logger?.info("Eth1 provider", {urls: providerUrls.map(toSafePrintableUrl).toString()});

this.rpc.emitter.on(JsonRpcHttpClientEvent.RESPONSE, () => {
const oldState = this.state;
2 changes: 1 addition & 1 deletion packages/beacon-node/src/execution/builder/http.ts
View file
Original file line number Diff line number Diff line change
@@ -50,7 +50,6 @@ export class ExecutionBuilderHttp implements IExecutionBuilder {
) {
const baseUrl = opts.urls[0];
if (!baseUrl) throw Error("No Url provided for executionBuilder");
logger?.info("External builder", {url: toSafePrintableUrl(baseUrl)});
this.api = getClient(
{
baseUrl,
@@ -59,6 +58,7 @@ export class ExecutionBuilderHttp implements IExecutionBuilder {
},
{config, metrics: metrics?.builderHttpClient}
);
logger?.info("External builder", {url: toSafePrintableUrl(baseUrl)});
Copy link
Member Author

Choose a reason for hiding this comment

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

Builder is a bit of an outlier right now, we allow to set multiple URLs via --builder.urls but only use the first URL (opts.urls[0]). I opted to only log the URL which is actually used.

@g11tech Is it intentional that we don't allow a fallback for the builder? I guess mev-boost is already doing the multiplexing and builder flow is stateful which makes fallback logic a bit problematic.

Maybe we want to reflect that in the CLI as well as currently it looks like it supports multiple urls

image

this.config = config;
this.issueLocalFcUWithFeeRecipient = opts.issueLocalFcUWithFeeRecipient;

2 changes: 1 addition & 1 deletion packages/beacon-node/src/execution/engine/index.ts
View file
Original file line number Diff line number Diff line change
@@ -31,7 +31,6 @@ export function getExecutionEngineHttp(
opts: ExecutionEngineHttpOpts,
modules: ExecutionEngineModules
): IExecutionEngine {
modules.logger.info("Execution client", {urls: opts.urls.map(toSafePrintableUrl).toString()});
const rpc = new JsonRpcHttpClient(opts.urls, {
...opts,
signal: modules.signal,
@@ -40,6 +39,7 @@ export function getExecutionEngineHttp(
jwtId: opts.jwtId,
jwtVersion: opts.jwtVersion,
});
modules.logger.info("Execution client", {urls: opts.urls.map(toSafePrintableUrl).toString()});
return new ExecutionEngineHttp(rpc, modules);
}

9 changes: 3 additions & 6 deletions packages/utils/src/url.ts
View file
Original file line number Diff line number Diff line change
@@ -21,12 +21,9 @@ export function isValidHttpUrl(urlStr: string): boolean {

/**
* Sanitize URL to prevent leaking user credentials in logs
*
* Note: `urlStr` must be a valid URL
*/
export function toSafePrintableUrl(urlStr: string): string {
try {
return new URL(urlStr).origin;
} catch (_) {
// Best effort to sanitize value if an invalid URL is provided
return urlStr.replace(/(.*?:\/\/|.*?:\/)?(.*?:.*?@)/, "$1");
}
return new URL(urlStr).origin;
Copy link
Contributor

Choose a reason for hiding this comment

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

This will strip out searchpart of URL, assuming this is fine.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is desired in case someone provides secrets in query params. I don't know of anybody that does this but we can't prevent users from doing that and passing credentials directly as part of the URL is common because it works with all clients.

Copy link
Contributor

Choose a reason for hiding this comment

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

Got it, somehow I assumed credentials could be passed via URL username/password properties

Copy link
Member Author

@nflaig nflaig Dec 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

In case of rescue node, they are passing the user credentials in the userinfo part of the URL. We move those into the Authorization header (#5884) before the request is sent but at the time it's logged at startup, the URL still contains credentials.

Copy link
Contributor

Choose a reason for hiding this comment

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

Note that calling URL with an invalid URL with throw an error. Is urlStr sure to be a valid URL in the normal Beacon Node init flow? Else users may get funny errors with the wrong URL.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch! Completely forgot about that, I added a note to the function now and moved all URL logs after they are already validated by http / jsonrpc client.

Also looked into sanitizing invalid URLs (in catch block) but that got too messy.

}
25 changes: 0 additions & 25 deletions packages/utils/test/unit/url.test.ts
View file

This file was deleted.

2 changes: 1 addition & 1 deletion packages/validator/src/validator.ts
View file
Original file line number Diff line number Diff line change
@@ -268,10 +268,10 @@ export class Validator {
let api: Api;
if (typeof opts.api === "string" || Array.isArray(opts.api)) {
const urls = typeof opts.api === "string" ? [opts.api] : opts.api;
logger.info("Beacon node", {urls: urls.map(toSafePrintableUrl).toString()});
// This new api instance can make do with default timeout as a faster timeout is
// not necessary since this instance won't be used for validator duties
api = getClient({urls, getAbortSignal: () => opts.abortController.signal}, {config, logger});
logger.info("Beacon node", {urls: urls.map(toSafePrintableUrl).toString()});
} else {
api = opts.api;
}