GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — crypto: add support for SM2 by tniessen · Pull Request #37066 · nodejs/node

Conversation

@tniessen

@tniessen tniessen commented Jan 25, 2021

Copy link
Copy Markdown
Member

These commits add support for SM2 from the SM9 standard. It requires a fair amount of hacks within our codebase, so if anyone has any other integration ideas, I'd be happy to hear them.

Asymmetric Key Type 'sm2'

A new asymmetric key type is added: 'sm2'.

Unlike previous asymmetric key types, it does not have a unique OID, and does, in fact, share an OID with 'ec', meaning that there is no longer a 1:1 correlation between OIDs and asymmetric key types.

Key Pair Generation

SM2 key pairs can be generated using crypto.generateKeyPair('sm2').

No options are accepted.

Digital Signatures

SM2 signatures require an additional "identifier" that must be specified by the signing and the verifying party. To accomodate this, an sm2Identifier option was added to crypto.sign and crypto.verify.

There is no documented way of producing SM2 signatures with the OpenSSL APIs that we are using in the Sign and Verify classes. Therefore, these classes currently do not support SM2 signatures.

Key Agreement

Currently not implemented due to lack of support within OpenSSL. Might also be insecure in its original form, see Xu et al. doi:10.1007/978-3-642-25513-7_12:

In this paper, we have shown that the SM2 key exchange protocol is potentially vulnerable to an unknown key-share attack. The weakness is due to the fact that the identifiers of the communicants are not appropriately integrated into the exchanged cryptographic messages. However, the attack presented here should not discourage use of the SM2 protocol, as long as appropriate countermeasures are taken. We also have suggested a simple modification to resist our described attacks while the merits of the original protocol are left unchanged.

Should be added to crypto.diffieHellman() if it makes sense in the future.

Key Import

Because the PKCS#8 and SPKI encodings of SM2 keys are indistinguishable from EC keys on the SM2 curve, the createPublicKey and createPrivateKey functions now accept an additional asymmetricKeyType option. If specified, Node.js will attempt to create a KeyObject of the given type. As a side effect, this allows users to enforce a specific asymmetric key type when importing key material, e.g., 'rsa'. For SM2, this means that the key will first be parsed as EC, and its type will then be changed to SM2 if the user specified asymmetricKeyType: 'sm2'.

The default behavior matches OpenSSL: When loading a key with OID 1.2.840.10045.2.1 on the SM2 curve without specifying asymmetricKeyType: 'sm2', it will create an EC key, not an SM2 key.

Note on SM3

While OpenSSL 1.1.1 supports SM3 as a hash function and as the digest for SM2 signatures, it does not allow SM3 as the digest for other signatures. Future releases might change that.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
tniessen added 3 commits January 25, 2021 18:35
@tniessen
crypto: add 'sm2' key type and key pair generation
70988ea
@tniessen
crypto: add support for SM2 one-shot signatures
4282617
@tniessen
crypto: allow adjusting key type on import
a52008c
@nodejs-github-bot nodejs-github-bot added the lib / src Issues and PRs involving general changes in the lib/ or src/ directories. label Jan 25, 2021
@tniessen tniessen added crypto Issues and PRs related to the crypto subsystem. notable-change PRs with changes that should be highlighted in changelogs. semver-minor PRs that contain new features and should be released in the next minor version. c++ Issues and PRs that require attention from people who are familiar with C++. and removed lib / src Issues and PRs involving general changes in the lib/ or src/ directories. labels Jan 25, 2021
@tniessen

tniessen commented Jan 25, 2021

Copy link
Copy Markdown
Member Author

CI failures are expected due to #37055.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@tniessen
test: ensure EC keys on the SM2 curve work
a76c7e4
jasnell
jasnell reviewed Jan 25, 2021
View reviewed changes
Comment thread doc/api/crypto.md
required only if the `format` is `'der'` and ignored if it is `'pem'`.
* `passphrase`: {string | Buffer} The passphrase to use for decryption.
* `encoding`: {string} The string encoding to use when `key` is a string.
* `asymmetricKeyType` {string} The requested asymmetric key type.

@jasnell jasnell Jan 25, 2021 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be good to document the expected values here (or link to where they are documented)

