src: refactor GetPeerCertificate #17372

danbev · 2017-11-28T14:06:43Z

These commits split the GetPeerCertificate function into smaller function as described in the TODO comment:

// TODO(indutny): Split it into multiple smaller functions

I've mostly left the code as it was and want to trigger CI to pick up any issues.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

Affected core subsystem(s)

src

danbev · 2017-11-28T14:07:12Z

CI: https://ci.nodejs.org/job/node-test-pull-request/11777/

addaleax · 2017-11-30T00:39:02Z

src/node_crypto.cc

+// Put issuer inside the object
+static void AddIssuer(X509** cert,
+                      const STACK_OF(X509)* const peer_certs,
+                      Local<Object>* const info,


Why is this a pointer?

The reason was so that code in GetPeerCertificate depends on the value being pointed to by info be updated. But I realise that info does not have to be a pointer and we only need update it's pointer for this to work. Not sure if that made sense but I'll add a commit and hopefully this will become clearer. Thanks for pointing this out.

addaleax · 2017-11-30T00:40:12Z

src/node_crypto.cc

+        continue;
+
+      Local<Object> ca_info = X509ToObject(env, ca);
+      (*info)->Set(env->issuercert_string(), ca_info);


Feel free to switch this over to the non-deprecated overload of Set() if you like :)

Ah good point, I'll update to use the function taking a context. Thanks

Related: it would be more efficient to move these two lines out of the loop. Likewise in GetLastIssuedCert().

addaleax · 2017-11-30T00:41:46Z

src/node_crypto.cc

