Skip to content

Commit 36fb790

Browse files
tniessentniessen
authored
crypto: fix X509Certificate toLegacyObject
PR-URL: #42124 Reviewed-By: Filip Skokan <panva.ip@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
1 parent eacd456 commit 36fb790

File tree

6 files changed

+61
-57
lines changed

6 files changed

+61
-57
lines changed

‎lib/internal/crypto/x509.js‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -56,6 +56,8 @@ const {
5656
kHandle,
5757
} = require('internal/crypto/util');
5858

59+
let lazyTranslatePeerCertificate;
60+
5961
const kInternalState = Symbol('kInternalState');
6062

6163
function isX509Certificate(value) {
@@ -345,7 +347,11 @@ class X509Certificate extends JSTransferable {
345347
}
346348

347349
toLegacyObject() {
348-
return this[kHandle].toLegacy();
350+
// TODO(tniessen): do not depend on translatePeerCertificate here, return
351+
// the correct legacy representation from the binding
352+
lazyTranslatePeerCertificate ??=
353+
require('_tls_common').translatePeerCertificate;
354+
return lazyTranslatePeerCertificate(this[kHandle].toLegacy());
349355
}
350356
}
351357

‎src/crypto/crypto_common.cc‎

Lines changed: 9 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -1284,43 +1284,23 @@ MaybeLocal<Value> GetPeerCert(
12841284

12851285
MaybeLocal<Object> X509ToObject(
12861286
Environment* env,
1287-
X509* cert,
1288-
bool names_as_string) {
1287+
X509* cert) {
12891288
EscapableHandleScope scope(env->isolate());
12901289
Local<Context> context = env->context();
12911290
Local<Object> info = Object::New(env->isolate());
12921291

12931292
BIOPointer bio(BIO_new(BIO_s_mem()));
12941293
CHECK(bio);
12951294

1296-
if (names_as_string) {
1297-
// TODO(tniessen): this branch should not have to exist. It is only here
1298-
// because toLegacyObject() does not actually return a legacy object, and
1299-
// instead represents subject and issuer as strings.
1300-
if (!Set<Value>(context,
1301-
info,
1302-
env->subject_string(),
1303-
GetSubject(env, bio, cert)) ||
1304-
!Set<Value>(context,
1305-
info,
1306-
env->issuer_string(),
1307-
GetIssuerString(env, bio, cert))) {
1308-
return MaybeLocal<Object>();
1309-
}
1310-
} else {
1311-
if (!Set<Value>(context,
1312-
info,
1313-
env->subject_string(),
1314-
GetX509NameObject<X509_get_subject_name>(env, cert)) ||
1315-
!Set<Value>(context,
1316-
info,
1317-
env->issuer_string(),
1318-
GetX509NameObject<X509_get_issuer_name>(env, cert))) {
1319-
return MaybeLocal<Object>();
1320-
}
1321-
}
1322-
13231295
if (!Set<Value>(context,
1296+
info,
1297+
env->subject_string(),
1298+
GetX509NameObject<X509_get_subject_name>(env, cert)) ||
1299+
!Set<Value>(context,
1300+
info,
1301+
env->issuer_string(),
1302+
GetX509NameObject<X509_get_issuer_name>(env, cert)) ||
1303+
!Set<Value>(context,
13241304
info,
13251305
env->subjectaltname_string(),
13261306
GetSubjectAltNameString(env, bio, cert)) ||

‎src/crypto/crypto_common.h‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -109,8 +109,7 @@ v8::MaybeLocal<v8::Object> ECPointToBuffer(
109109

110110
v8::MaybeLocal<v8::Object> X509ToObject(
111111
Environment* env,
112-
X509* cert,
113-
bool names_as_string = false);
112+
X509* cert);
114113

115114
v8::MaybeLocal<v8::Value> GetValidTo(
116115
Environment* env,

‎src/crypto/crypto_x509.cc‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -477,7 +477,7 @@ void X509Certificate::ToLegacy(const FunctionCallbackInfo<Value>& args) {
477477
X509Certificate* cert;
478478
ASSIGN_OR_RETURN_UNWRAP(&cert, args.Holder());
479479
Local<Value> ret;
480-
if (X509ToObject(env, cert->get(), true).ToLocal(&ret))
480+
if (X509ToObject(env, cert->get()).ToLocal(&ret))
481481
args.GetReturnValue().Set(ret);
482482
}
483483

‎test/parallel/test-crypto-x509.js‎

Lines changed: 25 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -198,27 +198,28 @@ const der = Buffer.from(
198198

199199
// Verify that legacy encoding works
200200
const legacyObjectCheck = {
201-
subject: 'C=US\n' +
202-
'ST=CA\n' +
203-
'L=SF\n' +
204-
'O=Joyent\n' +
205-
'OU=Node.js\n' +
206-
'CN=agent1\n' +
207-
'emailAddress=ry@tinyclouds.org',
208-
issuer:
209-
'C=US\n' +
210-
'ST=CA\n' +
211-
'L=SF\n' +
212-
'O=Joyent\n' +
213-
'OU=Node.js\n' +
214-
'CN=ca1\n' +
215-
'emailAddress=ry@tinyclouds.org',
216-
infoAccess:
217-
common.hasOpenSSL3 ?
218-
'OCSP - URI:http://ocsp.nodejs.org/\n' +
219-
'CA Issuers - URI:http://ca.nodejs.org/ca.cert' :
220-
'OCSP - URI:http://ocsp.nodejs.org/\n' +
221-
'CA Issuers - URI:http://ca.nodejs.org/ca.cert\n',
201+
subject: Object.assign(Object.create(null), {
202+
C: 'US',
203+
ST: 'CA',
204+
L: 'SF',
205+
O: 'Joyent',
206+
OU: 'Node.js',
207+
CN: 'agent1',
208+
emailAddress: 'ry@tinyclouds.org',
209+
}),
210+
issuer: Object.assign(Object.create(null), {
211+
C: 'US',
212+
ST: 'CA',
213+
L: 'SF',
214+
O: 'Joyent',
215+
OU: 'Node.js',
216+
CN: 'ca1',
217+
emailAddress: 'ry@tinyclouds.org',
218+
}),
219+
infoAccess: Object.assign(Object.create(null), {
220+
'OCSP - URI': ['http://ocsp.nodejs.org/'],
221+
'CA Issuers - URI': ['http://ca.nodejs.org/ca.cert']
222+
}),
222223
modulus: 'EF5440701637E28ABB038E5641F828D834C342A9D25EDBB86A2BF' +
223224
'6FBD809CB8E037A98B71708E001242E4DEB54C6164885F599DD87' +
224225
'A23215745955BE20417E33C4D0D1B80C9DA3DE419A2607195D2FB' +
@@ -243,9 +244,9 @@ const der = Buffer.from(
243244
const legacyObject = x509.toLegacyObject();
244245

245246
assert.deepStrictEqual(legacyObject.raw, x509.raw);
246-
assert.strictEqual(legacyObject.subject, legacyObjectCheck.subject);
247-
assert.strictEqual(legacyObject.issuer, legacyObjectCheck.issuer);
248-
assert.strictEqual(legacyObject.infoAccess, legacyObjectCheck.infoAccess);
247+
assert.deepStrictEqual(legacyObject.subject, legacyObjectCheck.subject);
248+
assert.deepStrictEqual(legacyObject.issuer, legacyObjectCheck.issuer);
249+
assert.deepStrictEqual(legacyObject.infoAccess, legacyObjectCheck.infoAccess);
249250
assert.strictEqual(legacyObject.modulus, legacyObjectCheck.modulus);
250251
assert.strictEqual(legacyObject.bits, legacyObjectCheck.bits);
251252
assert.strictEqual(legacyObject.exponent, legacyObjectCheck.exponent);

‎test/parallel/test-x509-escaping.js‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -241,6 +241,15 @@ const { hasOpenSSL3 } = common;
241241
assert.deepStrictEqual(peerCert.infoAccess,
242242
Object.assign(Object.create(null),
243243
expected.legacy));
244+
245+
// toLegacyObject() should also produce the same properties. However,
246+
// the X509Certificate is not aware of the chain, so we need to add
247+
// the circular issuerCertificate reference manually for the assertion
248+
// to be true.
249+
const obj = cert.toLegacyObject();
250+
assert.strictEqual(obj.issuerCertificate, undefined);
251+
obj.issuerCertificate = obj;
252+
assert.deepStrictEqual(peerCert, obj);
244253
},
245254
}, common.mustCall());
246255
}));
@@ -350,6 +359,15 @@ const { hasOpenSSL3 } = common;
350359
// self-signed. Otherwise, OpenSSL would have already rejected the
351360
// certificate while connecting to the TLS server.
352361
assert.deepStrictEqual(peerCert.issuer, expectedObject);
362+
363+
// toLegacyObject() should also produce the same properties. However,
364+
// the X509Certificate is not aware of the chain, so we need to add
365+
// the circular issuerCertificate reference manually for the assertion
366+
// to be true.
367+
const obj = cert.toLegacyObject();
368+
assert.strictEqual(obj.issuerCertificate, undefined);
369+
obj.issuerCertificate = obj;
370+
assert.deepStrictEqual(peerCert, obj);
353371
},
354372
}, common.mustCall());
355373
}));

0 commit comments

Comments
 (0)