[Neo Wallet Fix] Add cert import exception by Jim8y · Pull Request #3279 · neo-project/neo

Jim8y · 2024-05-27T01:17:13Z

Description

Dotnet has no implementation of ExportParameters for ECdsa, thus causing the Import method fail.

Fixes # (issue)

Type of change

Optimization (the change is only an optimization)
Style (the change is only a code style for better maintenance or standard purpose)
Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update

How Has This Been Tested?

Updated the Unit test of TestImportCert in UT_NEP6Wallet.cs

TestImportCert

Test Configuration:

Checklist:

My code follows the style guidelines of this project
I have performed a self-review of my code
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
My changes generate no new warnings
I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass locally with my changes
Any dependent changes have been merged and published in downstream modules

src/Neo/Wallets/NEP6/NEP6Wallet.cs

roman-khimov · 2024-05-28T07:37:38Z

src/Neo/Wallets/NEP6/NEP6Wallet.cs


        public override WalletAccount Import(X509Certificate2 cert)
        {
+            if (RuntimeInformation.IsOSPlatform(OSPlatform.OSX))


Can it be implemented with 3rd-party libraries for OSX?

have tried, cant extract the private key. nothing in the ecdsa or cert is implementated on the macos that can help.

not necessary, no one use this. exception is fine already.

If openssl is installed on the machine and configured right. dotnet will use it automatically.

but if it works with openssl installed, with this condition it wont never work

Jim8y · 2024-05-28T11:32:49Z

time to fix the random error:

Warning: D:\a\neo\neo\src\Neo\SmartContract\ApplicationEngine.cs(256,27): warning CS0618: 'ApplicationEngine.GasConsumed' is obsolete: 'This property is deprecated. Use FeeConsumed instead.' [D:\a\neo\neo\src\Neo\Neo.csproj::TargetFramework=net8.0]
C:\Program Files\dotnet\sdk\8.0.300\Microsoft.Common.CurrentVersion.targets(2401,5): error MSB3248: Parameter "AssemblyFiles" has invalid value "D:\a\neo\neo\src\Neo\bin\Debug\net8.0\Neo.dll". The process cannot access the file 'D:\a\neo\neo\src\Neo\bin\Debug\net8.0\Neo.dll' because it is being used by another process. [D:\a\neo\neo\src\Plugins\RpcServer\RpcServer.csproj::TargetFramework=net8.0]
Neo -> D:\a\neo\neo\src\Neo\bin\Debug\net8.0\Neo.dll
Neo -> D:\a\neo\neo\src\Neo\bin\Debug\netstandard2.1\Neo.dll

No reason to have this PR. Tests were working fine as before. So that shows that this works on macos.

Jim8y · 2024-05-28T22:28:58Z

No reason to have this PR. Tests were working fine as before. So that shows that this works on macos.

What a disrespectful expression, feels like i am faking the issue:

Test method Neo.UnitTests.Wallets.NEP6.UT_NEP6Wallet.TestImportCert threw exception: 
Interop+AppleCrypto+AppleCFErrorCryptographicException: The operation couldn’t be completed. (OSStatus error -67738 - CSSM Exception: -2147415780 CSSMERR_CSP_INVALID_KEYATTR_MASK)
    at Interop.AppleCrypto.TrySecKeyCopyExternalRepresentation(SafeSecKeyRefHandle key, Byte[]& externalRepresentation)
   at System.Security.Cryptography.EccSecurityTransforms.TryExportDataKeyParameters(SecKeyPair keys, Boolean includePrivateParameters, ECParameters& ecParameters)
   at System.Security.Cryptography.EccSecurityTransforms.ExportParameters(Boolean includePrivateParameters, Int32 keySizeInBits)
   at Neo.Wallets.NEP6.NEP6Wallet.Import(X509Certificate2 cert) in /Users/jinghuiliao/git/neo/src/Neo/Wallets/NEP6/NEP6Wallet.cs:line 229
   at Neo.UnitTests.Wallets.NEP6.UT_NEP6Wallet.TestImportCert() in /Users/jinghuiliao/git/neo/tests/Neo.UnitTests/Wallets/NEP6/UT_NEP6Wallet.cs:line 305
   at System.RuntimeMethodHandle.InvokeMethod(Object target, Void** arguments, Signature sig, Boolean isConstructor)
   at System.Reflection.MethodBaseInvoker.InvokeWithNoArgs(Object obj, BindingFlags invokeAttr)

Jim8y · 2024-05-28T22:41:23Z

@neo-project/core finally fixed with BouncyCastle

shargon

Use the same logic as?

neo/src/Neo/Wallets/Wallet.cs

Lines 413 to 423 in df06791

    
           public virtual WalletAccount Import(X509Certificate2 cert) 
        
           { 
        
               byte[] privateKey; 
        
               using (ECDsa ecdsa = cert.GetECDsaPrivateKey()) 
        
               { 
        
                   privateKey = ecdsa.ExportParameters(true).D; 
        
               } 
        
               WalletAccount account = CreateAccount(privateKey); 
        
               Array.Clear(privateKey, 0, privateKey.Length); 
        
               return account; 
        
           }

cschuchardt88 · 2024-05-29T08:32:32Z

What a disrespectful expression, feels like i am faking the issue:

Is this on your computer? Then does the tests work on github?

shargon · 2024-05-29T10:48:15Z

Use the same logic as?

neo/src/Neo/Wallets/Wallet.cs

Lines 413 to 423 in df06791

public virtual WalletAccount Import(X509Certificate2 cert)

{

byte[] privateKey;

using (ECDsa ecdsa = cert.GetECDsaPrivateKey())

{

privateKey = ecdsa.ExportParameters(true).D;

}

WalletAccount account = CreateAccount(privateKey);

Array.Clear(privateKey, 0, privateKey.Length);

return account;

}

That's logic doesn't work in mac?

cschuchardt88 · 2024-05-29T11:33:31Z

That's logic doesn't work in mac?

It would have to be working. It is currently in the code right now. I think his computer isn't configured right.

add exception

466bd6d

Jim8y requested a review from a team May 27, 2024 01:17

Jim8y added the Waiting for Review label May 27, 2024

Jimmy added 2 commits May 27, 2024 20:06

Merge branch 'master' into cert-error-macos

d91fcb3

Merge branch 'master' into cert-error-macos

3561caf

roman-khimov reviewed May 28, 2024

View reviewed changes

shargon previously approved these changes May 28, 2024

View reviewed changes

Jimmy added 2 commits May 28, 2024 15:41

Merge branch 'master' into cert-error-macos

0233207

Merge branch 'master' into cert-error-macos

24298b1

Jim8y requested a review from a team May 28, 2024 08:08

cschuchardt88 previously approved these changes May 28, 2024

View reviewed changes

Use using Org.BouncyCastle.Crypto;

5e023d0

Jim8y dismissed shargon’s stale review via 5e023d0 May 28, 2024 22:40

clean using

ca7705f

clean further using

6f65e6b

shargon reviewed May 29, 2024

View reviewed changes

update wallet as well

bf55865

Jim8y requested a review from a team May 30, 2024 00:31

ban the use of import on macos

00f46ab

cschuchardt88 approved these changes May 30, 2024

View reviewed changes

Jim8y requested a review from a team May 30, 2024 03:37

Merge branch 'master' into cert-error-macos

86b2d0c

shargon approved these changes May 30, 2024

View reviewed changes

Jim8y added the Ready to Merge label May 31, 2024

NGDAdmin merged commit 2c0ce81 into neo-project:master May 31, 2024

Jim8y deleted the cert-error-macos branch June 14, 2024 09:11

	public virtual WalletAccount Import(X509Certificate2 cert)
	{
	byte[] privateKey;
	using (ECDsa ecdsa = cert.GetECDsaPrivateKey())
	{
	privateKey = ecdsa.ExportParameters(true).D;
	}
	WalletAccount account = CreateAccount(privateKey);
	Array.Clear(privateKey, 0, privateKey.Length);
	return account;
	}

Conversation

Jim8y commented May 27, 2024

Description

Type of change

How Has This Been Tested?

Checklist:

Uh oh!

Uh oh!

roman-khimov May 28, 2024

Choose a reason for hiding this comment

Uh oh!

Jim8y May 28, 2024 • 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!

cschuchardt88 May 28, 2024

Choose a reason for hiding this comment

Uh oh!

Jim8y May 28, 2024

Choose a reason for hiding this comment

Uh oh!

cschuchardt88 May 28, 2024

Choose a reason for hiding this comment

Uh oh!

shargon May 30, 2024

Choose a reason for hiding this comment

Uh oh!

Jim8y commented May 28, 2024

Uh oh!

Jim8y commented May 28, 2024

Uh oh!

Jim8y commented May 28, 2024

Uh oh!

shargon left a comment • 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!

cschuchardt88 commented May 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shargon commented May 29, 2024

Uh oh!

cschuchardt88 commented May 29, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Jim8y May 28, 2024 •

edited

Loading

shargon left a comment •

edited

Loading

cschuchardt88 commented May 29, 2024 •

edited

Loading