From 0b6e2a3275f0b3e1e489aaa3b033410d32b60896 Mon Sep 17 00:00:00 2001 From: Michael Lumish Date: Wed, 5 Feb 2025 14:40:44 -0800 Subject: [PATCH 01/27] Add kokoro config for PSM interop security tests --- test/kokoro/psm-security.cfg | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) create mode 100644 test/kokoro/psm-security.cfg diff --git a/test/kokoro/psm-security.cfg b/test/kokoro/psm-security.cfg new file mode 100644 index 00000000..e6c733f3 --- /dev/null +++ b/test/kokoro/psm-security.cfg @@ -0,0 +1,30 @@ +# Copyright 2025 gRPC authors. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +# Config file for Kokoro (in protobuf text format) + +# Location of the continuous shell script in repository. +build_file: "grpc-node/packages/grpc-js-xds/scripts/psm-interop-test-node.sh" +timeout_mins: 360 +action { + define_artifacts { + regex: "artifacts/**/*sponge_log.xml" + regex: "artifacts/**/*.log" + strip_prefix: "artifacts" + } +} +env_vars { + key: "PSM_TEST_SUITE" + value: "security" +} From 7a255395bc667723f26f26521e5e1c74977d7d84 Mon Sep 17 00:00:00 2001 From: Michael Lumish Date: Thu, 6 Feb 2025 10:43:30 -0800 Subject: [PATCH 02/27] Make secure_mode parsing case-insensitive --- packages/grpc-js-xds/interop/xds-interop-server.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/grpc-js-xds/interop/xds-interop-server.ts b/packages/grpc-js-xds/interop/xds-interop-server.ts index 5ec77236..4b34c7de 100644 --- a/packages/grpc-js-xds/interop/xds-interop-server.ts +++ b/packages/grpc-js-xds/interop/xds-interop-server.ts @@ -228,11 +228,10 @@ function getIPv6Addresses(): string[] { async function main() { const argv = yargs - .string(['port', 'maintenance_port', 'address_type']) - .boolean(['secure_mode']) + .string(['port', 'maintenance_port', 'address_type', 'secure_mode']) .demandOption(['port']) .default('address_type', 'IPV4_IPV6') - .default('secure_mode', false) + .default('secure_mode', 'false') .parse() console.log('Starting xDS interop server. Args: ', argv); const healthImpl = new HealthImplementation({'': 'NOT_SERVING'}); @@ -250,7 +249,8 @@ async function main() { services: ['grpc.testing.TestService'] }) const addressType = argv.address_type.toUpperCase(); - if (argv.secure_mode) { + const secureMode = argv.secure_mode.toLowerCase() == 'true'; + if (secureMode) { if (addressType !== 'IPV4_IPV6') { throw new Error('Secure mode only supports IPV4_IPV6 address type'); } From a7219808db6f889a33cb8d13e4611573e0480c0d Mon Sep 17 00:00:00 2001 From: Michael Lumish Date: Thu, 6 Feb 2025 16:34:50 -0800 Subject: [PATCH 03/27] xds interop server: bind IPv4 in secure mode --- packages/grpc-js-xds/interop/xds-interop-server.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/grpc-js-xds/interop/xds-interop-server.ts b/packages/grpc-js-xds/interop/xds-interop-server.ts index 4b34c7de..59d76227 100644 --- a/packages/grpc-js-xds/interop/xds-interop-server.ts +++ b/packages/grpc-js-xds/interop/xds-interop-server.ts @@ -265,7 +265,7 @@ async function main() { const xdsCreds = new grpc_xds.XdsServerCredentials(grpc.ServerCredentials.createInsecure()); await Promise.all([ serverBindPromise(maintenanceServer, `[::]:${argv.maintenance_port}`, grpc.ServerCredentials.createInsecure()), - serverBindPromise(server, `[::]:${argv.port}`, xdsCreds) + serverBindPromise(server, `0.0.0.0:${argv.port}`, xdsCreds) ]); } else { const server = new grpc.Server({interceptors: [unifiedInterceptor]}); From 564e80f7364bdcc0b6052f48a5a3b4d5b448196e Mon Sep 17 00:00:00 2001 From: Michael Lumish Date: Wed, 12 Feb 2025 13:45:13 -0800 Subject: [PATCH 04/27] Enable http_filter tracer on server --- packages/grpc-js-xds/interop/test-server.Dockerfile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/grpc-js-xds/interop/test-server.Dockerfile b/packages/grpc-js-xds/interop/test-server.Dockerfile index 59fdc700..b20b0595 100644 --- a/packages/grpc-js-xds/interop/test-server.Dockerfile +++ b/packages/grpc-js-xds/interop/test-server.Dockerfile @@ -46,7 +46,7 @@ COPY --from=build /node/src/grpc-node/packages/grpc-js ./packages/grpc-js/ COPY --from=build /node/src/grpc-node/packages/grpc-js-xds ./packages/grpc-js-xds/ ENV GRPC_VERBOSITY="DEBUG" -ENV GRPC_TRACE=xds_client,server,xds_server +ENV GRPC_TRACE=xds_client,server,xds_server,http_filter # tini serves as PID 1 and enables the server to properly respond to signals. COPY --from=build /tini /tini From eed4d54537e45753b7d0a3c78ce19f19e99d6ed5 Mon Sep 17 00:00:00 2001 From: Michael Lumish Date: Wed, 12 Feb 2025 15:33:52 -0800 Subject: [PATCH 05/27] Don't require api_listener when validating Listener --- .../xds-resource-type/listener-resource-type.ts | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/packages/grpc-js-xds/src/xds-resource-type/listener-resource-type.ts b/packages/grpc-js-xds/src/xds-resource-type/listener-resource-type.ts index 46538145..f78bc7e6 100644 --- a/packages/grpc-js-xds/src/xds-resource-type/listener-resource-type.ts +++ b/packages/grpc-js-xds/src/xds-resource-type/listener-resource-type.ts @@ -262,14 +262,15 @@ export class ListenerResourceType extends XdsResourceType { private validateResource(context: XdsDecodeContext, message: Listener__Output): ValidationResult { const errors: string[] = []; - if ( - message.api_listener?.api_listener && - message.api_listener.api_listener.type_url === HTTP_CONNECTION_MANGER_TYPE_URL - ) { - const httpConnectionManager = decodeSingleResource(HTTP_CONNECTION_MANGER_TYPE_URL, message.api_listener!.api_listener.value); - errors.push(...validateHttpConnectionManager(httpConnectionManager).map(error => `api_listener.api_listener: ${error}`)); - } else { - errors.push(`api_listener.api_listener.type_url != ${HTTP_CONNECTION_MANGER_TYPE_URL}`); + if (message.api_listener?.api_listener) { + if ( + message.api_listener.api_listener.type_url === HTTP_CONNECTION_MANGER_TYPE_URL + ) { + const httpConnectionManager = decodeSingleResource(HTTP_CONNECTION_MANGER_TYPE_URL, message.api_listener!.api_listener.value); + errors.push(...validateHttpConnectionManager(httpConnectionManager).map(error => `api_listener.api_listener: ${error}`)); + } else { + errors.push(`api_listener.api_listener.type_url != ${HTTP_CONNECTION_MANGER_TYPE_URL}`); + } } if (message.listener_filters.length > 0) { errors.push('listener_filters populated'); From f6631f5162690600898cd4c00fed1678fc8182de Mon Sep 17 00:00:00 2001 From: Michael Lumish Date: Wed, 12 Feb 2025 15:34:09 -0800 Subject: [PATCH 06/27] Call xds library register function in interop server --- packages/grpc-js-xds/interop/xds-interop-server.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/packages/grpc-js-xds/interop/xds-interop-server.ts b/packages/grpc-js-xds/interop/xds-interop-server.ts index 59d76227..5cb42d2c 100644 --- a/packages/grpc-js-xds/interop/xds-interop-server.ts +++ b/packages/grpc-js-xds/interop/xds-interop-server.ts @@ -30,6 +30,8 @@ import { SimpleRequest__Output } from './generated/grpc/testing/SimpleRequest'; import { SimpleResponse } from './generated/grpc/testing/SimpleResponse'; import { ReflectionService } from '@grpc/reflection'; +grpc_xds.register(); + const packageDefinition = protoLoader.loadSync('grpc/testing/test.proto', { keepCase: true, defaults: true, From 2979fa706d49dc7401a7fe807b7fe1efb298b122 Mon Sep 17 00:00:00 2001 From: Michael Lumish Date: Wed, 12 Feb 2025 17:44:31 -0800 Subject: [PATCH 07/27] Enable transport and certificate_provider tracers --- packages/grpc-js-xds/interop/test-client.Dockerfile | 2 +- packages/grpc-js-xds/interop/test-server.Dockerfile | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/grpc-js-xds/interop/test-client.Dockerfile b/packages/grpc-js-xds/interop/test-client.Dockerfile index 8ef6105f..f8bb3aaf 100644 --- a/packages/grpc-js-xds/interop/test-client.Dockerfile +++ b/packages/grpc-js-xds/interop/test-client.Dockerfile @@ -42,7 +42,7 @@ COPY --from=build /node/src/grpc-node/packages/grpc-js ./packages/grpc-js/ COPY --from=build /node/src/grpc-node/packages/grpc-js-xds ./packages/grpc-js-xds/ ENV GRPC_VERBOSITY="DEBUG" -ENV GRPC_TRACE=xds_client,xds_resolver,xds_cluster_manager,cds_balancer,xds_cluster_resolver,xds_cluster_impl,priority,weighted_target,round_robin,resolving_load_balancer,subchannel,keepalive,dns_resolver,fault_injection,http_filter,csds,outlier_detection,server,server_call,ring_hash +ENV GRPC_TRACE=xds_client,xds_resolver,xds_cluster_manager,cds_balancer,xds_cluster_resolver,xds_cluster_impl,priority,weighted_target,round_robin,resolving_load_balancer,subchannel,keepalive,dns_resolver,fault_injection,http_filter,csds,outlier_detection,server,server_call,ring_hash,transport,certificate_provider ENV NODE_XDS_INTEROP_VERBOSITY=1 ENTRYPOINT [ "/nodejs/bin/node", "/node/src/grpc-node/packages/grpc-js-xds/build/interop/xds-interop-client" ] diff --git a/packages/grpc-js-xds/interop/test-server.Dockerfile b/packages/grpc-js-xds/interop/test-server.Dockerfile index b20b0595..59954a84 100644 --- a/packages/grpc-js-xds/interop/test-server.Dockerfile +++ b/packages/grpc-js-xds/interop/test-server.Dockerfile @@ -46,7 +46,7 @@ COPY --from=build /node/src/grpc-node/packages/grpc-js ./packages/grpc-js/ COPY --from=build /node/src/grpc-node/packages/grpc-js-xds ./packages/grpc-js-xds/ ENV GRPC_VERBOSITY="DEBUG" -ENV GRPC_TRACE=xds_client,server,xds_server,http_filter +ENV GRPC_TRACE=xds_client,server,xds_server,http_filter,certificate_provider # tini serves as PID 1 and enables the server to properly respond to signals. COPY --from=build /tini /tini From 6e901c1511c4851c33faf19970611cfdf8f59f0f Mon Sep 17 00:00:00 2001 From: Michael Lumish Date: Wed, 12 Feb 2025 18:03:20 -0800 Subject: [PATCH 08/27] Add more transport trace lines --- packages/grpc-js/src/transport.ts | 3 +++ 1 file changed, 3 insertions(+) diff --git a/packages/grpc-js/src/transport.ts b/packages/grpc-js/src/transport.ts index 3c2223e9..6e4fbd42 100644 --- a/packages/grpc-js/src/transport.ts +++ b/packages/grpc-js/src/transport.ts @@ -738,9 +738,12 @@ export class Http2SubchannelConnector implements SubchannelConnector { } let tcpConnection: net.Socket | null = null; let secureConnectResult: SecureConnectResult | null = null; + const addressString = this.trace(subchannelAddressToString(address)); try { tcpConnection = await this.tcpConnect(address, options); + this.trace(addressString + ' ' + 'Established TCP connection'); secureConnectResult = await secureConnector.connect(tcpConnection); + this.trace(addressString + ' ' + 'Established secure connection'); return this.createSession(secureConnectResult, address, options); } catch (e) { tcpConnection?.destroy(); From bb6fff7ff5a6f8b86d0d7fedc98c6581cd221aba Mon Sep 17 00:00:00 2001 From: Michael Lumish Date: Thu, 13 Feb 2025 10:55:12 -0800 Subject: [PATCH 09/27] Change connection handler to prependListener, add more trace logging --- packages/grpc-js/src/server.ts | 5 ++++- packages/grpc-js/src/transport.ts | 2 +- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/packages/grpc-js/src/server.ts b/packages/grpc-js/src/server.ts index b187765b..adefec01 100644 --- a/packages/grpc-js/src/server.ts +++ b/packages/grpc-js/src/server.ts @@ -576,9 +576,11 @@ export class Server { enableTrace: this.options['grpc-node.tls_enable_trace'] === 1 }; let areCredentialsValid = credentialsSettings !== null; + this.trace('Initial credentials valid: ' + areCredentialsValid); http2Server = http2.createSecureServer(secureServerOptions); - http2Server.on('connection', (socket: Socket) => { + http2Server.prependListener('connection', (socket: Socket) => { if (!areCredentialsValid) { + this.trace('Dropped connection from ' + JSON.stringify(socket.address()) + ' due to unloaded credentials'); socket.destroy(); } }); @@ -596,6 +598,7 @@ export class Server { (http2Server as http2.Http2SecureServer).setSecureContext(options); } areCredentialsValid = options !== null; + this.trace('Post-update credentials valid: ' + areCredentialsValid); } credentials._addWatcher(credsWatcher); http2Server.on('close', () => { diff --git a/packages/grpc-js/src/transport.ts b/packages/grpc-js/src/transport.ts index 6e4fbd42..055fb9dd 100644 --- a/packages/grpc-js/src/transport.ts +++ b/packages/grpc-js/src/transport.ts @@ -738,7 +738,7 @@ export class Http2SubchannelConnector implements SubchannelConnector { } let tcpConnection: net.Socket | null = null; let secureConnectResult: SecureConnectResult | null = null; - const addressString = this.trace(subchannelAddressToString(address)); + const addressString = subchannelAddressToString(address); try { tcpConnection = await this.tcpConnect(address, options); this.trace(addressString + ' ' + 'Established TCP connection'); From b44b14d831d0f08b58116912086675d75c87e15e Mon Sep 17 00:00:00 2001 From: Michael Lumish Date: Fri, 14 Feb 2025 13:06:08 -0800 Subject: [PATCH 10/27] Handle unauthorized TLS connections correctly --- packages/grpc-js/src/channel-credentials.ts | 8 ++++++++ packages/grpc-js/src/transport.ts | 4 ++-- 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/packages/grpc-js/src/channel-credentials.ts b/packages/grpc-js/src/channel-credentials.ts index c608ebe3..6242505e 100644 --- a/packages/grpc-js/src/channel-credentials.ts +++ b/packages/grpc-js/src/channel-credentials.ts @@ -262,6 +262,10 @@ class SecureConnectorImpl implements SecureConnector { }; return new Promise((resolve, reject) => { const tlsSocket = tlsConnect(tlsConnectOptions, () => { + if (!tlsSocket.authorized) { + reject(tlsSocket.authorizationError); + return; + } resolve({ socket: tlsSocket, secure: true @@ -340,6 +344,10 @@ class CertificateProviderChannelCredentialsImpl extends ChannelCredentials { ...connnectionOptions } const tlsSocket = tlsConnect(tlsConnectOptions, () => { + if (!tlsSocket.authorized) { + reject(tlsSocket.authorizationError); + return; + } resolve({ socket: tlsSocket, secure: true diff --git a/packages/grpc-js/src/transport.ts b/packages/grpc-js/src/transport.ts index 055fb9dd..bfb3f95b 100644 --- a/packages/grpc-js/src/transport.ts +++ b/packages/grpc-js/src/transport.ts @@ -225,8 +225,8 @@ class Http2Transport implements Transport { this.handleDisconnect(); }); - session.socket.once('close', () => { - this.trace('connection closed'); + session.socket.once('close', (hadError) => { + this.trace('connection closed. hadError=' + hadError); this.handleDisconnect(); }); From a8f981aefd23b8b874cb08b5d0ea0c1c0c2d0552 Mon Sep 17 00:00:00 2001 From: Michael Lumish Date: Fri, 14 Feb 2025 14:29:06 -0800 Subject: [PATCH 11/27] Enable heavy-duty TLS tracing in interop client and server --- packages/grpc-js-xds/interop/xds-interop-client.ts | 2 +- packages/grpc-js-xds/interop/xds-interop-server.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/grpc-js-xds/interop/xds-interop-client.ts b/packages/grpc-js-xds/interop/xds-interop-client.ts index 6db5aff9..0802ea1d 100644 --- a/packages/grpc-js-xds/interop/xds-interop-client.ts +++ b/packages/grpc-js-xds/interop/xds-interop-client.ts @@ -519,7 +519,7 @@ function main() { * channels do not share any subchannels. It does not have any * inherent function. */ console.log(`Interop client channel ${i} starting sending ${argv.qps} QPS to ${argv.server}`); - sendConstantQps(new loadedProto.grpc.testing.TestService(argv.server, grpc.credentials.createInsecure(), {'unique': i}), + sendConstantQps(new loadedProto.grpc.testing.TestService(argv.server, grpc.credentials.createInsecure(), {'unique': i, 'grpc-node.tls_enable_trace': 1}), argv.qps, argv.fail_on_failed_rpcs === 'true', callStatsTracker); diff --git a/packages/grpc-js-xds/interop/xds-interop-server.ts b/packages/grpc-js-xds/interop/xds-interop-server.ts index 5cb42d2c..683cb646 100644 --- a/packages/grpc-js-xds/interop/xds-interop-server.ts +++ b/packages/grpc-js-xds/interop/xds-interop-server.ts @@ -262,7 +262,7 @@ async function main() { reflection.addToServer(maintenanceServer); grpc.addAdminServicesToServer(maintenanceServer); - const server = new grpc_xds.XdsServer({interceptors: [testInfoInterceptor]}); + const server = new grpc_xds.XdsServer({interceptors: [testInfoInterceptor], 'grpc-node.tls_enable_trace': 1}); server.addService(loadedProto.grpc.testing.TestService.service, testServiceHandler); const xdsCreds = new grpc_xds.XdsServerCredentials(grpc.ServerCredentials.createInsecure()); await Promise.all([ From 5f12dc233f319b7effea46540421bde5deb3132c Mon Sep 17 00:00:00 2001 From: Michael Lumish Date: Wed, 19 Feb 2025 11:29:03 -0800 Subject: [PATCH 12/27] Add more trace logging --- .../interop/test-client.Dockerfile | 2 +- packages/grpc-js-xds/src/server.ts | 2 + packages/grpc-js-xds/src/xds-credentials.ts | 6 +- .../grpc-js-xds/test/test-xds-credentials.ts | 77 +++++++++++++++++++ 4 files changed, 85 insertions(+), 2 deletions(-) diff --git a/packages/grpc-js-xds/interop/test-client.Dockerfile b/packages/grpc-js-xds/interop/test-client.Dockerfile index f8bb3aaf..db608f95 100644 --- a/packages/grpc-js-xds/interop/test-client.Dockerfile +++ b/packages/grpc-js-xds/interop/test-client.Dockerfile @@ -42,7 +42,7 @@ COPY --from=build /node/src/grpc-node/packages/grpc-js ./packages/grpc-js/ COPY --from=build /node/src/grpc-node/packages/grpc-js-xds ./packages/grpc-js-xds/ ENV GRPC_VERBOSITY="DEBUG" -ENV GRPC_TRACE=xds_client,xds_resolver,xds_cluster_manager,cds_balancer,xds_cluster_resolver,xds_cluster_impl,priority,weighted_target,round_robin,resolving_load_balancer,subchannel,keepalive,dns_resolver,fault_injection,http_filter,csds,outlier_detection,server,server_call,ring_hash,transport,certificate_provider +ENV GRPC_TRACE=xds_client,xds_resolver,xds_cluster_manager,cds_balancer,xds_cluster_resolver,xds_cluster_impl,priority,weighted_target,round_robin,resolving_load_balancer,subchannel,keepalive,dns_resolver,fault_injection,http_filter,csds,outlier_detection,server,server_call,ring_hash,transport,certificate_provider,xds_channel_credentials ENV NODE_XDS_INTEROP_VERBOSITY=1 ENTRYPOINT [ "/nodejs/bin/node", "/node/src/grpc-node/packages/grpc-js-xds/build/interop/xds-interop-client" ] diff --git a/packages/grpc-js-xds/src/server.ts b/packages/grpc-js-xds/src/server.ts index 5ccbaa67..7b386ebb 100644 --- a/packages/grpc-js-xds/src/server.ts +++ b/packages/grpc-js-xds/src/server.ts @@ -159,6 +159,7 @@ class FilterChainEntry { } if (credentials instanceof XdsServerCredentials) { if (filterChain.transport_socket) { + trace('Using secure credentials'); const downstreamTlsContext = decodeSingleResource(DOWNSTREAM_TLS_CONTEXT_TYPE_URL, filterChain.transport_socket.typed_config!.value); const commonTlsContext = downstreamTlsContext.common_tls_context!; const instanceCertificateProvider = configParameters.xdsClient.getCertificateProvider(commonTlsContext.tls_certificate_provider_instance!.instance_name); @@ -185,6 +186,7 @@ class FilterChainEntry { } credentials = experimental.createCertificateProviderServerCredentials(instanceCertificateProvider, caCertificateProvider, downstreamTlsContext.require_client_certificate?.value ?? false); } else { + trace('Using fallback credentials'); credentials = credentials.getFallbackCredentials(); } } diff --git a/packages/grpc-js-xds/src/xds-credentials.ts b/packages/grpc-js-xds/src/xds-credentials.ts index 6edac916..f7d66ce4 100644 --- a/packages/grpc-js-xds/src/xds-credentials.ts +++ b/packages/grpc-js-xds/src/xds-credentials.ts @@ -15,11 +15,12 @@ * */ -import { CallCredentials, ChannelCredentials, ChannelOptions, ServerCredentials, VerifyOptions, experimental } from "@grpc/grpc-js"; +import { CallCredentials, ChannelCredentials, ChannelOptions, ServerCredentials, VerifyOptions, experimental, logVerbosity } from "@grpc/grpc-js"; import { CA_CERT_PROVIDER_KEY, IDENTITY_CERT_PROVIDER_KEY, SAN_MATCHER_KEY, SanMatcher } from "./load-balancer-cds"; import GrpcUri = experimental.GrpcUri; import SecureConnector = experimental.SecureConnector; import createCertificateProviderChannelCredentials = experimental.createCertificateProviderChannelCredentials; +import trace = experimental.trace; export class XdsChannelCredentials extends ChannelCredentials { constructor(private fallbackCredentials: ChannelCredentials) { @@ -33,6 +34,7 @@ export class XdsChannelCredentials extends ChannelCredentials { } _createSecureConnector(channelTarget: GrpcUri, options: ChannelOptions, callCredentials?: CallCredentials): SecureConnector { if (options[CA_CERT_PROVIDER_KEY]) { + trace(logVerbosity.DEBUG, 'xds_channel_credentials', 'Using secure credentials'); const verifyOptions: VerifyOptions = {}; if (options[SAN_MATCHER_KEY]) { const matcher = options[SAN_MATCHER_KEY] as SanMatcher; @@ -40,6 +42,7 @@ export class XdsChannelCredentials extends ChannelCredentials { if (cert.subjectaltname && matcher.apply(cert.subjectaltname)) { return undefined; } else { + trace(logVerbosity.DEBUG, 'xds_channel_credentials', 'No matching subject alternative name found in certificate'); return new Error('No matching subject alternative name found in certificate'); } } @@ -47,6 +50,7 @@ export class XdsChannelCredentials extends ChannelCredentials { const certProviderCreds = createCertificateProviderChannelCredentials(options[CA_CERT_PROVIDER_KEY], options[IDENTITY_CERT_PROVIDER_KEY] ?? null, verifyOptions); return certProviderCreds._createSecureConnector(channelTarget, options, callCredentials); } else { + trace(logVerbosity.DEBUG, 'xds_channel_credentials', 'Using fallback credentials'); return this.fallbackCredentials._createSecureConnector(channelTarget, options, callCredentials); } } diff --git a/packages/grpc-js-xds/test/test-xds-credentials.ts b/packages/grpc-js-xds/test/test-xds-credentials.ts index 27063d3e..0a235721 100644 --- a/packages/grpc-js-xds/test/test-xds-credentials.ts +++ b/packages/grpc-js-xds/test/test-xds-credentials.ts @@ -366,4 +366,81 @@ describe('Client xDS credentials', () => { }); } }); + describe('Client and server xDS credentials', () => { + let xdsServer: ControlPlaneServer; + let client: XdsTestClient; + beforeEach(done => { + xdsServer = new ControlPlaneServer(); + xdsServer.startServer(error => { + done(error); + }); + }); + afterEach(() => { + client?.close(); + xdsServer?.shutdownServer(); + }); + it('Should use identity and CA certificates when configured', async () => { + const [backend] = await createBackends(1, true, new XdsServerCredentials(ServerCredentials.createInsecure())); + const downstreamTlsContext: DownstreamTlsContext & AnyExtension = { + '@type': DOWNSTREAM_TLS_CONTEXT_TYPE_URL, + common_tls_context: { + tls_certificate_provider_instance: { + instance_name: 'test_certificates' + }, + validation_context: { + ca_certificate_provider_instance: { + instance_name: 'test_certificates' + } + } + }, + ocsp_staple_policy: 'LENIENT_STAPLING', + require_client_certificate: { + value: true + } + } + const baseServerListener: Listener = { + default_filter_chain: { + filter_chain_match: { + source_type: 'SAME_IP_OR_LOOPBACK' + }, + transport_socket: { + name: 'envoy.transport_sockets.tls', + typed_config: downstreamTlsContext + } + } + } + const serverRoute = new FakeServerRoute(backend.getPort(), 'serverRoute', baseServerListener); + xdsServer.setRdsResource(serverRoute.getRouteConfiguration()); + xdsServer.setLdsResource(serverRoute.getListener()); + xdsServer.addResponseListener((typeUrl, responseState) => { + if (responseState.state === 'NACKED') { + client?.stopCalls(); + assert.fail(`Client NACKED ${typeUrl} resource with message ${responseState.errorMessage}`); + } + }); + const upstreamTlsContext: UpstreamTlsContext = { + common_tls_context: { + tls_certificate_provider_instance: { + instance_name: 'test_certificates' + }, + validation_context: { + ca_certificate_provider_instance: { + instance_name: 'test_certificates' + } + } + } + }; + const cluster = new FakeEdsCluster('cluster1', 'endpoint1', [{backends: [backend], locality:{region: 'region1'}}], undefined, upstreamTlsContext); + const routeGroup = new FakeRouteGroup('listener1', 'route1', [{cluster: cluster}]); + await routeGroup.startAllBackends(xdsServer); + xdsServer.setEdsResource(cluster.getEndpointConfig()); + xdsServer.setCdsResource(cluster.getClusterConfig()); + xdsServer.setRdsResource(routeGroup.getRouteConfiguration()); + xdsServer.setLdsResource(routeGroup.getListener()); + client = XdsTestClient.createFromServer('listener1', xdsServer, new XdsChannelCredentials(credentials.createInsecure())); + const error = await client.sendOneCallAsync(); + assert.strictEqual(error, null); + }); + + }); }); From bdd0dc849939490f28b0b449f7d551bc13723034 Mon Sep 17 00:00:00 2001 From: Michael Lumish Date: Wed, 19 Feb 2025 13:39:02 -0800 Subject: [PATCH 13/27] Fix a bug that caused HTTP2 sessions to be considered connected early --- packages/grpc-js/src/transport.ts | 2 +- .../grpc-js/test/test-channel-credentials.ts | 19 +++++++++++++++++-- 2 files changed, 18 insertions(+), 3 deletions(-) diff --git a/packages/grpc-js/src/transport.ts b/packages/grpc-js/src/transport.ts index bfb3f95b..7f657011 100644 --- a/packages/grpc-js/src/transport.ts +++ b/packages/grpc-js/src/transport.ts @@ -685,7 +685,7 @@ export class Http2SubchannelConnector implements SubchannelConnector { let errorMessage = 'Failed to connect'; let reportedError = false; session.unref(); - session.once('connect', () => { + session.once('remoteSettings', () => { session.removeAllListeners(); resolve(new Http2Transport(session, address, options, remoteName)); this.session = null; diff --git a/packages/grpc-js/test/test-channel-credentials.ts b/packages/grpc-js/test/test-channel-credentials.ts index 1e1fb89c..43bce5da 100644 --- a/packages/grpc-js/test/test-channel-credentials.ts +++ b/packages/grpc-js/test/test-channel-credentials.ts @@ -201,6 +201,21 @@ describe('ChannelCredentials usage', () => { done(); } ); - - }) + }); + it('Should never connect when using insecure creds with a secure server', done => { + const client = new echoService(`localhost:${portNum}`, grpc.credentials.createInsecure()); + const deadline = new Date(); + deadline.setSeconds(deadline.getSeconds() + 1); + client.echo( + { value: 'test value', value2: 3 }, + new grpc.Metadata({waitForReady: true}), + {deadline}, + (error: ServiceError, response: any) => { + client.close(); + assert(error); + assert.strictEqual(error.code, grpc.status.DEADLINE_EXCEEDED); + done(); + } + ); + }); }); From 1fe3f7406c5f3dbd8a493ee75de075bb73d10814 Mon Sep 17 00:00:00 2001 From: Michael Lumish Date: Wed, 19 Feb 2025 13:40:20 -0800 Subject: [PATCH 14/27] Use xDS creds in interop client, remove verbose TLS logging --- packages/grpc-js-xds/interop/xds-interop-client.ts | 4 +++- packages/grpc-js-xds/interop/xds-interop-server.ts | 2 +- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/packages/grpc-js-xds/interop/xds-interop-client.ts b/packages/grpc-js-xds/interop/xds-interop-client.ts index 0802ea1d..2e142555 100644 --- a/packages/grpc-js-xds/interop/xds-interop-client.ts +++ b/packages/grpc-js-xds/interop/xds-interop-client.ts @@ -519,7 +519,9 @@ function main() { * channels do not share any subchannels. It does not have any * inherent function. */ console.log(`Interop client channel ${i} starting sending ${argv.qps} QPS to ${argv.server}`); - sendConstantQps(new loadedProto.grpc.testing.TestService(argv.server, grpc.credentials.createInsecure(), {'unique': i, 'grpc-node.tls_enable_trace': 1}), + const insecureCreds = grpc.credentials.createInsecure(); + const creds = new grpc_xds.XdsChannelCredentials(insecureCreds); + sendConstantQps(new loadedProto.grpc.testing.TestService(argv.server, creds, {'unique': i}), argv.qps, argv.fail_on_failed_rpcs === 'true', callStatsTracker); diff --git a/packages/grpc-js-xds/interop/xds-interop-server.ts b/packages/grpc-js-xds/interop/xds-interop-server.ts index 683cb646..5cb42d2c 100644 --- a/packages/grpc-js-xds/interop/xds-interop-server.ts +++ b/packages/grpc-js-xds/interop/xds-interop-server.ts @@ -262,7 +262,7 @@ async function main() { reflection.addToServer(maintenanceServer); grpc.addAdminServicesToServer(maintenanceServer); - const server = new grpc_xds.XdsServer({interceptors: [testInfoInterceptor], 'grpc-node.tls_enable_trace': 1}); + const server = new grpc_xds.XdsServer({interceptors: [testInfoInterceptor]}); server.addService(loadedProto.grpc.testing.TestService.service, testServiceHandler); const xdsCreds = new grpc_xds.XdsServerCredentials(grpc.ServerCredentials.createInsecure()); await Promise.all([ From e883425ef3df40c62c2b6648e0dbf1036e1c363d Mon Sep 17 00:00:00 2001 From: Michael Lumish Date: Wed, 19 Feb 2025 17:22:59 -0800 Subject: [PATCH 15/27] Wait for secure connectors to be usable before TCP connect --- packages/grpc-js/src/channel-credentials.ts | 29 ++++++++++++++++++--- packages/grpc-js/src/transport.ts | 14 +++++++--- 2 files changed, 36 insertions(+), 7 deletions(-) diff --git a/packages/grpc-js/src/channel-credentials.ts b/packages/grpc-js/src/channel-credentials.ts index 6242505e..b1c7ba2e 100644 --- a/packages/grpc-js/src/channel-credentials.ts +++ b/packages/grpc-js/src/channel-credentials.ts @@ -70,6 +70,7 @@ export interface SecureConnectResult { export interface SecureConnector { connect(socket: Socket): Promise; + waitForReady(): Promise; getCallCredentials(): CallCredentials; destroy(): void; } @@ -188,6 +189,9 @@ class InsecureChannelCredentialsImpl extends ChannelCredentials { secure: false }); }, + waitForReady: () => { + return Promise.resolve(); + }, getCallCredentials: () => { return callCredentials ?? CallCredentials.createEmpty(); }, @@ -276,6 +280,9 @@ class SecureConnectorImpl implements SecureConnector { }); }); } + waitForReady(): Promise { + return Promise.resolve(); + } getCallCredentials(): CallCredentials { return this.callCredentials; } @@ -333,17 +340,28 @@ class CertificateProviderChannelCredentialsImpl extends ChannelCredentials { connect(socket: Socket): Promise { return new Promise(async (resolve, reject) => { - const secureContext = await this.parent.getSecureContext(); + const secureContext = this.parent.getLatestSecureContext(); if (!secureContext) { reject(new Error('Failed to load credentials')); return; } + if (socket.closed) { + reject(new Error('Socket closed while loading credentials')); + } const connnectionOptions = getConnectionOptions(secureContext, this.parent.verifyOptions, this.channelTarget, this.options); const tlsConnectOptions: ConnectionOptions = { socket: socket, ...connnectionOptions } + const closeCallback = () => { + reject(new Error('Socket closed')); + }; + const errorCallback = (error: Error) => { + reject(error); + } const tlsSocket = tlsConnect(tlsConnectOptions, () => { + tlsSocket.removeListener('close', closeCallback); + tlsSocket.removeListener('error', errorCallback); if (!tlsSocket.authorized) { reject(tlsSocket.authorizationError); return; @@ -353,12 +371,15 @@ class CertificateProviderChannelCredentialsImpl extends ChannelCredentials { secure: true }); }); - tlsSocket.on('error', (error: Error) => { - reject(error); - }); + tlsSocket.once('close', closeCallback); + tlsSocket.once('error', errorCallback); }); } + async waitForReady(): Promise { + await this.parent.getSecureContext(); + } + getCallCredentials(): CallCredentials { return this.callCredentials; } diff --git a/packages/grpc-js/src/transport.ts b/packages/grpc-js/src/transport.ts index 7f657011..f6f36d4d 100644 --- a/packages/grpc-js/src/transport.ts +++ b/packages/grpc-js/src/transport.ts @@ -717,12 +717,19 @@ export class Http2SubchannelConnector implements SubchannelConnector { return proxiedSocket; } else { return new Promise((resolve, reject) => { + const closeCallback = () => { + reject(new Error('Socket closed')); + }; + const errorCallback = (error: Error) => { + reject(error); + } const socket = net.connect(address, () => { + socket.removeListener('close', closeCallback); + socket.removeListener('error', errorCallback); resolve(socket); }); - socket.once('error', (error) => { - reject(error); - }); + socket.once('close', closeCallback); + socket.once('error', errorCallback); }); } }); @@ -740,6 +747,7 @@ export class Http2SubchannelConnector implements SubchannelConnector { let secureConnectResult: SecureConnectResult | null = null; const addressString = subchannelAddressToString(address); try { + await secureConnector.waitForReady(); tcpConnection = await this.tcpConnect(address, options); this.trace(addressString + ' ' + 'Established TCP connection'); secureConnectResult = await secureConnector.connect(tcpConnection); From 87f703403c0b38df5cabaadba3a33a42469d70f9 Mon Sep 17 00:00:00 2001 From: Michael Lumish Date: Wed, 19 Feb 2025 17:23:42 -0800 Subject: [PATCH 16/27] Fix Listener resource validation --- packages/grpc-js-xds/src/xds-dependency-manager.ts | 4 ++++ .../src/xds-resource-type/listener-resource-type.ts | 3 +++ 2 files changed, 7 insertions(+) diff --git a/packages/grpc-js-xds/src/xds-dependency-manager.ts b/packages/grpc-js-xds/src/xds-dependency-manager.ts index 14a16c32..210cde6b 100644 --- a/packages/grpc-js-xds/src/xds-dependency-manager.ts +++ b/packages/grpc-js-xds/src/xds-dependency-manager.ts @@ -360,6 +360,10 @@ export class XdsDependencyManager { constructor(private xdsClient: XdsClient, private listenerResourceName: string, private dataPlaneAuthority: string, private watcher: XdsConfigWatcher) { this.ldsWatcher = new Watcher({ onResourceChanged: (update: Listener__Output) => { + if (!update.api_listener) { + this.trace('Received Listener resource not usable on client'); + return; + } this.latestListener = update; const httpConnectionManager = decodeSingleResource(HTTP_CONNECTION_MANGER_TYPE_URL, update.api_listener!.api_listener!.value); switch (httpConnectionManager.route_specifier) { diff --git a/packages/grpc-js-xds/src/xds-resource-type/listener-resource-type.ts b/packages/grpc-js-xds/src/xds-resource-type/listener-resource-type.ts index f78bc7e6..82586fe4 100644 --- a/packages/grpc-js-xds/src/xds-resource-type/listener-resource-type.ts +++ b/packages/grpc-js-xds/src/xds-resource-type/listener-resource-type.ts @@ -294,6 +294,9 @@ export class ListenerResourceType extends XdsResourceType { if (message.default_filter_chain) { errors.push(...validateFilterChain(context, message.default_filter_chain).map(error => `default_filter_chain: ${error}`)); } + if (!message.api_listener && !message.default_filter_chain && message.filter_chains.length === 0) { + errors.push('No api_listener and no filter_chains and no default_filter_chain'); + } if (errors.length === 0) { return { valid: true, From 5cf1a876e5301f6a19a657106d010b4ee17bfe83 Mon Sep 17 00:00:00 2001 From: Michael Lumish Date: Thu, 20 Feb 2025 12:55:38 -0800 Subject: [PATCH 17/27] Handle missing filter_chain_match differently, plus other fixes --- packages/grpc-js-xds/src/server.ts | 15 +++++++++++- .../grpc-js-xds/src/xds-dependency-manager.ts | 24 ++++++++++++------- packages/grpc-js/src/channel-credentials.ts | 2 +- 3 files changed, 30 insertions(+), 11 deletions(-) diff --git a/packages/grpc-js-xds/src/server.ts b/packages/grpc-js-xds/src/server.ts index 7b386ebb..ca78df3a 100644 --- a/packages/grpc-js-xds/src/server.ts +++ b/packages/grpc-js-xds/src/server.ts @@ -289,6 +289,7 @@ class ListenerConfig { handleConnection(socket: net.Socket) { const matchingFilter = selectMostSpecificallyMatchingFilter(this.filterChainEntries, socket) ?? this.defaultFilterChain; if (!matchingFilter) { + trace('Rejecting connection from ' + socket.remoteAddress + ': No filter matched'); socket.destroy(); return; } @@ -456,7 +457,19 @@ class BoundPortEntry { function normalizeFilterChainMatch(filterChainMatch: FilterChainMatch__Output | null): NormalizedFilterChainMatch[] { if (!filterChainMatch) { - return []; + filterChainMatch = { + address_suffix: '', + application_protocols: [], + destination_port: null, + direct_source_prefix_ranges: [], + prefix_ranges: [], + server_names: [], + source_ports: [], + source_prefix_ranges: [], + source_type: 'ANY', + suffix_len: null, + transport_protocol: 'raw_buffer' + }; } if (filterChainMatch.destination_port) { return []; diff --git a/packages/grpc-js-xds/src/xds-dependency-manager.ts b/packages/grpc-js-xds/src/xds-dependency-manager.ts index 210cde6b..57ab350a 100644 --- a/packages/grpc-js-xds/src/xds-dependency-manager.ts +++ b/packages/grpc-js-xds/src/xds-dependency-manager.ts @@ -362,6 +362,7 @@ export class XdsDependencyManager { onResourceChanged: (update: Listener__Output) => { if (!update.api_listener) { this.trace('Received Listener resource not usable on client'); + this.handleListenerDoesNotExist(); return; } this.latestListener = update; @@ -405,15 +406,7 @@ export class XdsDependencyManager { }, onResourceDoesNotExist: () => { this.trace('Resolution error: LDS resource does not exist'); - if (this.latestRouteConfigName) { - this.trace('RDS.cancelWatch(' + this.latestRouteConfigName + '): LDS resource does not exist'); - RouteConfigurationResourceType.cancelWatch(this.xdsClient, this.latestRouteConfigName, this.rdsWatcher); - this.latestRouteConfigName = null; - this.latestRouteConfiguration = null; - this.clusterRoots = []; - this.pruneOrphanClusters(); - } - this.watcher.onResourceDoesNotExist(`Listener ${listenerResourceName}`); + this.handleListenerDoesNotExist(); } }); this.rdsWatcher = new Watcher({ @@ -439,6 +432,19 @@ export class XdsDependencyManager { trace('[' + this.listenerResourceName + '] ' + text); } + private handleListenerDoesNotExist() { + if (this.latestRouteConfigName) { + this.trace('RDS.cancelWatch(' + this.latestRouteConfigName + '): LDS resource does not exist'); + RouteConfigurationResourceType.cancelWatch(this.xdsClient, this.latestRouteConfigName, this.rdsWatcher); + this.latestRouteConfigName = null; + this.latestRouteConfiguration = null; + this.clusterRoots = []; + this.pruneOrphanClusters(); + } + this.watcher.onResourceDoesNotExist(`Listener ${this.listenerResourceName}`); + + } + private maybeSendUpdate() { if (!this.latestListener) { this.trace('Not sending update: no Listener update received'); diff --git a/packages/grpc-js/src/channel-credentials.ts b/packages/grpc-js/src/channel-credentials.ts index b1c7ba2e..78167253 100644 --- a/packages/grpc-js/src/channel-credentials.ts +++ b/packages/grpc-js/src/channel-credentials.ts @@ -339,7 +339,7 @@ class CertificateProviderChannelCredentialsImpl extends ChannelCredentials { constructor(private parent: CertificateProviderChannelCredentialsImpl, private channelTarget: GrpcUri, private options: ChannelOptions, private callCredentials: CallCredentials) {} connect(socket: Socket): Promise { - return new Promise(async (resolve, reject) => { + return new Promise((resolve, reject) => { const secureContext = this.parent.getLatestSecureContext(); if (!secureContext) { reject(new Error('Failed to load credentials')); From 65f4d76f155fdf374a7e9cf04e2b56ac3d7959b5 Mon Sep 17 00:00:00 2001 From: Michael Lumish Date: Fri, 21 Feb 2025 09:42:17 -0800 Subject: [PATCH 18/27] Add SAN matcher trace logging --- packages/grpc-js-xds/src/load-balancer-cds.ts | 1 + packages/grpc-js-xds/src/xds-credentials.ts | 13 +++++++++---- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/packages/grpc-js-xds/src/load-balancer-cds.ts b/packages/grpc-js-xds/src/load-balancer-cds.ts index 23e8e7d1..aba74820 100644 --- a/packages/grpc-js-xds/src/load-balancer-cds.ts +++ b/packages/grpc-js-xds/src/load-balancer-cds.ts @@ -433,6 +433,7 @@ export class CdsLoadBalancer implements LoadBalancer { if (this.latestSanMatcher === null || !this.latestSanMatcher.equals(sanMatcher)) { this.latestSanMatcher = sanMatcher; } + trace('Configured subject alternative name matcher: ' + sanMatcher); childOptions[SAN_MATCHER_KEY] = this.latestSanMatcher; } this.childBalancer.updateAddressList(childEndpointList, typedChildConfig, childOptions); diff --git a/packages/grpc-js-xds/src/xds-credentials.ts b/packages/grpc-js-xds/src/xds-credentials.ts index f7d66ce4..1749e217 100644 --- a/packages/grpc-js-xds/src/xds-credentials.ts +++ b/packages/grpc-js-xds/src/xds-credentials.ts @@ -20,7 +20,12 @@ import { CA_CERT_PROVIDER_KEY, IDENTITY_CERT_PROVIDER_KEY, SAN_MATCHER_KEY, SanM import GrpcUri = experimental.GrpcUri; import SecureConnector = experimental.SecureConnector; import createCertificateProviderChannelCredentials = experimental.createCertificateProviderChannelCredentials; -import trace = experimental.trace; + +const TRACER_NAME = 'xds_channel_credentials'; + +function trace(text: string) { + experimental.trace(logVerbosity.DEBUG, TRACER_NAME, text); +} export class XdsChannelCredentials extends ChannelCredentials { constructor(private fallbackCredentials: ChannelCredentials) { @@ -34,7 +39,7 @@ export class XdsChannelCredentials extends ChannelCredentials { } _createSecureConnector(channelTarget: GrpcUri, options: ChannelOptions, callCredentials?: CallCredentials): SecureConnector { if (options[CA_CERT_PROVIDER_KEY]) { - trace(logVerbosity.DEBUG, 'xds_channel_credentials', 'Using secure credentials'); + trace('Using secure credentials'); const verifyOptions: VerifyOptions = {}; if (options[SAN_MATCHER_KEY]) { const matcher = options[SAN_MATCHER_KEY] as SanMatcher; @@ -42,7 +47,7 @@ export class XdsChannelCredentials extends ChannelCredentials { if (cert.subjectaltname && matcher.apply(cert.subjectaltname)) { return undefined; } else { - trace(logVerbosity.DEBUG, 'xds_channel_credentials', 'No matching subject alternative name found in certificate'); + trace('Subject alternative name not matched: ' + cert.subjectaltname); return new Error('No matching subject alternative name found in certificate'); } } @@ -50,7 +55,7 @@ export class XdsChannelCredentials extends ChannelCredentials { const certProviderCreds = createCertificateProviderChannelCredentials(options[CA_CERT_PROVIDER_KEY], options[IDENTITY_CERT_PROVIDER_KEY] ?? null, verifyOptions); return certProviderCreds._createSecureConnector(channelTarget, options, callCredentials); } else { - trace(logVerbosity.DEBUG, 'xds_channel_credentials', 'Using fallback credentials'); + trace('Using fallback credentials'); return this.fallbackCredentials._createSecureConnector(channelTarget, options, callCredentials); } } From 7d99c4a7aa7fa03c190d5fb3889e9082cc8b5a61 Mon Sep 17 00:00:00 2001 From: Michael Lumish Date: Fri, 21 Feb 2025 12:55:09 -0800 Subject: [PATCH 19/27] Fix handling of subject alternative names with colons --- packages/grpc-js-xds/src/load-balancer-cds.ts | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/packages/grpc-js-xds/src/load-balancer-cds.ts b/packages/grpc-js-xds/src/load-balancer-cds.ts index aba74820..c1d92d69 100644 --- a/packages/grpc-js-xds/src/load-balancer-cds.ts +++ b/packages/grpc-js-xds/src/load-balancer-cds.ts @@ -84,11 +84,13 @@ class DnsExactValueMatcher implements ValueMatcher { } } apply(entry: string): boolean { - let [type, value] = entry.split(':'); - if (!isSupportedSanType(type)) { + const colonIndex = entry.indexOf(':'); + if (colonIndex < 0) { return false; } - if (!value) { + const type = entry.substring(0, colonIndex); + let value = entry.substring(colonIndex + 1); + if (!isSupportedSanType(type)) { return false; } if (this.ignoreCase) { @@ -137,14 +139,16 @@ class SanEntryMatcher implements ValueMatcher { } } apply(entry: string): boolean { - let [type, value] = entry.split(':'); + const colonIndex = entry.indexOf(':'); + if (colonIndex < 0) { + return false; + } + const type = entry.substring(0, colonIndex); + let value = entry.substring(colonIndex + 1); if (!isSupportedSanType(type)) { return false; } value = canonicalizeSanEntryValue(type, value); - if (!entry) { - return false; - } return this.childMatcher.apply(value); } toString(): string { From 1e28a043308bcfa446cc43825338f032d5f434db Mon Sep 17 00:00:00 2001 From: Michael Lumish Date: Mon, 24 Feb 2025 15:12:31 -0800 Subject: [PATCH 20/27] Register xds listener with channelz --- packages/grpc-js-xds/src/server.ts | 3 ++- packages/grpc-js/src/server.ts | 32 +++++++++++++++++++++++------- 2 files changed, 27 insertions(+), 8 deletions(-) diff --git a/packages/grpc-js-xds/src/server.ts b/packages/grpc-js-xds/src/server.ts index ca78df3a..9bbf897c 100644 --- a/packages/grpc-js-xds/src/server.ts +++ b/packages/grpc-js-xds/src/server.ts @@ -628,8 +628,9 @@ export class XdsServer extends Server { if (!hostPort || !isValidIpPort(hostPort)) { throw new Error(`Listening port string must have the format IP:port with non-zero port, got ${port}`); } + const channelzRef = this.experimentalRegisterListenerToChannelz({host: hostPort.host, port: hostPort.port!}); const configParameters: ConfigParameters = { - createConnectionInjector: (credentials) => this.createConnectionInjector(credentials), + createConnectionInjector: (credentials) => this.experimentalCreateConnectionInjectorWithChannelzRef(credentials, channelzRef), drainGraceTimeMs: this.drainGraceTimeMs, listenerResourceNameTemplate: this.listenerResourceNameTemplate, xdsClient: this.xdsClient diff --git a/packages/grpc-js/src/server.ts b/packages/grpc-js/src/server.ts index adefec01..a73c9669 100644 --- a/packages/grpc-js/src/server.ts +++ b/packages/grpc-js/src/server.ts @@ -539,7 +539,12 @@ export class Server { throw new Error('Not implemented. Use bindAsync() instead'); } - private registerListenerToChannelz(boundAddress: SubchannelAddress) { + /** + * This API is experimental, so API stability is not guaranteed across minor versions. + * @param boundAddress + * @returns + */ + protected experimentalRegisterListenerToChannelz(boundAddress: SubchannelAddress) { return registerChannelzSocket( subchannelAddressToString(boundAddress), () => { @@ -649,7 +654,7 @@ export class Server { }; } - const channelzRef = this.registerListenerToChannelz( + const channelzRef = this.experimentalRegisterListenerToChannelz( boundSubchannelAddress ); this.listenerChildrenTracker.refChild(channelzRef); @@ -945,15 +950,17 @@ export class Server { ); } - createConnectionInjector(credentials: ServerCredentials): ConnectionInjector { + /** + * This API is experimental, so API stability is not guaranteed across minor versions. + * @param credentials + * @param channelzRef + * @returns + */ + protected experimentalCreateConnectionInjectorWithChannelzRef(credentials: ServerCredentials, channelzRef: SocketRef) { if (credentials === null || !(credentials instanceof ServerCredentials)) { throw new TypeError('creds must be a ServerCredentials object'); } const server = this.createHttp2Server(credentials); - const channelzRef = this.registerInjectorToChannelz(); - if (this.channelzEnabled) { - this.listenerChildrenTracker.refChild(channelzRef); - } const sessionsSet: Set = new Set(); this.http2Servers.set(server, { channelzRef: channelzRef, @@ -982,6 +989,17 @@ export class Server { }; } + createConnectionInjector(credentials: ServerCredentials): ConnectionInjector { + if (credentials === null || !(credentials instanceof ServerCredentials)) { + throw new TypeError('creds must be a ServerCredentials object'); + } + const channelzRef = this.registerInjectorToChannelz(); + if (this.channelzEnabled) { + this.listenerChildrenTracker.refChild(channelzRef); + } + return this.experimentalCreateConnectionInjectorWithChannelzRef(credentials, channelzRef); + } + private closeServer(server: AnyHttp2Server, callback?: () => void) { this.trace( 'Closing server with address ' + JSON.stringify(server.address()) From a9cfd7a533ab492394304d6f7a5313a87e9cde05 Mon Sep 17 00:00:00 2001 From: Michael Lumish Date: Tue, 25 Feb 2025 10:35:18 -0800 Subject: [PATCH 21/27] Register listener as child properly --- packages/grpc-js/src/server.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/grpc-js/src/server.ts b/packages/grpc-js/src/server.ts index a73c9669..cc8c3ab5 100644 --- a/packages/grpc-js/src/server.ts +++ b/packages/grpc-js/src/server.ts @@ -960,6 +960,9 @@ export class Server { if (credentials === null || !(credentials instanceof ServerCredentials)) { throw new TypeError('creds must be a ServerCredentials object'); } + if (this.channelzEnabled) { + this.listenerChildrenTracker.refChild(channelzRef); + } const server = this.createHttp2Server(credentials); const sessionsSet: Set = new Set(); this.http2Servers.set(server, { @@ -994,9 +997,6 @@ export class Server { throw new TypeError('creds must be a ServerCredentials object'); } const channelzRef = this.registerInjectorToChannelz(); - if (this.channelzEnabled) { - this.listenerChildrenTracker.refChild(channelzRef); - } return this.experimentalCreateConnectionInjectorWithChannelzRef(credentials, channelzRef); } From 822af6817fe175157fe57c1eaff6654acb936791 Mon Sep 17 00:00:00 2001 From: Michael Lumish Date: Tue, 25 Feb 2025 13:49:49 -0800 Subject: [PATCH 22/27] Only register once, add admin service response logging --- packages/grpc-js-xds/interop/xds-interop-server.ts | 4 ++++ packages/grpc-js-xds/src/index.ts | 6 ++++++ 2 files changed, 10 insertions(+) diff --git a/packages/grpc-js-xds/interop/xds-interop-server.ts b/packages/grpc-js-xds/interop/xds-interop-server.ts index 5cb42d2c..b8af6e3a 100644 --- a/packages/grpc-js-xds/interop/xds-interop-server.ts +++ b/packages/grpc-js-xds/interop/xds-interop-server.ts @@ -160,6 +160,10 @@ function adminServiceInterceptor(methodDescriptor: grpc.ServerMethodDefinition { next(listener); + }, + sendMessage: (message, next) => { + console.log(`Responded to request to method ${methodDescriptor.path}: ${JSON.stringify(message)}`); + next(message); } }; return new grpc.ServerInterceptingCall(call, responder); diff --git a/packages/grpc-js-xds/src/index.ts b/packages/grpc-js-xds/src/index.ts index 24a39b8c..b83992fc 100644 --- a/packages/grpc-js-xds/src/index.ts +++ b/packages/grpc-js-xds/src/index.ts @@ -33,10 +33,16 @@ import * as pick_first_lb from './lb-policy-registry/pick-first'; export { XdsServer } from './server'; export { XdsChannelCredentials, XdsServerCredentials } from './xds-credentials'; +let registered = false; + /** * Register the "xds:" name scheme with the @grpc/grpc-js library. */ export function register() { + if (registered) { + return; + } + registered = true; resolver_xds.setup(); load_balancer_cds.setup(); xds_cluster_impl.setup(); From 36c9a4fd4002a87a5b2aca7057a7e7069d1a1ed6 Mon Sep 17 00:00:00 2001 From: Michael Lumish Date: Tue, 25 Feb 2025 15:14:51 -0800 Subject: [PATCH 23/27] Represent IPv6-mapped IPv4 addresses as IPv4 in channelz --- packages/grpc-js/src/channelz.ts | 25 ++++++++++++++++++++----- 1 file changed, 20 insertions(+), 5 deletions(-) diff --git a/packages/grpc-js/src/channelz.ts b/packages/grpc-js/src/channelz.ts index c207e567..7242d716 100644 --- a/packages/grpc-js/src/channelz.ts +++ b/packages/grpc-js/src/channelz.ts @@ -460,6 +460,23 @@ function parseIPv6Chunk(addressChunk: string): number[] { return result.concat(...bytePairs); } +function isIPv6MappedIPv4(ipAddress: string) { + return isIPv6(ipAddress) && ipAddress.toLowerCase().startsWith('::ffff:') && isIPv4(ipAddress.substring(7)); +} + +/** + * Prerequisite: isIPv4(ipAddress) + * @param ipAddress + * @returns + */ +function ipv4AddressStringToBuffer(ipAddress: string): Buffer { + return Buffer.from( + Uint8Array.from( + ipAddress.split('.').map(segment => Number.parseInt(segment)) + ) + ); +} + /** * Converts an IPv4 or IPv6 address from string representation to binary * representation @@ -468,11 +485,9 @@ function parseIPv6Chunk(addressChunk: string): number[] { */ function ipAddressStringToBuffer(ipAddress: string): Buffer | null { if (isIPv4(ipAddress)) { - return Buffer.from( - Uint8Array.from( - ipAddress.split('.').map(segment => Number.parseInt(segment)) - ) - ); + return ipv4AddressStringToBuffer(ipAddress); + } else if (isIPv6MappedIPv4(ipAddress)) { + return ipv4AddressStringToBuffer(ipAddress.substring(7)); } else if (isIPv6(ipAddress)) { let leftSection: string; let rightSection: string; From 6965250011dbc7ef79df579cf92df95a56fe7c64 Mon Sep 17 00:00:00 2001 From: Michael Lumish Date: Wed, 26 Feb 2025 16:24:29 -0800 Subject: [PATCH 24/27] Handle secure context errors, fix server constructor argument handling --- packages/grpc-js-xds/src/xds-credentials.ts | 2 +- .../listener-resource-type.ts | 1 + .../grpc-js-xds/test/test-xds-credentials.ts | 63 +++++++- packages/grpc-js/src/channel-credentials.ts | 19 ++- packages/grpc-js/src/server-credentials.ts | 66 +++++---- packages/grpc-js/src/server.ts | 16 ++- packages/grpc-js/src/transport.ts | 54 ++++--- .../grpc-js/test/test-channel-credentials.ts | 134 +++++++++++++++++- .../grpc-js/test/test-server-credentials.ts | 23 +-- packages/grpc-js/test/test-server.ts | 2 +- 10 files changed, 302 insertions(+), 78 deletions(-) diff --git a/packages/grpc-js-xds/src/xds-credentials.ts b/packages/grpc-js-xds/src/xds-credentials.ts index 1749e217..89a97c50 100644 --- a/packages/grpc-js-xds/src/xds-credentials.ts +++ b/packages/grpc-js-xds/src/xds-credentials.ts @@ -64,7 +64,7 @@ export class XdsChannelCredentials extends ChannelCredentials { export class XdsServerCredentials extends ServerCredentials { constructor(private fallbackCredentials: ServerCredentials) { - super(); + super({}); } getFallbackCredentials() { diff --git a/packages/grpc-js-xds/src/xds-resource-type/listener-resource-type.ts b/packages/grpc-js-xds/src/xds-resource-type/listener-resource-type.ts index 82586fe4..59e91e3b 100644 --- a/packages/grpc-js-xds/src/xds-resource-type/listener-resource-type.ts +++ b/packages/grpc-js-xds/src/xds-resource-type/listener-resource-type.ts @@ -152,6 +152,7 @@ function validateTransportSocket(context: XdsDecodeContext, transportSocket: Tra return errors; } const downstreamTlsContext = decodeSingleResource(DOWNSTREAM_TLS_CONTEXT_TYPE_URL, transportSocket.typed_config.value); + trace('Decoded DownstreamTlsContext: ' + JSON.stringify(downstreamTlsContext, undefined, 2)); if (downstreamTlsContext.require_sni?.value) { errors.push(`DownstreamTlsContext.require_sni set`); } diff --git a/packages/grpc-js-xds/test/test-xds-credentials.ts b/packages/grpc-js-xds/test/test-xds-credentials.ts index 0a235721..27c45e2e 100644 --- a/packages/grpc-js-xds/test/test-xds-credentials.ts +++ b/packages/grpc-js-xds/test/test-xds-credentials.ts @@ -21,7 +21,7 @@ import { FakeEdsCluster, FakeRouteGroup, FakeServerRoute } from './framework'; import { ControlPlaneServer } from './xds-server'; import { XdsTestClient } from './client'; import { XdsChannelCredentials, XdsServerCredentials } from '../src'; -import { credentials, ServerCredentials, experimental } from '@grpc/grpc-js'; +import { credentials, ServerCredentials, experimental, status } from '@grpc/grpc-js'; import { readFileSync } from 'fs'; import * as path from 'path'; import { Listener } from '../src/generated/envoy/config/listener/v3/Listener'; @@ -441,6 +441,65 @@ describe('Client xDS credentials', () => { const error = await client.sendOneCallAsync(); assert.strictEqual(error, null); }); - + it('Should fail when the server expects a certificate and does not get one', async () => { + const [backend] = await createBackends(1, true, new XdsServerCredentials(ServerCredentials.createInsecure())); + const downstreamTlsContext: DownstreamTlsContext & AnyExtension = { + '@type': DOWNSTREAM_TLS_CONTEXT_TYPE_URL, + common_tls_context: { + tls_certificate_provider_instance: { + instance_name: 'test_certificates' + }, + validation_context: { + ca_certificate_provider_instance: { + instance_name: 'test_certificates' + } + } + }, + ocsp_staple_policy: 'LENIENT_STAPLING', + require_client_certificate: { + value: true + } + } + const baseServerListener: Listener = { + default_filter_chain: { + filter_chain_match: { + source_type: 'SAME_IP_OR_LOOPBACK' + }, + transport_socket: { + name: 'envoy.transport_sockets.tls', + typed_config: downstreamTlsContext + } + } + } + const serverRoute = new FakeServerRoute(backend.getPort(), 'serverRoute', baseServerListener); + xdsServer.setRdsResource(serverRoute.getRouteConfiguration()); + xdsServer.setLdsResource(serverRoute.getListener()); + xdsServer.addResponseListener((typeUrl, responseState) => { + if (responseState.state === 'NACKED') { + client?.stopCalls(); + assert.fail(`Client NACKED ${typeUrl} resource with message ${responseState.errorMessage}`); + } + }); + const upstreamTlsContext: UpstreamTlsContext = { + common_tls_context: { + validation_context: { + ca_certificate_provider_instance: { + instance_name: 'test_certificates' + } + } + } + }; + const cluster = new FakeEdsCluster('cluster1', 'endpoint1', [{backends: [backend], locality:{region: 'region1'}}], undefined, upstreamTlsContext); + const routeGroup = new FakeRouteGroup('listener1', 'route1', [{cluster: cluster}]); + await routeGroup.startAllBackends(xdsServer); + xdsServer.setEdsResource(cluster.getEndpointConfig()); + xdsServer.setCdsResource(cluster.getClusterConfig()); + xdsServer.setRdsResource(routeGroup.getRouteConfiguration()); + xdsServer.setLdsResource(routeGroup.getListener()); + client = XdsTestClient.createFromServer('listener1', xdsServer, new XdsChannelCredentials(credentials.createInsecure())); + const error = await client.sendOneCallAsync(); + assert(error); + assert.strictEqual(error.code, status.UNAVAILABLE); + }); }); }); diff --git a/packages/grpc-js/src/channel-credentials.ts b/packages/grpc-js/src/channel-credentials.ts index 78167253..16739a63 100644 --- a/packages/grpc-js/src/channel-credentials.ts +++ b/packages/grpc-js/src/channel-credentials.ts @@ -31,6 +31,8 @@ import { Socket } from 'net'; import { ChannelOptions } from './channel-options'; import { GrpcUri, parseUri, splitHostPort } from './uri-parser'; import { getDefaultAuthority } from './resolver'; +import { log } from './logging'; +import { LogVerbosity } from './constants'; // eslint-disable-next-line @typescript-eslint/no-explicit-any function verifyIsBufferOrNull(obj: any, friendlyName: string): void { @@ -475,12 +477,17 @@ class CertificateProviderChannelCredentialsImpl extends ChannelCredentials { if (this.identityCertificateProvider !== null && !this.latestIdentityUpdate) { return null; } - return createSecureContext({ - ca: this.latestCaUpdate.caCertificate, - key: this.latestIdentityUpdate?.privateKey, - cert: this.latestIdentityUpdate?.certificate, - ciphers: CIPHER_SUITES - }); + try { + return createSecureContext({ + ca: this.latestCaUpdate.caCertificate, + key: this.latestIdentityUpdate?.privateKey, + cert: this.latestIdentityUpdate?.certificate, + ciphers: CIPHER_SUITES + }); + } catch (e) { + log(LogVerbosity.ERROR, 'Failed to createSecureContext with error ' + (e as Error).message); + return null; + } } } diff --git a/packages/grpc-js/src/server-credentials.ts b/packages/grpc-js/src/server-credentials.ts index 68fc7603..5a61adde 100644 --- a/packages/grpc-js/src/server-credentials.ts +++ b/packages/grpc-js/src/server-credentials.ts @@ -32,7 +32,11 @@ export interface SecureContextWatcher { export abstract class ServerCredentials { private watchers: Set = new Set(); - private latestContextOptions: SecureServerOptions | null = null; + private latestContextOptions: SecureContextOptions | null = null; + constructor(private serverConstructorOptions: SecureServerOptions | null, contextOptions?: SecureContextOptions) { + this.latestContextOptions = contextOptions ?? null; + } + _addWatcher(watcher: SecureContextWatcher) { this.watchers.add(watcher); } @@ -42,16 +46,21 @@ export abstract class ServerCredentials { protected getWatcherCount() { return this.watchers.size; } - protected updateSecureContextOptions(options: SecureServerOptions | null) { + protected updateSecureContextOptions(options: SecureContextOptions | null) { this.latestContextOptions = options; for (const watcher of this.watchers) { watcher(this.latestContextOptions); } } - abstract _isSecure(): boolean; - _getSettings(): SecureServerOptions | null { + _isSecure(): boolean { + return this.serverConstructorOptions !== null; + } + _getSecureContextOptions(): SecureContextOptions | null { return this.latestContextOptions; } + _getConstructorOptions(): SecureServerOptions | null { + return this.serverConstructorOptions; + } _getInterceptors(): ServerInterceptor[] { return []; } @@ -101,18 +110,19 @@ export abstract class ServerCredentials { } return new SecureServerCredentials({ + requestCert: checkClientCertificate, + ciphers: CIPHER_SUITES, + }, { ca: rootCerts ?? getDefaultRootsData() ?? undefined, cert, key, - requestCert: checkClientCertificate, - ciphers: CIPHER_SUITES, }); } } class InsecureServerCredentials extends ServerCredentials { - _isSecure(): boolean { - return false; + constructor() { + super(null); } _getSettings(): null { @@ -127,17 +137,9 @@ class InsecureServerCredentials extends ServerCredentials { class SecureServerCredentials extends ServerCredentials { private options: SecureServerOptions; - constructor(options: SecureServerOptions) { - super(); - this.options = options; - } - - _isSecure(): boolean { - return true; - } - - _getSettings(): SecureServerOptions { - return this.options; + constructor(constructorOptions: SecureServerOptions, contextOptions: SecureContextOptions) { + super(constructorOptions, contextOptions); + this.options = {...constructorOptions, ...contextOptions}; } /** @@ -229,7 +231,11 @@ class CertificateProviderServerCredentials extends ServerCredentials { private caCertificateProvider: CertificateProvider | null, private requireClientCertificate: boolean ) { - super(); + super({ + requestCert: caCertificateProvider !== null, + rejectUnauthorized: requireClientCertificate, + ciphers: CIPHER_SUITES + }); } _addWatcher(watcher: SecureContextWatcher): void { if (this.getWatcherCount() === 0) { @@ -245,9 +251,6 @@ class CertificateProviderServerCredentials extends ServerCredentials { this.identityCertificateProvider.removeIdentityCertificateListener(this.identityCertificateUpdateListener); } } - _isSecure(): boolean { - return true; - } _equals(other: ServerCredentials): boolean { if (this === other) { return true; @@ -262,7 +265,7 @@ class CertificateProviderServerCredentials extends ServerCredentials { ) } - private calculateSecureContextOptions(): SecureServerOptions | null { + private calculateSecureContextOptions(): SecureContextOptions | null { if (this.latestIdentityUpdate === null) { return null; } @@ -271,10 +274,8 @@ class CertificateProviderServerCredentials extends ServerCredentials { } return { ca: this.latestCaUpdate?.caCertificate, - cert: this.latestIdentityUpdate.certificate, - key: this.latestIdentityUpdate.privateKey, - requestCert: this.latestIdentityUpdate !== null, - rejectUnauthorized: this.requireClientCertificate + cert: [this.latestIdentityUpdate.certificate], + key: [this.latestIdentityUpdate.privateKey], }; } @@ -307,7 +308,7 @@ export function createCertificateProviderServerCredentials( class InterceptorServerCredentials extends ServerCredentials { constructor(private readonly childCredentials: ServerCredentials, private readonly interceptors: ServerInterceptor[]) { - super(); + super({}); } _isSecure(): boolean { return this.childCredentials._isSecure(); @@ -338,8 +339,11 @@ class InterceptorServerCredentials extends ServerCredentials { override _removeWatcher(watcher: SecureContextWatcher): void { this.childCredentials._removeWatcher(watcher); } - override _getSettings(): SecureServerOptions | null { - return this.childCredentials._getSettings(); + override _getConstructorOptions(): SecureServerOptions | null { + return this.childCredentials._getConstructorOptions(); + } + override _getSecureContextOptions(): SecureContextOptions | null { + return this.childCredentials._getSecureContextOptions(); } } diff --git a/packages/grpc-js/src/server.ts b/packages/grpc-js/src/server.ts index cc8c3ab5..682a72ae 100644 --- a/packages/grpc-js/src/server.ts +++ b/packages/grpc-js/src/server.ts @@ -574,13 +574,15 @@ export class Server { private createHttp2Server(credentials: ServerCredentials) { let http2Server: http2.Http2Server | http2.Http2SecureServer; if (credentials._isSecure()) { - const credentialsSettings = credentials._getSettings(); + const constructorOptions = credentials._getConstructorOptions(); + const contextOptions = credentials._getSecureContextOptions(); const secureServerOptions: http2.SecureServerOptions = { ...this.commonServerOptions, - ...credentialsSettings, + ...constructorOptions, + ...contextOptions, enableTrace: this.options['grpc-node.tls_enable_trace'] === 1 }; - let areCredentialsValid = credentialsSettings !== null; + let areCredentialsValid = contextOptions !== null; this.trace('Initial credentials valid: ' + areCredentialsValid); http2Server = http2.createSecureServer(secureServerOptions); http2Server.prependListener('connection', (socket: Socket) => { @@ -600,7 +602,13 @@ export class Server { }); const credsWatcher: SecureContextWatcher = options => { if (options) { - (http2Server as http2.Http2SecureServer).setSecureContext(options); + const secureServer = http2Server as http2.Http2SecureServer; + try { + secureServer.setSecureContext(options); + } catch (e) { + logging.log(LogVerbosity.ERROR, 'Failed to set secure context with error ' + (e as Error).message); + options = null; + } } areCredentialsValid = options !== null; this.trace('Post-update credentials valid: ' + areCredentialsValid); diff --git a/packages/grpc-js/src/transport.ts b/packages/grpc-js/src/transport.ts index f6f36d4d..d14a2227 100644 --- a/packages/grpc-js/src/transport.ts +++ b/packages/grpc-js/src/transport.ts @@ -659,6 +659,10 @@ export class Http2SubchannelConnector implements SubchannelConnector { return Promise.reject(); } + if (secureConnectResult.socket.closed) { + return Promise.reject('Connection closed before starting HTTP/2 handshake'); + } + return new Promise((resolve, reject) => { let remoteName: string | null = null; let realTarget: GrpcUri = this.channelTarget; @@ -671,6 +675,26 @@ export class Http2SubchannelConnector implements SubchannelConnector { } const scheme = secureConnectResult.secure ? 'https' : 'http'; const targetPath = getDefaultAuthority(realTarget); + const closeHandler = () => { + this.session?.destroy(); + this.session = null; + // Leave time for error event to happen before rejecting + setImmediate(() => { + if (!reportedError) { + reportedError = true; + reject(`${errorMessage.trim()} (${new Date().toISOString()})`); + } + }); + }; + const errorHandler = (error: Error) => { + this.session?.destroy(); + errorMessage = (error as Error).message; + this.trace('connection failed with error ' + errorMessage); + if (!reportedError) { + reportedError = true; + reject(`${errorMessage} (${new Date().toISOString()})`); + } + }; const session = http2.connect(`${scheme}://${targetPath}`, { createConnection: (authority, option) => { return secureConnectResult.socket; @@ -687,27 +711,15 @@ export class Http2SubchannelConnector implements SubchannelConnector { session.unref(); session.once('remoteSettings', () => { session.removeAllListeners(); + secureConnectResult.socket.removeListener('close', closeHandler); + secureConnectResult.socket.removeListener('error', errorHandler); resolve(new Http2Transport(session, address, options, remoteName)); this.session = null; }); - session.once('close', () => { - this.session = null; - // Leave time for error event to happen before rejecting - setImmediate(() => { - if (!reportedError) { - reportedError = true; - reject(`${errorMessage} (${new Date().toISOString()})`); - } - }); - }); - session.once('error', error => { - errorMessage = (error as Error).message; - this.trace('connection failed with error ' + errorMessage); - if (!reportedError) { - reportedError = true; - reject(`${errorMessage} (${new Date().toISOString()})`); - } - }); + session.once('close', closeHandler); + session.once('error', errorHandler); + secureConnectResult.socket.once('close', closeHandler); + secureConnectResult.socket.once('error', errorHandler); }); } @@ -747,11 +759,13 @@ export class Http2SubchannelConnector implements SubchannelConnector { let secureConnectResult: SecureConnectResult | null = null; const addressString = subchannelAddressToString(address); try { + this.trace(addressString + ' Waiting for secureConnector to be ready'); await secureConnector.waitForReady(); + this.trace(addressString + ' secureConnector is ready'); tcpConnection = await this.tcpConnect(address, options); - this.trace(addressString + ' ' + 'Established TCP connection'); + this.trace(addressString + ' Established TCP connection'); secureConnectResult = await secureConnector.connect(tcpConnection); - this.trace(addressString + ' ' + 'Established secure connection'); + this.trace(addressString + ' Established secure connection'); return this.createSession(secureConnectResult, address, options); } catch (e) { tcpConnection?.destroy(); diff --git a/packages/grpc-js/test/test-channel-credentials.ts b/packages/grpc-js/test/test-channel-credentials.ts index 43bce5da..a03ec41b 100644 --- a/packages/grpc-js/test/test-channel-credentials.ts +++ b/packages/grpc-js/test/test-channel-credentials.ts @@ -28,6 +28,7 @@ import { ServiceClient, ServiceClientConstructor } from '../src/make-client'; import { assert2, loadProtoFile, mockFunction } from './common'; import { sendUnaryData, ServerUnaryCall, ServiceError } from '../src'; import { FileWatcherCertificateProvider } from '../src/certificate-provider'; +import { createCertificateProviderServerCredentials } from '../src/server-credentials'; const protoFile = path.join(__dirname, 'fixtures', 'echo_service.proto'); const echoService = loadProtoFile(protoFile) @@ -183,7 +184,7 @@ describe('ChannelCredentials usage', () => { const certificateProvider = new FileWatcherCertificateProvider({ caCertificateFile: `${__dirname}/fixtures/ca.pem`, certificateFile: `${__dirname}/fixtures/server1.pem`, - privateKeyFile: `${__dirname}/fixtures/server1.pem`, + privateKeyFile: `${__dirname}/fixtures/server1.key`, refreshIntervalMs: 1000 }); const channelCreds = createCertificateProviderChannelCredentials(certificateProvider, null); @@ -193,7 +194,6 @@ describe('ChannelCredentials usage', () => { }); client.echo( { value: 'test value', value2: 3 }, - new grpc.Metadata({waitForReady: true}), (error: ServiceError, response: any) => { client.close(); assert.ifError(error); @@ -219,3 +219,133 @@ describe('ChannelCredentials usage', () => { ); }); }); + +describe('Channel credentials mtls', () => { + let client: ServiceClient; + let server: grpc.Server; + let portNum: number; + let caCert: Buffer; + let keyValue: Buffer; + let certValue: Buffer; + const hostnameOverride = 'foo.test.google.fr'; + before(async () => { + const { ca, key, cert } = await pFixtures; + caCert = ca; + keyValue = key; + certValue = cert; + const serverCreds = grpc.ServerCredentials.createSsl(ca, [ + { private_key: key, cert_chain: cert }, + ], true); + return new Promise((resolve, reject) => { + server = new grpc.Server(); + server.addService(echoService.service, { + echo(call: ServerUnaryCall, callback: sendUnaryData) { + call.sendMetadata(call.metadata); + callback(null, call.request); + }, + }); + + server.bindAsync('localhost:0', serverCreds, (err, port) => { + if (err) { + reject(err); + return; + } + portNum = port; + resolve(); + }); + }); + }); + afterEach(() => { + client.close(); + }); + after(() => { + server.forceShutdown(); + }); + + it('Should work with client provided certificates', done => { + const channelCreds = ChannelCredentials.createSsl(caCert, keyValue, certValue); + client = new echoService(`localhost:${portNum}`, channelCreds, { + 'grpc.ssl_target_name_override': hostnameOverride, + 'grpc.default_authority': hostnameOverride, + }); + client.echo({ value: 'test value', value2: 3 }, (error: ServiceError, response: any) => { + assert.ifError(error); + done(); + }); + }); + it('Should fail if the client does not provide certificates', done => { + const channelCreds = ChannelCredentials.createSsl(caCert); + client = new echoService(`localhost:${portNum}`, channelCreds, { + 'grpc.ssl_target_name_override': hostnameOverride, + 'grpc.default_authority': hostnameOverride, + }); + client.echo({ value: 'test value', value2: 3 }, (error: ServiceError, response: any) => { + assert(error); + assert.strictEqual(error.code, grpc.status.UNAVAILABLE); + done(); + }); + }); +}); + +describe('Channel credentials certificate provider mtls', () => { + const certificateProvider = new FileWatcherCertificateProvider({ + caCertificateFile: `${__dirname}/fixtures/ca.pem`, + certificateFile: `${__dirname}/fixtures/server1.pem`, + privateKeyFile: `${__dirname}/fixtures/server1.key`, + refreshIntervalMs: 1000 + }); + const hostnameOverride = 'foo.test.google.fr'; + let client: ServiceClient; + let server: grpc.Server; + let portNum: number; + before(done => { + const serverCreds = createCertificateProviderServerCredentials(certificateProvider, certificateProvider, true); + server = new grpc.Server(); + server.addService(echoService.service, { + echo(call: ServerUnaryCall, callback: sendUnaryData) { + call.sendMetadata(call.metadata); + callback(null, call.request); + }, + }); + + server.bindAsync('localhost:0', serverCreds, (err, port) => { + if (err) { + done(err); + return; + } + portNum = port; + done(); + }); + }); + afterEach(() => { + client.close(); + }); + after(() => { + server.forceShutdown(); + }); + + it('Should work with client provided certificates', done => { + const channelCreds = createCertificateProviderChannelCredentials(certificateProvider, certificateProvider); + client = new echoService(`localhost:${portNum}`, channelCreds, { + 'grpc.ssl_target_name_override': hostnameOverride, + 'grpc.default_authority': hostnameOverride, + }); + client.echo({ value: 'test value', value2: 3 }, (error: ServiceError, response: any) => { + assert.ifError(error); + done(); + }); + }); + it('Should fail if the client does not provide certificates', done => { + const channelCreds = createCertificateProviderChannelCredentials(certificateProvider, null); + client = new echoService(`localhost:${portNum}`, channelCreds, { + 'grpc.ssl_target_name_override': hostnameOverride, + 'grpc.default_authority': hostnameOverride, + }); + client.echo({ value: 'test value', value2: 3 }, (error: ServiceError, response: any) => { + assert(error); + assert.strictEqual(error.code, grpc.status.UNAVAILABLE); + done(); + }); + }); + +}); diff --git a/packages/grpc-js/test/test-server-credentials.ts b/packages/grpc-js/test/test-server-credentials.ts index ec1740f7..672c9462 100644 --- a/packages/grpc-js/test/test-server-credentials.ts +++ b/packages/grpc-js/test/test-server-credentials.ts @@ -32,7 +32,7 @@ describe('Server Credentials', () => { const creds = ServerCredentials.createInsecure(); assert.strictEqual(creds._isSecure(), false); - assert.strictEqual(creds._getSettings(), null); + assert.strictEqual(creds._getSecureContextOptions(), null); }); }); @@ -41,16 +41,17 @@ describe('Server Credentials', () => { const creds = ServerCredentials.createSsl(ca, []); assert.strictEqual(creds._isSecure(), true); - assert.strictEqual(creds._getSettings()?.ca, ca); + assert.strictEqual(creds._getSecureContextOptions()?.ca, ca); }); it('accepts a boolean as the third argument', () => { const creds = ServerCredentials.createSsl(ca, [], true); assert.strictEqual(creds._isSecure(), true); - const settings = creds._getSettings(); - assert.strictEqual(settings?.ca, ca); - assert.strictEqual(settings?.requestCert, true); + const constructorOptions = creds._getConstructorOptions(); + const contextOptions = creds._getSecureContextOptions(); + assert.strictEqual(contextOptions?.ca, ca); + assert.strictEqual(constructorOptions?.requestCert, true); }); it('accepts an object with two buffers in the second argument', () => { @@ -58,9 +59,9 @@ describe('Server Credentials', () => { const creds = ServerCredentials.createSsl(null, keyCertPairs); assert.strictEqual(creds._isSecure(), true); - const settings = creds._getSettings(); - assert.deepStrictEqual(settings?.cert, [cert]); - assert.deepStrictEqual(settings?.key, [key]); + const contextOptions = creds._getSecureContextOptions(); + assert.deepStrictEqual(contextOptions?.cert, [cert]); + assert.deepStrictEqual(contextOptions?.key, [key]); }); it('accepts multiple objects in the second argument', () => { @@ -71,9 +72,9 @@ describe('Server Credentials', () => { const creds = ServerCredentials.createSsl(null, keyCertPairs, false); assert.strictEqual(creds._isSecure(), true); - const settings = creds._getSettings(); - assert.deepStrictEqual(settings?.cert, [cert, cert]); - assert.deepStrictEqual(settings?.key, [key, key]); + const contextOptions = creds._getSecureContextOptions(); + assert.deepStrictEqual(contextOptions?.cert, [cert, cert]); + assert.deepStrictEqual(contextOptions?.key, [key, key]); }); it('fails if the second argument is not an Array', () => { diff --git a/packages/grpc-js/test/test-server.ts b/packages/grpc-js/test/test-server.ts index 1ea14097..f259b27b 100644 --- a/packages/grpc-js/test/test-server.ts +++ b/packages/grpc-js/test/test-server.ts @@ -801,7 +801,7 @@ describe('Echo service', () => { class ToggleableSecureServerCredentials extends ServerCredentials { private contextOptions: SecureContextOptions; constructor(key: Buffer, cert: Buffer) { - super(); + super({}); this.contextOptions = {key, cert}; this.enable(); } From 510d68140b0eea526963cdb0c2ccc562f399a2f2 Mon Sep 17 00:00:00 2001 From: Michael Lumish Date: Wed, 26 Feb 2025 17:47:02 -0800 Subject: [PATCH 25/27] Apparently unset oneof is allowed --- .../listener-resource-type.ts | 38 ++++++++++--------- 1 file changed, 20 insertions(+), 18 deletions(-) diff --git a/packages/grpc-js-xds/src/xds-resource-type/listener-resource-type.ts b/packages/grpc-js-xds/src/xds-resource-type/listener-resource-type.ts index 59e91e3b..b8e9cba6 100644 --- a/packages/grpc-js-xds/src/xds-resource-type/listener-resource-type.ts +++ b/packages/grpc-js-xds/src/xds-resource-type/listener-resource-type.ts @@ -165,26 +165,28 @@ function validateTransportSocket(context: XdsDecodeContext, transportSocket: Tra } const commonTlsContext = downstreamTlsContext.common_tls_context; let validationContext: CertificateValidationContext__Output | null = null; - switch (commonTlsContext.validation_context_type) { - case 'validation_context_sds_secret_config': - errors.push('Unexpected DownstreamTlsContext.common_tls_context.validation_context_sds_secret_config'); - break; - case 'validation_context': - if (!commonTlsContext.validation_context) { - errors.push('Empty DownstreamTlsContext.common_tls_context.validation_context'); + if (commonTlsContext.validation_context_type) { + switch (commonTlsContext.validation_context_type) { + case 'validation_context_sds_secret_config': + errors.push('Unexpected DownstreamTlsContext.common_tls_context.validation_context_sds_secret_config'); break; - } - validationContext = commonTlsContext.validation_context; - break; - case 'combined_validation_context': - if (!commonTlsContext.combined_validation_context) { - errors.push('Empty DownstreamTlsContext.common_tls_context.combined_validation_context') + case 'validation_context': + if (!commonTlsContext.validation_context) { + errors.push('Empty DownstreamTlsContext.common_tls_context.validation_context'); + break; + } + validationContext = commonTlsContext.validation_context; break; - } - validationContext = commonTlsContext.combined_validation_context.default_validation_context; - break; - default: - errors.push(`Unsupported DownstreamTlsContext.common_tls_context.validation_context_type: ${commonTlsContext.validation_context_type}`); + case 'combined_validation_context': + if (!commonTlsContext.combined_validation_context) { + errors.push('Empty DownstreamTlsContext.common_tls_context.combined_validation_context') + break; + } + validationContext = commonTlsContext.combined_validation_context.default_validation_context; + break; + default: + errors.push(`Unsupported DownstreamTlsContext.common_tls_context.validation_context_type: ${commonTlsContext.validation_context_type}`); + } } if (downstreamTlsContext.require_client_certificate && !validationContext) { errors.push('DownstreamTlsContext.require_client_certificate set without any validationContext'); From 0ebb571bb7c1807c3d06c7233573a46b5529897c Mon Sep 17 00:00:00 2001 From: Michael Lumish Date: Thu, 27 Feb 2025 10:51:00 -0800 Subject: [PATCH 26/27] Don't unregister the xds server's channelz ref when destroying the connection injector --- packages/grpc-js-xds/src/server.ts | 5 ++++- packages/grpc-js/src/server.ts | 15 +++++++++++---- 2 files changed, 15 insertions(+), 5 deletions(-) diff --git a/packages/grpc-js-xds/src/server.ts b/packages/grpc-js-xds/src/server.ts index 9bbf897c..f06f1439 100644 --- a/packages/grpc-js-xds/src/server.ts +++ b/packages/grpc-js-xds/src/server.ts @@ -83,6 +83,7 @@ interface ConfigParameters { createConnectionInjector: (credentials: ServerCredentials) => ConnectionInjector; drainGraceTimeMs: number; listenerResourceNameTemplate: string; + unregisterChannelzRef: () => void; } class FilterChainEntry { @@ -452,6 +453,7 @@ class BoundPortEntry { this.tcpServer.close(); const resourceName = formatTemplateString(this.configParameters.listenerResourceNameTemplate, this.boundAddress); ListenerResourceType.cancelWatch(this.configParameters.xdsClient, resourceName, this.listenerWatcher); + this.configParameters.unregisterChannelzRef(); } } @@ -633,7 +635,8 @@ export class XdsServer extends Server { createConnectionInjector: (credentials) => this.experimentalCreateConnectionInjectorWithChannelzRef(credentials, channelzRef), drainGraceTimeMs: this.drainGraceTimeMs, listenerResourceNameTemplate: this.listenerResourceNameTemplate, - xdsClient: this.xdsClient + xdsClient: this.xdsClient, + unregisterChannelzRef: () => this.experimentalUnregisterListenerFromChannelz(channelzRef) }; const portEntry = new BoundPortEntry(configParameters, port, creds); const servingStatusListener: ServingStatusListener = statusObject => { diff --git a/packages/grpc-js/src/server.ts b/packages/grpc-js/src/server.ts index 682a72ae..9528b125 100644 --- a/packages/grpc-js/src/server.ts +++ b/packages/grpc-js/src/server.ts @@ -246,6 +246,7 @@ interface BoundPort { interface Http2ServerInfo { channelzRef: SocketRef; sessions: Set; + ownsChannelzRef: boolean; } interface SessionIdleTimeoutTracker { @@ -571,6 +572,10 @@ export class Server { ); } + protected experimentalUnregisterListenerFromChannelz(channelzRef: SocketRef) { + unregisterChannelzRef(channelzRef); + } + private createHttp2Server(credentials: ServerCredentials) { let http2Server: http2.Http2Server | http2.Http2SecureServer; if (credentials._isSecure()) { @@ -670,6 +675,7 @@ export class Server { this.http2Servers.set(http2Server, { channelzRef: channelzRef, sessions: new Set(), + ownsChannelzRef: true }); boundPortObject.listeningServers.add(http2Server); this.trace( @@ -964,7 +970,7 @@ export class Server { * @param channelzRef * @returns */ - protected experimentalCreateConnectionInjectorWithChannelzRef(credentials: ServerCredentials, channelzRef: SocketRef) { + protected experimentalCreateConnectionInjectorWithChannelzRef(credentials: ServerCredentials, channelzRef: SocketRef, ownsChannelzRef=false) { if (credentials === null || !(credentials instanceof ServerCredentials)) { throw new TypeError('creds must be a ServerCredentials object'); } @@ -975,7 +981,8 @@ export class Server { const sessionsSet: Set = new Set(); this.http2Servers.set(server, { channelzRef: channelzRef, - sessions: sessionsSet + sessions: sessionsSet, + ownsChannelzRef }); return { injectConnection: (connection: Duplex) => { @@ -1005,7 +1012,7 @@ export class Server { throw new TypeError('creds must be a ServerCredentials object'); } const channelzRef = this.registerInjectorToChannelz(); - return this.experimentalCreateConnectionInjectorWithChannelzRef(credentials, channelzRef); + return this.experimentalCreateConnectionInjectorWithChannelzRef(credentials, channelzRef, true); } private closeServer(server: AnyHttp2Server, callback?: () => void) { @@ -1014,7 +1021,7 @@ export class Server { ); const serverInfo = this.http2Servers.get(server); server.close(() => { - if (serverInfo) { + if (serverInfo && serverInfo.ownsChannelzRef) { this.listenerChildrenTracker.unrefChild(serverInfo.channelzRef); unregisterChannelzRef(serverInfo.channelzRef); } From 6094ebed618a337272bceb1a85a923defa7c5b02 Mon Sep 17 00:00:00 2001 From: Michael Lumish Date: Thu, 27 Feb 2025 13:05:16 -0800 Subject: [PATCH 27/27] Handle unset validation_config_type at use time --- packages/grpc-js-xds/src/server.ts | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/packages/grpc-js-xds/src/server.ts b/packages/grpc-js-xds/src/server.ts index f06f1439..8e9621e0 100644 --- a/packages/grpc-js-xds/src/server.ts +++ b/packages/grpc-js-xds/src/server.ts @@ -167,16 +167,18 @@ class FilterChainEntry { if (!instanceCertificateProvider) { throw new Error(`Invalid TLS context detected: unrecognized certificate instance name: ${commonTlsContext.tls_certificate_provider_instance!.instance_name}`); } - let validationContext: CertificateValidationContext__Output | null; - switch (commonTlsContext?.validation_context_type) { - case 'validation_context': - validationContext = commonTlsContext.validation_context!; - break; - case 'combined_validation_context': - validationContext = commonTlsContext.combined_validation_context!.default_validation_context; - break; - default: - throw new Error(`Invalid TLS context detected: invalid validation_context_type: ${commonTlsContext.validation_context_type}`); + let validationContext: CertificateValidationContext__Output | null = null; + if (commonTlsContext?.validation_context_type) { + switch (commonTlsContext?.validation_context_type) { + case 'validation_context': + validationContext = commonTlsContext.validation_context!; + break; + case 'combined_validation_context': + validationContext = commonTlsContext.combined_validation_context!.default_validation_context; + break; + default: + throw new Error(`Invalid TLS context detected: invalid validation_context_type: ${commonTlsContext.validation_context_type}`); + } } let caCertificateProvider: experimental.CertificateProvider | null = null; if (validationContext?.ca_certificate_provider_instance) {