Perhaps I should skip CIs at current stage...

Note that CIFuzz found a bug in this Pull request. It was not reported because CIfuzz was in dry_mode.

Note that CIFuzz found a bug in this Pull request. It was not reported because CIfuzz was in dry_mode.

Thanks and this PR is still not finished let's see if that exist after more commits are pushed.

how to pass values associated with the EVP_PKEY_CTX to the provider? (the SM2 identifier)

Since PR #11302 has been merged so I don't think this is a problem anymore

@InfoHunter said this in #11302 (comment):

Since PR #11302 has been merged so I don't think this is a problem anymore

I had a re-thought on this. It seems current design doesn't allow passing the dist-ID into a provider, so the user can only set the dist-ID in advance and then the calculation of the dist-ID is computed in the update process inside a provider. Is this what you originally intended? @levitte

Not sure I understand the issue. The SM2 ID is set with EVP_PKEY_CTX_set1_id(), and can be used after EVP_DigestSignInit() since #11302 was merged in. That's the intention. It mist still, however, be used before EVP_DigestSignUpdate() and EVP_DigestSignFinal() is called. Does that make sense? Have you found a place where this doesn't quite work out?

Well, the calling sequence you mentioned requires the caller to set the ID by using EVP_PKEY_CTX_set1_id(), and currently if the ID is not set then the call will fail. I was wondering if some implementations (provided by providers) of SM2 would like to handle the ID set by the user inside the provider - for instance to adjust the value of the ID. I am not pretty sure about this use case, but just found that the ID is transparent to the provider since it's calculated outside. Should this calculation be part of a provider implementation? It seems there is no way to intercept the EVP_PKEY_ctrl or custom_digest stuffs in a provider, right?

You're right that EVP_PKEY_CTX_set1_id() currently only works with legacy implementations. To work with providers, it needs to be rewritten in the same way that, for example, EVP_PKEY_CTX_set_ecdh_cofactor_mode() was rewritten.

The location for the EC control reimplementations is crypto/ec/ec_evp_lib.c

Actually I have some new ideas about the use case of handling the ID stuffs. It might be possible that some day in the future the compliance requirement changes to some new policies, say, the ID is required to less than 4 bytes..., so one can implement a 4-byte-ID-SM2 provider to comply with it... ;-)

But this is low priority - need to get this PR done first

Travis-CI failure is not relevant (timeout), considering to remove the WIP prologue

@mattcaswell , for the SM3 considerations, I found in the legacy part there is not such limit, so do we need to limit that in default provider or should we just make it compatible with what legacy part does (OpenSSL 1.1.1)

The WIP is removed from this PR.

Ping?

What is the arrangement for this PR? Any dependencies yet?

Ping @openssl/otc

Should this be included in any alpha version? maybe the next alpha-2?

This ought to be included at some point. It isn't essential for the alpha releases but it should be done before the first beta.

I do not expect that any of the core team will have time to do this, we're relying on you or a Chinese compatriot to convert it into a provider based solution. The conversion isn't difficult, but it could be a bit tedious.

@InfoHunter can you rebase this on latest master? A lot of things changed since the last push (sorry I did not find the time to review this sooner) so it would be better to test this against latest master.

I believe it doesn't make much sense to include this without the corresponding keymgmt part

Okay, no problem, I would probably find some time later next week to handle this.

• SM2 keygen is different (different ranges)

Okie, that's an argument.

• it should be possible to run ECDH/DSA over the so-called SM2 curve
• it should be possible to rune SM2DH/SM2DSA over any curve

Sure, but those signature operations, not keymgmt.

I believe there is no way around having 2 separate keymgmt given these conditions!

Only the keygen concern seems relevant for having a separate SM2 key type.

It seems there is no special treatment on the SM2 keygen in OpenSSL 1.1.1. It should just follow the standard EC_GFp_mont_method->keygen procedures.

Are you suggesting that we should fix this in provider implementation of SM2?

