Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Replace the deprecated SecKeychain with SecItem #46

Merged
merged 7 commits into from
Jul 1, 2024
Merged

Replace the deprecated SecKeychain with SecItem #46

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
8c73db9
Implement MacSystemKeychainAccess using SecItem API
purejava Feb 18, 2024
9fc29ce
kSecValueData must be of type NSData
purejava May 26, 2024
372d4fe
Set access control to default for testing purposes
purejava May 26, 2024
cd0cb40
Reduce to code without access control
purejava May 27, 2024
77c6e10
Fix indentation
purejava May 30, 2024
85a1a50
Bump the maven-build-plugins group across 1 directory with 8 updates …
dependabot[bot] Jun 3, 2024
0fcd405
Minor refactorings
tobihagemann Jul 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 8 additions & 8 deletions pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,8 +38,8 @@
<mockito.version>5.11.0</mockito.version>

<!-- build plugin dependencies -->
<dependency-check.version>9.1.0</dependency-check.version>
<nexus-staging.version>1.6.13</nexus-staging.version>
<dependency-check.version>9.2.0</dependency-check.version>
<nexus-staging.version>1.6.14</nexus-staging.version>
</properties>

<licenses>
Expand Down Expand Up @@ -116,7 +116,7 @@
<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-enforcer-plugin</artifactId>
<version>3.4.1</version>
<version>3.5.0</version>
<executions>
<execution>
<id>check-preconditions</id>
Expand Down Expand Up @@ -145,7 +145,7 @@
<plugin>
<groupId>org.codehaus.mojo</groupId>
<artifactId>exec-maven-plugin</artifactId>
<version>3.2.0</version>
<version>3.3.0</version>
<executions>
<execution>
<goals>
Expand Down Expand Up @@ -203,7 +203,7 @@
</plugin>
<plugin>
<artifactId>maven-source-plugin</artifactId>
<version>3.3.0</version>
<version>3.3.1</version>
<executions>
<execution>
<id>attach-sources</id>
Expand All @@ -215,7 +215,7 @@
</plugin>
<plugin>
<artifactId>maven-javadoc-plugin</artifactId>
<version>3.6.3</version>
<version>3.7.0</version>
<executions>
<execution>
<id>attach-javadocs</id>
Expand Down Expand Up @@ -307,7 +307,7 @@
<plugins>
<plugin>
<artifactId>maven-gpg-plugin</artifactId>
<version>3.2.2</version>
<version>3.2.4</version>
<executions>
<execution>
<id>sign-artifacts</id>
Expand Down Expand Up @@ -368,7 +368,7 @@
<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-deploy-plugin</artifactId>
<version>3.1.1</version>
<version>3.1.2</version>
</plugin>
</plugins>
</build>
Expand Down
142 changes: 64 additions & 78 deletions src/main/native/org_cryptomator_macos_keychain_MacKeychain_Native.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,117 +7,103 @@
//

#import "org_cryptomator_macos_keychain_MacKeychain_Native.h"
#import <Foundation/Foundation.h>
#import <Security/Security.h>

