Finish moving DSAOpenSsl to EVP_PKEY for the shim boundary.

[bartonjs]

Similar to the change for RSA, this removes SafeDsaHandle and changes the shim to work in terms
of EVP_PKEY* instead of DSA*, except for the DSAOpenSsl(IntPtr) constructor.

Related to #54282.
Contributes to #46526.

[ghost]

Tagging subscribers to this area: @bartonjs, @vcsjones, @krwq, @GrabYourPitchforks
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Author:	bartonjs
Assignees:	-
Labels:	area-System.Security
Milestone:	-

[bartonjs]

Looks like some of the "tell me about this DSA key" behaviors aren't as consistent as I'd like, since the pass/fail is by distro (works on 18.04 😄).

I see the failures on my mac, so: looking. (Though I won't turn away any parallel investigation advice)

[vcsjones]

I see the failures on my mac, so: looking.

I can look over the weekend. If you figure something out before then, please share so I don't duplicate the research.

[bartonjs]

Looks like it might be that (a) those tests cycle between keys ( { generated 1024, generated 2048, imported 2048, imported 1024 } ), and (b) only the tests that hit generated 1024 fail. All the errors look like Q is too big.

Seems like some builds are making FIPS 186-3 keys at 1024-bit, and some are making FIPS 186-2. Hopefully this is a one-liner.

[bartonjs]

Looks like I knew it was potentially a problem, but ultimately decided it wasn't needed. Well. That saves some work ;)

[bartonjs]

K, this round's errors:

• Doesn't compile with OpenSSL 3 headers.
  • I think it also fails with 1.1.1 headers?
• Didn't wire up a DSA handler in local_EVP_PKEY_check (OSSL 1.0.x)

Next commit should have both of those fixed.

[bartonjs]

I think I'm going to close this until after .NET 6 peels off of main, and likewise not pursue moving ECC fully to EVP_PKEY in this release (ECC was giving me some trouble, anyways).

When running this change with OpenSSL 3.0 the DSA tests which try to export a public key with private parameters fail with an OutOfMemoryException. Why? Well, because the (mostly generated) code walks down a path, does ERR_raise(ERR_LIB_PROV, ERR_R_NOT_A_PRIVATE_KEY) (the error we want), then down an error handling path goes "oh, building that interior piece failed, clearly we're out of memory ERR_raise(ERR_LIB_PROV, ERR_R_MALLOC_FAILURE) (not the error we want), and then no other error happened. So we exit CryptoNative_GetPkcs8PrivateKeySize with an error stack of { LIB_PROV|NOT_A_PRIVATE_KEY, LIB_PROV|MALLOC_FAILURE }. Our logic is to take the last (newest) error, and if it's MALLOC_FAILURE that we report an OutOfMemoryException.

Trying to rewind my brain 5 years, I'm pretty sure we take the last error because of all the places that we decide there was an error and we move on anyways without clearing it, and this was partly because of the expense of interacting with the error state in OpenSSL 0.9.8 and 1.0.x (with 1.1.0 (IIRC) it moved to a thread-local thing and got much less costly). What this error shows is that we're just out of touch and need to rethink exception processing for OpenSSL.

I /think/ that what we want is:

• ERR_clear_error() at the start of each operation.
• Change our current "keep asking what the error is until we run out" to something like long CryptoNative_GetErrorForException(out int handlerFlags) ((HandlerFlag)0x01 being "OutOfMemoryException"), which takes the oldest and clears the error queue, the logic being that the first thing that went wrong since we started is obviously "the thing that went wrong". (Maybe there'll be edge cases that say this also isn't always the right answer, though... but I hope not)

Because this is a pretty fundamental change in how we're interopping I think we want to make sure there's a chance to see if something complicated goes wrong, which means we want it in a preview release we can get good feedback on. And that means it's probably .NET 7 Preview 1.

---

The alternative is that we just say "eh, let's only throw OOM if that was the oldest error in the queue", but that feels like we're just missing the point and kicking the can a bit.

---

Leaving this open for another hour or two in case anyone wants to try swaying me back.

[vcsjones]

Agree waiting for the 6.0 branch to be taken ("snap" as I think you call it...) seems to be the most sensible thing to do.

[bartonjs]

Opened #55973 with a bigger braindump on the exception/error modelling, closing this until after that.