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: support CIDR blocks in no_proxy env variable #2876

Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
3 changes: 2 additions & 1 deletion doc/environment_variables.md
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ can be set.
A comma separated list of hostnames to connect to without using a proxy even
if a proxy is set. These variables are checked in order, and the first one
that has a value is used.
The `no_proxy` environment variable could be a comma-separated list of hostnames, IP addresses, or CIDR blocks (no_proxy=example.com,192.168.0.1,192.168.0.0/16).
melkouri marked this conversation as resolved.
Show resolved Hide resolved

* GRPC_SSL_CIPHER_SUITES
A colon separated list of cipher suites to use with OpenSSL
Expand Down Expand Up @@ -66,4 +67,4 @@ can be set.
* GRPC_NODE_USE_ALTERNATIVE_RESOLVER
Allows changing dns resolve behavior and parse DNS server authority as described in https://github.com/grpc/grpc/blob/master/doc/naming.md
- true - use alternative resolver
- false - use default resolver (default)
- false - use default resolver (default)
60 changes: 53 additions & 7 deletions packages/grpc-js/src/http_proxy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,56 @@ function getNoProxyHostList(): string[] {
}
}

interface CIDRNotation {
ip: number;
prefixLength: number;
}

/*
* The groups correspond to CIDR parts as follows:
* 1. ip
* 2. prefixLength
*/
const CIDR_REGEX = /^([0-9.]+)(?:\/([0-9]+))?$/;

export function parseCIDR(cidrString: string): CIDRNotation | null {
const parsedCIDR = CIDR_REGEX.exec(cidrString);
melkouri marked this conversation as resolved.
Show resolved Hide resolved
if (parsedCIDR && parsedCIDR.length === 3) {
return {
ip: ipToInt(parsedCIDR[1]),
prefixLength: parseInt(parsedCIDR[2] ?? '32', 10),
};
}
return null;
}

function ipToInt(ip: string) {
return ip.split(".").reduce((acc, octet) => (acc << 8) + parseInt(octet, 10), 0);
}

function isIpInCIDR(cidr: CIDRNotation, serverHost: string) {
const ip = cidr.ip;
const mask = -1 << (32 - cidr.prefixLength);
const hostIP = ipToInt(serverHost);

const networkAddress = ip & mask;
const broadcastAddress = networkAddress | ~mask;

return hostIP >= networkAddress && hostIP <= broadcastAddress;
melkouri marked this conversation as resolved.
Show resolved Hide resolved
}

function checkHostInNoProxyHostList(serverHost: string): boolean {
melkouri marked this conversation as resolved.
Show resolved Hide resolved
for (const host of getNoProxyHostList()) {
const parsedCIDR = parseCIDR(host);
// host is a single IP address or a CIDR notation or a domain
if (parsedCIDR && isIpInCIDR(parsedCIDR, serverHost) || serverHost.endsWith(host)) {
Copy link
Member

Choose a reason for hiding this comment

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

The case where a non-IP serverHost string is passed to isIpInCIDR is not explicitly handled, making the result unclear. Either this line should avoid making that call, or isIpInCIDR should short-circuit return false if the serverHost is not an IP address, or ipToInt should return an error value if ip is not an IP address and isIpInCIDR should check for that value.

Copy link
Member

Choose a reason for hiding this comment

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

The original logic uses host === serverHost to check for matches. That has been replaced here with serverHost.endsWith(host). That change is not related to supporting CIDR ranges, so it should be reverted.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The original logic where we handle single IPs is handled by isIpInCIDR(parsedCIDR, serverHost) as this latter transforms single IPs to CIDR notations (here).
This serverHost.endsWith(host) was to support the case where we include domain names/suffixes in the NO_PROXY (for example NO_PROXY=example.com,.local.,.local.).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This case wasn't stated in the issue, I'll update it and make sure it's well handled in the code. Thank you for raising this 🙇

trace('Not using proxy for target in no_proxy list: ' + serverHost);
melkouri marked this conversation as resolved.
Show resolved Hide resolved
return true;
}
}
return false;
}

export interface ProxyMapResult {
target: GrpcUri;
extraOptions: ChannelOptions;
Expand Down Expand Up @@ -147,13 +197,9 @@ export function mapProxyName(
return noProxyResult;
}
const serverHost = hostPort.host;
for (const host of getNoProxyHostList()) {
if (host === serverHost) {
trace(
'Not using proxy for target in no_proxy list: ' + uriToString(target)
);
return noProxyResult;
}
if (checkHostInNoProxyHostList(serverHost)) {
trace('Not using proxy for target in no_proxy list: ' + uriToString(target));
return noProxyResult;
}
const extraOptions: ChannelOptions = {
'grpc.http_connect_target': uriToString(target),
Expand Down