JNIEXPORT jint JNICALL Java_org_cryptomator_macos_keychain_MacKeychain_00024Native_storePassword(JNIEnv *env, jobject thisObj, jbyteArray service, jbyteArray key, jbyteArray password) {
const int serviceLen = (*env)->GetArrayLength(env, service);
jbyte *serviceStr = (*env)->GetByteArrayElements(env, service, NULL);
const int keyLen = (*env)->GetArrayLength(env, key);
jbyte *keyStr = (*env)->GetByteArrayElements(env, key, NULL);
const int pwLen = (*env)->GetArrayLength(env, password);
jbyte *pwStr = (*env)->GetByteArrayElements(env, password, NULL);
jsize length = (*env)->GetArrayLength(env, password);

// find existing:
SecKeychainItemRef itemRef = NULL;
OSStatus status = SecKeychainFindGenericPassword(
NULL, // default keychain
serviceLen, // length of service name
(char *)serviceStr, // service name
keyLen, // length of account name
(char *)keyStr, // account name
NULL, // length of password
NULL, // pointer to password data
&itemRef // the item reference
);
if (status == errSecSuccess) {
NSDictionary *query = @{
(__bridge id)kSecClass: (__bridge id)kSecClassGenericPassword,
(__bridge id)kSecAttrService: [NSString stringWithCString:(char *)serviceStr encoding:NSUTF8StringEncoding],
(__bridge id)kSecAttrAccount: [NSString stringWithCString:(char *)keyStr encoding:NSUTF8StringEncoding],
(__bridge id)kSecReturnAttributes: @YES,
(__bridge id)kSecReturnData: @YES,
(__bridge id)kSecMatchLimit: (__bridge id)kSecMatchLimitOne
};
CFDictionaryRef result = NULL;
OSStatus status = SecItemCopyMatching((__bridge CFDictionaryRef)query, (CFTypeRef *)&result);
if (status == errSecSuccess && result != NULL) {
// update existing:
status = SecKeychainItemModifyAttributesAndData(
itemRef, // the item reference
NULL, // no change to attributes
pwLen, // length of password
pwStr // pointer to password data
);
NSDictionary *attributesToUpdate = @{
(__bridge id)kSecValueData: [NSData dataWithBytes:pwStr length:length]
};
status = SecItemUpdate((__bridge CFDictionaryRef)query, (__bridge CFDictionaryRef)attributesToUpdate);
} else if (status == errSecItemNotFound) {
// add new:
status = SecKeychainAddGenericPassword(
NULL, // default keychain
serviceLen, // length of service name
(char *)serviceStr, // service name
keyLen, // length of account name
(char *)keyStr, // account name
pwLen, // length of password
pwStr, // pointer to password data
NULL // the item reference
);
NSDictionary *attributes = @{
(__bridge id)kSecClass: (__bridge id)kSecClassGenericPassword,
(__bridge id)kSecAttrService: [NSString stringWithCString:(char *)serviceStr encoding:NSUTF8StringEncoding],
(__bridge id)kSecAttrAccount: [NSString stringWithCString:(char *)keyStr encoding:NSUTF8StringEncoding],
(__bridge id)kSecValueData: [NSData dataWithBytes:pwStr length:length]
};
status = SecItemAdd((__bridge CFDictionaryRef)attributes, NULL);
} else {
NSLog(@"Error storing item in keychain. Status code: %d", (int)status);
}
Comment on lines +29 to 47
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Refactor to improve error handling and reduce code duplication.

The current code can be refactored to use a switch statement for better readability and maintainability.

-	if (status == errSecSuccess && result != NULL) {
-		// update existing:
-		NSDictionary *attributesToUpdate = @{
-			(__bridge id)kSecValueData: [NSData dataWithBytes:pwStr length:length]
-		};
-		status = SecItemUpdate((__bridge CFDictionaryRef)query, (__bridge CFDictionaryRef)attributesToUpdate);
-	} else if (status == errSecItemNotFound) {
-		// add new:
-		NSDictionary *attributes = @{
-			(__bridge id)kSecClass: (__bridge id)kSecClassGenericPassword,
-			(__bridge id)kSecAttrService: [NSString stringWithCString:(char *)serviceStr encoding:NSUTF8StringEncoding],
-			(__bridge id)kSecAttrAccount: [NSString stringWithCString:(char *)keyStr encoding:NSUTF8StringEncoding],
-			(__bridge id)kSecValueData: [NSData dataWithBytes:pwStr length:length]
-		};
-		status = SecItemAdd((__bridge CFDictionaryRef)attributes, NULL);
-	} else {
-		NSLog(@"Error storing item in keychain. Status code: %d", (int)status);
-	}
+	switch (status) {
+		case errSecSuccess:
+			if (result != NULL) {
+				NSDictionary *attributesToUpdate = @{(__bridge id)kSecValueData: [NSData dataWithBytes:pwStr length:length]};
+				status = SecItemUpdate((__bridge CFDictionaryRef)query, (__bridge CFDictionaryRef)attributesToUpdate);
+			}
+			break;
+		case errSecItemNotFound:
+			NSDictionary *attributes = @{
+				(__bridge id)kSecClass: (__bridge id)kSecClassGenericPassword,
+				(__bridge id)kSecAttrService: [NSString stringWithCString:(char *)serviceStr encoding:NSUTF8StringEncoding],
+				(__bridge id)kSecAttrAccount: [NSString stringWithCString:(char *)keyStr encoding:NSUTF8StringEncoding],
+				(__bridge id)kSecValueData: [NSData dataWithBytes:pwStr length:length]
+			};
+			status = SecItemAdd((__bridge CFDictionaryRef)attributes, NULL);
+			break;
+		default:
+			NSLog(@"Error storing item in keychain. Status code: %d", (int)status);
+			break;
+	}
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
OSStatus status = SecItemCopyMatching((__bridge CFDictionaryRef)query, (CFTypeRef *)&result);
if (status == errSecSuccess && result != NULL) {
// update existing:
status = SecKeychainItemModifyAttributesAndData(
itemRef, // the item reference
NULL, // no change to attributes
pwLen, // length of password
pwStr // pointer to password data
);
NSDictionary *attributesToUpdate = @{
(__bridge id)kSecValueData: [NSData dataWithBytes:pwStr length:length]
};
status = SecItemUpdate((__bridge CFDictionaryRef)query, (__bridge CFDictionaryRef)attributesToUpdate);
} else if (status == errSecItemNotFound) {
// add new:
status = SecKeychainAddGenericPassword(
NULL, // default keychain
serviceLen, // length of service name
(char *)serviceStr, // service name
keyLen, // length of account name
(char *)keyStr, // account name
pwLen, // length of password
pwStr, // pointer to password data
NULL // the item reference
);
NSDictionary *attributes = @{
(__bridge id)kSecClass: (__bridge id)kSecClassGenericPassword,
(__bridge id)kSecAttrService: [NSString stringWithCString:(char *)serviceStr encoding:NSUTF8StringEncoding],
(__bridge id)kSecAttrAccount: [NSString stringWithCString:(char *)keyStr encoding:NSUTF8StringEncoding],
(__bridge id)kSecValueData: [NSData dataWithBytes:pwStr length:length]
};
status = SecItemAdd((__bridge CFDictionaryRef)attributes, NULL);
} else {
NSLog(@"Error storing item in keychain. Status code: %d", (int)status);
}
OSStatus status = SecItemCopyMatching((__bridge CFDictionaryRef)query, (CFTypeRef *)&result);
switch (status) {
case errSecSuccess:
if (result != NULL) {
NSDictionary *attributesToUpdate = @{(__bridge id)kSecValueData: [NSData dataWithBytes:pwStr length:length]};
status = SecItemUpdate((__bridge CFDictionaryRef)query, (__bridge CFDictionaryRef)attributesToUpdate);
}
break;
case errSecItemNotFound:
NSDictionary *attributes = @{
(__bridge id)kSecClass: (__bridge id)kSecClassGenericPassword,
(__bridge id)kSecAttrService: [NSString stringWithCString:(char *)serviceStr encoding:NSUTF8StringEncoding],
(__bridge id)kSecAttrAccount: [NSString stringWithCString:(char *)keyStr encoding:NSUTF8StringEncoding],
(__bridge id)kSecValueData: [NSData dataWithBytes:pwStr length:length]
};
status = SecItemAdd((__bridge CFDictionaryRef)attributes, NULL);
break;
default:
NSLog(@"Error storing item in keychain. Status code: %d", (int)status);
break;
}


(*env)->ReleaseByteArrayElements(env, service, serviceStr, JNI_ABORT);
(*env)->ReleaseByteArrayElements(env, key, keyStr, JNI_ABORT);
(*env)->ReleaseByteArrayElements(env, password, pwStr, JNI_ABORT);
if (itemRef) {
CFRelease(itemRef);
}
return status;
}

JNIEXPORT jbyteArray JNICALL Java_org_cryptomator_macos_keychain_MacKeychain_00024Native_loadPassword(JNIEnv *env, jobject thisObj, jbyteArray service, jbyteArray key) {
const int serviceLen = (*env)->GetArrayLength(env, service);
jbyte *serviceStr = (*env)->GetByteArrayElements(env, service, NULL);
const int keyLen = (*env)->GetArrayLength(env, key);
jbyte *keyStr = (*env)->GetByteArrayElements(env, key, NULL);
void *pwStr = NULL;
UInt32 pwLen;
OSStatus status = SecKeychainFindGenericPassword(
NULL, // default keychain
serviceLen, // length of service name
(char *)serviceStr, // service name
keyLen, // length of account name
(char *)keyStr, // account name
&pwLen, // length of password
&pwStr, // pointer to password data
NULL // the item reference
);

jbyteArray result;
if (status == errSecSuccess) {
result = (*env)->NewByteArray(env, pwLen);
(*env)->SetByteArrayRegion(env, result, 0, pwLen, pwStr);
} else {
result = NULL;
// find existing:
NSDictionary *query = @{
(__bridge id)kSecClass: (__bridge id)kSecClassGenericPassword,
(__bridge id)kSecAttrService: [NSString stringWithCString:(char *)serviceStr encoding:NSUTF8StringEncoding],
(__bridge id)kSecAttrAccount: [NSString stringWithCString:(char *)keyStr encoding:NSUTF8StringEncoding],
(__bridge id)kSecReturnAttributes: @YES,
(__bridge id)kSecReturnData: @YES,
(__bridge id)kSecMatchLimit: (__bridge id)kSecMatchLimitOne
};
CFDictionaryRef result = NULL;
OSStatus status = SecItemCopyMatching((__bridge CFDictionaryRef)query, (CFTypeRef *)&result);
jbyteArray password = NULL;
if (status == errSecSuccess && result != NULL) {
// retrieve password:
NSDictionary *attributes = (__bridge_transfer NSDictionary *)result;
NSData *passwordData = attributes[(__bridge id)kSecValueData];
password = (*env)->NewByteArray(env, (int)passwordData.length);
(*env)->SetByteArrayRegion(env, password, 0, (int)passwordData.length, (jbyte *)passwordData.bytes);
} else if (status != errSecItemNotFound) {
NSLog(@"Error retrieving item from keychain. Status code: %d", (int)status);
}
Comment on lines +69 to 79
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Refactor to improve error handling and reduce code duplication.

The current code can be refactored to use a switch statement for better readability and maintainability.

-	if (status == errSecSuccess && result != NULL) {
-		// retrieve password:
-		NSDictionary *attributes = (__bridge_transfer NSDictionary *)result;
-		NSData *passwordData = attributes[(__bridge id)kSecValueData];
-		password = (*env)->NewByteArray(env, (int)passwordData.length);
-		(*env)->SetByteArrayRegion(env, password, 0, (int)passwordData.length, (jbyte *)passwordData.bytes);
-	} else if (status != errSecItemNotFound) {
-		NSLog(@"Error retrieving item from keychain. Status code: %d", (int)status);
-	}
+	switch (status) {
+		case errSecSuccess:
+			if (result != NULL) {
+				NSDictionary *attributes = (__bridge_transfer NSDictionary *)result;
+				NSData *passwordData = attributes[(__bridge id)kSecValueData];
+				password = (*env)->NewByteArray(env, (int)passwordData.length);
+				(*env)->SetByteArrayRegion(env, password, 0, (int)passwordData.length, (jbyte *)passwordData.bytes);
+			}
+			break;
+		default:
+			NSLog(@"Error retrieving item from keychain. Status code: %d", (int)status);
+			break;
+	}
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
OSStatus status = SecItemCopyMatching((__bridge CFDictionaryRef)query, (CFTypeRef *)&result);
jbyteArray password = NULL;
if (status == errSecSuccess && result != NULL) {
// retrieve password:
NSDictionary *attributes = (__bridge_transfer NSDictionary *)result;
NSData *passwordData = attributes[(__bridge id)kSecValueData];
password = (*env)->NewByteArray(env, (int)passwordData.length);
(*env)->SetByteArrayRegion(env, password, 0, (int)passwordData.length, (jbyte *)passwordData.bytes);
} else if (status != errSecItemNotFound) {
NSLog(@"Error retrieving item from keychain. Status code: %d", (int)status);
}
OSStatus status = SecItemCopyMatching((__bridge CFDictionaryRef)query, (CFTypeRef *)&result);
jbyteArray password = NULL;
switch (status) {
case errSecSuccess:
if (result != NULL) {
NSDictionary *attributes = (__bridge_transfer NSDictionary *)result;
NSData *passwordData = attributes[(__bridge id)kSecValueData];
password = (*env)->NewByteArray(env, (int)passwordData.length);
(*env)->SetByteArrayRegion(env, password, 0, (int)passwordData.length, (jbyte *)passwordData.bytes);
}
break;
default:
NSLog(@"Error retrieving item from keychain. Status code: %d", (int)status);
break;
}


(*env)->ReleaseByteArrayElements(env, service, serviceStr, JNI_ABORT);
(*env)->ReleaseByteArrayElements(env, key, keyStr, JNI_ABORT);
if (pwStr) {
SecKeychainItemFreeContent(NULL, pwStr);
}
return result;
return password;
}

JNIEXPORT jint JNICALL Java_org_cryptomator_macos_keychain_MacKeychain_00024Native_deletePassword(JNIEnv *env, jobject thisObj, jbyteArray service, jbyteArray key) {
const int serviceLen = (*env)->GetArrayLength(env, service);
jbyte *serviceStr = (*env)->GetByteArrayElements(env, service, NULL);
const int keyLen = (*env)->GetArrayLength(env, key);
jbyte *keyStr = (*env)->GetByteArrayElements(env, key, NULL);
SecKeychainItemRef itemRef = NULL;
OSStatus status = SecKeychainFindGenericPassword(
NULL, // default keychain
serviceLen, // length of service name
(char *)serviceStr, // service name
keyLen, // length of account name
(char *)keyStr, // account name
NULL, // length of password
NULL, // pointer to password data
&itemRef // the item reference
);

if (status == errSecSuccess) {
status = SecKeychainItemDelete(
itemRef // the item reference
);
// find existing:
NSDictionary *query = @{
(__bridge id)kSecClass: (__bridge id)kSecClassGenericPassword,
(__bridge id)kSecAttrService: [NSString stringWithCString:(char *)serviceStr encoding:NSUTF8StringEncoding],
(__bridge id)kSecAttrAccount: [NSString stringWithCString:(char *)keyStr encoding:NSUTF8StringEncoding],
(__bridge id)kSecMatchLimit: (__bridge id)kSecMatchLimitOne
};
CFDictionaryRef result = NULL;
OSStatus status = SecItemCopyMatching((__bridge CFDictionaryRef)query, (CFTypeRef *)&result);
if (status == errSecSuccess && result != NULL) {
// delete:
status = SecItemDelete((__bridge CFDictionaryRef)query);
} else if (status != errSecItemNotFound) {
NSLog(@"Error deleting item from keychain. Status code: %d", (int)status);
}
Comment on lines +98 to 104
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Refactor to improve error handling and reduce code duplication.

The current code can be refactored to use a switch statement for better readability and maintainability.

-	if (status == errSecSuccess && result != NULL) {
-		// delete:
-		status = SecItemDelete((__bridge CFDictionaryRef)query);
-	} else if (status != errSecItemNotFound) {
-		NSLog(@"Error deleting item from keychain. Status code: %d", (int)status);
-	}
+	switch (status) {
+		case errSecSuccess:
+			if (result != NULL) {
+				status = SecItemDelete((__bridge CFDictionaryRef)query);
+			}
+			break;
+		default:
+			NSLog(@"Error deleting item from keychain. Status code: %d", (int)status);
+			break;
+	}
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
OSStatus status = SecItemCopyMatching((__bridge CFDictionaryRef)query, (CFTypeRef *)&result);
if (status == errSecSuccess && result != NULL) {
// delete:
status = SecItemDelete((__bridge CFDictionaryRef)query);
} else if (status != errSecItemNotFound) {
NSLog(@"Error deleting item from keychain. Status code: %d", (int)status);
}
OSStatus status = SecItemCopyMatching((__bridge CFDictionaryRef)query, (CFTypeRef *)&result);
switch (status) {
case errSecSuccess:
if (result != NULL) {
status = SecItemDelete((__bridge CFDictionaryRef)query);
}
break;
default:
NSLog(@"Error deleting item from keychain. Status code: %d", (int)status);
break;
}


(*env)->ReleaseByteArrayElements(env, service, serviceStr, JNI_ABORT);
(*env)->ReleaseByteArrayElements(env, key, keyStr, JNI_ABORT);
if (itemRef) {
CFRelease(itemRef);
}
return status;
}
Loading