From fa6e0d971910875ddbd3a3096560017550095d98 Mon Sep 17 00:00:00 2001 From: Sitaram Kalluri Date: Thu, 5 Oct 2023 16:11:21 +0530 Subject: [PATCH 1/5] feat: Persist OTPs in keystore and prevent reuse of OTPs --- .../src/verb/handler/enroll_verb_handler.dart | 38 ++++----- .../src/verb/handler/otp_verb_handler.dart | 50 ++++++++--- packages/at_secondary_server/pubspec.yaml | 8 +- .../test/enroll_verb_test.dart | 2 +- .../test/otp_verb_test.dart | 83 ++++++++++++++++--- .../test/enroll_verb_test.dart | 30 ++++++- .../test/otp_verb_test.dart | 50 +++++++++++ 7 files changed, 214 insertions(+), 47 deletions(-) create mode 100644 tests/at_functional_test/test/otp_verb_test.dart diff --git a/packages/at_secondary_server/lib/src/verb/handler/enroll_verb_handler.dart b/packages/at_secondary_server/lib/src/verb/handler/enroll_verb_handler.dart index 31b43f930..7a59ad8ac 100644 --- a/packages/at_secondary_server/lib/src/verb/handler/enroll_verb_handler.dart +++ b/packages/at_secondary_server/lib/src/verb/handler/enroll_verb_handler.dart @@ -50,15 +50,15 @@ class EnrollVerbHandler extends AbstractVerbHandler { try { // Ensure that enrollParams are present for all enroll operation // Exclude operation 'list' which does not have enrollParams - if (verbParams[enrollParams] == null) { + if (verbParams[AtConstants.enrollParams] == null) { if (operation != 'list') { logger.severe( - 'Enroll params is empty | EnrollParams: ${verbParams[enrollParams]}'); + 'Enroll params is empty | EnrollParams: ${verbParams[AtConstants.enrollParams]}'); throw IllegalArgumentException('Enroll parameters not provided'); } } else { - enrollVerbParams = - EnrollParams.fromJson(jsonDecode(verbParams[enrollParams]!)); + enrollVerbParams = EnrollParams.fromJson( + jsonDecode(verbParams[AtConstants.enrollParams]!)); } switch (operation) { case 'request': @@ -116,14 +116,18 @@ class EnrollVerbHandler extends AbstractVerbHandler { throw AtThrottleLimitExceeded( 'Enrollment requests have exceeded the limit within the specified time frame'); } - if (!atConnection.getMetaData().isAuthenticated) { - var otp = enrollParams.otp; - if (otp == null || - (await OtpVerbHandler.cache.get(otp.toString()) == null)) { + + // OTP is sent only in enrollment request which is submitted on + // unauthenticated connection. + if (atConnection.getMetaData().isAuthenticated == false) { + Response otpResponse = await OtpVerbHandler(keyStore) + .processInternal('otp:validate:${enrollParams.otp}', atConnection); + if (otpResponse.data == 'invalid') { throw AtEnrollmentException( 'invalid otp. Cannot process enroll request'); } } + var enrollNamespaces = enrollParams.namespaces ?? {}; var newEnrollmentId = Uuid().v4(); var key = @@ -154,8 +158,8 @@ class EnrollVerbHandler extends AbstractVerbHandler { await _storeEncryptionKeys(newEnrollmentId, enrollParams, currentAtSign); // store this apkam as default pkam public key for old clients // The keys with AT_PKAM_PUBLIC_KEY does not sync to client. - await keyStore.put( - AT_PKAM_PUBLIC_KEY, AtData()..data = enrollParams.apkamPublicKey!); + await keyStore.put(AtConstants.atPkamPublicKey, + AtData()..data = enrollParams.apkamPublicKey!); enrollData = AtData()..data = jsonEncode(enrollmentValue.toJson()); } else { enrollmentValue.approval = EnrollApproval(EnrollStatus.pending.name); @@ -224,10 +228,6 @@ class EnrollVerbHandler extends AbstractVerbHandler { // If an enrollment is approved, we need the enrollment to be active // to subsequently revoke the enrollment. Hence reset TTL and // expiredAt on metadata. - /* TODO: Currently TTL is reset on all the enrollments. - However, if the enrollment state is denied or revoked, - unless we wanted to display denied or revoked enrollments in the UI, - we can let the TTL be, so that the enrollment will be deleted subsequently.*/ await _updateEnrollmentValueAndResetTTL( '$enrollmentKey$currentAtSign', enrollDataStoreValue); // when enrollment is approved store the apkamPublicKey of the enrollment @@ -252,13 +252,13 @@ class EnrollVerbHandler extends AbstractVerbHandler { var privKeyJson = {}; privKeyJson['value'] = enrollParams.encryptedDefaultEncryptedPrivateKey; await keyStore.put( - '$newEnrollmentId.$defaultEncryptionPrivateKey.$enrollManageNamespace$atSign', + '$newEnrollmentId.${AtConstants.defaultEncryptionPrivateKey}.$enrollManageNamespace$atSign', AtData()..data = jsonEncode(privKeyJson), skipCommit: true); var selfKeyJson = {}; selfKeyJson['value'] = enrollParams.encryptedDefaultSelfEncryptionKey; await keyStore.put( - '$newEnrollmentId.$defaultSelfEncryptionKey.$enrollManageNamespace$atSign', + '$newEnrollmentId.${AtConstants.defaultSelfEncryptionKey}.$enrollManageNamespace$atSign', AtData()..data = jsonEncode(selfKeyJson), skipCommit: true); } @@ -338,7 +338,7 @@ class EnrollVerbHandler extends AbstractVerbHandler { String key, EnrollParams enrollParams, String atSign) async { try { var notificationValue = {}; - notificationValue[apkamEncryptedSymmetricKey] = + notificationValue[AtConstants.apkamEncryptedSymmetricKey] = enrollParams.encryptedAPKAMSymmetricKey; logger.finer('notificationValue:$notificationValue'); final atNotification = (AtNotificationBuilder() @@ -355,10 +355,10 @@ class EnrollVerbHandler extends AbstractVerbHandler { logger.finer('notification generated: $notificationId'); } on Exception catch (e, trace) { logger.severe( - 'Exception while storing notification key $enrollmentId. Exception $e. Trace $trace'); + 'Exception while storing notification key ${AtConstants.enrollmentId}. Exception $e. Trace $trace'); } on Error catch (e, trace) { logger.severe( - 'Error while storing notification key $enrollmentId. Error $e. Trace $trace'); + 'Error while storing notification key ${AtConstants.enrollmentId}. Error $e. Trace $trace'); } } diff --git a/packages/at_secondary_server/lib/src/verb/handler/otp_verb_handler.dart b/packages/at_secondary_server/lib/src/verb/handler/otp_verb_handler.dart index ac8839cb7..39c628e77 100644 --- a/packages/at_secondary_server/lib/src/verb/handler/otp_verb_handler.dart +++ b/packages/at_secondary_server/lib/src/verb/handler/otp_verb_handler.dart @@ -1,24 +1,25 @@ import 'dart:collection'; import 'dart:math'; import 'package:at_commons/at_commons.dart'; +import 'package:at_secondary/src/server/at_secondary_impl.dart'; +import 'package:at_secondary/src/utils/secondary_util.dart'; import 'package:at_server_spec/at_server_spec.dart'; +import 'package:meta/meta.dart'; import 'package:uuid/uuid.dart'; import 'abstract_verb_handler.dart'; import 'package:at_server_spec/at_verb_spec.dart'; import 'package:at_persistence_secondary_server/at_persistence_secondary_server.dart'; -import 'package:expire_cache/expire_cache.dart'; class OtpVerbHandler extends AbstractVerbHandler { static Otp otpVerb = Otp(); - static final expireDuration = Duration(seconds: 90); - static ExpireCache cache = - ExpireCache(expireDuration: expireDuration); + + @visibleForTesting + int otpExpiryInMills = Duration(minutes: 5).inMilliseconds; OtpVerbHandler(SecondaryKeyStore keyStore) : super(keyStore); @override - bool accept(String command) => - command == 'otp:get' || command.startsWith('otp:validate'); + bool accept(String command) => command.startsWith('otp'); @override Verb getVerb() => otpVerb; @@ -29,6 +30,10 @@ class OtpVerbHandler extends AbstractVerbHandler { HashMap verbParams, InboundConnection atConnection) async { final operation = verbParams['operation']; + if (verbParams[AtConstants.ttl] != null && + verbParams[AtConstants.ttl]!.isNotEmpty) { + otpExpiryInMills = int.parse(verbParams[AtConstants.ttl]!); + } switch (operation) { case 'get': if (!atConnection.getMetaData().isAuthenticated) { @@ -40,17 +45,38 @@ class OtpVerbHandler extends AbstractVerbHandler { } // If OTP generated do not have digits, generate again. while (RegExp(r'\d').hasMatch(response.data!) == false); - await cache.set(response.data!, response.data!); + await keyStore.put( + 'private:${response.data}${AtSecondaryServerImpl.getInstance().currentAtSign}', + AtData() + ..data = + '${DateTime.now().toUtc().add(Duration(milliseconds: otpExpiryInMills)).millisecondsSinceEpoch}' + ..metaData = (AtMetaData()..ttl = otpExpiryInMills)); break; case 'validate': - String? otp = verbParams['otp']; - if (otp != null && (await cache.get(otp)) == otp) { + var isValid = await isOTPValid(verbParams['otp']); + if (isValid) { response.data = 'valid'; - } else { - response.data = 'invalid'; + return; } - break; + response.data = 'invalid'; + } + } + + Future isOTPValid(String? otp) async { + if (otp == null) { + return false; + } + String otpKey = + 'private:${otp.toLowerCase()}${AtSecondaryServerImpl.getInstance().currentAtSign}'; + AtData otpAtData; + try { + otpAtData = await keyStore.get(otpKey); + } on KeyNotFoundException { + return false; } + // Remove the key from keystore to prevent reuse of OTP. + await keyStore.remove(otpKey); + return SecondaryUtil.isActiveKey(otpAtData); } /// This function generates a UUID and converts it into a 6-character alpha-numeric string. diff --git a/packages/at_secondary_server/pubspec.yaml b/packages/at_secondary_server/pubspec.yaml index 81201c836..73e6c37bf 100644 --- a/packages/at_secondary_server/pubspec.yaml +++ b/packages/at_secondary_server/pubspec.yaml @@ -26,7 +26,6 @@ dependencies: at_server_spec: 3.0.15 at_persistence_spec: 2.0.14 at_persistence_secondary_server: 3.0.57 - expire_cache: ^2.0.1 intl: ^0.18.1 json_annotation: ^4.8.0 version: 3.0.2 @@ -35,6 +34,13 @@ dependencies: yaml: 3.1.2 logging: 1.2.0 +dependency_overrides: + at_commons: + git: + url: https://github.com/atsign-foundation/at_libraries.git + path: packages/at_commons + ref: expire_otp_changes + dev_dependencies: test: ^1.24.4 coverage: ^1.6.1 diff --git a/packages/at_secondary_server/test/enroll_verb_test.dart b/packages/at_secondary_server/test/enroll_verb_test.dart index 6e48d943d..badf1820a 100644 --- a/packages/at_secondary_server/test/enroll_verb_test.dart +++ b/packages/at_secondary_server/test/enroll_verb_test.dart @@ -316,7 +316,7 @@ void main() { test('A test to verify enrollment request without otp throws exception', () async { String enrollmentRequest = - 'enroll:request:{"appname":"wavi","devicename":"mydevice","namespaces":{"wavi":"r"},"apkampublickey":"dummy_apkam_public_key"}'; + 'enroll:request:{"appName":"wavi","deviceName":"mydevice","namespaces":{"wavi":"r"},"apkamPublicKey":"dummy_apkam_public_key"}'; HashMap verbParams = getVerbParam(VerbSyntax.enroll, enrollmentRequest); inboundConnection.getMetaData().isAuthenticated = false; diff --git a/packages/at_secondary_server/test/otp_verb_test.dart b/packages/at_secondary_server/test/otp_verb_test.dart index 1f7a0276c..a6174b695 100644 --- a/packages/at_secondary_server/test/otp_verb_test.dart +++ b/packages/at_secondary_server/test/otp_verb_test.dart @@ -3,7 +3,6 @@ import 'dart:collection'; import 'package:at_commons/at_commons.dart'; import 'package:at_secondary/src/utils/handler_util.dart'; import 'package:at_secondary/src/verb/handler/otp_verb_handler.dart'; -import 'package:expire_cache/expire_cache.dart'; import 'package:test/test.dart'; import 'test_utils.dart'; @@ -13,19 +12,20 @@ void main() { setUp(() async { await verbTestsSetUp(); }); - test('A test to verify OTP generated is 6-character length', () { + + test('A test to verify OTP generated is 6-character length', () async { Response response = Response(); HashMap verbParams = getVerbParam(VerbSyntax.otp, 'otp:get'); inboundConnection.getMetaData().isAuthenticated = true; OtpVerbHandler otpVerbHandler = OtpVerbHandler(secondaryKeyStore); - otpVerbHandler.processVerb(response, verbParams, inboundConnection); + await otpVerbHandler.processVerb(response, verbParams, inboundConnection); expect(response.data, isNotNull); expect(response.data!.length, 6); assert(RegExp('\\d').hasMatch(response.data!)); }); - test('A test to verify same OTP is not returned', () { + test('A test to verify same OTP is not returned', () async { Set otpSet = {}; for (int i = 1; i <= 1000; i++) { Response response = Response(); @@ -33,7 +33,8 @@ void main() { getVerbParam(VerbSyntax.otp, 'otp:get'); inboundConnection.getMetaData().isAuthenticated = true; OtpVerbHandler otpVerbHandler = OtpVerbHandler(secondaryKeyStore); - otpVerbHandler.processVerb(response, verbParams, inboundConnection); + await otpVerbHandler.processVerb( + response, verbParams, inboundConnection); expect(response.data, isNotNull); expect(response.data!.length, 6); assert(RegExp('\\d').hasMatch(response.data!)); @@ -42,6 +43,35 @@ void main() { } expect(otpSet.length, 1000); }); + + test('A test to verify otp:get with TTL set is active before TTL is met', + () async { + Response response = Response(); + inboundConnection.getMetaData().isAuthenticated = true; + HashMap verbParams = + getVerbParam(VerbSyntax.otp, 'otp:get:ttl:1000'); + OtpVerbHandler otpVerbHandler = OtpVerbHandler(secondaryKeyStore); + await otpVerbHandler.processVerb(response, verbParams, inboundConnection); + String? otp = response.data; + verbParams = getVerbParam(VerbSyntax.otp, 'otp:validate:$otp'); + await otpVerbHandler.processVerb(response, verbParams, inboundConnection); + expect(response.data, 'valid'); + }); + + test('A test to verify otp:get with TTL set expires after the TTL is met', + () async { + Response response = Response(); + inboundConnection.getMetaData().isAuthenticated = true; + HashMap verbParams = + getVerbParam(VerbSyntax.otp, 'otp:get:ttl:1'); + OtpVerbHandler otpVerbHandler = OtpVerbHandler(secondaryKeyStore); + await otpVerbHandler.processVerb(response, verbParams, inboundConnection); + String? otp = response.data; + await Future.delayed(Duration(seconds: 1)); + verbParams = getVerbParam(VerbSyntax.otp, 'otp:validate:$otp'); + await otpVerbHandler.processVerb(response, verbParams, inboundConnection); + expect(response.data, 'invalid'); + }, timeout: Timeout(Duration(minutes: 10))); tearDown(() async => await verbTestsTearDown()); }); @@ -90,14 +120,45 @@ void main() { HashMap verbParams = getVerbParam(VerbSyntax.otp, 'otp:get'); inboundConnection.getMetaData().isAuthenticated = true; - OtpVerbHandler.cache = - ExpireCache(expireDuration: Duration(microseconds: 1)); OtpVerbHandler otpVerbHandler = OtpVerbHandler(secondaryKeyStore); - print(OtpVerbHandler.cache.expireDuration.inMicroseconds); - await Future.delayed(Duration(microseconds: 2)); + otpVerbHandler.otpExpiryInMills = 1; await otpVerbHandler.processVerb(response, verbParams, inboundConnection); - verbParams = - getVerbParam(VerbSyntax.otp, 'otp:validate:${response.data}'); + String? otp = response.data; + await Future.delayed(Duration(milliseconds: 2)); + response = Response(); + verbParams = getVerbParam(VerbSyntax.otp, 'otp:validate:$otp'); + await otpVerbHandler.processVerb(response, verbParams, inboundConnection); + expect(response.data, 'invalid'); + }); + + test( + 'A test to verify otp:validate return invalid when otp does not exist in keystore', + () async { + Response response = Response(); + String otp = 'ABC123'; + HashMap verbParams = + getVerbParam(VerbSyntax.otp, 'otp:validate:$otp'); + OtpVerbHandler otpVerbHandler = OtpVerbHandler(secondaryKeyStore); + await otpVerbHandler.processVerb(response, verbParams, inboundConnection); + expect(response.data, 'invalid'); + }); + + test('A test to verify otp:validate invalidates an OTP after it is used', + () async { + Response response = Response(); + HashMap verbParams = + getVerbParam(VerbSyntax.otp, 'otp:get'); + inboundConnection.getMetaData().isAuthenticated = true; + + OtpVerbHandler otpVerbHandler = OtpVerbHandler(secondaryKeyStore); + await otpVerbHandler.processVerb(response, verbParams, inboundConnection); + String? otp = response.data; + // attempt #1 should return a valid response + verbParams = getVerbParam(VerbSyntax.otp, 'otp:validate:$otp'); + await otpVerbHandler.processVerb(response, verbParams, inboundConnection); + expect(response.data, 'valid'); + // attempt #2 should return a invalid response + verbParams = getVerbParam(VerbSyntax.otp, 'otp:validate:$otp'); await otpVerbHandler.processVerb(response, verbParams, inboundConnection); expect(response.data, 'invalid'); }); diff --git a/tests/at_functional_test/test/enroll_verb_test.dart b/tests/at_functional_test/test/enroll_verb_test.dart index afa4e56db..062c91ae3 100644 --- a/tests/at_functional_test/test/enroll_verb_test.dart +++ b/tests/at_functional_test/test/enroll_verb_test.dart @@ -764,9 +764,6 @@ void main() { socketConnection1!, 'config:set:timeFrameInMills=100\n'); configResponse = await read(); expect(configResponse.trim(), 'data:ok'); - await socket_writer(socketConnection1!, 'otp:get'); - otp = await read(); - otp = otp.replaceAll('data:', '').trim(); }); test( @@ -775,6 +772,9 @@ void main() { SecureSocket unAuthenticatedConnection = await secure_socket_connection(firstAtsignServer, firstAtsignPort); socket_listener(unAuthenticatedConnection); + await socket_writer(socketConnection1!, 'otp:get'); + otp = await read(); + otp = otp.replaceAll('data:', '').trim(); var enrollRequest = 'enroll:request:{"appName":"wavi","deviceName":"pixel","namespaces":{"wavi":"rw"},"otp":"$otp","apkamPublicKey":"${pkamPublicKeyMap[firstAtsign]!}"}\n'; await socket_writer(unAuthenticatedConnection, enrollRequest); @@ -782,6 +782,10 @@ void main() { jsonDecode((await read()).replaceAll('data:', '')); expect(enrollmentResponse['status'], 'pending'); expect(enrollmentResponse['enrollmentId'], isNotNull); + + await socket_writer(socketConnection1!, 'otp:get'); + otp = await read(); + otp = otp.replaceAll('data:', '').trim(); enrollRequest = 'enroll:request:{"appName":"wavi","deviceName":"pixel","namespaces":{"wavi":"rw"},"otp":"$otp","apkamPublicKey":"${pkamPublicKeyMap[firstAtsign]!}"}\n'; await socket_writer(unAuthenticatedConnection, enrollRequest); @@ -798,6 +802,10 @@ void main() { SecureSocket unAuthenticatedConnection = await secure_socket_connection(firstAtsignServer, firstAtsignPort); socket_listener(unAuthenticatedConnection); + + await socket_writer(socketConnection1!, 'otp:get'); + otp = await read(); + otp = otp.replaceAll('data:', '').trim(); var enrollRequest = 'enroll:request:{"appName":"wavi","deviceName":"pixel","namespaces":{"wavi":"rw"},"otp":"$otp","apkamPublicKey":"${pkamPublicKeyMap[firstAtsign]!}"}\n'; await socket_writer(unAuthenticatedConnection, enrollRequest); @@ -805,6 +813,10 @@ void main() { jsonDecode((await read()).replaceAll('data:', '')); expect(enrollmentResponse['status'], 'pending'); expect(enrollmentResponse['enrollmentId'], isNotNull); + + await socket_writer(socketConnection1!, 'otp:get'); + otp = await read(); + otp = otp.replaceAll('data:', '').trim(); enrollRequest = 'enroll:request:{"appName":"wavi","deviceName":"pixel","namespaces":{"wavi":"rw"},"otp":"$otp","apkamPublicKey":"${pkamPublicKeyMap[firstAtsign]!}"}\n'; await socket_writer(unAuthenticatedConnection, enrollRequest); @@ -825,6 +837,10 @@ void main() { SecureSocket unAuthenticatedConnection = await secure_socket_connection(firstAtsignServer, firstAtsignPort); socket_listener(unAuthenticatedConnection); + + await socket_writer(socketConnection1!, 'otp:get'); + otp = await read(); + otp = otp.replaceAll('data:', '').trim(); var enrollRequest = 'enroll:request:{"appName":"wavi","deviceName":"pixel","namespaces":{"wavi":"rw"},"otp":"$otp","apkamPublicKey":"${pkamPublicKeyMap[firstAtsign]!}"}\n'; await socket_writer(unAuthenticatedConnection, enrollRequest); @@ -832,6 +848,10 @@ void main() { jsonDecode((await read()).replaceAll('data:', '')); expect(enrollmentResponse['status'], 'pending'); expect(enrollmentResponse['enrollmentId'], isNotNull); + + await socket_writer(socketConnection1!, 'otp:get'); + otp = await read(); + otp = otp.replaceAll('data:', '').trim(); enrollRequest = 'enroll:request:{"appName":"wavi","deviceName":"pixel","namespaces":{"wavi":"rw"},"otp":"$otp","apkamPublicKey":"${pkamPublicKeyMap[firstAtsign]!}"}\n'; await socket_writer(unAuthenticatedConnection, enrollRequest); @@ -844,6 +864,10 @@ void main() { SecureSocket secondUnAuthenticatedConnection2 = await secure_socket_connection(firstAtsignServer, firstAtsignPort); socket_listener(secondUnAuthenticatedConnection2); + + await socket_writer(socketConnection1!, 'otp:get'); + otp = await read(); + otp = otp.replaceAll('data:', '').trim(); enrollRequest = 'enroll:request:{"appName":"wavi","deviceName":"pixel","namespaces":{"wavi":"rw"},"otp":"$otp","apkamPublicKey":"${pkamPublicKeyMap[firstAtsign]!}"}\n'; await socket_writer(secondUnAuthenticatedConnection2, enrollRequest); diff --git a/tests/at_functional_test/test/otp_verb_test.dart b/tests/at_functional_test/test/otp_verb_test.dart new file mode 100644 index 000000000..96be2e630 --- /dev/null +++ b/tests/at_functional_test/test/otp_verb_test.dart @@ -0,0 +1,50 @@ +import 'dart:io'; + +import 'package:at_functional_test/conf/config_util.dart'; +import 'package:test/test.dart'; + +import 'functional_test_commons.dart'; + +void main() { + late SecureSocket authenticatedConnection; + String firstAtSignServer = + ConfigUtil.getYaml()!['first_atsign_server']['first_atsign_url']; + int firstAtSignPort = + ConfigUtil.getYaml()!['first_atsign_server']['first_atsign_port']; + String firstAtSign = + ConfigUtil.getYaml()!['first_atsign_server']['first_atsign_name']; + + setUp(() async { + authenticatedConnection = + await secure_socket_connection(firstAtSignServer, firstAtSignPort); + socket_listener(authenticatedConnection); + }); + + group('A group of tests related to OTP generation and expiration', () { + test('A test to generate OTP and returns valid before OTP is not expired', + () async { + await prepare(authenticatedConnection, firstAtSign); + await socket_writer(authenticatedConnection, + 'otp:get:ttl:${Duration(minutes: 1).inMilliseconds}'); + String otp = (await read()).trim().replaceAll('data:', ''); + expect(otp, isNotEmpty); + + await socket_writer(authenticatedConnection, 'otp:validate:$otp'); + String response = (await read()).replaceAll('data:', '').trim(); + expect(response, 'valid'); + }); + + test('A test to generate OTP and returns invalid when TTL is met', + () async { + await prepare(authenticatedConnection, firstAtSign); + await socket_writer(authenticatedConnection, 'otp:get:ttl:1'); + String otp = (await read()).trim().replaceAll('data:', ''); + expect(otp, isNotEmpty); + + await Future.delayed(Duration(seconds: 1)); + await socket_writer(authenticatedConnection, 'otp:validate:$otp'); + String response = (await read()).replaceAll('data:', '').trim(); + expect(response, 'invalid'); + }); + }); +} From 62dbbb1d0e5110fcc85a2fd1dcb1752f168dc9a1 Mon Sep 17 00:00:00 2001 From: Sitaram Kalluri Date: Sat, 14 Oct 2023 12:22:25 +0530 Subject: [PATCH 2/5] fix: Move isOTPValid to abstract_verb_handler.dart --- .../verb/handler/abstract_verb_handler.dart | 23 +++++++++++++++++++ .../src/verb/handler/enroll_verb_handler.dart | 6 ++--- .../src/verb/handler/otp_verb_handler.dart | 18 --------------- 3 files changed, 25 insertions(+), 22 deletions(-) diff --git a/packages/at_secondary_server/lib/src/verb/handler/abstract_verb_handler.dart b/packages/at_secondary_server/lib/src/verb/handler/abstract_verb_handler.dart index 5e0c52b50..6613c666c 100644 --- a/packages/at_secondary_server/lib/src/verb/handler/abstract_verb_handler.dart +++ b/packages/at_secondary_server/lib/src/verb/handler/abstract_verb_handler.dart @@ -152,4 +152,27 @@ abstract class AbstractVerbHandler implements VerbHandler { return false; } } + + + /// This function checks the validity of a provided OTP. + /// It returns true if the OTP is valid; otherwise, it returns false. + /// If the OTP is not found in the keystore, it also returns false. + /// + /// Additionally, this function removes the OTP from the keystore to prevent its reuse. + Future isOTPValid(String? otp) async { + if (otp == null) { + return false; + } + String otpKey = + 'private:${otp.toLowerCase()}${AtSecondaryServerImpl.getInstance().currentAtSign}'; + AtData otpAtData; + try { + otpAtData = await keyStore.get(otpKey); + } on KeyNotFoundException { + return false; + } + // Remove the key from keystore to prevent reuse of OTP. + await keyStore.remove(otpKey); + return SecondaryUtil.isActiveKey(otpAtData); + } } diff --git a/packages/at_secondary_server/lib/src/verb/handler/enroll_verb_handler.dart b/packages/at_secondary_server/lib/src/verb/handler/enroll_verb_handler.dart index 7a59ad8ac..604fa3abc 100644 --- a/packages/at_secondary_server/lib/src/verb/handler/enroll_verb_handler.dart +++ b/packages/at_secondary_server/lib/src/verb/handler/enroll_verb_handler.dart @@ -8,7 +8,6 @@ import 'package:at_secondary/src/server/at_secondary_config.dart'; import 'package:at_secondary/src/constants/enroll_constants.dart'; import 'package:at_secondary/src/enroll/enroll_datastore_value.dart'; import 'package:at_secondary/src/utils/notification_util.dart'; -import 'package:at_secondary/src/verb/handler/otp_verb_handler.dart'; import 'package:at_server_spec/at_server_spec.dart'; import 'package:at_server_spec/at_verb_spec.dart'; import 'package:meta/meta.dart'; @@ -120,9 +119,8 @@ class EnrollVerbHandler extends AbstractVerbHandler { // OTP is sent only in enrollment request which is submitted on // unauthenticated connection. if (atConnection.getMetaData().isAuthenticated == false) { - Response otpResponse = await OtpVerbHandler(keyStore) - .processInternal('otp:validate:${enrollParams.otp}', atConnection); - if (otpResponse.data == 'invalid') { + var isValid = await isOTPValid(enrollParams.otp); + if (!isValid) { throw AtEnrollmentException( 'invalid otp. Cannot process enroll request'); } diff --git a/packages/at_secondary_server/lib/src/verb/handler/otp_verb_handler.dart b/packages/at_secondary_server/lib/src/verb/handler/otp_verb_handler.dart index 39c628e77..6dab5e465 100644 --- a/packages/at_secondary_server/lib/src/verb/handler/otp_verb_handler.dart +++ b/packages/at_secondary_server/lib/src/verb/handler/otp_verb_handler.dart @@ -2,7 +2,6 @@ import 'dart:collection'; import 'dart:math'; import 'package:at_commons/at_commons.dart'; import 'package:at_secondary/src/server/at_secondary_impl.dart'; -import 'package:at_secondary/src/utils/secondary_util.dart'; import 'package:at_server_spec/at_server_spec.dart'; import 'package:meta/meta.dart'; import 'package:uuid/uuid.dart'; @@ -62,23 +61,6 @@ class OtpVerbHandler extends AbstractVerbHandler { } } - Future isOTPValid(String? otp) async { - if (otp == null) { - return false; - } - String otpKey = - 'private:${otp.toLowerCase()}${AtSecondaryServerImpl.getInstance().currentAtSign}'; - AtData otpAtData; - try { - otpAtData = await keyStore.get(otpKey); - } on KeyNotFoundException { - return false; - } - // Remove the key from keystore to prevent reuse of OTP. - await keyStore.remove(otpKey); - return SecondaryUtil.isActiveKey(otpAtData); - } - /// This function generates a UUID and converts it into a 6-character alpha-numeric string. /// /// The process involves converting the UUID to a hashcode, then transforming the hashcode From 4282265605cce434ed6f2060e1b0619f069e3654 Mon Sep 17 00:00:00 2001 From: Sitaram Kalluri Date: Tue, 17 Oct 2023 12:57:13 +0530 Subject: [PATCH 3/5] fix: Remove otp:validate and delete otp at the end of processing enrollment --- packages/at_secondary_server/CHANGELOG.md | 2 + .../verb/handler/abstract_verb_handler.dart | 2 - .../src/verb/handler/enroll_verb_handler.dart | 3 ++ .../src/verb/handler/otp_verb_handler.dart | 7 --- .../test/enroll_verb_test.dart | 26 +++++++++- .../test/otp_verb_test.dart | 50 +++---------------- .../test/otp_verb_test.dart | 50 ------------------- 7 files changed, 38 insertions(+), 102 deletions(-) delete mode 100644 tests/at_functional_test/test/otp_verb_test.dart diff --git a/packages/at_secondary_server/CHANGELOG.md b/packages/at_secondary_server/CHANGELOG.md index 197669c6c..2be2a3835 100644 --- a/packages/at_secondary_server/CHANGELOG.md +++ b/packages/at_secondary_server/CHANGELOG.md @@ -2,6 +2,8 @@ - fix: Implement notify ephemeral changes - Send notification with value without caching the key on receiver's secondary server - feat: Implement AtRateLimiter to limit the enrollment requests on a particular connection - fix: Upgraded at_commons to 3.0.56 +- fix: Enable client to set OTP expiry via OTP verb +- fix: Prevent reuse of OTP ## 3.0.35 - chore: Upgraded at_persistence_secondary_server to 3.0.57 for memory optimization in commit log - feat: APKAM keys verb implementation diff --git a/packages/at_secondary_server/lib/src/verb/handler/abstract_verb_handler.dart b/packages/at_secondary_server/lib/src/verb/handler/abstract_verb_handler.dart index 6613c666c..e39f9f05d 100644 --- a/packages/at_secondary_server/lib/src/verb/handler/abstract_verb_handler.dart +++ b/packages/at_secondary_server/lib/src/verb/handler/abstract_verb_handler.dart @@ -171,8 +171,6 @@ abstract class AbstractVerbHandler implements VerbHandler { } on KeyNotFoundException { return false; } - // Remove the key from keystore to prevent reuse of OTP. - await keyStore.remove(otpKey); return SecondaryUtil.isActiveKey(otpAtData); } } diff --git a/packages/at_secondary_server/lib/src/verb/handler/enroll_verb_handler.dart b/packages/at_secondary_server/lib/src/verb/handler/enroll_verb_handler.dart index 604fa3abc..215f8104c 100644 --- a/packages/at_secondary_server/lib/src/verb/handler/enroll_verb_handler.dart +++ b/packages/at_secondary_server/lib/src/verb/handler/enroll_verb_handler.dart @@ -173,6 +173,9 @@ class EnrollVerbHandler extends AbstractVerbHandler { } logger.finer('enrollData: $enrollData'); await keyStore.put('$key$currentAtSign', enrollData, skipCommit: true); + // Remove the OTP from keystore to prevent reuse. + await keyStore.remove( + 'private:${enrollParams.otp?.toLowerCase()}${AtSecondaryServerImpl.getInstance().currentAtSign}'); } /// Handles enrollment approve, deny and revoke requests. diff --git a/packages/at_secondary_server/lib/src/verb/handler/otp_verb_handler.dart b/packages/at_secondary_server/lib/src/verb/handler/otp_verb_handler.dart index 6dab5e465..6901f373d 100644 --- a/packages/at_secondary_server/lib/src/verb/handler/otp_verb_handler.dart +++ b/packages/at_secondary_server/lib/src/verb/handler/otp_verb_handler.dart @@ -51,13 +51,6 @@ class OtpVerbHandler extends AbstractVerbHandler { '${DateTime.now().toUtc().add(Duration(milliseconds: otpExpiryInMills)).millisecondsSinceEpoch}' ..metaData = (AtMetaData()..ttl = otpExpiryInMills)); break; - case 'validate': - var isValid = await isOTPValid(verbParams['otp']); - if (isValid) { - response.data = 'valid'; - return; - } - response.data = 'invalid'; } } diff --git a/packages/at_secondary_server/test/enroll_verb_test.dart b/packages/at_secondary_server/test/enroll_verb_test.dart index badf1820a..236bf5cb7 100644 --- a/packages/at_secondary_server/test/enroll_verb_test.dart +++ b/packages/at_secondary_server/test/enroll_verb_test.dart @@ -41,7 +41,6 @@ void main() { OtpVerbHandler otpVerbHandler = OtpVerbHandler(secondaryKeyStore); await otpVerbHandler.processVerb( response, otpVerbParams, inboundConnection); - print('OTP: ${response.data}'); // Enroll request 2 enrollmentRequest = 'enroll:request:{"appName":"wavi","deviceName":"mydevice","namespaces":{"buzz":"r"},"otp":"${response.data}","apkamPublicKey":"dummy_apkam_public_key"}'; @@ -81,6 +80,31 @@ void main() { expect(enrollmentValue.namespaces.containsKey('__manage'), true); expect(enrollmentValue.namespaces.containsKey('*'), true); }); + + test('A test to verify OTP is deleted once it is used to submit an enrollment',() async { + Response response = Response(); + // OTP Verb + inboundConnection.getMetaData().isAuthenticated = true; + inboundConnection.getMetaData().sessionID = 'dummy_session'; + HashMap otpVerbParams = + getVerbParam(VerbSyntax.otp, 'otp:get'); + OtpVerbHandler otpVerbHandler = OtpVerbHandler(secondaryKeyStore); + await otpVerbHandler.processVerb( + response, otpVerbParams, inboundConnection); + String otp = response.data!; + + String enrollmentRequest = + 'enroll:request:{"appName":"wavi","deviceName":"mydevice","namespaces":{"buzz":"r"},"otp":"$otp","apkamPublicKey":"dummy_apkam_public_key"}'; + HashMap enrollmentRequestVerbParams = + getVerbParam(VerbSyntax.enroll, enrollmentRequest); + inboundConnection.getMetaData().isAuthenticated = false; + EnrollVerbHandler enrollVerbHandler = EnrollVerbHandler(secondaryKeyStore); + await enrollVerbHandler.processVerb( + response, enrollmentRequestVerbParams, inboundConnection); + String enrollmentId = jsonDecode(response.data!)['enrollmentId']; + expect(enrollmentId, isNotNull); + expect(await enrollVerbHandler.isOTPValid(otp), false); + }); tearDown(() async => await verbTestsTearDown()); }); group('A group of tests to verify enroll list operation', () { diff --git a/packages/at_secondary_server/test/otp_verb_test.dart b/packages/at_secondary_server/test/otp_verb_test.dart index a6174b695..a113cf8ce 100644 --- a/packages/at_secondary_server/test/otp_verb_test.dart +++ b/packages/at_secondary_server/test/otp_verb_test.dart @@ -53,9 +53,7 @@ void main() { OtpVerbHandler otpVerbHandler = OtpVerbHandler(secondaryKeyStore); await otpVerbHandler.processVerb(response, verbParams, inboundConnection); String? otp = response.data; - verbParams = getVerbParam(VerbSyntax.otp, 'otp:validate:$otp'); - await otpVerbHandler.processVerb(response, verbParams, inboundConnection); - expect(response.data, 'valid'); + expect(await otpVerbHandler.isOTPValid(otp), true); }); test('A test to verify otp:get with TTL set expires after the TTL is met', @@ -68,10 +66,8 @@ void main() { await otpVerbHandler.processVerb(response, verbParams, inboundConnection); String? otp = response.data; await Future.delayed(Duration(seconds: 1)); - verbParams = getVerbParam(VerbSyntax.otp, 'otp:validate:$otp'); - await otpVerbHandler.processVerb(response, verbParams, inboundConnection); - expect(response.data, 'invalid'); - }, timeout: Timeout(Duration(minutes: 10))); + expect(await otpVerbHandler.isOTPValid(otp), false); + }); tearDown(() async => await verbTestsTearDown()); }); @@ -96,11 +92,11 @@ void main() { tearDown(() async => await verbTestsTearDown()); }); - group('A group of tests related to otp:validate', () { + group('A group of tests related to OTP validity', () { setUp(() async { await verbTestsSetUp(); }); - test('A test to verify otp:validate returns valid when OTP is active', + test('A test to verify isOTPValid method returns valid when OTP is active', () async { Response response = Response(); HashMap verbParams = @@ -108,10 +104,7 @@ void main() { inboundConnection.getMetaData().isAuthenticated = true; OtpVerbHandler otpVerbHandler = OtpVerbHandler(secondaryKeyStore); await otpVerbHandler.processVerb(response, verbParams, inboundConnection); - verbParams = - getVerbParam(VerbSyntax.otp, 'otp:validate:${response.data}'); - await otpVerbHandler.processVerb(response, verbParams, inboundConnection); - expect(response.data, 'valid'); + expect(await otpVerbHandler.isOTPValid(response.data), true); }); test('A test to verify otp:validate returns invalid when OTP is expired', @@ -125,42 +118,15 @@ void main() { await otpVerbHandler.processVerb(response, verbParams, inboundConnection); String? otp = response.data; await Future.delayed(Duration(milliseconds: 2)); - response = Response(); - verbParams = getVerbParam(VerbSyntax.otp, 'otp:validate:$otp'); - await otpVerbHandler.processVerb(response, verbParams, inboundConnection); - expect(response.data, 'invalid'); + expect(await otpVerbHandler.isOTPValid(otp), false); }); test( 'A test to verify otp:validate return invalid when otp does not exist in keystore', () async { - Response response = Response(); String otp = 'ABC123'; - HashMap verbParams = - getVerbParam(VerbSyntax.otp, 'otp:validate:$otp'); OtpVerbHandler otpVerbHandler = OtpVerbHandler(secondaryKeyStore); - await otpVerbHandler.processVerb(response, verbParams, inboundConnection); - expect(response.data, 'invalid'); - }); - - test('A test to verify otp:validate invalidates an OTP after it is used', - () async { - Response response = Response(); - HashMap verbParams = - getVerbParam(VerbSyntax.otp, 'otp:get'); - inboundConnection.getMetaData().isAuthenticated = true; - - OtpVerbHandler otpVerbHandler = OtpVerbHandler(secondaryKeyStore); - await otpVerbHandler.processVerb(response, verbParams, inboundConnection); - String? otp = response.data; - // attempt #1 should return a valid response - verbParams = getVerbParam(VerbSyntax.otp, 'otp:validate:$otp'); - await otpVerbHandler.processVerb(response, verbParams, inboundConnection); - expect(response.data, 'valid'); - // attempt #2 should return a invalid response - verbParams = getVerbParam(VerbSyntax.otp, 'otp:validate:$otp'); - await otpVerbHandler.processVerb(response, verbParams, inboundConnection); - expect(response.data, 'invalid'); + expect(await otpVerbHandler.isOTPValid(otp), false); }); tearDown(() async => await verbTestsTearDown()); }); diff --git a/tests/at_functional_test/test/otp_verb_test.dart b/tests/at_functional_test/test/otp_verb_test.dart deleted file mode 100644 index 96be2e630..000000000 --- a/tests/at_functional_test/test/otp_verb_test.dart +++ /dev/null @@ -1,50 +0,0 @@ -import 'dart:io'; - -import 'package:at_functional_test/conf/config_util.dart'; -import 'package:test/test.dart'; - -import 'functional_test_commons.dart'; - -void main() { - late SecureSocket authenticatedConnection; - String firstAtSignServer = - ConfigUtil.getYaml()!['first_atsign_server']['first_atsign_url']; - int firstAtSignPort = - ConfigUtil.getYaml()!['first_atsign_server']['first_atsign_port']; - String firstAtSign = - ConfigUtil.getYaml()!['first_atsign_server']['first_atsign_name']; - - setUp(() async { - authenticatedConnection = - await secure_socket_connection(firstAtSignServer, firstAtSignPort); - socket_listener(authenticatedConnection); - }); - - group('A group of tests related to OTP generation and expiration', () { - test('A test to generate OTP and returns valid before OTP is not expired', - () async { - await prepare(authenticatedConnection, firstAtSign); - await socket_writer(authenticatedConnection, - 'otp:get:ttl:${Duration(minutes: 1).inMilliseconds}'); - String otp = (await read()).trim().replaceAll('data:', ''); - expect(otp, isNotEmpty); - - await socket_writer(authenticatedConnection, 'otp:validate:$otp'); - String response = (await read()).replaceAll('data:', '').trim(); - expect(response, 'valid'); - }); - - test('A test to generate OTP and returns invalid when TTL is met', - () async { - await prepare(authenticatedConnection, firstAtSign); - await socket_writer(authenticatedConnection, 'otp:get:ttl:1'); - String otp = (await read()).trim().replaceAll('data:', ''); - expect(otp, isNotEmpty); - - await Future.delayed(Duration(seconds: 1)); - await socket_writer(authenticatedConnection, 'otp:validate:$otp'); - String response = (await read()).replaceAll('data:', '').trim(); - expect(response, 'invalid'); - }); - }); -} From da3a4cae899288c4baee1461cbee4e8bc3f4453e Mon Sep 17 00:00:00 2001 From: Sitaram Kalluri Date: Tue, 17 Oct 2023 20:35:32 +0530 Subject: [PATCH 4/5] fix: Replace dependency overrides with hosted version of at_commons --- packages/at_secondary_server/pubspec.yaml | 11 ++--------- 1 file changed, 2 insertions(+), 9 deletions(-) diff --git a/packages/at_secondary_server/pubspec.yaml b/packages/at_secondary_server/pubspec.yaml index d7d762eb5..982ef2778 100644 --- a/packages/at_secondary_server/pubspec.yaml +++ b/packages/at_secondary_server/pubspec.yaml @@ -19,7 +19,7 @@ dependencies: basic_utils: 5.6.1 ecdsa: 0.0.4 encrypt: 5.0.3 - at_commons: 3.0.56 + at_commons: 3.0.57 at_utils: 3.0.15 at_chops: 1.0.4 at_lookup: 3.0.40 @@ -30,17 +30,10 @@ dependencies: json_annotation: ^4.8.0 version: 3.0.2 meta: 1.10.0 - mutex: 3.0.1 + mutex: 3.1.0 yaml: 3.1.2 logging: 1.2.0 -dependency_overrides: - at_commons: - git: - url: https://github.com/atsign-foundation/at_libraries.git - path: packages/at_commons - ref: expire_otp_changes - dev_dependencies: test: ^1.24.4 coverage: ^1.6.1 From 2e49ad493233d72b17c2a28c8b5ad58ab697f12b Mon Sep 17 00:00:00 2001 From: Sitaram Kalluri Date: Wed, 18 Oct 2023 10:47:38 +0530 Subject: [PATCH 5/5] fix: Add "default" case to switch in processVerb --- .../lib/src/verb/handler/otp_verb_handler.dart | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/packages/at_secondary_server/lib/src/verb/handler/otp_verb_handler.dart b/packages/at_secondary_server/lib/src/verb/handler/otp_verb_handler.dart index 6901f373d..b9b9ae7cd 100644 --- a/packages/at_secondary_server/lib/src/verb/handler/otp_verb_handler.dart +++ b/packages/at_secondary_server/lib/src/verb/handler/otp_verb_handler.dart @@ -18,7 +18,7 @@ class OtpVerbHandler extends AbstractVerbHandler { OtpVerbHandler(SecondaryKeyStore keyStore) : super(keyStore); @override - bool accept(String command) => command.startsWith('otp'); + bool accept(String command) => command == 'otp:get'; @override Verb getVerb() => otpVerb; @@ -51,6 +51,8 @@ class OtpVerbHandler extends AbstractVerbHandler { '${DateTime.now().toUtc().add(Duration(milliseconds: otpExpiryInMills)).millisecondsSinceEpoch}' ..metaData = (AtMetaData()..ttl = otpExpiryInMills)); break; + default: + throw InvalidSyntaxException('$operation is not a valid operation'); } }