Skip to content

Commit

Permalink
fix(cli-repl): use tls allowPartialTrustChain flag MONGOSH-1878 (#2181)
Browse files Browse the repository at this point in the history
This should bring back startup performance to pre-2.3.1 levels.

Also pins kerberos to 2.1.0, since bumping it actually broke our
Windows build.
  • Loading branch information
addaleax authored Oct 7, 2024
1 parent 7863068 commit 5e27c4f
Show file tree
Hide file tree
Showing 11 changed files with 295 additions and 75 deletions.
187 changes: 122 additions & 65 deletions package-lock.json

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@
"husky": "^9.0.11",
"mocha": "^10.2.0",
"mongodb": "^6.9.0",
"mongodb-runner": "^5.6.1",
"mongodb-runner": "^5.7.0",
"node-gyp": "^9.0.0",
"nyc": "^15.1.0",
"pkg-up": "^3.1.0",
Expand Down
2 changes: 1 addition & 1 deletion packages/arg-parser/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@
"mongodb-connection-string-url": "^3.0.1"
},
"devDependencies": {
"@mongodb-js/devtools-connect": "^3.2.11",
"@mongodb-js/devtools-connect": "^3.3.0",
"@mongodb-js/eslint-config-mongosh": "^1.0.0",
"@mongodb-js/prettier-config-devtools": "^1.0.1",
"@mongodb-js/tsconfig-mongosh": "^1.0.0",
Expand Down
2 changes: 1 addition & 1 deletion packages/cli-repl/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@
}
},
"dependencies": {
"@mongodb-js/devtools-proxy-support": "^0.3.10",
"@mongodb-js/devtools-proxy-support": "^0.4.0",
"@mongosh/arg-parser": "0.0.0-dev.0",
"@mongosh/autocomplete": "0.0.0-dev.0",
"@mongosh/editor": "0.0.0-dev.0",
Expand Down
6 changes: 6 additions & 0 deletions packages/cli-repl/src/run.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,12 @@ import { TimingCategories } from '@mongosh/types';
import './webpack-self-inspection';
import { systemCA } from '@mongodb-js/devtools-proxy-support';

if ('boxednode' in process) {
// compiled executable
// https://github.com/mongodb-js/devtools-shared/pull/476
(process as any).__tlsSupportsAllowPartialTrustChainFlag = true;
}

// TS does not yet have type definitions for v8.startupSnapshot
if ((v8 as any)?.startupSnapshot?.isBuildingSnapshot?.()) {
// Import a few nested deps of dependencies that cannot be included in the
Expand Down
4 changes: 2 additions & 2 deletions packages/e2e-tests/test/e2e-tls.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -165,7 +165,7 @@ describe('e2e TLS', function () {
const result = await shell.waitForPromptOrExit();
expect(result.state).to.equal('exit');
shell.assertContainsOutput(
/unable to verify the first certificate|self[- ]signed certificate in certificate chain/
/unable to verify the first certificate|self[- ]signed certificate in certificate chain|unable to get (local )?issuer certificate/
);
});

Expand All @@ -182,7 +182,7 @@ describe('e2e TLS', function () {
const result = await shell.waitForPromptOrExit();
expect(result.state).to.equal('exit');
shell.assertContainsOutput(
/unable to verify the first certificate|self[- ]signed certificate in certificate chain/
/unable to verify the first certificate|self[- ]signed certificate in certificate chain|unable to get (local )?issuer certificate/
);
});

Expand Down
2 changes: 1 addition & 1 deletion packages/logging/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
"node": ">=14.15.1"
},
"dependencies": {
"@mongodb-js/devtools-connect": "^3.2.11",
"@mongodb-js/devtools-connect": "^3.3.0",
"@mongosh/errors": "0.0.0-dev.0",
"@mongosh/history": "0.0.0-dev.0",
"@mongosh/types": "0.0.0-dev.0",
Expand Down
4 changes: 2 additions & 2 deletions packages/service-provider-server/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@
}
},
"dependencies": {
"@mongodb-js/devtools-connect": "^3.2.11",
"@mongodb-js/devtools-connect": "^3.3.0",
"@mongodb-js/oidc-plugin": "^1.1.1",
"@mongosh/errors": "0.0.0-dev.0",
"@mongosh/service-provider-core": "0.0.0-dev.0",
Expand All @@ -58,7 +58,7 @@
"socks": "^2.8.3"
},
"optionalDependencies": {
"kerberos": "^2.2.0",
"kerberos": "2.1.0",
"mongodb-client-encryption": "^6.1.0"
},
"devDependencies": {
Expand Down
2 changes: 1 addition & 1 deletion packages/snippet-manager/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@
"unitTestsOnly": true
},
"dependencies": {
"@mongodb-js/devtools-proxy-support": "^0.3.10",
"@mongodb-js/devtools-proxy-support": "^0.4.0",
"@mongosh/errors": "0.0.0-dev.0",
"@mongosh/shell-api": "0.0.0-dev.0",
"@mongosh/types": "0.0.0-dev.0",
Expand Down
2 changes: 1 addition & 1 deletion packages/types/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@
"unitTestsOnly": true
},
"dependencies": {
"@mongodb-js/devtools-connect": "^3.2.11"
"@mongodb-js/devtools-connect": "^3.3.0"
},
"devDependencies": {
"@mongodb-js/eslint-config-mongosh": "^1.0.0",
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,157 @@
diff --git a/lib/internal/tls/secure-context.js b/lib/internal/tls/secure-context.js
index b0f971e4eef273..84e74105fdbba9 100644
--- a/lib/internal/tls/secure-context.js
+++ b/lib/internal/tls/secure-context.js
@@ -130,6 +130,7 @@ function configSecureContext(context, options = kEmptyObject, name = 'options')
validateObject(options, name);

const {
+ allowPartialTrustChain,
ca,
cert,
ciphers = getDefaultCiphers(),
@@ -182,6 +183,10 @@ function configSecureContext(context, options = kEmptyObject, name = 'options')
context.addRootCerts();
}

+ if (allowPartialTrustChain) {
+ context.setAllowPartialTrustChain();
+ }
+
if (cert) {
setCerts(context, ArrayIsArray(cert) ? cert : [cert], `${name}.cert`);
}
diff --git a/src/crypto/crypto_context.cc b/src/crypto/crypto_context.cc
index 1d60f7e856075a..cef0c877c67643 100644
--- a/src/crypto/crypto_context.cc
+++ b/src/crypto/crypto_context.cc
@@ -273,6 +273,8 @@ Local<FunctionTemplate> SecureContext::GetConstructorTemplate(
SetProtoMethod(isolate, tmpl, "setKey", SetKey);
SetProtoMethod(isolate, tmpl, "setCert", SetCert);
SetProtoMethod(isolate, tmpl, "addCACert", AddCACert);
+ SetProtoMethod(
+ isolate, tmpl, "setAllowPartialTrustChain", SetAllowPartialTrustChain);
SetProtoMethod(isolate, tmpl, "addCRL", AddCRL);
SetProtoMethod(isolate, tmpl, "addRootCerts", AddRootCerts);
SetProtoMethod(isolate, tmpl, "setCipherSuites", SetCipherSuites);
@@ -354,6 +356,7 @@ void SecureContext::RegisterExternalReferences(
registry->Register(AddCACert);
registry->Register(AddCRL);
registry->Register(AddRootCerts);
+ registry->Register(SetAllowPartialTrustChain);
registry->Register(SetCipherSuites);
registry->Register(SetCiphers);
registry->Register(SetSigalgs);
@@ -715,17 +718,39 @@ void SecureContext::SetCert(const FunctionCallbackInfo<Value>& args) {
USE(sc->AddCert(env, std::move(bio)));
}

+// NOLINTNEXTLINE(runtime/int)
+void SecureContext::SetX509StoreFlag(unsigned long flags) {
+ X509_STORE* cert_store = GetCertStoreOwnedByThisSecureContext();
+ CHECK_EQ(1, X509_STORE_set_flags(cert_store, flags));
+}
+
+X509_STORE* SecureContext::GetCertStoreOwnedByThisSecureContext() {
+ if (own_cert_store_cache_ != nullptr) return own_cert_store_cache_;
+
+ X509_STORE* cert_store = SSL_CTX_get_cert_store(ctx_.get());
+ if (cert_store == GetOrCreateRootCertStore()) {
+ cert_store = NewRootCertStore();
+ SSL_CTX_set_cert_store(ctx_.get(), cert_store);
+ }
+
+ return own_cert_store_cache_ = cert_store;
+}
+
+void SecureContext::SetAllowPartialTrustChain(
+ const FunctionCallbackInfo<Value>& args) {
+ SecureContext* sc;
+ ASSIGN_OR_RETURN_UNWRAP(&sc, args.This());
+ sc->SetX509StoreFlag(X509_V_FLAG_PARTIAL_CHAIN);
+}
+
void SecureContext::SetCACert(const BIOPointer& bio) {
ClearErrorOnReturn clear_error_on_return;
if (!bio) return;
- X509_STORE* cert_store = SSL_CTX_get_cert_store(ctx_.get());
while (X509Pointer x509 = X509Pointer(PEM_read_bio_X509_AUX(
bio.get(), nullptr, NoPasswordCallback, nullptr))) {
- if (cert_store == GetOrCreateRootCertStore()) {
- cert_store = NewRootCertStore();
- SSL_CTX_set_cert_store(ctx_.get(), cert_store);
- }
- CHECK_EQ(1, X509_STORE_add_cert(cert_store, x509.get()));
+ CHECK_EQ(1,
+ X509_STORE_add_cert(GetCertStoreOwnedByThisSecureContext(),
+ x509.get()));
CHECK_EQ(1, SSL_CTX_add_client_CA(ctx_.get(), x509.get()));
}
}
@@ -754,11 +779,7 @@ Maybe<bool> SecureContext::SetCRL(Environment* env, const BIOPointer& bio) {
return Nothing<bool>();
}

- X509_STORE* cert_store = SSL_CTX_get_cert_store(ctx_.get());
- if (cert_store == GetOrCreateRootCertStore()) {
- cert_store = NewRootCertStore();
- SSL_CTX_set_cert_store(ctx_.get(), cert_store);
- }
+ X509_STORE* cert_store = GetCertStoreOwnedByThisSecureContext();

CHECK_EQ(1, X509_STORE_add_crl(cert_store, crl.get()));
CHECK_EQ(1,
@@ -1042,8 +1063,6 @@ void SecureContext::LoadPKCS12(const FunctionCallbackInfo<Value>& args) {
sc->issuer_.reset();
sc->cert_.reset();

- X509_STORE* cert_store = SSL_CTX_get_cert_store(sc->ctx_.get());
-
DeleteFnPtr<PKCS12, PKCS12_free> p12;
EVPKeyPointer pkey;
X509Pointer cert;
@@ -1097,11 +1116,7 @@ void SecureContext::LoadPKCS12(const FunctionCallbackInfo<Value>& args) {
for (int i = 0; i < sk_X509_num(extra_certs.get()); i++) {
X509* ca = sk_X509_value(extra_certs.get(), i);

- if (cert_store == GetOrCreateRootCertStore()) {
- cert_store = NewRootCertStore();
- SSL_CTX_set_cert_store(sc->ctx_.get(), cert_store);
- }
- X509_STORE_add_cert(cert_store, ca);
+ X509_STORE_add_cert(sc->GetCertStoreOwnedByThisSecureContext(), ca);
SSL_CTX_add_client_CA(sc->ctx_.get(), ca);
}
ret = true;
diff --git a/src/crypto/crypto_context.h b/src/crypto/crypto_context.h
index 607b0984ba647a..4c76bdc5ec1a7c 100644
--- a/src/crypto/crypto_context.h
+++ b/src/crypto/crypto_context.h
@@ -65,6 +65,9 @@ class SecureContext final : public BaseObject {
void SetCACert(const BIOPointer& bio);
void SetRootCerts();

+ void SetX509StoreFlag(unsigned long flags); // NOLINT(runtime/int)
+ X509_STORE* GetCertStoreOwnedByThisSecureContext();
+
// TODO(joyeecheung): track the memory used by OpenSSL types
SET_NO_MEMORY_INFO()
SET_MEMORY_INFO_NAME(SecureContext)
@@ -91,6 +94,8 @@ class SecureContext final : public BaseObject {
#endif // !OPENSSL_NO_ENGINE
static void SetCert(const v8::FunctionCallbackInfo<v8::Value>& args);
static void AddCACert(const v8::FunctionCallbackInfo<v8::Value>& args);
+ static void SetAllowPartialTrustChain(
+ const v8::FunctionCallbackInfo<v8::Value>& args);
static void AddCRL(const v8::FunctionCallbackInfo<v8::Value>& args);
static void AddRootCerts(const v8::FunctionCallbackInfo<v8::Value>& args);
static void SetCipherSuites(const v8::FunctionCallbackInfo<v8::Value>& args);
@@ -143,6 +148,8 @@ class SecureContext final : public BaseObject {
SSLCtxPointer ctx_;
X509Pointer cert_;
X509Pointer issuer_;
+ // Non-owning cache for SSL_CTX_get_cert_store(ctx_.get())
+ X509_STORE* own_cert_store_cache_ = nullptr;
#ifndef OPENSSL_NO_ENGINE
bool client_cert_engine_provided_ = false;
EnginePointer private_key_engine_;

0 comments on commit 5e27c4f

Please sign in to comment.