WIP: Add e_cng, a new engine that interfaces with MS CNG/Ncrypt and uses STORE by levitte · Pull Request #3481 · openssl/openssl

levitte · 2017-05-16T21:26:31Z

This PR doesn't build on its own for now, it needs to be merged #2688 to build.

Checklist

documentation is added or updated
tests are added or updated

levitte · 2017-05-16T21:26:47Z

@snhenson, you might be interested in this

levitte · 2017-05-16T21:30:04Z

For those interested, I maintain a merge of this and #2688 here, and it gets built on my personal Travis CI

mattcaswell · 2017-05-17T07:48:42Z

Awesome!

levitte · 2017-05-17T08:14:16Z

Maybe time to start reviewing the STORE project for real? (hint hint nudge nudge) ;-)

dot-asm · 2017-05-17T12:18:26Z

Configurations/10-main.conf

It's tricky one. Formally there is no reason to link everything with ncrypt.lib. Well, of course in static build ncrypt.lib needs to be there for every .exe that pulls in e_cng.obj. But in shared build cng.dll would be the only one that needs to be linked with it. MSC has solution for that, and it's #pragma comment(lib,"ncrypt") that can be added directly to e_cng.c. And it's going to work out in both static and dynamic builds. [The pragma is tolerated by mingw compilers, but does nothing.] Another argument against adding it as above is that people still seem to be asking about compiling for legacy version, and there is no ncrypt.lib there. Even in such case above mentioned #pragma would do the trick, because it can be conditional...

I think it can be argued that crypt32.lib shouldn't be there with the argument that only capi.dll needs it as well, no? I was pondering another variant, to add language in build.info to specify extra external libraries to be used with specific programs or shared libraries. Would it be worth it? Maybe. For now, though, I'll use the pragma, thanks for pointing it out.

I think it can be argued that crypt32.lib shouldn't be there

I think so, yes. But there is difference. crypt32.lib is present in legacy SDKs, while ncrypt.lib is not. This is kind of the real concern. Having library on command line you don't link with has no side effects (on Windows), but lib-file needs to be present if you refer to in on command line irregardless. In other words it's availability of ncrypt.lib file in user's SDK that is concern.

dot-asm · 2017-05-17T12:28:03Z

engines/e_cng.c

MSDN says include ncrypt.h and I don't see no ncrypt.h. Well, nothing prevents wincrypt.h from including ncrypt.h, but then question would be if all SDKs do that... Maybe they all do, did you verify?

I have verified that wincrypt.h includes ncrypt.h indeed. The reason I do this is the lines following, where I detect if ncrypt is present or not.

I have verified that wincrypt.h includes ncrypt.h indeed.

Question was if all of them do. But of course one can always say that if wincrypt.h doesn't include ncrypt.h, then we don't compile it. It would be appropriate to mention this in more verbose comment...

dot-asm · 2017-05-17T12:29:48Z

engines/e_cng.c

I'd suggest a little bit more verbose comment...

dot-asm · 2017-05-17T12:40:07Z

engines/e_cng.c

I didn't follow the code thoroughly, but question is if CP_UTF8 would be more appropriate. Underlying question is if in can contain non-ASCII characters. Trouble is that CP_ACP can wipe some of them. I'd say that CP_UTF8 should be always preferred unless there is recognized reason to use CP_ACP.

I used CP_ACP because e_capi.c does so. I was wondering, though, if that was the right choice. What can we expect from the command prompt these days?

I used CP_ACP because e_capi.c does so.

It doesn't mean that it's right :-) I mean it surely needs an overhaul too.

What can we expect from the command prompt these days?

Command line options are UTF8-ised, and so should be console input. I'll double-check... And I'll have a look at e_capi.c. One can also put it this way. Let's aim for CP_UTF8, because it's the only one that should work, and fix remaining code paths. As opposite to trying to see what happens to work right now... Just to clarify, go for CP_UTF8 :-)

levitte · 2017-05-18T07:00:51Z

