Add Windows Hello support #83
Conversation
Otherwise it crashes the JVM
using Windows Hello where appropriate
WalkthroughThis pull request introduces several significant changes across multiple files, primarily focusing on enhancing the integration of Windows Hello for keychain access and management. The The Possibly related PRs
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
🧹 Outside diff range and nitpick comments (5)
src/main/java/org/cryptomator/windows/keychain/WinHello.java (1)
1-5: Add class documentation and consider making the class final.Since this is a security-critical component handling Windows Hello integration:
- Add class-level documentation explaining the purpose, usage, and security considerations
- Consider making the class final to prevent inheritance which could potentially bypass security measures
- Add @SInCE tag for API versioning
package org.cryptomator.windows.keychain; import org.cryptomator.windows.common.NativeLibLoader; +/** + * Provides Windows Hello-based encryption and decryption capabilities for secure key storage. + * This class interfaces with Windows Hello through JNI to perform cryptographic operations + * using Windows Hello keys. + * + * @since 1.0.0 + */ -class WinHello { +final class WinHello {src/test/java/org/cryptomator/windows/keychain/WindowsProtectedKeychainAccessTest.java (1)
Line range hint
16-17: Add nested test class for Windows Hello specific scenarios.Based on the PR objectives, the current test coverage is insufficient for the Windows Hello implementation. Consider adding a nested test class to cover the Windows Hello-specific functionality.
Suggested test scenarios:
- Challenge-response mechanism
- Signature verification
- Hash computation for encryption key
- Error handling for Windows Hello failures
Would you like me to help create these test cases?
src/main/java/org/cryptomator/windows/keychain/WindowsProtectedKeychainAccess.java (2)
Line range hint
117-125: Ensure sensitive data is cleared even if an exception occursIn the
loadPassphrasemethod, sensitive data incleartextandbuf.array()is cleared after usage. However, if an exception is thrown bywindowsHello.getEncryptionKeyordataProtection.unprotect, the sensitive data may not be cleared, potentially leaving it in memory.Wrap the decryption and clearing logic in a
try-finallyblock to guarantee that sensitive data is always cleared from memory.byte[] cleartext = null; try { cleartext = entry.requireAuth() ? windowsHello.getEncryptionKey(entry.ciphertext(), entry.salt()) : dataProtection.unprotect(entry.ciphertext(), entry.salt()); if (cleartext == null) { return null; } CharBuffer buf = UTF_8.decode(ByteBuffer.wrap(cleartext)); char[] passphrase = new char[buf.remaining()]; buf.get(passphrase); Arrays.fill(buf.array(), (char) 0x00); return passphrase; } finally { if (cleartext != null) { Arrays.fill(cleartext, (byte) 0x00); } }
Line range hint
129-135: Potential data loss due to keychain file overwriting on parsing failureIn the
loadKeychainEntriesIfNeededmethod, if the keychain file cannot be parsed—possibly due to the introduction of the newrequireAuthfield inKeychainEntry—the application logs a warning and proceeds to create a new keychain:if (keychainEntries == null) { LOG.info("Unable to load existing keychain file, creating new keychain."); keychainEntries = new HashMap<>(); }This behavior can lead to the loss of existing stored passphrases, resulting in users being unable to access their vaults.
Consider implementing a migration strategy to handle the deserialization of legacy
KeychainEntryobjects without therequireAuthfield. This can be achieved by:
- Providing default values for missing fields during deserialization.
- Configuring the
ObjectMapperto be lenient with missing fields.- Implementing a custom deserializer for
KeychainEntry.This approach will maintain backward compatibility and prevent unintended data loss.
src/main/native/org_cryptomator_windows_keychain_WinHello_Native.cpp (1)
26-35: Handle Null Inputs Consistently in Helper FunctionsThe helper functions
jbyteArrayToVector,vectorToJbyteArray,vectorToIBuffer, andiBufferToVectorshould have consistent null handling to prevent potential null dereferences.Ensure all helper functions check for null inputs and handle them appropriately.
Also applies to: 37-41, 43-47, 49-57
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
- .gitignore (1 hunks)
- Makefile (1 hunks)
- src/main/headers/org_cryptomator_windows_keychain_WinHello_Native.h (1 hunks)
- src/main/java/org/cryptomator/windows/keychain/KeychainEntry.java (1 hunks)
- src/main/java/org/cryptomator/windows/keychain/WinHello.java (1 hunks)
- src/main/java/org/cryptomator/windows/keychain/WindowsProtectedKeychainAccess.java (4 hunks)
- src/main/native/org_cryptomator_windows_keychain_WinHello_Native.cpp (1 hunks)
- src/test/java/org/cryptomator/windows/keychain/WindowsProtectedKeychainAccessTest.java (1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/main/headers/org_cryptomator_windows_keychain_WinHello_Native.h
🧰 Additional context used
🔇 Additional comments (5)
src/main/java/org/cryptomator/windows/keychain/KeychainEntry.java (2)
7-9: LGTM! Well-designed convenience constructor.The convenience constructor provides good backward compatibility by defaulting
requireAuthto false, maintaining existing behavior for code that doesn't need Windows Hello authentication.
6-6: Consider defensive copying of byte arrays.The record implementation looks good and the new
requireAuthfield with@JsonPropertyannotation is well-integrated. However, be aware that despite the record's immutability, thebyte[]fields forciphertextandsaltare still mutable and could be modified after construction.Let's verify if there are any direct modifications to these arrays in the codebase:
Consider applying this defensive copying pattern:
record KeychainEntry(@JsonProperty("ciphertext") byte[] ciphertext, @JsonProperty("salt") byte[] salt, @JsonProperty("requireAuth") boolean requireAuth) { public KeychainEntry { ciphertext = ciphertext.clone(); salt = salt.clone(); } public byte[] ciphertext() { return ciphertext.clone(); } public byte[] salt() { return salt.clone(); } }.gitignore (1)
4-6: LGTM! Appropriate additions for Windows C++ development.The new patterns correctly exclude Windows-specific build artifacts (object files, libraries, and export files) that should not be tracked in version control.
Makefile (1)
1-13: Verify Windows Hello functionality in CI environmentSince this Makefile is crucial for building the Windows Hello integration, we should verify the build environment setup.
✅ Verification successful
Let me gather more information about the Windows Hello implementation to better understand the build requirements.
Let me check one more thing to verify all native source files and headers:
Based on the comprehensive analysis, I can now provide the final response:
Windows Hello integration build setup is correctly configured
The Makefile changes are appropriate for building the Windows Hello integration as:
- The implementation in
org_cryptomator_windows_keychain_WinHello_Native.cppcorrectly uses the required Windows Hello APIs and C++/WinRT headers- All necessary Windows Kit headers are properly included in the build path
- The compiler flags (
/EHsc /std:c++17) match the C++/WinRT requirements- Required Windows libraries for cryptography and UI (crypt32.lib, windowsapp.lib) are properly linked
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for Windows Hello related files and dependencies # Check for Windows Hello related source files echo "Checking for Windows Hello implementation files..." rg -l "Windows.Security.Credentials.UI" # Check for required Windows Kit headers echo "Checking for Windows Kit headers usage..." rg -l "winrt/Windows.Security.Credentials.h"Length of output: 357
Script:
#!/bin/bash # Check the actual Windows Hello implementation echo "Checking Windows Hello implementation details..." cat src/main/native/org_cryptomator_windows_keychain_WinHello_Native.cpp # Check for any additional header includes echo -e "\nChecking for additional header includes..." rg -l "#include.*Windows" src/main/native/ # Check if there are any other native source files echo -e "\nListing all native source files..." fd -e cpp -e h src/main/native/Length of output: 9249
Script:
#!/bin/bash # List all files in the native directory correctly echo "Listing all native source files..." fd . 'src/main/native/' # Check for the JNI header file echo -e "\nChecking for the JNI header file..." fd -e h . 'src/main/headers/'Length of output: 660
src/test/java/org/cryptomator/windows/keychain/WindowsProtectedKeychainAccessTest.java (1)
29-33: 🛠️ Refactor suggestionVerify WinHello mock configuration.
The WinHello mock is created but no behavior is configured. Consider adding mock configurations to test Windows Hello-specific scenarios.
Add mock configurations to test authentication scenarios:
WinHello winHello = Mockito.mock(WinHello.class); +// Mock Windows Hello authentication success/failure scenarios +Mockito.when(winHello.encrypt(Mockito.any(), Mockito.anyBoolean())).thenReturn(new byte[]{1, 2, 3}); +Mockito.when(winHello.decrypt(Mockito.any())).thenReturn(new byte[]{4, 5, 6});
src/main/java/org/cryptomator/windows/keychain/WindowsProtectedKeychainAccess.java
Outdated
Show resolved
Hide resolved
src/main/native/org_cryptomator_windows_keychain_WinHello_Native.cpp
Outdated
Show resolved
Hide resolved
src/main/native/org_cryptomator_windows_keychain_WinHello_Native.cpp
Outdated
Show resolved
Hide resolved
src/main/native/org_cryptomator_windows_keychain_WinHello_Native.cpp
Outdated
Show resolved
Hide resolved
src/main/native/org_cryptomator_windows_keychain_WinHello_Native.cpp
Outdated
Show resolved
Hide resolved
|
Just for the record: It seems keepassXC follows a similar cryptographic approach, see https://github.com/keepassxreboot/keepassxc/blob/d03ffc228ca56a0a4418a1e8ef1998e538118efc/src/quickunlock/WindowsHello.cpp#L60-L100 |
|
@purejava Thanks for the excellent PR documentation! We need apporval of the cryptographic model from @overheadhunter, but as i already mentioned, keepassxc is doing something similar. Regarding the compiler framework: I welcome the change to MSVC. We are on Windows anyway, so using the Microsoft tools only has benefits. Additionally, we are fit for the future (iirc mingw only works up to version 10.x or something, afterwards there are compile problems). Regarding status:
This is indeed a lot. But why do we have to update the entry on an unlock? Is this due to the random challenge we give WindowsHello to sign? |
Good question. It has been like this for a long time, maybe a leftover from the time, before saving passphrases to keychains was implemented (just a guess). When a vault is unlocked with a passphrase that got retrieved from a keychain, I don't see a reason to save it again right away. |
There was a problem hiding this comment.
The (java) implementation itself looks very good. I'll have a deeper look at the C++ impl, once the two requests are adressed.
The requests are based mainly on a talk with @overheadhunter.
src/main/native/org_cryptomator_windows_keychain_WinHello_Native.cpp
Outdated
Show resolved
Hide resolved
src/main/native/org_cryptomator_windows_keychain_WinHello_Native.cpp
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (2)
src/main/native/org_cryptomator_windows_keychain_WinHello_Native.cpp (2)
25-25: Improve naming and documentation of the HKDF info parameterThe static
infobuffer's purpose as the HKDF info parameter should be more clearly indicated in its name and documented with its cryptographic significance.-static IBuffer info = CryptographicBuffer::ConvertStringToBinary(L"EncryptionKey", BinaryStringEncoding::Utf8); +static IBuffer hkdfInfoParameter = CryptographicBuffer::ConvertStringToBinary(L"CryptomatorWinHelloHKDF", BinaryStringEncoding::Utf8);
110-115: Improve Windows Hello credential status handlingThe error message for failed credential retrieval should include the specific status for better debugging.
if (result.Status() == KeyCredentialStatus::CredentialAlreadyExists) { result = KeyCredentialManager::OpenAsync(s_winHelloKeyName).get(); } else if (result.Status() != KeyCredentialStatus::Success) { - std::cout << "Failed to retrieve Windows Hello credential." << std::endl; + std::cout << "Failed to retrieve Windows Hello credential. Status: " + << static_cast<int>(result.Status()) << std::endl; return false; }
src/main/native/org_cryptomator_windows_keychain_WinHello_Native.cpp
Outdated
Show resolved
Hide resolved
src/main/native/org_cryptomator_windows_keychain_WinHello_Native.cpp
Outdated
Show resolved
Hide resolved
instead of using SHA256 Replace AesCbcPkcs7 by AesCbc for the symetric encryption
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (1)
src/main/native/org_cryptomator_windows_keychain_WinHello_Native.cpp (1)
25-25: Consider using a more secure info string for HKDFThe current static info string "EncryptionKey" is predictable. While HKDF is designed to handle this, using a more unique value would improve security.
Consider using a combination of application-specific data:
-static IBuffer info = CryptographicBuffer::ConvertStringToBinary(L"EncryptionKey", BinaryStringEncoding::Utf8); +static IBuffer info = CryptographicBuffer::ConvertStringToBinary(L"Cryptomator-WinHello-v1", BinaryStringEncoding::Utf8);
src/main/native/org_cryptomator_windows_keychain_WinHello_Native.cpp
Outdated
Show resolved
Hide resolved
src/main/native/org_cryptomator_windows_keychain_WinHello_Native.cpp
Outdated
Show resolved
Hide resolved
src/main/native/org_cryptomator_windows_keychain_WinHello_Native.cpp
Outdated
Show resolved
Hide resolved
src/main/native/org_cryptomator_windows_keychain_WinHello_Native.cpp
Outdated
Show resolved
Hide resolved
|
I need to see whether this compiles and test it. |
|
Shall we implement a cache for the many prompts or remove updating the keychain entry on unlocking a vault? |
I can't get it to compile. But i managed to add a VS solution file. Should i push it in order to get code suggestions and inspection for VS 2022? Also have already some fixes for it 😉
I'll needd to check with @overheadhunter about this. |
(cherry picked from commit 7cc28d0)
|
@purejava A path during build contained a space, which led to wrong arguments
Fixed with ef57359 Waiting for https://github.com/purejava/integrations-win/pull/1 now. |
Feature: Common base class for windows Hello and DPAPI keychains
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (16)
src/main/java/org/cryptomator/windows/keychain/Util.java (3)
30-33: Prevent instantiation of utility class.Since this is a utility class with only static methods, consider making it final and adding a private constructor to prevent instantiation.
Apply this diff:
-public class Util { +public final class Util { private static final Path USER_HOME_REL = Path.of("~"); private static final Path USER_HOME = Path.of(System.getProperty("user.home")); + + private Util() { + throw new AssertionError("Utility class should not be instantiated"); + }
42-49: Add documentation and consider making salt size configurable.While the implementation is secure and efficient, consider:
- Adding Javadoc to document the cryptographic purpose and size of the salt
- Making the salt size configurable for flexibility
Apply this diff:
+ /** + * Generates a cryptographic salt using UUID.randomUUID(). + * The resulting salt is 16 bytes (128 bits) long and is suitable for use + * with the Windows Hello encryption implementation. + * + * @return a 16-byte random salt + */ static byte[] generateSalt() { byte[] result = new byte[2 * Long.BYTES]; UUID uuid = UUID.randomUUID();Consider making the salt size configurable:
+ private static final int DEFAULT_SALT_SIZE = 2 * Long.BYTES; + + /** + * Generates a cryptographic salt of the specified size. + * + * @param saltSize the desired size of the salt in bytes + * @return a random salt of the specified size + * @throws IllegalArgumentException if saltSize is less than 16 bytes + */ + static byte[] generateSalt(int saltSize) { + if (saltSize < 16) { + throw new IllegalArgumentException("Salt size must be at least 16 bytes"); + } + byte[] result = new byte[saltSize]; + int offset = 0; + while (offset < saltSize) { + UUID uuid = UUID.randomUUID(); + ByteBuffer buf = ByteBuffer.wrap(result, offset, Math.min(16, saltSize - offset)); + buf.putLong(uuid.getMostSignificantBits()); + if (saltSize - offset > 8) { + buf.putLong(uuid.getLeastSignificantBits()); + } + offset += 16; + } + return result; + } + + /** + * Generates a cryptographic salt using the default size of 16 bytes. + * + * @return a 16-byte random salt + */ + static byte[] generateSalt() { + return generateSalt(DEFAULT_SALT_SIZE); + }
1-51: Consider strengthening the security architecture.Given this class's critical role in Windows Hello integration:
- Consider adding input validation for paths to prevent path traversal attacks
- Add logging for security-relevant operations
- Consider making security-critical methods package-private to restrict access
Would you like me to provide specific implementation suggestions for these security enhancements?
src/main/java/org/cryptomator/windows/keychain/WindowsProtectedKeychainAccess.java (1)
18-18: Typo in comment: 'constructuor' should be 'constructor'There's a minor typo in the comment on line 18. Correcting it enhances code readability.
src/test/java/org/cryptomator/windows/keychain/WindowsProtectedKeychainAccessIntegrationTest.java (2)
37-38: Avoid relying on test method execution orderUsing
@TestMethodOrderand@Orderannotations can make tests fragile due to dependencies on execution order. It's best practice for tests to be independent to ensure reliability.
43-43: Remove unused parameter 'tmpDir' inbeforeEachmethodThe
@TempDir Path tmpDirparameter in thebeforeEachmethod is not used and can be removed to improve code clarity.src/main/java/org/cryptomator/windows/keychain/FileKeychain.java (1)
85-101: Handle IOException in 'parse' method to try other keychain pathsIn the
parsemethod, if anIOExceptionoccurs while reading a keychain file, aKeychainAccessExceptionis thrown, which stops the loading process and prevents trying other keychain paths.Consider logging the exception and returning
Optional.empty()to allow the method to proceed with other paths:} catch (IOException e) { - //TODO: we could ignore this - throw new KeychainAccessException("Failed to read keychain from path " + keychainPath, e); + LOG.warn("Failed to read keychain from path {} due to IOException: {}. Continuing with next path.", keychainPath, e.getMessage()); + return Optional.empty(); }src/main/java/org/cryptomator/windows/keychain/WindowsKeychainAccessBase.java (1)
28-43: Ensure exceptions are properly propagated in 'encryptPassphrase' methodWhile the use of a try-finally block to clear sensitive data is appropriate, ensure that any exceptions thrown during encryption are properly propagated to the caller. If the native
encryptmethod throws an exception, it should not be unintentionally swallowed.Consider explicitly handling exceptions to maintain the expected flow of the application.
src/test/java/org/cryptomator/windows/keychain/WinDataProtectionTest.java (1)
Line range hint
5-23: Rename test method to reflect updated method namesSince the methods
protectandunprotecthave been renamed toencryptanddecrypt, consider renaming the test methodtestProtectAndUnprotecttotestEncryptAndDecryptfor consistency and clarity.Apply this diff:
- public void testProtectAndUnprotect() { + public void testEncryptAndDecrypt() {src/main/java/org/cryptomator/windows/keychain/WinDataProtection.java (1)
8-11: Consider aligning native method names with Java method namesThe Java methods are named
encrypt/decryptbut they call native methods namedprotect/unprotect. This inconsistency could cause confusion during maintenance.private static class Native { - public native byte[] protect(byte[] cleartext, byte[] salt); - public native byte[] unprotect(byte[] ciphertext, byte[] salt); + public native byte[] encrypt(byte[] cleartext, byte[] salt); + public native byte[] decrypt(byte[] ciphertext, byte[] salt); }Also applies to: 12-15
src/main/java/org/cryptomator/windows/keychain/WindowsHelloKeychainAccess.java (1)
12-13: Document the priority value rationaleThe
@Priority(1001)value seems arbitrary. Consider documenting why this specific value was chosen and how it relates to other providers.Add a comment explaining the priority value:
+/** + * Priority is set higher than WindowsProtectedKeychainAccess (1000) to prefer + * Windows Hello when available. + */ @Priority(1001) @OperatingSystem(OperatingSystem.Value.WINDOWS)src/test/java/org/cryptomator/windows/keychain/WindowsKeychainAccessBaseTest.java (2)
22-77: Enhance test coverage with additional scenariosThe current tests cover basic functionality but miss important edge cases.
Consider adding tests for:
- Empty passphrases
- Very long passphrases
- Special characters in passphrases
- Invalid key identifiers
- Concurrent access scenarios
Example test:
@Test public void storeEmptyPassphrase() throws KeychainAccessException { var encrypted = new byte[0]; when(passphraseCryptor.encrypt(any(), any())).thenReturn(encrypted); winKeychainBase.storePassphrase("test3000", "display3000", ""); verify(keychain).put(eq("test3000"), argThat(entry -> Arrays.equals(entry.ciphertext(), encrypted))); }
32-32: Use meaningful test data constantsTest data uses magic values without clear meaning. Consider extracting test data into named constants or using a test data builder pattern.
+private static final class TestData { + static final String KEY_ID = "test-key-1"; + static final String DISPLAY_NAME = "Test Key 1"; + static final byte[] SAMPLE_CIPHERTEXT = {'x','y','z'}; + static final byte[] SAMPLE_CLEARTEXT = {'a','b','c'}; + static final byte[] SAMPLE_SALT = {'s'}; +}Also applies to: 52-53, 66-69
src/test/java/org/cryptomator/windows/keychain/FileKeychainTest.java (3)
54-56: Follow conventional test method naming patternsThe test method names ('puttin', 'gettin', 'removin', 'changin') use informal naming. Consider following the standard test naming conventions, such as:
testPutorshouldPutValueInKeychaintestGetorshouldRetrieveValueFromKeychaintestRemoveorshouldRemoveValueFromKeychaintestChangeorshouldChangeExistingValueThis makes the test purpose clearer and maintains consistency with standard Java testing practices.
Also applies to: 68-70, 80-82, 94-96
54-106: Add test cases for edge cases in CRUD operationsThe current CRUD operation tests only verify method calls and null results. Consider adding test cases for:
- Adding/updating entries with actual values
- Handling duplicate keys
- Handling invalid input values
- Verifying actual data persistence
181-218: Consider adding more path parsing test casesThe
ParsePathstest class has good coverage but could benefit from additional test cases:
- Multiple consecutive path separators (e.g., "path1;;path2")
- Mixed path formats (Windows vs Unix-style paths)
- Path with environment variables
- Unicode characters in paths
- Maximum path length scenarios
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (15)
src/main/java/module-info.java(2 hunks)src/main/java/org/cryptomator/windows/keychain/FileKeychain.java(1 hunks)src/main/java/org/cryptomator/windows/keychain/Util.java(1 hunks)src/main/java/org/cryptomator/windows/keychain/WinDataProtection.java(1 hunks)src/main/java/org/cryptomator/windows/keychain/WindowsHello.java(1 hunks)src/main/java/org/cryptomator/windows/keychain/WindowsHelloKeychainAccess.java(1 hunks)src/main/java/org/cryptomator/windows/keychain/WindowsKeychainAccessBase.java(1 hunks)src/main/java/org/cryptomator/windows/keychain/WindowsProtectedKeychainAccess.java(1 hunks)src/test/java/org/cryptomator/windows/keychain/FileKeychainTest.java(1 hunks)src/test/java/org/cryptomator/windows/keychain/WinDataProtectionTest.java(1 hunks)src/test/java/org/cryptomator/windows/keychain/WindowsHelloTest.java(1 hunks)src/test/java/org/cryptomator/windows/keychain/WindowsKeychainAccessBaseTest.java(1 hunks)src/test/java/org/cryptomator/windows/keychain/WindowsProtectedKeychainAccessIntegrationTest.java(1 hunks)src/test/java/org/cryptomator/windows/keychain/WindowsProtectedKeychainAccessTest.java(0 hunks)src/test/resources/org/cryptomator/windows/keychain/keychain.v1.2.2.json(0 hunks)
💤 Files with no reviewable changes (2)
- src/test/java/org/cryptomator/windows/keychain/WindowsProtectedKeychainAccessTest.java
- src/test/resources/org/cryptomator/windows/keychain/keychain.v1.2.2.json
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/java/module-info.java
- src/test/java/org/cryptomator/windows/keychain/WindowsHelloTest.java
🧰 Additional context used
📓 Learnings (3)
src/main/java/org/cryptomator/windows/keychain/WindowsHelloKeychainAccess.java (3)
Learnt from: infeo
PR: cryptomator/integrations-win#83
File: src/main/java/org/cryptomator/windows/keychain/WinHello.java:29-41
Timestamp: 2024-11-12T05:57:38.010Z
Learning: In the context of the Cryptomator Windows integration, it's acceptable to assume that required native libraries are installed on Windows, so explicit error handling for library loading may not be necessary.
Learnt from: purejava
PR: cryptomator/integrations-win#83
File: src/test/java/org/cryptomator/windows/keychain/WinHelloTest.java:21-29
Timestamp: 2024-11-18T20:41:44.031Z
Learning: In the test class `WinHelloTest` in `src/test/java/org/cryptomator/windows/keychain/WinHelloTest.java`, it's not necessary to validate the `KeychainAccessProvider` result or use `assertInstanceOf` in the `setup` method because the class hierarchy is known.
Learnt from: purejava
PR: cryptomator/integrations-win#83
File: src/main/java/org/cryptomator/windows/keychain/WindowsProtectedKeychainAccess.java:97-106
Timestamp: 2024-11-17T18:13:51.481Z
Learning: In the `storePassphrase` method of `WindowsProtectedKeychainAccess.java`, wrapping the encryption logic in a try-finally block to ensure sensitive data is cleared could alter the program flow because `KeychainAccessException` needs to propagate to the application using this library.
src/test/java/org/cryptomator/windows/keychain/WindowsKeychainAccessBaseTest.java (2)
Learnt from: purejava
PR: cryptomator/integrations-win#83
File: src/test/java/org/cryptomator/windows/keychain/WinHelloTest.java:21-29
Timestamp: 2024-11-18T20:41:44.031Z
Learning: In the test class `WinHelloTest` in `src/test/java/org/cryptomator/windows/keychain/WinHelloTest.java`, it's not necessary to validate the `KeychainAccessProvider` result or use `assertInstanceOf` in the `setup` method because the class hierarchy is known.
Learnt from: purejava
PR: cryptomator/integrations-win#83
File: src/test/java/org/cryptomator/windows/keychain/WinHelloTest.java:31-40
Timestamp: 2024-11-18T20:49:01.853Z
Learning: For `WinHelloTest`, prefer keeping tests concise by including only the specific test needed, without adding additional test cases or complexity if unnecessary.
src/main/java/org/cryptomator/windows/keychain/WindowsKeychainAccessBase.java (1)
Learnt from: purejava
PR: cryptomator/integrations-win#83
File: src/main/java/org/cryptomator/windows/keychain/WindowsProtectedKeychainAccess.java:97-106
Timestamp: 2024-11-17T18:13:51.481Z
Learning: In the `storePassphrase` method of `WindowsProtectedKeychainAccess.java`, wrapping the encryption logic in a try-finally block to ensure sensitive data is cleared could alter the program flow because `KeychainAccessException` needs to propagate to the application using this library.
🔇 Additional comments (7)
src/main/java/org/cryptomator/windows/keychain/Util.java (2)
1-29: LGTM: Package structure and imports are well-organized.
The imports are appropriate for the functionality provided by this utility class.
34-40:
Ensure proper expansion of '~' in path resolution
The current implementation has the same path resolution issue as previously identified. The Path class doesn't interpret '~' as the home directory, making this check unreliable.
src/main/java/org/cryptomator/windows/keychain/WindowsHello.java (1)
5-36: Implementation looks good and follows best practices
The WindowsHello class correctly implements the PassphraseCryptor interface using lazy loading of the native library, ensuring efficient resource management. The initialization-on-demand pattern is properly applied.
src/main/java/org/cryptomator/windows/keychain/FileKeychain.java (1)
105-106:
Correct 'getFirst()' to 'get(0)' when accessing 'keychainPaths' list
At line 105, the method attempts to access the first element of keychainPaths using getFirst(), but the List interface does not have a getFirst() method. This will cause a compilation error.
Apply this diff to fix the method call:
- var keychainFile = keychainPaths.getFirst(); //Note: we are always storing the keychain to the first entry to use the 'newest' keychain path and thus migrate old data
+ var keychainFile = keychainPaths.get(0); //Note: we are always storing the keychain to the first entry to use the 'newest' keychain path and thus migrate old dataLikely invalid or redundant comment.
src/main/java/org/cryptomator/windows/keychain/WindowsHelloKeychainAccess.java (1)
16-16: Consider validating keychain paths from system property
The paths from WINDOWS_HELLO_KEYCHAIN_PATHS_PROPERTY are used directly without validation.
Let's verify the current usage of this property:
Consider adding path validation in the constructor:
public WindowsHelloKeychainAccess() {
+ String paths = System.getProperty(WINDOWS_HELLO_KEYCHAIN_PATHS_PROPERTY);
+ if (paths != null) {
+ for (String path : paths.split(System.getProperty("path.separator"))) {
+ if (!new File(path).getParentFile().canWrite()) {
+ throw new IllegalStateException("Keychain path is not writable: " + path);
+ }
+ }
+ }
super(new FileKeychain(WINDOWS_HELLO_KEYCHAIN_PATHS_PROPERTY), new WindowsHello());
}Also applies to: 19-21
src/test/java/org/cryptomator/windows/keychain/FileKeychainTest.java (2)
48-52: Consider using initialized paths in FileKeychain constructor
The fileKeychain is initialized with an empty list (List.of()), while keychainPaths is initialized with three paths. This might lead to unexpected behavior in tests where the paths are needed. Consider using keychainPaths in the FileKeychain constructor if the paths are intended to be used.
1-220: Consider adding Windows Hello specific test cases
Given that this PR introduces Windows Hello support, consider adding test cases that verify:
- Integration with Windows Hello authentication
- Handling of Windows Hello specific errors
- Security aspects of the randomized signature approach
- Caching mechanism behavior (if implemented)
This would ensure that the Windows Hello integration is thoroughly tested.
src/test/java/org/cryptomator/windows/keychain/WindowsKeychainAccessBaseTest.java
Show resolved
Hide resolved
They need to return NULL in case of an error, due to the integrations API
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/main/java/org/cryptomator/windows/keychain/FileKeychain.java (3)
56-82: Consider enhancing logging for better diagnostics.While the implementation is correct, consider adding debug logging when:
- A keychain path is skipped
- Migration from old paths is successful
void loadInternal() throws KeychainAccessException { if (keychainPaths.isEmpty()) { throw new KeychainAccessException("No path specified to store keychain"); } boolean useExisting = false; for (Path keychainPath : keychainPaths) { Optional<Map<String, KeychainEntry>> maybeKeychain = parse(keychainPath); if (maybeKeychain.isPresent()) { cache = maybeKeychain.get(); useExisting = true; + LOG.debug("Successfully loaded keychain from {}", keychainPath); break; + } else { + LOG.debug("Skipped keychain path: {}", keychainPath); } }
98-99: Address TODO comment regarding IOException handling.The current implementation throws KeychainAccessException on IOException, which is appropriate for critical errors. However, if you want to make it more resilient:
Would you like me to propose an implementation that:
- Logs the IOException at WARN level
- Returns Optional.empty() for recoverable errors
- Only throws KeychainAccessException for critical permission/access issues?
103-113: Consider implementing atomic write operation.The current implementation might leave the file in an inconsistent state if the process crashes during write. Consider using atomic write operations:
synchronized void save() throws KeychainAccessException { var keychainFile = keychainPaths.getFirst(); LOG.debug("Writing keychain to {}", keychainFile); - try (OutputStream out = Files.newOutputStream(keychainFile, StandardOpenOption.WRITE, StandardOpenOption.CREATE, StandardOpenOption.TRUNCATE_EXISTING); + var tempFile = keychainFile.resolveSibling(keychainFile.getFileName() + ".tmp"); + try (OutputStream out = Files.newOutputStream(tempFile, StandardOpenOption.WRITE, StandardOpenOption.CREATE, StandardOpenOption.TRUNCATE_EXISTING); Writer writer = new OutputStreamWriter(out, UTF_8)) { JSON_MAPPER.writeValue(writer, cache); + Files.move(tempFile, keychainFile, StandardCopyOption.ATOMIC_MOVE, StandardCopyOption.REPLACE_EXISTING); } catch (IOException e) { + try { + Files.deleteIfExists(tempFile); + } catch (IOException suppressed) { + e.addSuppressed(suppressed); + } throw new KeychainAccessException("Could not write keychain to path " + keychainFile, e); } }src/test/java/org/cryptomator/windows/keychain/FileKeychainTest.java (1)
26-219: Consider adding tests for additional edge cases.The test coverage is good but could be enhanced with the following scenarios:
- Test concurrent access to verify thread safety
- Test handling of corrupted files (not just malformed JSON)
- Test handling of permission denied scenarios
Would you like me to provide implementations for these additional test cases?
🧰 Tools
🪛 ast-grep (0.31.1)
[warning] 47-49: Detected a cookie where the
Secureflag is either missing or disabled. TheSecurecookie flag instructs the browser to forbid sending the cookie over an insecure HTTP request. Set theSecureflag totrueso the cookie will only be sent over HTTPS.
Context: (keychainFileDir.resolve("keychain.json"), //
keychainFileDir.resolve("keychain_2.json"), //
keychainFileDir.resolve("keychain_3.json"))
Note: [CWE-614]: Sensitive Cookie in HTTPS Session Without 'Secure' Attribute [OWASP A05:2021]: Security Misconfiguration [REFERENCES]
- https://owasp.org/Top10/A05_2021-Security_Misconfiguration(missing-secure-java)
[warning] 59-59: Detected a cookie where the
Secureflag is either missing or disabled. TheSecurecookie flag instructs the browser to forbid sending the cookie over an insecure HTTP request. Set theSecureflag totrueso the cookie will only be sent over HTTPS.
Context: ("test3000", new KeychainEntry(new byte[] {}, new byte[] {}))
Note: [CWE-614]: Sensitive Cookie in HTTPS Session Without 'Secure' Attribute [OWASP A05:2021]: Security Misconfiguration [REFERENCES]
- https://owasp.org/Top10/A05_2021-Security_Misconfiguration(missing-secure-java)
[warning] 99-99: Detected a cookie where the
Secureflag is either missing or disabled. TheSecurecookie flag instructs the browser to forbid sending the cookie over an insecure HTTP request. Set theSecureflag totrueso the cookie will only be sent over HTTPS.
Context: ("test3000", new KeychainEntry(new byte[] {}, new byte[] {}))
Note: [CWE-614]: Sensitive Cookie in HTTPS Session Without 'Secure' Attribute [OWASP A05:2021]: Security Misconfiguration [REFERENCES]
- https://owasp.org/Top10/A05_2021-Security_Misconfiguration(missing-secure-java)
[warning] 125-125: Detected a cookie where the
Secureflag is either missing or disabled. TheSecurecookie flag instructs the browser to forbid sending the cookie over an insecure HTTP request. Set theSecureflag totrueso the cookie will only be sent over HTTPS.
Context: (new FileKeychain(keychainPaths))
Note: [CWE-614]: Sensitive Cookie in HTTPS Session Without 'Secure' Attribute [OWASP A05:2021]: Security Misconfiguration [REFERENCES]
- https://owasp.org/Top10/A05_2021-Security_Misconfiguration(missing-secure-java)
[warning] 147-147: Detected a cookie where the
Secureflag is either missing or disabled. TheSecurecookie flag instructs the browser to forbid sending the cookie over an insecure HTTP request. Set theSecureflag totrueso the cookie will only be sent over HTTPS.
Context: (keychainFileDir.resolve("foo/bar"))
Note: [CWE-614]: Sensitive Cookie in HTTPS Session Without 'Secure' Attribute [OWASP A05:2021]: Security Misconfiguration [REFERENCES]
- https://owasp.org/Top10/A05_2021-Security_Misconfiguration(missing-secure-java)
[warning] 188-188: Detected a cookie where the
Secureflag is either missing or disabled. TheSecurecookie flag instructs the browser to forbid sending the cookie over an insecure HTTP request. Set theSecureflag totrueso the cookie will only be sent over HTTPS.
Context: (Path.of("C:\foo\bar"))
Note: [CWE-614]: Sensitive Cookie in HTTPS Session Without 'Secure' Attribute [OWASP A05:2021]: Security Misconfiguration [REFERENCES]
- https://owasp.org/Top10/A05_2021-Security_Misconfiguration(missing-secure-java)
[warning] 189-189: Detected a cookie where the
Secureflag is either missing or disabled. TheSecurecookie flag instructs the browser to forbid sending the cookie over an insecure HTTP request. Set theSecureflag totrueso the cookie will only be sent over HTTPS.
Context: (Path.of("bar\kuz"))
Note: [CWE-614]: Sensitive Cookie in HTTPS Session Without 'Secure' Attribute [OWASP A05:2021]: Security Misconfiguration [REFERENCES]
- https://owasp.org/Top10/A05_2021-Security_Misconfiguration(missing-secure-java)
[warning] 202-202: Detected a cookie where the
Secureflag is either missing or disabled. TheSecurecookie flag instructs the browser to forbid sending the cookie over an insecure HTTP request. Set theSecureflag totrueso the cookie will only be sent over HTTPS.
Context: (System.getProperty("user.home"))
Note: [CWE-614]: Sensitive Cookie in HTTPS Session Without 'Secure' Attribute [OWASP A05:2021]: Security Misconfiguration [REFERENCES]
- https://owasp.org/Top10/A05_2021-Security_Misconfiguration(missing-secure-java)
[warning] 205-205: Detected a cookie where the
Secureflag is either missing or disabled. TheSecurecookie flag instructs the browser to forbid sending the cookie over an insecure HTTP request. Set theSecureflag totrueso the cookie will only be sent over HTTPS.
Context: (Path.of("this\~\not"))
Note: [CWE-614]: Sensitive Cookie in HTTPS Session Without 'Secure' Attribute [OWASP A05:2021]: Security Misconfiguration [REFERENCES]
- https://owasp.org/Top10/A05_2021-Security_Misconfiguration(missing-secure-java)
[warning] 206-206: Detected a cookie where the
Secureflag is either missing or disabled. TheSecurecookie flag instructs the browser to forbid sending the cookie over an insecure HTTP request. Set theSecureflag totrueso the cookie will only be sent over HTTPS.
Context: (userHome.resolve("foo\bar"))
Note: [CWE-614]: Sensitive Cookie in HTTPS Session Without 'Secure' Attribute [OWASP A05:2021]: Security Misconfiguration [REFERENCES]
- https://owasp.org/Top10/A05_2021-Security_Misconfiguration(missing-secure-java)
[warning] 214-214: Detected a cookie where the
Secureflag is either missing or disabled. TheSecurecookie flag instructs the browser to forbid sending the cookie over an insecure HTTP request. Set theSecureflag totrueso the cookie will only be sent over HTTPS.
Context: (Path.of("C:\foo"))
Note: [CWE-614]: Sensitive Cookie in HTTPS Session Without 'Secure' Attribute [OWASP A05:2021]: Security Misconfiguration [REFERENCES]
- https://owasp.org/Top10/A05_2021-Security_Misconfiguration(missing-secure-java)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/java/org/cryptomator/windows/keychain/FileKeychain.java(1 hunks)src/test/java/org/cryptomator/windows/keychain/FileKeychainTest.java(1 hunks)
🧰 Additional context used
🪛 ast-grep (0.31.1)
src/main/java/org/cryptomator/windows/keychain/FileKeychain.java
[warning] 45-45: Detected a cookie where the Secure flag is either missing or disabled. The Secure cookie flag instructs the browser to forbid sending the cookie over an insecure HTTP request. Set the Secure flag to true so the cookie will only be sent over HTTPS.
Context: (System.getProperty(keychainPathsProperty, ""), System.getProperty("path.separator"))
Note: [CWE-614]: Sensitive Cookie in HTTPS Session Without 'Secure' Attribute [OWASP A05:2021]: Security Misconfiguration [REFERENCES]
- https://owasp.org/Top10/A05_2021-Security_Misconfiguration
(missing-secure-java)
src/test/java/org/cryptomator/windows/keychain/FileKeychainTest.java
[warning] 47-49: Detected a cookie where the Secure flag is either missing or disabled. The Secure cookie flag instructs the browser to forbid sending the cookie over an insecure HTTP request. Set the Secure flag to true so the cookie will only be sent over HTTPS.
Context: (keychainFileDir.resolve("keychain.json"), //
keychainFileDir.resolve("keychain_2.json"), //
keychainFileDir.resolve("keychain_3.json"))
Note: [CWE-614]: Sensitive Cookie in HTTPS Session Without 'Secure' Attribute [OWASP A05:2021]: Security Misconfiguration [REFERENCES]
- https://owasp.org/Top10/A05_2021-Security_Misconfiguration
(missing-secure-java)
[warning] 59-59: Detected a cookie where the Secure flag is either missing or disabled. The Secure cookie flag instructs the browser to forbid sending the cookie over an insecure HTTP request. Set the Secure flag to true so the cookie will only be sent over HTTPS.
Context: ("test3000", new KeychainEntry(new byte[] {}, new byte[] {}))
Note: [CWE-614]: Sensitive Cookie in HTTPS Session Without 'Secure' Attribute [OWASP A05:2021]: Security Misconfiguration [REFERENCES]
- https://owasp.org/Top10/A05_2021-Security_Misconfiguration
(missing-secure-java)
[warning] 99-99: Detected a cookie where the Secure flag is either missing or disabled. The Secure cookie flag instructs the browser to forbid sending the cookie over an insecure HTTP request. Set the Secure flag to true so the cookie will only be sent over HTTPS.
Context: ("test3000", new KeychainEntry(new byte[] {}, new byte[] {}))
Note: [CWE-614]: Sensitive Cookie in HTTPS Session Without 'Secure' Attribute [OWASP A05:2021]: Security Misconfiguration [REFERENCES]
- https://owasp.org/Top10/A05_2021-Security_Misconfiguration
(missing-secure-java)
[warning] 125-125: Detected a cookie where the Secure flag is either missing or disabled. The Secure cookie flag instructs the browser to forbid sending the cookie over an insecure HTTP request. Set the Secure flag to true so the cookie will only be sent over HTTPS.
Context: (new FileKeychain(keychainPaths))
Note: [CWE-614]: Sensitive Cookie in HTTPS Session Without 'Secure' Attribute [OWASP A05:2021]: Security Misconfiguration [REFERENCES]
- https://owasp.org/Top10/A05_2021-Security_Misconfiguration
(missing-secure-java)
[warning] 147-147: Detected a cookie where the Secure flag is either missing or disabled. The Secure cookie flag instructs the browser to forbid sending the cookie over an insecure HTTP request. Set the Secure flag to true so the cookie will only be sent over HTTPS.
Context: (keychainFileDir.resolve("foo/bar"))
Note: [CWE-614]: Sensitive Cookie in HTTPS Session Without 'Secure' Attribute [OWASP A05:2021]: Security Misconfiguration [REFERENCES]
- https://owasp.org/Top10/A05_2021-Security_Misconfiguration
(missing-secure-java)
[warning] 188-188: Detected a cookie where the Secure flag is either missing or disabled. The Secure cookie flag instructs the browser to forbid sending the cookie over an insecure HTTP request. Set the Secure flag to true so the cookie will only be sent over HTTPS.
Context: (Path.of("C:\foo\bar"))
Note: [CWE-614]: Sensitive Cookie in HTTPS Session Without 'Secure' Attribute [OWASP A05:2021]: Security Misconfiguration [REFERENCES]
- https://owasp.org/Top10/A05_2021-Security_Misconfiguration
(missing-secure-java)
[warning] 189-189: Detected a cookie where the Secure flag is either missing or disabled. The Secure cookie flag instructs the browser to forbid sending the cookie over an insecure HTTP request. Set the Secure flag to true so the cookie will only be sent over HTTPS.
Context: (Path.of("bar\kuz"))
Note: [CWE-614]: Sensitive Cookie in HTTPS Session Without 'Secure' Attribute [OWASP A05:2021]: Security Misconfiguration [REFERENCES]
- https://owasp.org/Top10/A05_2021-Security_Misconfiguration
(missing-secure-java)
[warning] 202-202: Detected a cookie where the Secure flag is either missing or disabled. The Secure cookie flag instructs the browser to forbid sending the cookie over an insecure HTTP request. Set the Secure flag to true so the cookie will only be sent over HTTPS.
Context: (System.getProperty("user.home"))
Note: [CWE-614]: Sensitive Cookie in HTTPS Session Without 'Secure' Attribute [OWASP A05:2021]: Security Misconfiguration [REFERENCES]
- https://owasp.org/Top10/A05_2021-Security_Misconfiguration
(missing-secure-java)
[warning] 205-205: Detected a cookie where the Secure flag is either missing or disabled. The Secure cookie flag instructs the browser to forbid sending the cookie over an insecure HTTP request. Set the Secure flag to true so the cookie will only be sent over HTTPS.
Context: (Path.of("this\~\not"))
Note: [CWE-614]: Sensitive Cookie in HTTPS Session Without 'Secure' Attribute [OWASP A05:2021]: Security Misconfiguration [REFERENCES]
- https://owasp.org/Top10/A05_2021-Security_Misconfiguration
(missing-secure-java)
[warning] 206-206: Detected a cookie where the Secure flag is either missing or disabled. The Secure cookie flag instructs the browser to forbid sending the cookie over an insecure HTTP request. Set the Secure flag to true so the cookie will only be sent over HTTPS.
Context: (userHome.resolve("foo\bar"))
Note: [CWE-614]: Sensitive Cookie in HTTPS Session Without 'Secure' Attribute [OWASP A05:2021]: Security Misconfiguration [REFERENCES]
- https://owasp.org/Top10/A05_2021-Security_Misconfiguration
(missing-secure-java)
[warning] 214-214: Detected a cookie where the Secure flag is either missing or disabled. The Secure cookie flag instructs the browser to forbid sending the cookie over an insecure HTTP request. Set the Secure flag to true so the cookie will only be sent over HTTPS.
Context: (Path.of("C:\foo"))
Note: [CWE-614]: Sensitive Cookie in HTTPS Session Without 'Secure' Attribute [OWASP A05:2021]: Security Misconfiguration [REFERENCES]
- https://owasp.org/Top10/A05_2021-Security_Misconfiguration
(missing-secure-java)
🔇 Additional comments (2)
src/main/java/org/cryptomator/windows/keychain/FileKeychain.java (2)
35-44: LGTM! Well-structured class with proper thread-safety considerations.
The class is well-designed with:
- Thread-safe cache using ConcurrentHashMap
- Proper volatile flag for loaded state
- Clear interface implementation
161-165: 🛠️ Refactor suggestion
Implement writability check as suggested in TODO comment.
The current implementation only checks if paths exist but doesn't verify if they're writable, which could lead to runtime failures.
public boolean isSupported() {
- //TODO: actually, we would like the location to be writable as well
- return !keychainPaths.isEmpty();
+ if (keychainPaths.isEmpty()) {
+ return false;
+ }
+ try {
+ Path path = keychainPaths.getFirst();
+ if (Files.exists(path)) {
+ return Files.isWritable(path);
+ }
+ Path parent = path.getParent();
+ return parent != null && Files.isWritable(parent);
+ } catch (SecurityException e) {
+ LOG.warn("Security exception while checking keychain path writability", e);
+ return false;
+ }
}Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
README.md (1)
Line range hint
1-1: Add implementation details sectionConsider adding a new section documenting:
- The cryptographic approach used for Windows Hello integration
- Known limitations or behaviors (e.g., Windows Hello prompt frequency)
- Security considerations and threat model
This would help users and developers understand the implementation choices and security implications.
🧹 Nitpick comments (8)
README.md (2)
9-10: Documentation needs enhancement for the new Windows Hello propertiesWhile the new properties are documented, they would benefit from additional details:
- For
windowsHelloKeyId:
- Expected format/value
- Whether it needs to be manually configured or is auto-generated
- For
windowsHelloKeychainPaths:
- Example paths or format
- Relationship with existing
keychainPathsproperty- Whether multiple paths are supported (given plural name)
Consider adding a new section with example configurations and security considerations for Windows Hello integration.
Line range hint
15-19: Enhance development setup documentationGiven the C++/WinRT implementation and reported compilation issues, consider adding:
- IDE setup instructions (particularly for Visual Studio 2022)
- Required C++/WinRT components or SDK versions
- Troubleshooting section for common compilation issues
src/test/java/org/cryptomator/windows/common/WinStringsTest.java (2)
13-16: Expand test coverage with more edge casesThe current test cases only cover basic ASCII strings. Consider adding:
- Empty string
- Unicode characters
- Special Windows characters (path separators, etc.)
- Maximum length strings
@CsvSource(value = { // convert utf16-le string to hex with https://dencode.com/en/string/hex, append null terminator "foo, '66 00 6f 00 6f 00 00 00'", - "bar, '62 00 61 00 72 00 00 00'" + "bar, '62 00 61 00 72 00 00 00'", + "'', '00 00'", + "'Hello 世界', '48 00 65 00 6C 00 6C 00 6F 00 20 00 16 4E 4C 75 00 00'", + "'C:\\Windows\\Path', '43 00 3A 00 5C 00 57 00 69 00 6E 00 64 00 6F 00 77 00 73 00 5C 00 50 00 61 00 74 00 68 00 00 00'"
3-3: Avoid test class dependenciesThe test relies on
ByteArrayConverterfromWinShellLinksTest. Consider moving the converter to a common test utility class or implementing it locally.src/main/java/org/cryptomator/windows/autostart/WinShellLinks.java (1)
23-27: Consider enhancing error handling for HRESULT codesThe method returns raw HRESULT codes. Consider wrapping these in meaningful exceptions or providing a way to translate them to user-friendly messages.
+ /** + * Maps Windows HRESULT codes to meaningful messages + * @param hresult The HRESULT code returned by native method + * @return A human-readable error message + */ + private String getErrorMessage(int hresult) { + switch (hresult) { + case 0: return "Success"; + case 0x80004005: return "Unspecified error creating shortcut"; + case 0x80070003: return "Path not found"; + default: return String.format("Unknown error (0x%08X)", hresult); + } + }src/main/native/org_cryptomator_windows_keychain_WindowsHello_Native.cpp (3)
23-24: Consider encapsulating global variables.The global variables could be encapsulated within a class or namespace to prevent potential naming conflicts and improve maintainability.
+namespace WindowsHelloUtils { + static std::atomic<int> g_promptFocusCount{ 0 }; + static IBuffer info = CryptographicBuffer::ConvertStringToBinary(L"EncryptionKey", BinaryStringEncoding::Utf8); +}
129-129: Optimize parameter passing.The
keyIdparameter should be passed by const reference to avoid unnecessary copying of the string.-bool deriveEncryptionKey(const std::wstring keyId, const std::vector<uint8_t>& challenge, IBuffer& key) +bool deriveEncryptionKey(const std::wstring& keyId, const std::vector<uint8_t>& challenge, IBuffer& key)🧰 Tools
🪛 cppcheck (2.10-2)
[performance] 129-129: Function parameter 'keyId' should be passed by const reference.
(passedByValue)
253-266: Enhance error reporting for better diagnostics.Consider adding more context to error messages, such as the operation being performed when the error occurred.
-std::cerr << "Error: " << winrt::to_string(message) << " (HRESULT: 0x" << std::hex << hr << ")" << std::endl; +std::cerr << "Error during encryption: " << winrt::to_string(message) << " (HRESULT: 0x" << std::hex << hr << ")" << std::endl;Also applies to: 342-355
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
README.md(1 hunks)src/main/headers/org_cryptomator_windows_keychain_WindowsHello_Native.h(1 hunks)src/main/java/org/cryptomator/windows/autostart/WinShellLinks.java(2 hunks)src/main/java/org/cryptomator/windows/common/WinStrings.java(1 hunks)src/main/java/org/cryptomator/windows/keychain/WindowsHello.java(1 hunks)src/main/java/org/cryptomator/windows/keychain/WindowsHelloKeychainAccess.java(1 hunks)src/main/native/org_cryptomator_windows_keychain_WindowsHello_Native.cpp(1 hunks)src/test/java/org/cryptomator/windows/autostart/WinShellLinksTest.java(0 hunks)src/test/java/org/cryptomator/windows/common/WinStringsTest.java(1 hunks)src/test/java/org/cryptomator/windows/keychain/WindowsHelloTest.java(1 hunks)
💤 Files with no reviewable changes (1)
- src/test/java/org/cryptomator/windows/autostart/WinShellLinksTest.java
🚧 Files skipped from review as they are similar to previous changes (3)
- src/main/java/org/cryptomator/windows/keychain/WindowsHelloKeychainAccess.java
- src/test/java/org/cryptomator/windows/keychain/WindowsHelloTest.java
- src/main/java/org/cryptomator/windows/keychain/WindowsHello.java
🧰 Additional context used
🪛 cppcheck (2.10-2)
src/main/native/org_cryptomator_windows_keychain_WindowsHello_Native.cpp
[performance] 129-129: Function parameter 'keyId' should be passed by const reference.
(passedByValue)
🔇 Additional comments (3)
src/main/headers/org_cryptomator_windows_keychain_WindowsHello_Native.h (1)
1-37: LGTM! Well-structured JNI header file.
The header file follows JNI conventions with proper include guards and extern "C" linkage specification. The method declarations are clear and well-documented.
src/main/native/org_cryptomator_windows_keychain_WindowsHello_Native.cpp (2)
1-22: LGTM! Comprehensive header inclusions.
All necessary headers for Windows Hello integration and cryptographic operations are properly included.
242-248: Use secure memory wiping for sensitive data.
The use of std::fill to clear sensitive data may not guarantee that the data is actually erased from memory.
Also applies to: 330-337
|
Sooo, @purejava hope you can still see your contribution here 😅 I adjusted the code at some places, most remarkable changes:
If you have questions, comments or feedback, just comment here or in the code. I'll make a vertical test soon to see how Cryptomator works with it. |
That's totally fine @infeo. Thanks for the deep review of the PR and the changes.
Looking forward to your results: 😄 |
This adds the option to require user authentication to access a vault, secured by Windows Hello.
The base implementation is the same as I did it for Touch ID before, and so depends on cryptomator/cryptomator#3311 as well.
Native implementation
Compared to the Mac, Windows has some limitations with regard to Windows Hello, which neither offers a mechanism to secure a vault passphrase with symmetric encryption, nor an option to secure secrets with Windows Hello directly.
Reading this, I decided to go the same direction and implement symmetric encryption, but with a randomized signature due to security reasons.
This works as follows, see below.
- Create a randomized challenge; to keep things simple, I used the salt created inWindowsProtectedKeychainAccessanywayUse this hash as the encryption key for the vault passphrase with AesCbcPkcs7Compiler and Framework
To keep the code as simple as possible, I decided to use C++/WinRT. The Buildtools for Visual Studio 2022 provide the Windows Runtime header files and the required clang compiler. I could not get it to compile with g++, although it's supposed to be compatible to the Microsoft compiler.
Status
The implementation is feature ready and working. However, I am not convinced about the Windows Hello prompt popping up that often. Three times on a common unlock (check_if_passphrase_is_stored, load the passphrase and update it). There are even more prompts popping up on checking the check-box in the settings to enable Windows Hello, as the existing keychain entries are migrated.
There needs to be a solution designed for this, a mechanism to cache the passphrase in memory for a certain amount of time, but I need your OK to proceed into this direction.
I decided to open this PR to ask for an opinion on the previous question and to start discussion on the implementation so far.
To-Do list
integrations-winproviders, as they were compiled with clang; Unit tests are ok toughintegrations-winusing the Windows Runtime can be compiled with CIintegrations-winin an IDE; right now, this can be done using the Developer command promptEdit: update description for encryption