Conversation
added secure storage (windows) secure-storage for windows woking
| Task<Result<Address>> GetNextReceiveAddress(WalletId id); | ||
| Task<Result<WalletId>> CreateWallet(string name, string seedwords, Maybe<string> passphrase, string encryptionKey, BitcoinNetwork network); | ||
| Task<Result<WalletId>> CreateWallet(string name, string encryptionKey, BitcoinNetwork network); | ||
| Task<Result<WalletId>> CreateWallet(string name, string seedwords, Maybe<string> passphrase, BitcoinNetwork network); |
There was a problem hiding this comment.
I wonder if we should leave the existing way as well?
There was a problem hiding this comment.
I agree, this should be an overload.
| .Bind(wallet => walletEncryption.Decrypt(wallet, "DEFAULT")) // Try to decrypt the wallet with the default encryption key ("DEFAULT") | ||
| .Map(data => (data.SeedWords, Maybe<string>.None)) // On success, return its seedwords (assume no passphrase) | ||
| .Compensate(_ => provider.RequestSensitiveData(walletId)); // On failure, delegate to the flow to the inner provider. This is, the default encryption key failed to decrypt the wallet didn't work. | ||
| .Bind(wallets => wallets.TryFirst().ToResult("Wallet not found")) |
There was a problem hiding this comment.
has something changed here? or only removed comments?
| // Set file attributes to hidden and system for additional security | ||
| File.SetAttributes(keyFilePath, FileAttributes.ReadOnly | FileAttributes.System); | ||
|
|
||
| Console.WriteLine($"[SecureStorage] Master key stored for wallet: {walletId}"); |
There was a problem hiding this comment.
we have loggers so use that instead of console
| /// Stores a master encryption key securely using Windows DPAPI. | ||
| /// If no key is provided, generates a new one. | ||
| public async Task<Result<string>> StoreMasterKeyAsync( | ||
| string walletId, |
There was a problem hiding this comment.
the masterkey is per wallet? I am not sure we need that level of separation we can just use one master key for all wallets I suppose?
There was a problem hiding this comment.
I guess it is not such a big deal but it doeas put a dependency on the database, we must know the wallet id in order to decrypt a wallet.
| if (encryptResult.IsFailure) | ||
| return Result.Failure<Domain.Wallet>(encryptResult.Error); | ||
|
|
||
| // remove existing entry with same ID before saving — safe on retry |
There was a problem hiding this comment.
remove existing entry with same ID before saving — safe on retry
what is this comment exactly not sure it is even correct?
|
|
||
| private async Task<Result> SaveEncryptedWalletToStoreAsync(string name, string encryptionKey, WalletData walletData, | ||
| WalletId walletId) | ||
| private async Task<Result> SaveEncryptedWalletToStoreAsync(string name, string encryptionKey, WalletData walletData, WalletId walletId) |
There was a problem hiding this comment.
here we have an encryptionKey?
| services.AddSingleton<ITransactionHistory, TransactionHistory>(); | ||
| services.TryAddSingleton<IWalletAccountBalanceService, WalletAccountBalanceService>(); | ||
| services.AddHttpClient(); | ||
| services.AddSingleton<ISecureStorage, DpapiSecureStorage>(); |
There was a problem hiding this comment.
what about none windows devices?
|
|
||
| private static string GenerateMasterKey() | ||
| { | ||
| using var rng = RandomNumberGenerator.Create(); |
There was a problem hiding this comment.
I think you need to use the cryptographically secure random number generator here
| public async Task<Result<WalletId>> CreateWallet(string name, string seedWords, Maybe<string> passphrase , BitcoinNetwork network) | ||
| { | ||
| var wallet = await walletFactory.CreateWallet(name ?? SingleWalletName, seedWords, passphrase, encryptionKey, network); | ||
| var wallet = await walletFactory.CreateWallet(name ?? SingleWalletName, seedWords, passphrase, network); |
There was a problem hiding this comment.
If you remove this we will only support Windows. you need to have a check if we can create without a password perhaps.
| public interface IWalletFactory | ||
| { | ||
| Task<Result<Domain.Wallet>> CreateWallet(string name, string seedwords, Maybe<string> passphrase, string encryptionKey, BitcoinNetwork network); | ||
| Task<Result<Domain.Wallet>> CreateWallet(string name, string seedwords, Maybe<string> passphrase, BitcoinNetwork network); |
There was a problem hiding this comment.
We should have both options here.
| services.AddSingleton<ITransactionHistory, TransactionHistory>(); | ||
| services.TryAddSingleton<IWalletAccountBalanceService, WalletAccountBalanceService>(); | ||
| services.AddHttpClient(); | ||
| services.AddSingleton<ISecureStorage, DpapiSecureStorage>(); |
There was a problem hiding this comment.
Should have a way to check the OS so we know it works as well
| public Task<Result<IWallet>> GetOrCreate() | ||
| { | ||
| return walletAppService.CreateWallet("<default>", GetUniqueId(), bitcoinNetwork()) | ||
| return walletAppService.CreateWallet("<default>", bitcoinNetwork()) |
There was a problem hiding this comment.
Delete the method GetUniqueId as well
DavidGershony
left a comment
There was a problem hiding this comment.
Needs some changes
Secure Wallet Storage Implementation (Windows)
Overview
This PR implements passwordless wallet encryption using Windows DPAPI (Data Protection API) and AES-256-GCM authenticated encryption. Users no longer need to enter or remember encryption passwords when creating or importing wallets.
How It Works
Two-Layer Encryption Architecture
The implementation uses an envelope encryption pattern with two distinct layers:
OS-Level Protection (DPAPI)
%AppData%\Angor\secure\{walletId}.keyWallet Data Encryption (AES-GCM)
Wallet Creation Flow
%AppData%\Angor\secure\{walletId}.keyWallet Access Flow
Security Properties
What's Protected:
Platform Support
Current: Windows only (DPAPI)
Future: Cross-platform support planned via platform-specific secure storage:
The AES-GCM encryption layer remains identical across platforms — only the master key storage mechanism changes.
Breaking Changes
IWalletFactory.CreateWalletsignature changed — removedencryptionKeyparameterWhy Two Layers?
This approach separates concerns:
Benefits:
Testing
Manual testing performed on Windows 11. Verify:
.keyfiles created in%AppData%\Angor\secure\