From bfce1cb4a9e85a651640c6e6ada4411bac9cdc4a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dominik=20Sch=C3=BCrmann?= Date: Thu, 2 Nov 2017 13:54:56 +0100 Subject: [PATCH 1/9] Fix GNUK version comparison. 1.2.5 already supports reset, use class to make 1.2.10 bigger as 1.2.9 --- .../securitytoken/SecurityTokenInfo.java | 65 +++++++++++++++++-- .../securitytoken/usb/UsbTransport.java | 7 +- 2 files changed, 64 insertions(+), 8 deletions(-) diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/SecurityTokenInfo.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/SecurityTokenInfo.java index 4e5137da2..987560edd 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/SecurityTokenInfo.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/SecurityTokenInfo.java @@ -9,6 +9,7 @@ import java.util.regex.Matcher; import java.util.regex.Pattern; import android.os.Parcelable; +import android.support.annotation.NonNull; import android.support.annotation.Nullable; import com.google.auto.value.AutoValue; @@ -94,7 +95,7 @@ public abstract class SecurityTokenInfo implements Parcelable { public enum TokenType { YUBIKEY_NEO, YUBIKEY_4, FIDESMO, NITROKEY_PRO, NITROKEY_STORAGE, NITROKEY_START, - GNUK_OLD, GNUK_UNKNOWN, GNUK_NEWER_1_25, LEDGER_NANO_S, UNKNOWN + GNUK_OLD, GNUK_UNKNOWN, GNUK_1_25_AND_NEWER, LEDGER_NANO_S, UNKNOWN } private static final HashSet SUPPORTED_USB_TOKENS = new HashSet<>(Arrays.asList( @@ -104,14 +105,14 @@ public abstract class SecurityTokenInfo implements Parcelable { TokenType.NITROKEY_STORAGE, TokenType.GNUK_OLD, TokenType.GNUK_UNKNOWN, - TokenType.GNUK_NEWER_1_25 + TokenType.GNUK_1_25_AND_NEWER )); private static final HashSet SUPPORTED_USB_RESET = new HashSet<>(Arrays.asList( TokenType.YUBIKEY_NEO, TokenType.YUBIKEY_4, TokenType.NITROKEY_PRO, - TokenType.GNUK_NEWER_1_25 + TokenType.GNUK_1_25_AND_NEWER )); private static final HashSet SUPPORTED_USB_PUT_KEY = new HashSet<>(Arrays.asList( @@ -141,7 +142,7 @@ public abstract class SecurityTokenInfo implements Parcelable { return isKnownSupported || isNfcTransport; } - public static String parseGnukVersionString(String serialNo) { + public static Version parseGnukVersionString(String serialNo) { if (serialNo == null) { return null; } @@ -150,6 +151,60 @@ public abstract class SecurityTokenInfo implements Parcelable { if (!matcher.matches()) { return null; } - return matcher.group(1); + return new Version(matcher.group(1)); + } + + public static class Version implements Comparable { + + private String version; + + public final String get() { + return this.version; + } + + public Version(String version) { + if (version == null) { + throw new IllegalArgumentException("Version can not be null"); + } + if (!version.matches("[0-9]+(\\.[0-9]+)*")) { + throw new IllegalArgumentException("Invalid version format"); + } + this.version = version; + } + + @Override + public int compareTo(@NonNull Version that) { + String[] thisParts = this.get().split("\\."); + String[] thatParts = that.get().split("\\."); + int length = Math.max(thisParts.length, thatParts.length); + for (int i = 0; i < length; i++) { + int thisPart = i < thisParts.length ? + Integer.parseInt(thisParts[i]) : 0; + int thatPart = i < thatParts.length ? + Integer.parseInt(thatParts[i]) : 0; + if (thisPart < thatPart) { + return -1; + } + if (thisPart > thatPart) { + return 1; + } + } + return 0; + } + + @Override + public boolean equals(Object that) { + if (this == that) { + return true; + } + if (that == null) { + return false; + } + if (this.getClass() != that.getClass()) { + return false; + } + return this.compareTo((Version) that) == 0; + } + } } diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/usb/UsbTransport.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/usb/UsbTransport.java index ae37841ef..504aec07f 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/usb/UsbTransport.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/usb/UsbTransport.java @@ -219,9 +219,10 @@ public class UsbTransport implements Transport { } case VENDOR_FSIJ: { String serialNo = usbConnection.getSerial(); - String gnukVersion = SecurityTokenInfo.parseGnukVersionString(serialNo); - boolean versionBigger125 = gnukVersion != null && "1.2.5".compareTo(gnukVersion) < 0; - return versionBigger125 ? TokenType.GNUK_NEWER_1_25 : TokenType.GNUK_OLD; + SecurityTokenInfo.Version gnukVersion = SecurityTokenInfo.parseGnukVersionString(serialNo); + boolean versionGreaterEquals125 = gnukVersion != null + && new SecurityTokenInfo.Version("1.2.5").compareTo(gnukVersion) <= 0; + return versionGreaterEquals125 ? TokenType.GNUK_1_25_AND_NEWER : TokenType.GNUK_OLD; } case VENDOR_LEDGER: { return TokenType.LEDGER_NANO_S; From 90310b7036ef0dbb4d730f14a731390ee9a5e4c8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dominik=20Sch=C3=BCrmann?= Date: Thu, 2 Nov 2017 18:52:11 +0100 Subject: [PATCH 2/9] Read life cycle management from historical bytes --- .../securitytoken/CardCapabilities.java | 44 ++++++++++++++++--- .../OpenPgpCommandApduFactory.java | 8 ++-- .../securitytoken/SecurityTokenUtilsTest.java | 15 +++++++ 3 files changed, 57 insertions(+), 10 deletions(-) diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/CardCapabilities.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/CardCapabilities.java index dc8a65e6d..2b5fd6151 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/CardCapabilities.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/CardCapabilities.java @@ -20,24 +20,32 @@ package org.sufficientlysecure.keychain.securitytoken; import org.sufficientlysecure.keychain.securitytoken.usb.UsbTransportException; import java.nio.ByteBuffer; +import java.util.Arrays; @SuppressWarnings("WeakerAccess") class CardCapabilities { private static final int MASK_CHAINING = 1 << 7; private static final int MASK_EXTENDED = 1 << 6; - private byte[] mCapabilityBytes; + private static final int STATUS_INDICATOR_NO_INFORMATION = 0x00; + private static final int STATUS_INDICATOR_INITIALISATION_STATE = 0x03; + private static final int STATUS_INDICATOR_OPERATIONAL_STATE = 0x05; + + private static final byte[] EXPECTED_PROCESSING_STATUS_BYTES = {(byte) 0x90, (byte) 0x00}; + + private byte[] historicalBytes; + private byte[] capabilityBytes; public CardCapabilities(byte[] historicalBytes) throws UsbTransportException { if ((historicalBytes == null) || (historicalBytes[0] != 0x00)) { throw new UsbTransportException("Invalid historical bytes category indicator byte"); } - - mCapabilityBytes = getCapabilitiesBytes(historicalBytes); + this.historicalBytes = historicalBytes; + capabilityBytes = getCapabilitiesBytes(historicalBytes); } public CardCapabilities() { - mCapabilityBytes = null; + capabilityBytes = null; } private static byte[] getCapabilitiesBytes(byte[] historicalBytes) { @@ -57,10 +65,34 @@ class CardCapabilities { } public boolean hasChaining() { - return mCapabilityBytes != null && (mCapabilityBytes[2] & MASK_CHAINING) != 0; + return capabilityBytes != null && (capabilityBytes[2] & MASK_CHAINING) != 0; } public boolean hasExtended() { - return mCapabilityBytes != null && (mCapabilityBytes[2] & MASK_EXTENDED) != 0; + return capabilityBytes != null && (capabilityBytes[2] & MASK_EXTENDED) != 0; + } + + public boolean hasResetSupport() throws UsbTransportException { + byte[] lastBytes = Arrays.copyOfRange(historicalBytes, historicalBytes.length - 2, historicalBytes.length); + boolean hasExpectedLastBytes = Arrays.equals(lastBytes, EXPECTED_PROCESSING_STATUS_BYTES); + + // Yk neo simply ends with 0x0000 + if (!hasExpectedLastBytes) { + return true; + } + + int statusIndicatorByte = historicalBytes[historicalBytes.length - 3]; + switch (statusIndicatorByte) { + case STATUS_INDICATOR_NO_INFORMATION: { + return false; + } + case STATUS_INDICATOR_INITIALISATION_STATE: + case STATUS_INDICATOR_OPERATIONAL_STATE: { + return true; + } + default: { + throw new UsbTransportException("Status indicator byte not specified in OpenPGP specification"); + } + } } } diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/OpenPgpCommandApduFactory.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/OpenPgpCommandApduFactory.java index e5bc66611..719ab7e38 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/OpenPgpCommandApduFactory.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/OpenPgpCommandApduFactory.java @@ -144,13 +144,13 @@ class OpenPgpCommandApduFactory { } @NonNull - CommandApdu createReactivate2Command() { - return CommandApdu.create(CLA, INS_ACTIVATE_FILE, P1_EMPTY, P2_EMPTY); + CommandApdu createReactivate1Command() { + return CommandApdu.create(CLA, INS_TERMINATE_DF, P1_EMPTY, P2_EMPTY); } @NonNull - CommandApdu createReactivate1Command() { - return CommandApdu.create(CLA, INS_TERMINATE_DF, P1_EMPTY, P2_EMPTY); + CommandApdu createReactivate2Command() { + return CommandApdu.create(CLA, INS_ACTIVATE_FILE, P1_EMPTY, P2_EMPTY); } @NonNull diff --git a/OpenKeychain/src/test/java/org/sufficientlysecure/keychain/securitytoken/SecurityTokenUtilsTest.java b/OpenKeychain/src/test/java/org/sufficientlysecure/keychain/securitytoken/SecurityTokenUtilsTest.java index 826300fb5..2cff99b78 100644 --- a/OpenKeychain/src/test/java/org/sufficientlysecure/keychain/securitytoken/SecurityTokenUtilsTest.java +++ b/OpenKeychain/src/test/java/org/sufficientlysecure/keychain/securitytoken/SecurityTokenUtilsTest.java @@ -167,16 +167,31 @@ public class SecurityTokenUtilsTest extends Mockito { capabilities = new CardCapabilities(Hex.decode("007300008000000000000000000000")); Assert.assertEquals(capabilities.hasChaining(), true); Assert.assertEquals(capabilities.hasExtended(), false); + Assert.assertEquals(capabilities.hasResetSupport(), true); // Yk 4 capabilities = new CardCapabilities(Hex.decode("0073000080059000")); Assert.assertEquals(capabilities.hasChaining(), true); Assert.assertEquals(capabilities.hasExtended(), false); + Assert.assertEquals(capabilities.hasResetSupport(), true); // Nitrokey pro capabilities = new CardCapabilities(Hex.decode("0031c573c00140059000")); Assert.assertEquals(capabilities.hasChaining(), false); Assert.assertEquals(capabilities.hasExtended(), true); + Assert.assertEquals(capabilities.hasResetSupport(), true); + + // GNUK without Life Cycle Management + capabilities = new CardCapabilities(Hex.decode("00318473800180009000")); + Assert.assertEquals(capabilities.hasChaining(), true); + Assert.assertEquals(capabilities.hasExtended(), false); + Assert.assertEquals(capabilities.hasResetSupport(), false); + + // GNUK with Life Cycle Management: ./configure --enable-factory-reset + capabilities = new CardCapabilities(Hex.decode("00318473800180059000")); + Assert.assertEquals(capabilities.hasChaining(), true); + Assert.assertEquals(capabilities.hasExtended(), false); + Assert.assertEquals(capabilities.hasResetSupport(), true); } @Test From 8acf62a0e80f538610095faaf5128d9d842f2a91 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dominik=20Sch=C3=BCrmann?= Date: Thu, 2 Nov 2017 19:13:44 +0100 Subject: [PATCH 3/9] Use check for life cycle management to determine if token supports reset --- .../securitytoken/CardCapabilities.java | 5 ++++- .../securitytoken/SecurityTokenConnection.java | 3 ++- .../securitytoken/SecurityTokenInfo.java | 17 ++++++++++------- .../securitytoken/SecurityTokenUtilsTest.java | 10 +++++----- 4 files changed, 21 insertions(+), 14 deletions(-) diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/CardCapabilities.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/CardCapabilities.java index 2b5fd6151..de7cf7012 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/CardCapabilities.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/CardCapabilities.java @@ -17,7 +17,10 @@ package org.sufficientlysecure.keychain.securitytoken; +import org.bouncycastle.util.encoders.Hex; +import org.sufficientlysecure.keychain.Constants; import org.sufficientlysecure.keychain.securitytoken.usb.UsbTransportException; +import org.sufficientlysecure.keychain.util.Log; import java.nio.ByteBuffer; import java.util.Arrays; @@ -72,7 +75,7 @@ class CardCapabilities { return capabilityBytes != null && (capabilityBytes[2] & MASK_EXTENDED) != 0; } - public boolean hasResetSupport() throws UsbTransportException { + public boolean hasLifeCycleManagement() throws UsbTransportException { byte[] lastBytes = Arrays.copyOfRange(historicalBytes, historicalBytes.length - 2, historicalBytes.length); boolean hasExpectedLastBytes = Arrays.equals(lastBytes, EXPECTED_PROCESSING_STATUS_BYTES); diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/SecurityTokenConnection.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/SecurityTokenConnection.java index b9be3cbe0..e26808e07 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/SecurityTokenConnection.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/SecurityTokenConnection.java @@ -990,11 +990,12 @@ public class SecurityTokenConnection { String userId = getUserId(); String url = getUrl(); byte[] pwInfo = getPwStatusBytes(); + boolean hasLifeCycleManagement = mCardCapabilities.hasLifeCycleManagement(); TransportType transportType = mTransport.getTransportType(); SecurityTokenInfo info = SecurityTokenInfo - .create(transportType, tokenType, fingerprints, aid, userId, url, pwInfo[4], pwInfo[6]); + .create(transportType, tokenType, fingerprints, aid, userId, url, pwInfo[4], pwInfo[6], hasLifeCycleManagement); if (! info.isSecurityTokenSupported()) { throw new UnsupportedSecurityTokenException(); diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/SecurityTokenInfo.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/SecurityTokenInfo.java index 987560edd..8b443cd7d 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/SecurityTokenInfo.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/SecurityTokenInfo.java @@ -35,6 +35,7 @@ public abstract class SecurityTokenInfo implements Parcelable { public abstract String getUrl(); public abstract int getVerifyRetries(); public abstract int getVerifyAdminRetries(); + public abstract boolean hasLifeCycleManagement(); public boolean isEmpty() { return getFingerprints().isEmpty(); @@ -42,7 +43,8 @@ public abstract class SecurityTokenInfo implements Parcelable { public static SecurityTokenInfo create(TransportType transportType, TokenType tokenType, byte[][] fingerprints, byte[] aid, String userId, String url, - int verifyRetries, int verifyAdminRetries) { + int verifyRetries, int verifyAdminRetries, + boolean hasLifeCycleSupport) { ArrayList fingerprintList = new ArrayList<>(fingerprints.length); for (byte[] fingerprint : fingerprints) { if (!Arrays.equals(EMPTY_ARRAY, fingerprint)) { @@ -50,7 +52,7 @@ public abstract class SecurityTokenInfo implements Parcelable { } } return new AutoValue_SecurityTokenInfo( - transportType, tokenType, fingerprintList, aid, userId, url, verifyRetries, verifyAdminRetries); + transportType, tokenType, fingerprintList, aid, userId, url, verifyRetries, verifyAdminRetries, hasLifeCycleSupport); } public static SecurityTokenInfo newInstanceDebugKeyserver() { @@ -59,7 +61,7 @@ public abstract class SecurityTokenInfo implements Parcelable { } return SecurityTokenInfo.create(TransportType.NFC, TokenType.UNKNOWN, new byte[][] { KeyFormattingUtils.convertFingerprintHexFingerprint("1efdb4845ca242ca6977fddb1f788094fd3b430a") }, - Hex.decode("010203040506"), "yubinu2@mugenguild.com", null, 3, 3); + Hex.decode("010203040506"), "yubinu2@mugenguild.com", null, 3, 3, true); } public static SecurityTokenInfo newInstanceDebugUri() { @@ -68,7 +70,7 @@ public abstract class SecurityTokenInfo implements Parcelable { } return SecurityTokenInfo.create(TransportType.NFC, TokenType.UNKNOWN, new byte[][] { KeyFormattingUtils.convertFingerprintHexFingerprint("4700BA1AC417ABEF3CC7765AD686905837779C3E") }, - Hex.decode("010203040506"), "yubinu2@mugenguild.com", "http://valodim.stratum0.net/mryubinu2.asc", 3, 3); + Hex.decode("010203040506"), "yubinu2@mugenguild.com", "http://valodim.stratum0.net/mryubinu2.asc", 3, 3, true); } public static SecurityTokenInfo newInstanceDebugLocked() { @@ -77,7 +79,7 @@ public abstract class SecurityTokenInfo implements Parcelable { } return SecurityTokenInfo.create(TransportType.NFC, TokenType.UNKNOWN, new byte[][] { KeyFormattingUtils.convertFingerprintHexFingerprint("4700BA1AC417ABEF3CC7765AD686905837779C3E") }, - Hex.decode("010203040506"), "yubinu2@mugenguild.com", "http://valodim.stratum0.net/mryubinu2.asc", 0, 3); + Hex.decode("010203040506"), "yubinu2@mugenguild.com", "http://valodim.stratum0.net/mryubinu2.asc", 0, 3, true); } public static SecurityTokenInfo newInstanceDebugLockedHard() { @@ -86,7 +88,7 @@ public abstract class SecurityTokenInfo implements Parcelable { } return SecurityTokenInfo.create(TransportType.NFC, TokenType.UNKNOWN, new byte[][] { KeyFormattingUtils.convertFingerprintHexFingerprint("4700BA1AC417ABEF3CC7765AD686905837779C3E") }, - Hex.decode("010203040506"), "yubinu2@mugenguild.com", "http://valodim.stratum0.net/mryubinu2.asc", 0, 0); + Hex.decode("010203040506"), "yubinu2@mugenguild.com", "http://valodim.stratum0.net/mryubinu2.asc", 0, 0, true); } public enum TransportType { @@ -138,8 +140,9 @@ public abstract class SecurityTokenInfo implements Parcelable { public boolean isResetSupported() { boolean isKnownSupported = SUPPORTED_USB_RESET.contains(getTokenType()); boolean isNfcTransport = getTransportType() == TransportType.NFC; + boolean hasLifeCycleManagement = hasLifeCycleManagement(); - return isKnownSupported || isNfcTransport; + return (isKnownSupported || isNfcTransport) && hasLifeCycleManagement; } public static Version parseGnukVersionString(String serialNo) { diff --git a/OpenKeychain/src/test/java/org/sufficientlysecure/keychain/securitytoken/SecurityTokenUtilsTest.java b/OpenKeychain/src/test/java/org/sufficientlysecure/keychain/securitytoken/SecurityTokenUtilsTest.java index 2cff99b78..ed952550e 100644 --- a/OpenKeychain/src/test/java/org/sufficientlysecure/keychain/securitytoken/SecurityTokenUtilsTest.java +++ b/OpenKeychain/src/test/java/org/sufficientlysecure/keychain/securitytoken/SecurityTokenUtilsTest.java @@ -167,31 +167,31 @@ public class SecurityTokenUtilsTest extends Mockito { capabilities = new CardCapabilities(Hex.decode("007300008000000000000000000000")); Assert.assertEquals(capabilities.hasChaining(), true); Assert.assertEquals(capabilities.hasExtended(), false); - Assert.assertEquals(capabilities.hasResetSupport(), true); + Assert.assertEquals(capabilities.hasLifeCycleManagement(), true); // Yk 4 capabilities = new CardCapabilities(Hex.decode("0073000080059000")); Assert.assertEquals(capabilities.hasChaining(), true); Assert.assertEquals(capabilities.hasExtended(), false); - Assert.assertEquals(capabilities.hasResetSupport(), true); + Assert.assertEquals(capabilities.hasLifeCycleManagement(), true); // Nitrokey pro capabilities = new CardCapabilities(Hex.decode("0031c573c00140059000")); Assert.assertEquals(capabilities.hasChaining(), false); Assert.assertEquals(capabilities.hasExtended(), true); - Assert.assertEquals(capabilities.hasResetSupport(), true); + Assert.assertEquals(capabilities.hasLifeCycleManagement(), true); // GNUK without Life Cycle Management capabilities = new CardCapabilities(Hex.decode("00318473800180009000")); Assert.assertEquals(capabilities.hasChaining(), true); Assert.assertEquals(capabilities.hasExtended(), false); - Assert.assertEquals(capabilities.hasResetSupport(), false); + Assert.assertEquals(capabilities.hasLifeCycleManagement(), false); // GNUK with Life Cycle Management: ./configure --enable-factory-reset capabilities = new CardCapabilities(Hex.decode("00318473800180059000")); Assert.assertEquals(capabilities.hasChaining(), true); Assert.assertEquals(capabilities.hasExtended(), false); - Assert.assertEquals(capabilities.hasResetSupport(), true); + Assert.assertEquals(capabilities.hasLifeCycleManagement(), true); } @Test From b56a420aed7aba2bc8f727976d5a72c75567d0b9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dominik=20Sch=C3=BCrmann?= Date: Thu, 2 Nov 2017 19:21:37 +0100 Subject: [PATCH 4/9] Enable Gnuk 1.2.5 for put key --- .../keychain/securitytoken/SecurityTokenInfo.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/SecurityTokenInfo.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/SecurityTokenInfo.java index 8b443cd7d..99173cc00 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/SecurityTokenInfo.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/SecurityTokenInfo.java @@ -120,7 +120,8 @@ public abstract class SecurityTokenInfo implements Parcelable { private static final HashSet SUPPORTED_USB_PUT_KEY = new HashSet<>(Arrays.asList( TokenType.YUBIKEY_NEO, TokenType.YUBIKEY_4, // Not clear, will be tested: https://github.com/open-keychain/open-keychain/issues/2069 - TokenType.NITROKEY_PRO + TokenType.NITROKEY_PRO, + TokenType.GNUK_1_25_AND_NEWER )); public boolean isSecurityTokenSupported() { From 3a818e8cdef59d80b6c4577ae47ae2152032c672 Mon Sep 17 00:00:00 2001 From: Vincent Breitmoser Date: Tue, 31 Oct 2017 14:55:53 +0100 Subject: [PATCH 5/9] improve security token connection unit tests --- .../SecurityTokenConnection.java | 3 +- .../SecurityTokenConnectionTest.java | 159 ++++++++++++++---- .../resources/test-keys/token-import-rsa.sec | Bin 0 -> 1236 bytes 3 files changed, 129 insertions(+), 33 deletions(-) create mode 100644 OpenKeychain/src/test/resources/test-keys/token-import-rsa.sec diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/SecurityTokenConnection.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/SecurityTokenConnection.java index e26808e07..19962b164 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/SecurityTokenConnection.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/SecurityTokenConnection.java @@ -516,7 +516,8 @@ public class SecurityTokenConnection { * 0xB8: Decipherment Key * 0xA4: Authentication Key */ - private void putKey(KeyType slot, CanonicalizedSecretKey secretKey, Passphrase passphrase, Passphrase adminPin) + @VisibleForTesting + void putKey(KeyType slot, CanonicalizedSecretKey secretKey, Passphrase passphrase, Passphrase adminPin) throws IOException { RSAPrivateCrtKey crtSecretKey; ECPrivateKey ecSecretKey; diff --git a/OpenKeychain/src/test/java/org/sufficientlysecure/keychain/securitytoken/SecurityTokenConnectionTest.java b/OpenKeychain/src/test/java/org/sufficientlysecure/keychain/securitytoken/SecurityTokenConnectionTest.java index e79b3add3..aa1f95b3a 100644 --- a/OpenKeychain/src/test/java/org/sufficientlysecure/keychain/securitytoken/SecurityTokenConnectionTest.java +++ b/OpenKeychain/src/test/java/org/sufficientlysecure/keychain/securitytoken/SecurityTokenConnectionTest.java @@ -2,22 +2,28 @@ package org.sufficientlysecure.keychain.securitytoken; import java.io.IOException; +import java.util.LinkedList; import org.bouncycastle.util.encoders.Hex; import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; -import org.mockito.InOrder; -import org.mockito.internal.verification.VerificationModeFactory; +import org.mockito.invocation.InvocationOnMock; +import org.mockito.stubbing.Answer; import org.robolectric.RuntimeEnvironment; import org.robolectric.shadows.ShadowLog; import org.sufficientlysecure.keychain.KeychainTestRunner; +import org.sufficientlysecure.keychain.operations.results.OperationResult.OperationLog; +import org.sufficientlysecure.keychain.pgp.CanonicalizedSecretKey; +import org.sufficientlysecure.keychain.pgp.CanonicalizedSecretKeyRing; +import org.sufficientlysecure.keychain.pgp.UncachedKeyRing; import org.sufficientlysecure.keychain.securitytoken.SecurityTokenInfo.TokenType; import org.sufficientlysecure.keychain.securitytoken.SecurityTokenInfo.TransportType; import org.sufficientlysecure.keychain.util.Passphrase; -import static org.mockito.Matchers.eq; -import static org.mockito.Mockito.inOrder; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertTrue; +import static org.mockito.Matchers.any; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; @@ -28,6 +34,9 @@ public class SecurityTokenConnectionTest { private Transport transport; + LinkedList expectCommands; + LinkedList expectReplies; + @Before public void setUp() throws Exception { ShadowLog.stream = System.out; @@ -35,31 +44,46 @@ public class SecurityTokenConnectionTest { transport = mock(Transport.class); when(transport.getTransportType()).thenReturn(TransportType.USB); when(transport.getTokenTypeIfAvailable()).thenReturn(TokenType.YUBIKEY_NEO); + + expectCommands = new LinkedList<>(); + expectReplies = new LinkedList<>(); + when(transport.transceive(any(CommandApdu.class))).thenAnswer(new Answer() { + @Override + public ResponseApdu answer(InvocationOnMock invocation) throws Throwable { + CommandApdu commandApdu = invocation.getArgumentAt(0, CommandApdu.class); + System.out.println("<< " + commandApdu); + System.out.println("<< " + Hex.toHexString(commandApdu.toBytes())); + + CommandApdu expectedApdu = expectCommands.poll(); + assertEquals(expectedApdu, commandApdu); + + ResponseApdu responseApdu = expectReplies.poll(); + System.out.println(">> " + responseApdu); + System.out.println(">> " + Hex.toHexString(responseApdu.toBytes())); + return responseApdu; + } + }); } @Test public void test_connectToDevice() throws Exception { SecurityTokenConnection securityTokenConnection = new SecurityTokenConnection(transport, new Passphrase("123456"), new OpenPgpCommandApduFactory()); - String[] dialog = { - "00a4040006d27600012401", // select openpgp applet - "9000", - "00ca006e00", // get application related data + expect("00a4040006d27600012401", "9000"); // select openpgp applet + expect("00ca006e00", "6e81de4f10d27600012401020000060364311500005f520f0073000080000000000000000000007381b7c00af" + "00000ff04c000ff00ffc106010800001103c206010800001103c306010800001103c407007f7f7f03030" + "3c53c4ec5fee25c4e89654d58cad8492510a89d3c3d8468da7b24e15bfc624c6a792794f15b7599915f7" + "03aab55ed25424d60b17026b7b06c6ad4b9be30a3c63c000000000000000000000000000000000000000" + "000000000000000000000000000000000000000000000000000000000000000000000000000000000cd0" + - "c59cd0f2a59cd0af059cd0c959000" - }; - expect(transport, dialog); + "c59cd0f2a59cd0af059cd0c959000"); // get application related data securityTokenConnection.connectToDevice(RuntimeEnvironment.application); verify(transport).connect(); - verifyDialog(transport, dialog); + verifyDialog(); } @Test @@ -78,34 +102,105 @@ public class SecurityTokenConnectionTest { securityTokenConnection.setConnectionCapabilities(openPgpCapabilities); securityTokenConnection.determineTokenType(); - String[] dialog = { - "00ca006500", - "65095b005f2d005f3501399000", - "00ca5f5000", - "9000", - }; - expect(transport, dialog); + expect("00ca006500", "65095b005f2d005f3501399000"); + expect("00ca5f5000", "9000"); securityTokenConnection.getTokenInfo(); - verifyDialog(transport, dialog); + verifyDialog(); } - private void expect(Transport transport, String... dialog) throws IOException { - for (int i = 0; i < dialog.length; i += 2) { - CommandApdu command = CommandApdu.fromBytes(Hex.decode(dialog[i])); - ResponseApdu response = ResponseApdu.fromBytes(Hex.decode(dialog[i + 1])); - when(transport.transceive(eq(command))).thenReturn(response); - } + @Test + public void testPutKey() throws Exception { + /* + Security.insertProviderAt(new BouncyCastleProvider(), 1); + ShadowLog.stream = System.out; + + SaveKeyringParcel.Builder builder = SaveKeyringParcel.buildNewKeyringParcel(); + builder.addSubkeyAdd(SubkeyAdd.createSubkeyAdd( + Algorithm.RSA, 2048, null, KeyFlags.CERTIFY_OTHER | KeyFlags.SIGN_DATA, 0L)); + + builder.addUserId("gnuk"); + builder.setNewUnlock(ChangeUnlockParcel.createUnLockParcelForNewKey(new Passphrase())); + + PgpKeyOperation op = new PgpKeyOperation(null); + + PgpEditKeyResult result = op.createSecretKeyRing(builder.build()); + Assert.assertTrue("initial test key creation must succeed", result.success()); + Assert.assertNotNull("initial test key creation must succeed", result.getRing()); + */ + + UncachedKeyRing staticRing = readRingFromResource("/test-keys/token-import-rsa.sec"); + + CanonicalizedSecretKeyRing canRing = (CanonicalizedSecretKeyRing) staticRing.canonicalize(new OperationLog(), 0); + CanonicalizedSecretKey signKey = canRing.getSecretKey(); + signKey.unlock(null); + + SecurityTokenConnection securityTokenConnection = + new SecurityTokenConnection(transport, new Passphrase("123456"), new OpenPgpCommandApduFactory()); + OpenPgpCapabilities openPgpCapabilities = new OpenPgpCapabilities( + Hex.decode( + "6e81de4f10d27600012401020000060364311500005f520f0073000080000000000000000000007381b7c00af" + + "00000ff04c000ff00ffc106010800001103c206010800001103c306010800001103c407007f7f7f03" + + "0303c53c4ec5fee25c4e89654d58cad8492510a89d3c3d8468da7b24e15bfc624c6a792794f15b759" + + "9915f703aab55ed25424d60b17026b7b06c6ad4b9be30a3c63c000000000000000000000000000000" + + "000000000000000000000000000000000000000000000000000000000000000000000000000000000" + + "000000000cd0c59cd0f2a59cd0af059cd0c95" + )); + securityTokenConnection.setConnectionCapabilities(openPgpCapabilities); + securityTokenConnection.determineTokenType(); + + expect("00200083083132333435363738", "9000"); + expect("10db3fffff4d8203a2b6007f48159103928180938180948180958180968180978201005f48820383010001be5e36a094" + + "58313f9594f3f76a972989dfa1d4a416f7f461c8a4ccf9b9de8ee59447e44b4a4833a00c655ae5516214a72efa5c140" + + "fd7d429d9b15805c77c881e6ad10711b4614d2183497a5a6d36ed221146301ce6ccf42581004c313d533d14c57abc32" + + "886419665b67091d652aa6cb074da682135115657d90752fb9d2e69fffd7580adddf1d7822d9d40220401056674b933" + + "efeb3bc51eafe2c5a5162ec2b466591b689d9af07004bb81a509c7601043a2da871f5989e4e338b901afb9ba8f2b8bc" + + "18ac3300e750bda2a0886459363542bb5b59cab2ed93", "9000"); + expect("10db3fffff4388c486b602a3f6a63164afe55139ad9f4ed230421b4039b09f5c0b7c609ba9927f1f7c4db7c3895bbe8e" + + "58787ddae295468d034a0c29964b80f3551d308722de7ac698e840eb68c81c75d140ac347e35d8167bd9ac175610f81" + + "1f049061b72ebc491d89610fc6ba1344c8d03e2871181f0408f87779149a1b1835705b407baf28c30e94da82c8e15b8" + + "45f2473deee6987f29a2d25232361818fd83283a09592345ac56d9a56408ef5b19065d6d5252aeff1469c85686c61c4" + + "e62b541461320dbbb532d4a28e2d5a6da2c3e7c4d100204efd33b92a2ed85e2f2576eb6ee9a58415ea446ccad86dff4" + + "97a45917080bbea1c0406647e1b16ba623b3f7913f14", "9000"); + expect("10db3fffff538db405cb9f108add09f9b3557b7d45b49c8d6cf7c69cb582ce3e3674b9a58b71ed49d2c7a2027955ba0a" + + "be596a11add7bfb5d2a08bd6ed9cdf2e0fc5b8e4396ecc8c801715569d89912f2a4336b5f75a9a04ae8ca460c6266c7" + + "830213f724c5957dc44054699fa1a9adc2c48472ede53a7b77ea3353ccf75394f1e65100eb49ccbdc603de36f2f11ce" + + "ce6e36a2587d4338466917d28edf0e75a8706748ddf64af3d3b4f129f991be3ffb024c13038806fb6d32f0dc20adb28" + + "8fc190985dc9d0a976e108dcecffdf94b97a0de2f94ff6c8996fa6aaeeb97cc9d466fa8f92e2dd179c24b46bd165a68" + + "efbdce4e397e841e44ffa48991fa23abbd6ff4d0c387", "9000"); + expect("00db3fffa9a048a9ca323b4867c504d61af02048b4af787b0994fd71b9bc39dda6a4f3b610297c8b35affde21a53ec49" + + "54c6b1da6403a7cb627555686acc779ca19fb14d7ec2ca3655571f36587e025bdc0ce053193a7c4be1db17e9ba5788e" + + "db43f81f02ef07cc7c1d967e8435fba00e986ab30fa69746857fc082b5b797d5eea3c6fb1be4a1a12bba89d4ca8c204" + + "e2702d93e9316b6176726121dd29c8a8b75ec19e1deb09e4cc3b95b054541d", "9000"); + + + securityTokenConnection.putKey(KeyType.SIGN, signKey, new Passphrase("123456"), new Passphrase("12345678")); + + + verifyDialog(); } - private void verifyDialog(Transport transport, String... dialog) throws IOException { - InOrder inOrder = inOrder(transport); - for (int i = 0; i < dialog.length; i += 2) { - CommandApdu command = CommandApdu.fromBytes(Hex.decode(dialog[i])); - inOrder.verify(transport).transceive(eq(command)); - } + private void debugTransceive() throws IOException { + } + + private void expect(String commandApdu, String responseApdu) { + expect(CommandApdu.fromBytes(Hex.decode(commandApdu)), ResponseApdu.fromBytes(Hex.decode(responseApdu))); + } + + private void expect(CommandApdu commandApdu, ResponseApdu responseApdu) { + expectCommands.add(commandApdu); + expectReplies.add(responseApdu); + } + + private void verifyDialog() { + assertTrue(expectCommands.isEmpty()); + assertTrue(expectReplies.isEmpty()); + } + + UncachedKeyRing readRingFromResource(String name) throws Exception { + return UncachedKeyRing.fromStream(SecurityTokenConnectionTest.class.getResourceAsStream(name)).next(); } } \ No newline at end of file diff --git a/OpenKeychain/src/test/resources/test-keys/token-import-rsa.sec b/OpenKeychain/src/test/resources/test-keys/token-import-rsa.sec new file mode 100644 index 0000000000000000000000000000000000000000..1889c5f478ca33d43e5deb62f85c1bb327bf9880 GIT binary patch literal 1236 zcmV;_1S|WM1DFI^_g19=2mro6`vOc81BeFuZ8GrOAg!{9{22*_+?@)SZV-*m&;9vJ zm!RG+l>cmrmilV0>zB-(MsKM3E-le{!b?WI7FuZUz0OWKeuN%G|D=hL`Xj5oZ}iZ^ zhoDHQ$}&4hXT=288t@=Uw6A!336%YDxx6{urlj+>5Gj0%HLv~R8dK~^RK~H|WCN$m zVs%w$YRq?>p`Wo$e!|K&RaYN2SbhRq+zjAT89IDR;oBGKx>tzpv_FCHF7SNE!P#f% zLtnZ8>4vK?`e}4%SNsSoTX}t6>O60;zDgPryQrN^sKNx|a4nPRF>7IVa$zCeDafd| zUcsIn>j~t{JC(3hR2={j0RRC22mU!GaT&ahjiv-D?H{{X^!ymWtFk@07mt}q>IYE* z3R?-&T4p)8V{+p_<+!csgWe+AFZqP=?VMNZ(xBK(9`)nJ_~(my8}>#$h3Wuhc$L(a zV3t!%8D`zh8*f2?=lPk+zB1uLl52xEfHLb^!!BEvRwzn4V5E zi;x=oo2c@*ycn!A0OwG>qM(RmSvEC7yIWbxvh9;Yh{T4r0;Be(F=VghQ8}%jPSP+! z8$da*pIi%kV4JCue;<5Jx5ESfM?Ju{y?z)gp>Qyw&_Zlv?6qC1MU>U5g<21Q?)Glc zeN15=2~!9k3wlUf0Y0&B0+05(0Bvx+oEMH60v7FAN;RB(NE%toaBGFzuCgp$3u&+K z)cC=ye#2wY+w8{Ee5%5}?9&XH;c}p*BnJ9U^;XXb$_Me5lH34CzR?%XJo~f+XKr3JDO+A3d4?UDya-tOx+D zEvv>gekoNRT2X#q9~#>{m5xPBn3T{GC5mXQ(k7I9vfz`|%hSK+NQ=-izUMyvzoRiT z9H-W&oAyS;6ZXJ0#Xh0Q-|9vV{kM6Al4AY?8eNsXNT- z%(E{zo4=6r`ugLUhDp_RlIIuK_!%xams88RxUn6p)CbMKcbj=x3ih7+dN+eC%5x2x yUlX&xe@I(&Y72CAj;(khWm7>SgZf7ZL(C74NZr}EKVZD>LGO`F^VHT2NMLWT$5Yk- literal 0 HcmV?d00001 From 2cf3e27e5135f033e6d0c5d0ed6238c04fdef710 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dominik=20Sch=C3=BCrmann?= Date: Fri, 3 Nov 2017 14:06:26 +0100 Subject: [PATCH 6/9] First set Admin PIN, then PIN to prevent Gnuk from going into 'admin less mode' --- .../keychain/securitytoken/SecurityTokenConnection.java | 4 ++++ .../keychain/ui/SecurityTokenOperationActivity.java | 8 +++++--- 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/SecurityTokenConnection.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/SecurityTokenConnection.java index 19962b164..beed8a119 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/SecurityTokenConnection.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/SecurityTokenConnection.java @@ -209,6 +209,10 @@ public class SecurityTokenConnection { } } + public void resetPw3Validation() { + mPw3Validated = false; + } + @VisibleForTesting void determineTokenType() throws IOException { tokenType = mTransport.getTokenTypeIfAvailable(); diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/ui/SecurityTokenOperationActivity.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/ui/SecurityTokenOperationActivity.java index 1834c2762..8b808266d 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/ui/SecurityTokenOperationActivity.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/ui/SecurityTokenOperationActivity.java @@ -25,7 +25,6 @@ package org.sufficientlysecure.keychain.ui; import java.io.IOException; import java.nio.ByteBuffer; import java.util.Arrays; -import java.util.Map; import android.content.Intent; import android.os.AsyncTask; @@ -293,9 +292,12 @@ public class SecurityTokenOperationActivity extends BaseSecurityTokenActivity { mInputParcel = mInputParcel.withCryptoData(subkeyBytes, tokenSerialNumber); } - // change PINs afterwards - stConnection.resetPin(newPin, adminPin); + // First set Admin PIN, then PIN. + // Order is important for Gnuk, otherwise it will be set up in "admin less mode". + // http://www.fsij.org/doc-gnuk/gnuk-passphrase-setting.html#set-up-pw1-pw3-and-reset-code stConnection.modifyPw3Pin(newAdminPin, adminPin); + stConnection.resetPw3Validation(); + stConnection.resetPin(newPin, new Passphrase(new String(newAdminPin))); break; } From da186ca49f29097dc8cddb73470137a2526447ab Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dominik=20Sch=C3=BCrmann?= Date: Sun, 5 Nov 2017 19:10:59 +0100 Subject: [PATCH 7/9] Reset PW3 validation directly in modifyPw3Pin --- .../keychain/securitytoken/SecurityTokenConnection.java | 6 ++---- .../keychain/ui/SecurityTokenOperationActivity.java | 1 - 2 files changed, 2 insertions(+), 5 deletions(-) diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/SecurityTokenConnection.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/SecurityTokenConnection.java index beed8a119..3e685fec6 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/SecurityTokenConnection.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/SecurityTokenConnection.java @@ -209,10 +209,6 @@ public class SecurityTokenConnection { } } - public void resetPw3Validation() { - mPw3Validated = false; - } - @VisibleForTesting void determineTokenType() throws IOException { tokenType = mTransport.getTokenTypeIfAvailable(); @@ -291,6 +287,8 @@ public class SecurityTokenConnection { CommandApdu changePin = commandFactory.createChangePw3Command(pin, newAdminPin); ResponseApdu response = communicate(changePin); + mPw3Validated = false; + if (!response.isSuccess()) { throw new CardException("Failed to change PIN", response.getSw()); } diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/ui/SecurityTokenOperationActivity.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/ui/SecurityTokenOperationActivity.java index 8b808266d..a3ee80b84 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/ui/SecurityTokenOperationActivity.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/ui/SecurityTokenOperationActivity.java @@ -296,7 +296,6 @@ public class SecurityTokenOperationActivity extends BaseSecurityTokenActivity { // Order is important for Gnuk, otherwise it will be set up in "admin less mode". // http://www.fsij.org/doc-gnuk/gnuk-passphrase-setting.html#set-up-pw1-pw3-and-reset-code stConnection.modifyPw3Pin(newAdminPin, adminPin); - stConnection.resetPw3Validation(); stConnection.resetPin(newPin, new Passphrase(new String(newAdminPin))); break; From 221eb194d94b6639d97a5814fe9da5b560bf3d92 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dominik=20Sch=C3=BCrmann?= Date: Sun, 5 Nov 2017 22:59:11 +0100 Subject: [PATCH 8/9] Merge SUPPORTED_PUT_KEY and SUPPORTED_RESET --- .../keychain/securitytoken/SecurityTokenInfo.java | 13 +++---------- 1 file changed, 3 insertions(+), 10 deletions(-) diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/SecurityTokenInfo.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/SecurityTokenInfo.java index 99173cc00..e987b2d78 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/SecurityTokenInfo.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/SecurityTokenInfo.java @@ -110,14 +110,7 @@ public abstract class SecurityTokenInfo implements Parcelable { TokenType.GNUK_1_25_AND_NEWER )); - private static final HashSet SUPPORTED_USB_RESET = new HashSet<>(Arrays.asList( - TokenType.YUBIKEY_NEO, - TokenType.YUBIKEY_4, - TokenType.NITROKEY_PRO, - TokenType.GNUK_1_25_AND_NEWER - )); - - private static final HashSet SUPPORTED_USB_PUT_KEY = new HashSet<>(Arrays.asList( + private static final HashSet SUPPORTED_USB_SETUP = new HashSet<>(Arrays.asList( TokenType.YUBIKEY_NEO, TokenType.YUBIKEY_4, // Not clear, will be tested: https://github.com/open-keychain/open-keychain/issues/2069 TokenType.NITROKEY_PRO, @@ -132,14 +125,14 @@ public abstract class SecurityTokenInfo implements Parcelable { } public boolean isPutKeySupported() { - boolean isKnownSupported = SUPPORTED_USB_PUT_KEY.contains(getTokenType()); + boolean isKnownSupported = SUPPORTED_USB_SETUP.contains(getTokenType()); boolean isNfcTransport = getTransportType() == TransportType.NFC; return isKnownSupported || isNfcTransport; } public boolean isResetSupported() { - boolean isKnownSupported = SUPPORTED_USB_RESET.contains(getTokenType()); + boolean isKnownSupported = SUPPORTED_USB_SETUP.contains(getTokenType()); boolean isNfcTransport = getTransportType() == TransportType.NFC; boolean hasLifeCycleManagement = hasLifeCycleManagement(); From 106dbdf4a97fbb19c1b6f37805461b7790f74b12 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dominik=20Sch=C3=BCrmann?= Date: Sun, 5 Nov 2017 23:13:18 +0100 Subject: [PATCH 9/9] Simplify SecurityTokenInfo.Version using AutoValue --- .../securitytoken/SecurityTokenInfo.java | 36 +++++-------------- .../securitytoken/usb/UsbTransport.java | 2 +- 2 files changed, 9 insertions(+), 29 deletions(-) diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/SecurityTokenInfo.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/SecurityTokenInfo.java index e987b2d78..5b453a442 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/SecurityTokenInfo.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/SecurityTokenInfo.java @@ -148,31 +148,25 @@ public abstract class SecurityTokenInfo implements Parcelable { if (!matcher.matches()) { return null; } - return new Version(matcher.group(1)); + return Version.create(matcher.group(1)); } - public static class Version implements Comparable { + @AutoValue + public static abstract class Version implements Comparable { - private String version; + abstract String getVersion(); - public final String get() { - return this.version; - } - - public Version(String version) { - if (version == null) { - throw new IllegalArgumentException("Version can not be null"); - } + public static Version create(@NonNull String version) { if (!version.matches("[0-9]+(\\.[0-9]+)*")) { throw new IllegalArgumentException("Invalid version format"); } - this.version = version; + return new AutoValue_SecurityTokenInfo_Version(version); } @Override public int compareTo(@NonNull Version that) { - String[] thisParts = this.get().split("\\."); - String[] thatParts = that.get().split("\\."); + String[] thisParts = this.getVersion().split("\\."); + String[] thatParts = that.getVersion().split("\\."); int length = Math.max(thisParts.length, thatParts.length); for (int i = 0; i < length; i++) { int thisPart = i < thisParts.length ? @@ -189,19 +183,5 @@ public abstract class SecurityTokenInfo implements Parcelable { return 0; } - @Override - public boolean equals(Object that) { - if (this == that) { - return true; - } - if (that == null) { - return false; - } - if (this.getClass() != that.getClass()) { - return false; - } - return this.compareTo((Version) that) == 0; - } - } } diff --git a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/usb/UsbTransport.java b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/usb/UsbTransport.java index 504aec07f..69e257e36 100644 --- a/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/usb/UsbTransport.java +++ b/OpenKeychain/src/main/java/org/sufficientlysecure/keychain/securitytoken/usb/UsbTransport.java @@ -221,7 +221,7 @@ public class UsbTransport implements Transport { String serialNo = usbConnection.getSerial(); SecurityTokenInfo.Version gnukVersion = SecurityTokenInfo.parseGnukVersionString(serialNo); boolean versionGreaterEquals125 = gnukVersion != null - && new SecurityTokenInfo.Version("1.2.5").compareTo(gnukVersion) <= 0; + && SecurityTokenInfo.Version.create("1.2.5").compareTo(gnukVersion) <= 0; return versionGreaterEquals125 ? TokenType.GNUK_1_25_AND_NEWER : TokenType.GNUK_OLD; } case VENDOR_LEDGER: {