Note that the legacy code for SM2 has changed in master, to avoid the dumber quirks that are still present in 1.1.1

• SM2 keygen is different (different ranges)

Okie, that's an argument.

• it should be possible to run ECDH/DSA over the so-called SM2 curve
• it should be possible to rune SM2DH/SM2DSA over any curve

Sure, but those signature operations, not keymgmt.

I believe there is no way around having 2 separate keymgmt given these conditions!

Only the keygen concern seems relevant for having a separate SM2 key type.

@levitte they are still valid arguments because of this

The keymgmt says what are the operations names, so SM2 cannot reuse the EC keymgmt and then USE different operations!

SM2 keygen is different (different ranges)

It seems there is no special treatment on the SM2 keygen in OpenSSL 1.1.1. It should just follow the standard EC_GFp_mont_method->keygen procedures.

Are you suggesting that we should fix this in provider implementation of SM2?

We have an open issue regarding the defect in the 1.1.1 implementation: #8435 (comment)

The reason why it wasn't fixed yet is exactly because of how much in 1.1.1 SM2 and ec have been tied together due to the internal architecture and serialization/deserialization issues.
In 3.0.0 we should aim for a saner implementation where the 2 cryptosystems are handled distinctly and code duplication is avoided in smarter ways!

The keymgmt says what are the operations names, so SM2 cannot reuse the EC keymgmt and then USE different operations!

We might have to re-think the returned value, for example make that a NULL-terminated array of string pointers instead of just one string. That would allow us to return { "ECDSA", "SM2", NULL } for possible signature operations. To be checked against a fetch EVP_SIGNATURE with EVP_SIGNATURE_is_a... something like that.

In 3.0.0 we should aim for a saner implementation where the 2 cryptosystems are handled distinctly and code duplication is avoided in smarter ways!

We can only do so much! If the keys in PKCS#8 form don't have any indication other than the SM2 curve, we only have an educated guess, and in doing so, we would remove any possibility to run ECDSA with the SM2 curve or SM2 signatures with other curves.

Deserialization is an issue bacuase the standardization of SM2 made them indistinguishable, true.

Somewhere in the deserialization step we probably will need to keep defaulting to "create an SM2 key if this is serialized as an EC key over the so-called-SM2-curve".

Yet it should be possible from an application that can sidestep or implement its own deserialization stage, to ask a provider to generate an EC key on the SM2 curve, and viceversa to generate an SM2 key on this nist curve.

I don't see huge benefits in unifying again the SM2 and ec keymgmt, adding the complexity of multiple alternative options for the operations that a given key object supports.

Yet it should be possible from an application that can sidestep or implement its own deserialization stage, to ask a provider to generate an EC key on the SM2 curve, and viceversa to generate an SM2 key on this nist curve.

... and then it gets saved to file, and the next time it's read from that same file, it's suddenly potentially a different key type. If that isn't a source of confusion, I don't know what is.

Yet it should be possible from an application that can sidestep or implement its own deserialization stage, to ask a provider to generate an EC key on the SM2 curve, and viceversa to generate an SM2 key on this nist curve.

... and then it gets saved to file, and the next time it's read from that same file, it's suddenly potentially a different key type. If that isn't a source of confusion, I don't know what is.

Well, I don't think the library can solve a confusion that is embedded in the result of the standardization process.

Even having a single keymgmt for SM2 and EC will cause confusion if a user checks out the saved file: it is indistinguishable from an EC key on the SM2 curve.
Looking from this perspective, it is already confusing for a user in 1.1.1 to see that a key that looks like an EC key on disk does not compute ECDH or ECDSA when used for derive/sign!

