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

Jump to bottom

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

Merged
merged 4 commits into from
Dec 12, 2023
Merged

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

Show file tree
Hide file tree
Changes from all commits
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
4 changes: 2 additions & 2 deletions packages/beacon-node/src/eth1/provider/eth1Provider.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import {toHexString} from "@chainsafe/ssz";
import {phase0} from "@lodestar/types";
import {ChainConfig} from "@lodestar/config";
import {fromHex, isErrorAborted, createElapsedTimeTracker} from "@lodestar/utils";
import {fromHex, isErrorAborted, createElapsedTimeTracker, toSafePrintableUrl} from "@lodestar/utils";
import {Logger} from "@lodestar/logger";

import {FetchError, isFetchError} from "@lodestar/api";
Expand Down Expand Up @@ -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.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
Expand All @@ -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;
Expand Down
4 changes: 2 additions & 2 deletions packages/beacon-node/src/execution/builder/http.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import {ChainForkConfig} from "@lodestar/config";
import {Logger} from "@lodestar/logger";
import {getClient, Api as BuilderApi} from "@lodestar/api/builder";
import {SLOTS_PER_EPOCH, ForkExecution} from "@lodestar/params";

import {toSafePrintableUrl} from "@lodestar/utils";
import {ApiError} from "@lodestar/api";
import {Metrics} from "../../metrics/metrics.js";
import {IExecutionBuilder} from "./interface.js";
Expand Down Expand Up @@ -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", {urls: opts.urls.toString()});
this.api = getClient(
{
baseUrl,
Expand All @@ -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;

Expand Down
4 changes: 2 additions & 2 deletions packages/beacon-node/src/execution/engine/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import {fromHex} from "@lodestar/utils";
import {fromHex, toSafePrintableUrl} from "@lodestar/utils";
import {JsonRpcHttpClient} from "../../eth1/provider/jsonRpcHttpClient.js";
import {IExecutionEngine} from "./interface.js";
import {ExecutionEngineDisabled} from "./disabled.js";
Expand Down Expand Up @@ -31,7 +31,6 @@ export function getExecutionEngineHttp(
opts: ExecutionEngineHttpOpts,
modules: ExecutionEngineModules
): IExecutionEngine {
modules.logger.info("Execution client", {urls: opts.urls.toString()});
const rpc = new JsonRpcHttpClient(opts.urls, {
...opts,
signal: modules.signal,
Expand All @@ -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);
}

Expand Down
4 changes: 2 additions & 2 deletions packages/cli/src/cmds/validator/signers/logSigners.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import {Signer, SignerLocal, SignerRemote, SignerType} from "@lodestar/validator";
import {LogLevel, Logger} from "@lodestar/utils";
import {LogLevel, Logger, toSafePrintableUrl} from "@lodestar/utils";

/**
* Log each pubkeys for auditing out keys are loaded from the logs
Expand Down Expand Up @@ -27,7 +27,7 @@ export function logSigners(logger: Pick<Logger, LogLevel.info>, signers: Signer[
}

for (const {url, pubkeys} of groupExternalSignersByUrl(remoteSigners)) {
logger.info(`External signers on URL: ${url}`);
logger.info(`External signers on URL: ${toSafePrintableUrl(url)}`);
for (const pubkey of pubkeys) {
logger.info(pubkey);
}
Expand Down
2 changes: 1 addition & 1 deletion packages/utils/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ export * from "./sleep.js";
export * from "./sort.js";
export * from "./timeout.js";
export {type RecursivePartial, bnToNum} from "./types.js";
export * from "./validation.js";
export * from "./url.js";
export * from "./verifyMerkleBranch.js";
export * from "./promise.js";
export * from "./waitFor.js";
9 changes: 9 additions & 0 deletions packages/utils/src/validation.ts → packages/utils/src/url.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,3 +18,12 @@ export function isValidHttpUrl(urlStr: string): boolean {

return url.protocol === "http:" || url.protocol === "https:";
}

/**
* Sanitize URL to prevent leaking user credentials in logs
*
* Note: `urlStr` must be a valid URL
*/
export function toSafePrintableUrl(urlStr: string): string {
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.

}
4 changes: 2 additions & 2 deletions packages/validator/src/validator.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import {toHexString} from "@chainsafe/ssz";
import {BLSPubkey, phase0, ssz} from "@lodestar/types";
import {createBeaconConfig, BeaconConfig, ChainForkConfig} from "@lodestar/config";
import {Genesis} from "@lodestar/types/phase0";
import {Logger} from "@lodestar/utils";
import {Logger, toSafePrintableUrl} from "@lodestar/utils";
import {getClient, Api, routes, ApiError} from "@lodestar/api";
import {computeEpochAtSlot, getCurrentSlot} from "@lodestar/state-transition";
import {Clock, IClock} from "./util/clock.js";
Expand Down Expand Up @@ -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.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;
}
Expand Down