From a STORE point of view, this is currently fairly complete. It enumerates key providers, cert stores, keys (I hope, I don't seem to have any) and certs (the last two given a provider or a store), and fetches keys (I hope) and certs (this I know for certain).

On to the key calculations part...

levitte · 2017-05-18T07:05:31Z

Also, fancier search capabilities will be added in not too long (I know @snhenson's wondering about that part)

adityadigumarti · 2017-12-06T19:50:26Z

Any idea when store changes and the cng engine this will be merged?

…NCrypt)

levitte · 2019-07-27T07:16:25Z

Any idea when store changes and the cng engine this will be merged?

Not really. Our major focus for 3.0 is replumbing and the FIPS module, so it's quite possible that this gets pushed to a later version, like 3.1.

dhoelzl · 2021-04-16T11:38:29Z

engines/e_cng.c

+        goto err;
+    }
+    if (NCryptExportKey(khandle, 0, BCRYPT_PUBLIC_KEY_BLOB, NULL,
+                        (PBYTE)exported, size, NULL, 0) != ERROR_SUCCESS) {


Passing NULL to pcbResult at this point causes NCryptExportKey to fail with 0x800706F4 in my test case. Passing &size resolves that problem.

levitte · 2021-04-16T13:01:39Z

I had forgotten about this... right now, my thoughts are to throw it away and create a provider instead

dhoelzl · 2021-04-21T08:29:53Z

engines/e_cng.c

+        return -1;
+    }
+
+    if (NCryptEncrypt(ctx->khandle, (PBYTE)from, flen, NULL, to, flen, &len,


As far as I have seen, NCryptEncrypt will use the public key from the key handle instead of the private key, so NCryptDecrypt needs to be used here to perform the RSA operation with the private key.

NCryptDecrypt however probably can't be configured to add padding before the RSA operation with the private key as required here (RSA private enrypt) as it's designed to be used for decryption with the private key but not for signing/encryption and therefore removes the padding after the RSA operation (RSA private decrypt) instead of adding it before.
For that reason, NCryptDecrypt can only be used with RSA_NO_PADDING here, as with that RSA private encrypt and RSA private decrypt result in the same operation.

When OpenSSL uses RSA signing with PSS padding, this method is called with RSA_NO_PADDING as the padding has already be applied to the buffer before, so that is no problem in that case.

Client certificate authentication with TLS 1.2+ works in my test case, when NCryptDecrypt with RSA_NO_PADDING is used here (See #12859).

Please tell me if I am wrong.

Yes, it is true that from my testing that when using TLSv1.3, the NCryptDecrypt should be used instead of NCryptEncrypt with the NCRYPT_NO_PADDING_FLAG as the padding of the buffer.

levitte · 2021-07-02T12:05:05Z

I was informed just now that someone else has made a CNG engine: https://github.com/rticommunity/openssl-cng-engine
I'm closing this PR in its favor.

levitte force-pushed the store2-e_cng branch from 0d21d5c to e14807c Compare May 17, 2017 08:09

dot-asm reviewed May 17, 2017

View reviewed changes

levitte force-pushed the store2-e_cng branch 3 times, most recently from 3e2c984 to 0556a9b Compare May 18, 2017 06:58

levitte force-pushed the store2-e_cng branch 3 times, most recently from d7d841c to a34fd58 Compare May 26, 2017 21:14

levitte mentioned this pull request Jun 29, 2017

Add the STORE module take 2, for loaded certs, keys and more given a URI (cont) #3542

Closed

2 tasks

levitte force-pushed the store2-e_cng branch 2 times, most recently from e6c67b6 to 8338528 Compare July 20, 2017 15:12

mattcaswell added this to the Post 1.1.1 milestone Jan 24, 2018

levitte force-pushed the store2-e_cng branch from 8338528 to 25ddb80 Compare February 23, 2018 20:41

richsalz added after 1.1.1 and removed after 1.1.1 labels May 6, 2018

levitte added 5 commits June 21, 2018 07:23

New engine to support Microsoft's Next Generation crypto API (CNG or …

829e066

…NCrypt)

Add key enumerator

69a050b

Add certificate enumerator

9ca0615

Add certificate loader

ccd718a

e_cng: add RSA methods

5f91301

levitte force-pushed the store2-e_cng branch from 25ddb80 to 5f91301 Compare June 21, 2018 05:24

levitte mentioned this pull request Jul 9, 2018

Replace CAPI with newer API engine #6338

Open

mspncp removed the after 1.1.1 label Oct 25, 2019

MrAnno mentioned this pull request Sep 11, 2020

TLS >=1.2 won't work on Windows in client mode when the client is authenticated (CAPI engine) #12859

Closed

dhoelzl reviewed Apr 16, 2021

View reviewed changes

dhoelzl reviewed Apr 21, 2021

View reviewed changes

t8m modified the milestones: Post 1.1.1, Post 3.0.0 May 12, 2021

t8m added branch: master Applies to master branch triaged: feature The issue/pr requests/adds a feature labels May 12, 2021

levitte closed this Jul 2, 2021

levitte deleted the store2-e_cng branch July 2, 2021 12:05

Uh oh!

Conversation

levitte commented May 16, 2017

Checklist

Uh oh!

levitte commented May 16, 2017

Uh oh!

levitte commented May 16, 2017

Uh oh!

mattcaswell commented May 17, 2017

Uh oh!

levitte commented May 17, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

levitte May 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

levitte commented May 18, 2017

Uh oh!

levitte commented May 18, 2017

Uh oh!

adityadigumarti commented Dec 6, 2017

Uh oh!

levitte commented Jul 27, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

levitte commented Apr 16, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

levitte commented Jul 2, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

levitte May 17, 2017 •

edited

Loading