This is my personal opinion (and I am biased because it's my proposal), but I think that:

• an EC keymgmt, with its own keygen, that is associated with ECDH/ECDSA operations
• a distinct SM2 keymgmt, with its own different keygen, that is associated with SM2DH/SM2DSA operations
• an EC deserializer, that defaults to calling the EC keymgmt, unless the so-called-SM2-curve OID is detected for which SM2 keymgmt is used instead

is going to be cleaner than

• a single keymgmt that does
  • EC keygen or SM2 keygen (*)
  • ECDH or SM2DH (*)
  • ECDSA or SM2DSA (*)
• an EC deserializer, that sets the internal flags that choose among EC operations or SM2 operations depending on detecting the so-called-SM2-curve OID.

(*): depending on something internal and difficult to reach or override

Yes, in both cases there will still be the issue of the confusion caused by the indistinguishability of the serialized format, but we can't really escape it until the SM2 spec is revised (and probably not even then, to retain compatibility with 1.1.1), but at least we avoid creating an overloaded key object standing for 2 different cryptosystems (keys are generated in different ways, they are associated with different operations) that only share (unluckily) the serialization format.

Maybe @bbbrumley can share his thoughts about this as well, given that he has been struggling with trying to fix #8435 in 1.1.1 and can have even more insights on what could be done better at this time.

I don't see huge benefits in unifying again the SM2 and ec keymgmt, adding the complexity of multiple alternative options for the operations that a given key object supports.

One the advantages could be less code duplication. According to the discussions above, It seems the only difference we need to take care is the key generation process in the keymgmt scope.

One the advantages could be less code duplication. According to the discussions above, It seems the only difference we need to take care is the key generation process in the keymgmt scope.

Why do you need to duplicate code? You could set a flag of some sort in the keydata (like we now do for the RSA sub-types), all that's needed then is a different constructor, and then pay attention to that flag in the few places where that's needed. The only thing that would be duplicated is the OSSL_DISPATCH array. I would much rather than a lot of code copying.

This is just my opinion as some random guy on the Internet.

We might have to re-think the returned value, for example make that a NULL-terminated array of string pointers instead of just one string. That would allow us to return { "ECDSA", "SM2", NULL } for possible signature operations

@levitte I feel like this is similar to the line of thinking that led to the 111 mess with SM2. "Well let's just piggy back off this -- it's similar enough" and turns out it wasn't, right?

and then it gets saved to file, and the next time it's read from that same file, it's suddenly potentially a different key type. If that isn't a source of confusion, I don't know what is.

I agree! And what you're doing in master is much better than the 111 state.

This is my personal opinion (and I am biased because it's my proposal), but I think that

@romen I agree with this proposal.

Even if it means duplicate code here and there, keep them as separate as possible.

The way I look at it is this: what if SM2 had it's own OID that wasn't under the legacy EC hierarchy? Yes that is a standardization mistake, but put that aside. Step into fantasy land for a minute.

• Would you still be trying to tie in SM2 to the EC module at any level?
• Including key management?
• Does OpenSSL do that for ed25519? I can write that curve as a legacy curve, too ...

Ignore the standards mistake, and ask yourself these questions. I think you'll find the right decision for OpenSSL then.

This is just my opinion as some random guy on the Internet.

Thanks for the chuckle in these trying times!

I just pushed a new commit containing the separated SM2 key management skeleton (and it's only a skeleton so please consider this as WIP at current stage). The current implementation is just a copy of the EC key management. According to what Nicola proposed in the emails during the weekend, I need to replace the content with SM2 implementation. We can have a discussion on this on the upcoming online meeting on Tuesday ;-)

We could house the SM2 keymgmt in providers/implementations/keymgmt/ec_kmgmt.c and do something similar to the RSA and RSA-PSS keymgmt:

Most pointers in the dispatch table are identical, and only the ones that need to differ are actually different. This way we minimize duplication.

At a first examination, I think the SM2 keymgmt should differ from the EC one in the following:

• OSSL_FUNC_KEYMGMT_GEN (it should be a separate function, very similar to the EC one, but internally instead of EC_KEY_generate_key(ec); we should call a new internal function that behaves like it, but uses the proper range for picking the secret scalar)
• OSSL_FUNC_KEYMGMT_QUERY_OPERATION_NAME (pointing to the SM2 specific operations like you have in the current version of this PR)
• OSSL_FUNC_KEYMGMT_GET_PARAMS

  • OSSL_PKEY_PARAM_SECURITY_BITS might be defined differently for the SM2 curve?
    

    openssl/providers/implementations/keymgmt/ec_kmgmt.c

    Lines 484 to 520 in 7e4f01d

    	if ((p = OSSL_PARAM_locate(params, OSSL_PKEY_PARAM_SECURITY_BITS)) != NULL) {
    	int ecbits, sec_bits;
    	ecbits = EC_GROUP_order_bits(ecg);
    	/*
    	* The following estimates are based on the values published
    	* in Table 2 of "NIST Special Publication 800-57 Part 1 Revision 4"
    	* at http://dx.doi.org/10.6028/NIST.SP.800-57pt1r4 .
    	*
    	* Note that the above reference explicitly categorizes algorithms in a
    	* discrete set of values {80, 112, 128, 192, 256}, and that it is
    	* relevant only for NIST approved Elliptic Curves, while OpenSSL
    	* applies the same logic also to other curves.
    	*
    	* Classifications produced by other standardazing bodies might differ,
    	* so the results provided for "bits of security" by this provider are
    	* to be considered merely indicative, and it is the users'
    	* responsibility to compare these values against the normative
    	* references that may be relevant for their intent and purposes.
    	*/
    	if (ecbits >= 512)
    	sec_bits = 256;
    	else if (ecbits >= 384)
    	sec_bits = 192;
    	else if (ecbits >= 256)
    	sec_bits = 128;
    	else if (ecbits >= 224)
    	sec_bits = 112;
    	else if (ecbits >= 160)
    	sec_bits = 80;
    	else
    	sec_bits = ecbits / 2;
    	if (!OSSL_PARAM_set_int(p, sec_bits))
    	return 0;
    	}

  • OSSL_PKEY_PARAM_USE_COFACTOR_ECDH, OSSL_PKEY_PARAM_TLS_ENCODED_PT shouldn't be supported

• OSSL_FUNC_KEYMGMT_GETTABLE_PARAMS, OSSL_FUNC_KEYMGMT_SET_PARAMS, OSSL_FUNC_KEYMGMT_SETTABLE_PARAMS should be similarly revised (seems like maybe there are no settable params leftover, but maybe there is the opportunity to add the SM2-specific USER_ID personalization string as a parameter here!)
• OSSL_FUNC_KEYMGMT_IMPORT, this could probably point to a wrapper around the existing internal ec_import function. It's only goal would be to filter import calls, so that the SM2 keymgmt only accepts keys/params that are on the so-called "SM2 curve" and fail otherwise.

I am going to draw back the latest commits containing the keymgmt code in this PR and open a new PR to address this.

At a first examination, I think the SM2 keymgmt should differ from the EC one in the following:
OSSL_FUNC_KEYMGMT_GEN (it should be a separate function, very similar to the EC one, but internally instead of EC_KEY_generate_key(ec); we should call a new internal function that behaves like it, but uses the proper range for picking the secret scalar)

And SM2 needn't to provide the ability of specifying curves, did we have a consensus on this yesterday?

@InfoHunter

And SM2 needn't to provide the ability of specifying curves, did we have a consensus on this yesterday?

Regarding this I think the best course of action (and minimal work) is to leave the EC flexibility underneath, but filter so that if the curve id does not match import fails.

That way when another supported curve is added to part 5 of the SM2 standards we just have to add one more entry to the allowlist and now we don't need to refractor extra code in the keymgmt.

but maybe there is the opportunity to add the SM2-specific USER_ID personalization string as a parameter here!

The SM2 user ID seems not a param with the key itself, this ID is involved in the SM2 signature computation progress which has been addressed by @levitte months ago IIRC. So it seems we don't need to consider supporting this in the key management part of SM2

As per #12536 (comment) and #12536 (comment)

This PR is going to moved into PR #12536 , so this one can be closed now.