+
+static X509* GetLastIssuedCert(X509** cert,
+                               const SSL* const ssl,
+                               Local<Object>* const info,


Similar question here

bnoordhuis · 2017-11-30T10:14:34Z

src/node_crypto.cc

+// Put issuer inside the object
+static void AddIssuer(X509** cert,
+                      const STACK_OF(X509)* const peer_certs,
+                      Local<Object>* const info,


You don't have to pass a pointer since you don't replace the Object, just add properties to it.

Thanks, I've changed this now (locally).

bnoordhuis · 2017-11-30T10:16:50Z

src/node_crypto.cc

@@ -1974,7 +1974,78 @@ static Local<Object> X509ToObject(Environment* env, X509* cert) {
 }


-// TODO(indutny): Split it into multiple smaller functions
+// Put issuer inside the object


Not a hugely useful comment. If you call the function AddIssuerToObject(), it's self-documenting.

I like that, I'll rename the function.

bnoordhuis · 2017-11-30T10:18:20Z

src/node_crypto.cc

+      (*info)->Set(env->issuercert_string(), ca_info);
+      *info = ca_info;
+
+      // NOTE: Intentionally freeing cert that is not used anymore


Since you're here, can you punctuate the comments?

Absolutely.

bnoordhuis · 2017-11-30T10:21:51Z

src/node_crypto.cc

+                      Local<Object>* const info,
+                      Environment* const env) {
+  *cert = sk_X509_delete(peer_certs, 0);
+  while (sk_X509_num(peer_certs) > 0) {


Unrelated observation: I think this could just be for (;;) { - the same check is effectively done at the end of the loop when i == 0.

Nice, I'll take a closer look at this.

bnoordhuis · 2017-11-30T10:26:15Z

src/node_crypto.cc

+      return false;
+  }
+  return true;
+}


It might be nicer (certainly more Obviously Correct) to move the sk_X509_pop_free() on error into this function.

I'll take a look and fix this. Thanks

I've pushed a few commit but I've not addressed this yet. Will revisit this next week. I was wondering if you meant that I add sk_X509_pop_free() to this function in addition to the one in GetPeerCertificate which I think is needed as peer_cert is used after this function?

bnoordhuis · 2017-11-30T10:30:27Z

src/node_crypto.cc

+        continue;
+
+      Local<Object> ca_info = X509ToObject(env, ca);
+      (*info)->Set(env->issuercert_string(), ca_info);


Related: it would be more efficient to move these two lines out of the loop. Likewise in GetLastIssuedCert().

bnoordhuis · 2017-11-30T10:31:19Z

src/node_crypto.cc

-    if (!sk_X509_push(peer_certs, cert))
-      goto done;
+  if (!CloneSSLCerts(&cert, &peer_certs, ssl_certs)) {
+    goto done;
  }


Can you drop the braces for consistency with the surrounding code?

Yeah, I'll remove them

bnoordhuis · 2017-11-30T10:34:32Z

src/node_crypto.cc

-// TODO(indutny): Split it into multiple smaller functions
+// Put issuer inside the object
+static void AddIssuer(X509** cert,
+                      const STACK_OF(X509)* const peer_certs,


peer_certs can't be const because it's modified inside the function.

Thanks, I've changed that now.

danbev · 2017-12-02T13:54:23Z

Related: it would be more efficient to move these two lines out of the loop. Likewise in > GetLastIssuedCert().

I've pushed a few commit but I've not addressed this yet. Will revisit this next week.

danbev · 2017-12-02T13:55:15Z

Rebased CI: https://ci.nodejs.org/job/node-test-pull-request/11852/

danbev · 2017-12-05T08:05:14Z

Local<Object> ca_info = X509ToObject(env, ca);
info->Set(env->issuercert_string(), ca_info);
Related: it would be more efficient to move these two lines out of the loop. Likewise in GetLastIssuedCert().

@bnoordhuis I've been looking into moving these lines out of the loop and ran into an issue. My understanding is that this code is creating a link/chain of #issuerCertificate from the object instance. Something like:

object -> #issuerCertificate -> ca_info -> #issuerCertificate -> ca_info

Without setting the #issuerCertificate in the loop a certificates would be skipped. Sorry if I'm missing something here, I've been looking at this for a while now and think I need some help.

danbev · 2017-12-05T08:06:04Z

CI: https://ci.nodejs.org/job/node-test-pull-request/11881/

addaleax · 2017-12-05T11:55:29Z

src/node_crypto.cc

-    Local<Object> ca_info = X509ToObject(env, ca);
-    info->Set(env->issuercert_string(), ca_info);
-    info = ca_info;
+  // Clone `ssl_certs`, because we are going to destruct it.


nit: destruct → destroy? :)

Perhaps this comment could be remove now. Just reading it again perhaps the method signature should be changed to:

if (CloneSSLCerts(&cert, ssl_certs, &peer_certs)) {

What do you think, would that be clear without the comment?

Yep, self-explanatory now.

bnoordhuis · 2017-12-06T00:38:58Z

src/node_crypto.cc

+        continue;
+
+      Local<Object> ca_info = X509ToObject(env, ca);
+      object->Set(context, env->issuercert_string(), ca_info).ToChecked();


I think we've settled on .FromJust() (232 vs 44 hits for .ToChecked() at the time of writing.)

Thanks, I'll correct these.

bnoordhuis · 2017-12-06T00:46:33Z

src/node_crypto.cc

+      break;
+
+    Local<Object> ca_info = X509ToObject(env, ca);
+    issuer_chain->Set(context, env->issuercert_string(), ca_info).ToChecked();


bnoordhuis · 2017-12-06T00:58:33Z

src/node_crypto.cc

-    // NOTE: Intentionally freeing cert that is not used anymore
-    X509_free(cert);
+    issuer_chain = AddIssuerToObject(result, &cert, peer_certs, env);
+    issuer_chain = GetLastIssuedCert(&cert, w->ssl_, issuer_chain, env);


You're right you can't factor out the .Set() calls because they build up a linked list but that arguably makes AddIssuerToObject() a bit of a misnomer.

Can you try to pass arguments in the same order to both functions?

Perhaps a better name would be AddIssuerChainToObject?

Can you try to pass arguments in the same order to both functions?

Yep, I'll update the order. Thanks

bnoordhuis · 2017-12-06T00:59:40Z

src/node_crypto.cc

-    Local<Object> ca_info = X509ToObject(env, ca);
-    info->Set(env->issuercert_string(), ca_info);
-    info = ca_info;
+  // Clone `ssl_certs`, because we are going to destruct it.


Yep, self-explanatory now.

addaleax · 2017-12-10T02:35:53Z

CI: https://ci.nodejs.org/job/node-test-commit/14720/

danbev · 2017-12-12T07:04:11Z

test/windows-fanned failure looks unrelated

console output:

ERROR: Error fetching remote repo 'jenkins_tmp'
hudson.plugins.git.GitException: Failed to fetch from [email protected]:janeasystems/node_binary_tmp.git
	at hudson.plugins.git.GitSCM.fetchFrom(GitSCM.java:825)
	at hudson.plugins.git.GitSCM.retrieveChanges(GitSCM.java:1092)
	at hudson.plugins.git.GitSCM.checkout(GitSCM.java:1123)
	at hudson.scm.SCM.checkout(SCM.java:495)
	at hudson.model.AbstractProject.checkout(AbstractProject.java:1202)
	at hudson.model.AbstractBuild$AbstractBuildExecution.defaultCheckout(AbstractBuild.java:574)
	at jenkins.scm.SCMCheckoutStrategy.checkout(SCMCheckoutStrategy.java:86)
	at hudson.model.AbstractBuild$AbstractBuildExecution.run(AbstractBuild.java:499)
	at hudson.model.Run.execute(Run.java:1724)
	at hudson.matrix.MatrixRun.run(MatrixRun.java:146)
	at hudson.model.ResourceController.execute(ResourceController.java:97)
	at hudson.model.Executor.run(Executor.java:421)
Caused by: hudson.plugins.git.GitException: Command "git reset --hard" returned status code 128:
stdout: 
stderr: fatal: Unable to create 'c:/workspace/node-test-binary-windows/.git/index.lock': File exists.

danbev · 2017-12-12T07:05:16Z

@bnoordhuis When you get a chance would you be able to take a look at the lastest updates and see what you think?

bnoordhuis

LGTM with two requests.

bnoordhuis · 2017-12-12T09:54:27Z

src/node_crypto.cc

+    }
+  }
+  if (!result) {
+    sk_X509_pop_free(*peer_certs, X509_free);


In a similar vein, can you add *peer_certs = nullptr;?

bnoordhuis · 2017-12-12T09:57:11Z

src/node_crypto.cc

+  }
+  sk_X509_pop_free(peer_certs, X509_free);
+  return object;
+}


