Fix incorrect extension handling

[mbwhite]

@yorkie Sorry about the previous PR - one of the fixes for handling extension in the certificate was missing.
Corrected that, and added an additional example to the test file.

/cc @sstone1

[mbwhite]

Further investigation has confirmed that though the original fix does build with Node 10 (and other versions), there is a problem with the handling of the strings within a certificate that has extensions.

Specifically

{ version: 2,
  subject: { commonName: 'MyTestUserWithAttrs' },
  issuer: { commonName: 'fabric-ca-server' },
  serial: '1E4998E9F44FD00353BF3681C0A0A431964F5275',
  notBefore: 2017-09-08T03:42:00.000Z,
  notAfter: 2018-09-08T03:42:00.000Z,
  subjectHash: '9cd7895b',
  signatureAlgorithm: 'ecdsa-with-SHA256',
  fingerPrint: 'B6:0B:37:7C:DF:D2:7A:08:0B:98:BF:52:A4:2C:DC:4E:CC:70:91:E1',
  publicKey: { algorithm: 'id-ecPublicKey' },
  altNames: [ 'Anils-MacBook-Pro.local' ],
  extensions: 
   { keyUsage: 'Certificate Sign',
     basicConstraints: 'CA:FALSE',
     subjectKeyIdentifier: 'D8:28:B4:C0:BC:92:4A:D3:C3:8C:54:6C:08:86:33:10:A6:8D:83:AE',
     authorityKeyIdentifier: 'keyid:C4:B3:FE:76:0D:E2:DE:3C:FC:75:FB:AE:55:86:04:F0:BB:7F:F6:01',
     subjectAlternativeName: 'DNS:Anils-MacBook-Pro.local',
     '1.2.3.4.5.6.7.8.1': '\u0004\u001a{"attrs":{"attr1":"val1"}}' } }

The additional \u0004\u001a appear; I've correct this back in this PR for node8 (not node 10)

[Southern]

Okay. What we're not going to do is ruin a stable project with PRs that are missing things. Which PR are you referring to having missing extension handling? That PR may need to be reverted if there is something missing that should be there.

Also, I am not seeing any test returning this on Travis. This needs to be added as a test certificate if it is truly an issue in this module, as we do not want a regression in the future.

This also looks like a locally generated certificate. I would be interested to know what the command looked like to generate this certificate, as it could have been something in the command that generated this specific certificate.

[Southern]

Also, I find it weird that this is the ONLY extension that seems to be returning these extra unicode characters. That leads me to suspect that it is indeed something that happened during the generation of this specific certificate. I would expect it to be an issue with more than just this one attributes extension if there was indeed something wrong with the parsing of extensions.

[mbwhite]

@Southern @yorkie thank you for you responses; please be assured that I have no wish at all to affect that stability of anything. As a maintainer myself I understand the challenges.

The PR for Node10 support had been open several months; this was getting increasing a point of concern as Node 10 was now LTS. So with PR 74, I was attempting to address the concerns of the previous PR (specifically code formatting) that had prevented the it being merged.

When this updated module ran within out systems tests, it was discovered that for x certificate with extensions, two extra characters appeared - as cited above.

This to our eyes was a regression so therefore had to be reverted irrespective of the effect on Node10.
In continuing to review the exact problem I've since added to this PR

• the certificate that causes the issue to the directory with other certs
• corrected the problem that meant this string wasn't being parsed correctly

For our cert this is successful and the existing certs it also succeeds per test results.

In researching more about the parsing of these strings; it appears a complex area . Rather than there being anything wrong with the libraries basic structure, the incorrect choice of API was used I believe; this has been adjusted

/cc @sstone1

[Southern]

As mentioned above, I would be interested in seeing what the command was that generated this certificate. This looks like it could have simply been someone hitting CTRL+<some random key> on their keyboard while typing out the command and did not realize that they had done it, as that would input 2 extra characters.

We would also need copies of these certificates that you claim are an issue. The only certificate that I see having an issue is one generated by an Anil, and that could be due the issue just mentioned. If there are other certificates causing the same issue, I would be interested in inspecting those as well. Extra characters within extensions have not been an issue to my knowledge, and I would like to test this on my side to ensure that it actually is an issue with this module and not something that is a user error.

I am not looking for reasons this should be merged. I will be testing this myself, before it is merged, since there has already been one pull request that is apparently missing something or even incorrect, as the title of this PR says. I will also be reverting #74 if I do not see an issue with our current certificates that we use for testing when I revert the change locally.

[mbwhite]

I will attempt to find the command that created this certificate - it was in our test suite (which I can point you to on Github https://github.com/hyperledger/fabric-chaincode-node/blob/release-1.4/fabric-shim/test/unit/client-identity.js)

The title of this PR is referring to the fact that I ported the changes in #71 , with changes minimal formatting changes. This tripped over the problem with our certificate.

This code came in with PR 71

    if (!X509V3_EXT_print(ext_bio, ext, 0, 0)) {
      unsigned char *buf = NULL;
      int len = i2d_ASN1_OCTET_STRING(X509_EXTENSION_get_data(ext), &buf);
      if (len >= 0) {
          BIO_write(ext_bio, static_cast<const void *>(buf), len);
      }

That I have now replaced with

        ASN1_STRING_print(ext_bio, X509_EXTENSION_get_data(ext));