(I know there's a paragraph added below but listing the values here would be helpful)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

tniessen reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
jasnell
jasnell reviewed Jan 25, 2021
View reviewed changes
Comment thread doc/api/crypto.md
`crypto.constants.RSA_PSS_SALTLEN_DIGEST` sets the salt length to the digest
size, `crypto.constants.RSA_PSS_SALTLEN_MAX_SIGN` (default) sets it to the
maximum permissible value.
* `sm2Identifier` {ArrayBuffer|Buffer|TypedArray|DataView} For SM2, this option

@jasnell jasnell Jan 25, 2021

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer to make this a more generic name, e.g. just identifier perhaps.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
jasnell
jasnell reviewed Jan 25, 2021
View reviewed changes
Comment thread lib/internal/crypto/keys.js
else if (typeStr === 'x448')
asymmetricKeyType = EVP_PKEY_X448;
else
throw new ERR_INVALID_ARG_VALUE('options.asymmetricKeyType', typeStr);

@jasnell jasnell Jan 25, 2021

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A switch statement would be nicer here from a code readability point of view.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

tniessen reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

@Mifrill Mifrill Nov 17, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest key-map object instead:

const keyTypeMap = {
  'dh': EVP_PKEY_DH,
  'dsa': EVP_PKEY_DSA,
  'ec': EVP_PKEY_EC,
  'ed25519': EVP_PKEY_ED25519,
  'ed448': EVP_PKEY_ED448,
  'rsa': EVP_PKEY_RSA,
  'rsa-pss': EVP_PKEY_RSA_PSS,
  'sm2': EVP_PKEY_SM2,
  'x25519': EVP_PKEY_X25519,
  'x448': EVP_PKEY_X448,
  null: undefined // Map null to undefined
};
const asymmetricKeyType = keyTypeMap[typeStr];
if (asymmetricKeyType === undefined) { // if asymmetricKeyType null or not in keyTypeMap
  throw new ERR_INVALID_ARG_VALUE('options.asymmetricKeyType', typeStr);
}

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@mscdex

mscdex commented Jan 25, 2021

Copy link
Copy Markdown
Contributor

Where is SM2 used?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@tniessen tniessen mentioned this pull request Jan 26, 2021
Supported asymmetric key types #26996
Closed
9 of 16 tasks
@tniessen

tniessen commented Jan 26, 2021

Copy link
Copy Markdown
Member Author

Where is SM2 used?

Not sure. I am trying to address the remaining key types from #26996.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@jasnell

jasnell commented Jan 26, 2021

Copy link
Copy Markdown
Member

As far as I know, SM2 is primarily used in China (it was developed by a Chinese agency). There's not a lot of literature available on it or where exactly it is used, if at all, outside of that region. It seems specialized enough that I'm not really convinced that there's justification for exposing it generally in core.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@mscdex

mscdex commented Jan 26, 2021

Copy link
Copy Markdown
Contributor

I would vote to just leave it out for now and if there is a pressing need to add it in the future we can revisit it then, especially considering it requires some hacks to add support for it.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@tniessen tniessen mentioned this pull request Feb 5, 2021
crypto: introduce crypto/promises #37218
Closed
8 tasks
@guybedford
guybedford force-pushed the master branch from dc5a5da to 8e46568 Compare March 29, 2021 21:33
This was referenced Oct 6, 2021
Replace OpenSSL 1.1.1 with OpenSSL 3.0.0 #40106
Closed
deps: update openssl to quictls/openssl 3.0.0+quic #38512
Closed
@tniessen

tniessen commented Oct 26, 2021

Copy link
Copy Markdown
Member Author

@nodejs/crypto @jasnell @mscdex The switch to OpenSSL 3 has changed how Node.js treats EC keys on the SM2 curve, see #40106 (comment) and #38512 (comment).

While OpenSSL now defaults to SM2 for such keys, I understand that there is no interest in this feature from this project.

Given the status quo, which allows importing keys that Node.js does not support, I see the following options:

  1. Assign the 'sm2' type to the EVP_PKEY_SM2 key type so that asymmetricKeyType would at least not be undefined for SM2 keys.
    • Using the keys would still be difficult/impossible.
    • Key generation is also inconsistent, in that generating an EC key on the SM2 curve yields an EC key, but importing the key yields an SM2 key.
  2. Restore the pre-17 behavior and convert all EVP_PKEY_SM2 keys into EVP_PKEY_EC keys upon import.
    • I am not entirely sure how to do this, but it's likely possible.
    • It goes against the decision made by OpenSSL.
  3. Throw upon loading an EC key on the SM2 curve, avoiding any ambiguities.
    • This is possible, but probably semver-major.

I can update this PR with whatever approach is chosen.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@tniessen

tniessen commented Nov 2, 2021

Copy link
Copy Markdown
Member Author

Ping @nodejs/crypto

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@mscdex

mscdex commented Nov 2, 2021

Copy link
Copy Markdown
Contributor

Of the choices, I'd vote for option 3.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@Trott
Trott force-pushed the main branch from 2d76238 to ca3ed36 Compare November 12, 2022 01:49
@tniessen tniessen mentioned this pull request Jul 8, 2024
The signature and verification of SM2 return incorrect results in crypto.createSign and crypto.createVerify #53761
Closed
@github-actions

github-actions Bot commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

This pull request has been marked as stale due to 90 days of inactivity.
It will be automatically closed in 30 days if no further activity occurs. If this is still relevant, please leave a comment or update it to keep it open.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@github-actions github-actions Bot added the stale Issues and PRs marked stale due to inactivity and scheduled for automatic closure. label Jul 28, 2026
@avivkeller avivkeller closed this Aug 9, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnell jasnell jasnell left review comments
+1 more reviewer
@Mifrill Mifrill Mifrill left review comments
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. crypto Issues and PRs related to the crypto subsystem. notable-change PRs with changes that should be highlighted in changelogs. semver-minor PRs that contain new features and should be released in the next minor version. stale Issues and PRs marked stale due to inactivity and scheduled for automatic closure.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

6 participants

@tniessen @mscdex @jasnell @Mifrill @nodejs-github-bot @avivkeller
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.