From 952eedf4ab45c810d008c949ca2f16fdada82990 Mon Sep 17 00:00:00 2001 From: Simon Abykov <90252673+abyss638@users.noreply.github.com> Date: Tue, 17 Sep 2024 00:10:38 +0100 Subject: [PATCH 1/2] Handle properly non-ascii values in credentials --- .../macosx/KeychainSecurityCliStore.java | 39 ++++++++++++++----- .../StorageProviderTest.java | 20 ++++++++-- 2 files changed, 47 insertions(+), 12 deletions(-) diff --git a/src/main/java/com/microsoft/credentialstorage/implementation/macosx/KeychainSecurityCliStore.java b/src/main/java/com/microsoft/credentialstorage/implementation/macosx/KeychainSecurityCliStore.java index be6f4f9..5b066f7 100644 --- a/src/main/java/com/microsoft/credentialstorage/implementation/macosx/KeychainSecurityCliStore.java +++ b/src/main/java/com/microsoft/credentialstorage/implementation/macosx/KeychainSecurityCliStore.java @@ -11,6 +11,7 @@ import java.io.InputStreamReader; import java.io.PrintWriter; import java.io.StringReader; +import java.nio.charset.StandardCharsets; import java.util.HashMap; import java.util.Map; import java.util.regex.Matcher; @@ -37,17 +38,17 @@ public class KeychainSecurityCliStore { private static final Pattern MetadataLinePattern = Pattern.compile ( // ^(\w+):\s"(.+)" - "^(\\w+):\\s\"(.+)\"" + "^(\\w+):\\s((?:0x[0-9A-Fa-f]*\\s+)?\"(.+)\")" ); - enum SecretKind { + protected enum SecretKind { Credential, Token, TokenPair_Access_Token, TokenPair_Refresh_Token } - enum AttributeParsingState { + protected enum AttributeParsingState { Spaces, StringKey, HexKey, @@ -115,7 +116,7 @@ private static void parseMetadataLine(final String line, final Map credentialStorage = StorageProvider.getCredentialStorage(true, StorageProvider.SecureOption.PREFERRED); + assertTrue(credentialStorage.isSecure()); + + StoredCredential credentialsToStore = new StoredCredential("TästUser", "TästPassword".toCharArray()); + credentialStorage.add(key, credentialsToStore); + + StoredCredential stored = credentialStorage.get(key); + assertEquals("TästUser", stored.getUsername()); + assertEquals("TästPassword", new String(stored.getPassword())); + } + private SecretStore getStore(final boolean secure) { return new SecretStore<>() { @Override From 09b3b4bb10d714629119f806db3e05d081a5de14 Mon Sep 17 00:00:00 2001 From: Simon Abykov <90252673+abyss638@users.noreply.github.com> Date: Tue, 17 Sep 2024 12:24:59 +0100 Subject: [PATCH 2/2] Handle properly non-ascii and special characters in credentials --- .../macosx/KeychainSecurityCliStore.java | 15 +++++++++++-- .../StorageProviderTest.java | 16 -------------- ...ychainSecurityBackedCredentialStoreIT.java | 15 +++++++++++++ ...eychainSecurityBackedTokenPairStoreIT.java | 21 ++++++++++++++----- .../KeychainSecurityBackedTokenStoreIT.java | 14 +++++++++++++ 5 files changed, 58 insertions(+), 23 deletions(-) diff --git a/src/main/java/com/microsoft/credentialstorage/implementation/macosx/KeychainSecurityCliStore.java b/src/main/java/com/microsoft/credentialstorage/implementation/macosx/KeychainSecurityCliStore.java index 5b066f7..c72633b 100644 --- a/src/main/java/com/microsoft/credentialstorage/implementation/macosx/KeychainSecurityCliStore.java +++ b/src/main/java/com/microsoft/credentialstorage/implementation/macosx/KeychainSecurityCliStore.java @@ -26,7 +26,7 @@ public class KeychainSecurityCliStore { private static final String ACCOUNT_PARAMETER = "-a"; private static final String SERVICE_PARAMETER = "-s"; private static final String KIND_PARAMETER = "-D"; - private static final String PASSWORD_PARAMETER = "-w"; + private static final String PASSWORD_PARAMETER = "-X"; private static final String UPDATE_IF_ALREADY_EXISTS = "-U"; private static final int ITEM_NOT_FOUND_EXIT_CODE = 44; private static final int USER_INTERACTION_NOT_ALLOWED_EXIT_CODE = 36; @@ -310,7 +310,7 @@ protected static void write(final SecretKind secretKind, final String serviceNam UPDATE_IF_ALREADY_EXISTS, ACCOUNT_PARAMETER, accountName, SERVICE_PARAMETER, serviceName, - PASSWORD_PARAMETER, password, + PASSWORD_PARAMETER, getHexString(password), KIND_PARAMETER, secretKind.name() }; final Process process = addProcessBuilder.start(); @@ -389,4 +389,15 @@ private static String hexStringToString(String hexString) { } return new String(bytes, StandardCharsets.UTF_8); } + + private static String getHexString(char[] input) { + StringBuilder hexString = new StringBuilder(); + + byte[] bytes = new String(input).getBytes(StandardCharsets.UTF_8); + for (byte b : bytes) { + hexString.append(Integer.toHexString(b & 0xFF)); + } + + return hexString.toString(); + } } diff --git a/src/test/java/com/microsoft/credentialstorage/StorageProviderTest.java b/src/test/java/com/microsoft/credentialstorage/StorageProviderTest.java index b8e1a3f..a2f46a9 100644 --- a/src/test/java/com/microsoft/credentialstorage/StorageProviderTest.java +++ b/src/test/java/com/microsoft/credentialstorage/StorageProviderTest.java @@ -3,7 +3,6 @@ package com.microsoft.credentialstorage; -import com.microsoft.credentialstorage.model.StoredCredential; import com.microsoft.credentialstorage.model.StoredToken; import org.junit.Test; @@ -65,21 +64,6 @@ public void nonPersisted_doNotCareSecurity_shouldUseGenerator() { assertFalse(actual.isSecure()); } - @Test - public void persisted_shouldStoreNonAscii() { - final String key = "NotExisting"; - - final SecretStore credentialStorage = StorageProvider.getCredentialStorage(true, StorageProvider.SecureOption.PREFERRED); - assertTrue(credentialStorage.isSecure()); - - StoredCredential credentialsToStore = new StoredCredential("TästUser", "TästPassword".toCharArray()); - credentialStorage.add(key, credentialsToStore); - - StoredCredential stored = credentialStorage.get(key); - assertEquals("TästUser", stored.getUsername()); - assertEquals("TästPassword", new String(stored.getPassword())); - } - private SecretStore getStore(final boolean secure) { return new SecretStore<>() { @Override diff --git a/src/test/java/com/microsoft/credentialstorage/implementation/macosx/KeychainSecurityBackedCredentialStoreIT.java b/src/test/java/com/microsoft/credentialstorage/implementation/macosx/KeychainSecurityBackedCredentialStoreIT.java index b44b92e..93880b6 100644 --- a/src/test/java/com/microsoft/credentialstorage/implementation/macosx/KeychainSecurityBackedCredentialStoreIT.java +++ b/src/test/java/com/microsoft/credentialstorage/implementation/macosx/KeychainSecurityBackedCredentialStoreIT.java @@ -85,4 +85,19 @@ public void e2eTestStoreReadDeleteAnotherAccount() { final StoredCredential nonExistent = underTest.get(key); assertNull("Credential can still be read from store", nonExistent); } + + @Test + public void get_shouldHandleNonAsciiValues() { + final String key = "KeychainTest:http://test.com:Credential"; + + StoredCredential credential = new StoredCredential("TästUser", "\"TästPassword\"!@#$%^&*()-_=+{}[]:;\"'<>,.?/\\~`".toCharArray()); + boolean success = underTest.add(key, credential); + assertTrue("Storing credential failed", success); + + final StoredCredential readCred = underTest.get(key); + + assertNotNull("Credential not found", readCred); + assertEquals("Retrieved Credential.Username is different", "TästUser", readCred.getUsername()); + assertArrayEquals("Retrieved Credential.Password is different", "\"TästPassword\"!@#$%^&*()-_=+{}[]:;\"'<>,.?/\\~`".toCharArray(), readCred.getPassword()); + } } diff --git a/src/test/java/com/microsoft/credentialstorage/implementation/macosx/KeychainSecurityBackedTokenPairStoreIT.java b/src/test/java/com/microsoft/credentialstorage/implementation/macosx/KeychainSecurityBackedTokenPairStoreIT.java index a7c958f..e854885 100644 --- a/src/test/java/com/microsoft/credentialstorage/implementation/macosx/KeychainSecurityBackedTokenPairStoreIT.java +++ b/src/test/java/com/microsoft/credentialstorage/implementation/macosx/KeychainSecurityBackedTokenPairStoreIT.java @@ -7,11 +7,7 @@ import org.junit.Before; import org.junit.Test; -import static org.junit.Assert.assertArrayEquals; -import static org.junit.Assert.assertFalse; -import static org.junit.Assert.assertNotNull; -import static org.junit.Assert.assertNull; -import static org.junit.Assert.assertTrue; +import static org.junit.Assert.*; import static org.junit.Assume.assumeTrue; public class KeychainSecurityBackedTokenPairStoreIT { @@ -51,4 +47,19 @@ public void e2eTestStoreReadDelete() { StoredTokenPair nonExistent = underTest.get(key); assertNull("Token can still be read from store", nonExistent); } + + @Test + public void get_shouldHandleNonAsciiValues() { + final String key = "KeychainTest:http://test.com:TokenPair"; + + StoredTokenPair tokenPair = new StoredTokenPair("TästAccess".toCharArray(), "\"TästRefresh\"!@#$%^&*()-_=+{}[]:;\"'<>,.?/\\~`".toCharArray()); + boolean success = underTest.add(key, tokenPair); + assertTrue("Storing token pair failed", success); + + final StoredTokenPair readTokenPair = underTest.get(key); + + assertNotNull("Token pair not found", readTokenPair); + assertArrayEquals("Retrieved TokenPair.AccessToken is different", "TästAccess".toCharArray(), readTokenPair.getAccessToken().getValue()); + assertArrayEquals("Retrieved TokenPair.RefreshToken is different", "\"TästRefresh\"!@#$%^&*()-_=+{}[]:;\"'<>,.?/\\~`".toCharArray(), readTokenPair.getRefreshToken().getValue()); + } } \ No newline at end of file diff --git a/src/test/java/com/microsoft/credentialstorage/implementation/macosx/KeychainSecurityBackedTokenStoreIT.java b/src/test/java/com/microsoft/credentialstorage/implementation/macosx/KeychainSecurityBackedTokenStoreIT.java index 672e086..8c5a6b5 100644 --- a/src/test/java/com/microsoft/credentialstorage/implementation/macosx/KeychainSecurityBackedTokenStoreIT.java +++ b/src/test/java/com/microsoft/credentialstorage/implementation/macosx/KeychainSecurityBackedTokenStoreIT.java @@ -77,4 +77,18 @@ public void e2eTestStoreReadDeleteAnotherAccount() { StoredToken nonExistentToken = underTest.get(key); assertNull(nonExistentToken); } + + @Test + public void get_shouldHandleNonAsciiAndSpecialValues() { + final String key = "KeychainTest:http://test.com:Token"; + + StoredToken tokenPair = new StoredToken("\"TästAccess\"!@#$%^&*()-_=+{}[]:;\"'<>,.?/\\~`".toCharArray(), StoredTokenType.ACCESS); + boolean success = underTest.add(key, tokenPair); + assertTrue("Storing token pair failed", success); + + final StoredToken readToken = underTest.get(key); + + assertNotNull("Token not found", readToken); + assertArrayEquals("Retrieved Token is different", "\"TästAccess\"!@#$%^&*()-_=+{}[]:;\"'<>,.?/\\~`".toCharArray(), readToken.getValue()); + } } \ No newline at end of file