Just a suggestion but it would be better if this function didn't mutate and invalidate peer_certs.

Failing that, perhaps pass peer_certs as an indirect pointer and set *peer_certs = nullptr; here?

Sounds good, I'll take a look. Thanks

jasnell

LGTM with @bnoordhuis suggestion

BridgeAR · 2018-01-19T17:15:30Z

@danbev would you be so kind and please update this accordingly to the suggestions? :-)

danbev · 2018-01-20T05:52:25Z

@BridgeAR I hope to revisit this next week (though I've got a sick kid here and I might not be working at all next week by the looks of it)

BridgeAR · 2018-01-31T02:11:05Z

Ping @danbev

BridgeAR · 2018-02-07T03:39:16Z

Ping @danbev

danbev · 2018-02-07T04:57:38Z

@BridgeAR I've just not had time to revisit this yet but intend to.

BridgeAR · 2018-03-02T04:20:09Z

CI https://ci.nodejs.org/job/node-test-pull-request/13433/

BridgeAR · 2018-03-02T04:20:51Z

@bnoordhuis @jasnell @addaleax PTAL

danbev · 2018-03-02T10:59:03Z

I'm setting this to in-progress as I've not hade time to look closer at @bnoordhuis latest suggestions. I hope to make some time for this soon.

PR-URL: nodejs#17372 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: James M Snell <[email protected]>

bnoordhuis · 2018-03-02T16:02:45Z

#19087 - had 30 minutes to kill. :-)

danbev · 2018-03-03T07:40:57Z

Closing in favour of #19087

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem. labels Nov 28, 2017

addaleax approved these changes Nov 30, 2017

View reviewed changes

bnoordhuis reviewed Nov 30, 2017

View reviewed changes

danbev force-pushed the refactor_getpeercertificate branch from 7042685 to 476e116 Compare December 2, 2017 13:50

addaleax reviewed Dec 5, 2017

View reviewed changes

bnoordhuis reviewed Dec 6, 2017

View reviewed changes

MylesBorins force-pushed the master branch from b7405ab to 7f086dd Compare December 8, 2017 16:37

bnoordhuis approved these changes Dec 12, 2017

View reviewed changes

jasnell approved these changes Dec 12, 2017

View reviewed changes

maclover7 added the wip Issues and PRs that are still a work in progress. label Dec 19, 2017

maclover7 force-pushed the master branch from bb5575a to 993b716 Compare January 26, 2018 22:02

cjihrig force-pushed the master branch from 993b716 to 082f952 Compare January 26, 2018 22:36

src: refactor GetPeerCertificate

d00841b

danbev force-pushed the refactor_getpeercertificate branch from 9806458 to d00841b Compare February 22, 2018 06:12

BridgeAR added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed wip Issues and PRs that are still a work in progress. labels Mar 2, 2018

danbev added the wip Issues and PRs that are still a work in progress. label Mar 2, 2018

bnoordhuis mentioned this pull request Mar 2, 2018

src: use std::unique_ptr for STACK_OF(X509) #19087

Closed

danbev closed this Mar 3, 2018

danbev deleted the refactor_getpeercertificate branch March 6, 2018 11:48

src: refactor GetPeerCertificate #17372

src: refactor GetPeerCertificate #17372

Conversation

danbev commented Nov 28, 2017

Checklist

Affected core subsystem(s)

danbev commented Nov 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danbev commented Dec 2, 2017

danbev commented Dec 2, 2017

danbev commented Dec 5, 2017

danbev commented Dec 5, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danbev Dec 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

addaleax commented Dec 10, 2017

danbev commented Dec 12, 2017

danbev commented Dec 12, 2017

bnoordhuis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasnell left a comment

Choose a reason for hiding this comment

BridgeAR commented Jan 19, 2018

danbev commented Jan 20, 2018

BridgeAR commented Jan 31, 2018

BridgeAR commented Feb 7, 2018

danbev commented Feb 7, 2018

BridgeAR commented Mar 2, 2018

BridgeAR commented Mar 2, 2018

danbev commented Mar 2, 2018

bnoordhuis commented Mar 2, 2018

danbev commented Mar 3, 2018

danbev Dec 6, 2017 •

edited

Loading