From c3630b43e309b55448b9ea3fde8902e4e2540bbd Mon Sep 17 00:00:00 2001 From: Srie Teja Date: Thu, 5 Oct 2023 03:56:52 +0530 Subject: [PATCH 1/8] fix: ensure otp is invalidated after use --- .../src/verb/handler/enroll_verb_handler.dart | 3 +- .../src/verb/handler/otp_verb_handler.dart | 28 +++++++++++-------- 2 files changed, 18 insertions(+), 13 deletions(-) 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..1d88367eb 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 @@ -118,8 +118,7 @@ class EnrollVerbHandler extends AbstractVerbHandler { } if (!atConnection.getMetaData().isAuthenticated) { var otp = enrollParams.otp; - if (otp == null || - (await OtpVerbHandler.cache.get(otp.toString()) == null)) { + if (!await OtpVerbHandler.isValidOtp(otp)) { 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 ac8839cb7..c13512f9d 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 @@ -31,10 +31,10 @@ class OtpVerbHandler extends AbstractVerbHandler { final operation = verbParams['operation']; switch (operation) { case 'get': - if (!atConnection.getMetaData().isAuthenticated) { - throw UnAuthenticatedException( - 'otp:get requires authenticated connection'); - } + // if (!atConnection.getMetaData().isAuthenticated) { + // throw UnAuthenticatedException( + // 'otp:get requires authenticated connection'); + // } do { response.data = _generateOTP(); } @@ -43,16 +43,22 @@ class OtpVerbHandler extends AbstractVerbHandler { await cache.set(response.data!, response.data!); break; case 'validate': - String? otp = verbParams['otp']; - if (otp != null && (await cache.get(otp)) == otp) { - response.data = 'valid'; - } else { - response.data = 'invalid'; - } + response.data = + await isValidOtp(verbParams['otp']) ? 'valid' : 'invalid'; break; } } + static Future isValidOtp(String? otp) async { + if (otp != null && (await cache.get(otp)) == otp) { + // if an opt is validated, remove that otp from the cache + await cache.invalidate(otp); + return true; + } else { + return false; + } + } + /// 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 @@ -88,7 +94,7 @@ class OtpVerbHandler extends AbstractVerbHandler { int randomAscii; do { randomAscii = minAscii + Random().nextInt((maxAscii - minAscii) + 1); - // 79 is the ASCII value of "O". If randamAscii is 79, generate again. + // 79 is the ASCII value of "O". If randomAscii is 79, generate again. } while (randomAscii == 79); return String.fromCharCode(randomAscii); } From 167636c4457d5b219f03d4a2db6a40c6177d8298 Mon Sep 17 00:00:00 2001 From: Srie Teja Date: Thu, 5 Oct 2023 03:57:22 +0530 Subject: [PATCH 2/8] test: add unit test to ensure the change works --- .../test/otp_verb_test.dart | 22 +++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/packages/at_secondary_server/test/otp_verb_test.dart b/packages/at_secondary_server/test/otp_verb_test.dart index 1f7a0276c..501081dd1 100644 --- a/packages/at_secondary_server/test/otp_verb_test.dart +++ b/packages/at_secondary_server/test/otp_verb_test.dart @@ -84,6 +84,28 @@ void main() { expect(response.data, 'valid'); }); + test('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'); + }); + test('A test to verify otp:validate returns invalid when OTP is expired', () async { Response response = Response(); From ce9f1d89b91c81ff1e933d45cb1e00a9da722e43 Mon Sep 17 00:00:00 2001 From: Srie Teja Date: Thu, 5 Oct 2023 03:57:31 +0530 Subject: [PATCH 3/8] test: add functional test to ensure the change works --- .../test/all_verbs_test.dart | 22 ++++++++++++++++++- 1 file changed, 21 insertions(+), 1 deletion(-) diff --git a/tests/at_functional_test/test/all_verbs_test.dart b/tests/at_functional_test/test/all_verbs_test.dart index d638a8e64..9411d166a 100644 --- a/tests/at_functional_test/test/all_verbs_test.dart +++ b/tests/at_functional_test/test/all_verbs_test.dart @@ -147,8 +147,28 @@ void main() async { response = await read(); print('config verb response $response'); expect(response, contains('data:25')); - }); + test('fetch and validate otp', ()async{ + await socket_writer(socketFirstAtsign!, 'otp:get'); + String otp = (await read()).replaceFirst('data:', ''); + expect(otp.length, 6); + // validate otp + await socket_writer(socketFirstAtsign!, 'otp:validate:$otp'); + expect(await read(), 'data:valid'); + }); + + test('ensure otp is invalidated after use', ()async{ + await socket_writer(socketFirstAtsign!, 'otp:get'); + String otp = (await read()).replaceFirst('data:', ''); + expect(otp.length, 6); + // validate otp attempt #1 should be valid + await socket_writer(socketFirstAtsign!, 'otp:validate:$otp'); + expect(await read(), 'data:valid'); + // validate otp attempt #2 should be invalid + await socket_writer(socketFirstAtsign!, 'otp:validate:$otp'); + expect(await read(), 'data:invalid'); + }); + }); tearDown(() { //Closing the socket connection From 1517d91bb3e82364a7d4a02ed9b3c5f4c3e26812 Mon Sep 17 00:00:00 2001 From: Srie Teja Date: Thu, 5 Oct 2023 03:59:00 +0530 Subject: [PATCH 4/8] fix: revert unnecessary code --- .../lib/src/verb/handler/otp_verb_handler.dart | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) 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 c13512f9d..9b1235867 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 @@ -31,10 +31,10 @@ class OtpVerbHandler extends AbstractVerbHandler { final operation = verbParams['operation']; switch (operation) { case 'get': - // if (!atConnection.getMetaData().isAuthenticated) { - // throw UnAuthenticatedException( - // 'otp:get requires authenticated connection'); - // } + if (!atConnection.getMetaData().isAuthenticated) { + throw UnAuthenticatedException( + 'otp:get requires authenticated connection'); + } do { response.data = _generateOTP(); } From e844a779a67bc1b2be6aaed555b39f55fa477c92 Mon Sep 17 00:00:00 2001 From: Srie Teja Date: Fri, 6 Oct 2023 03:27:04 +0530 Subject: [PATCH 5/8] test: generate a new otp for each new request --- .../test/enroll_verb_test.dart | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/tests/at_functional_test/test/enroll_verb_test.dart b/tests/at_functional_test/test/enroll_verb_test.dart index afa4e56db..340ecf167 100644 --- a/tests/at_functional_test/test/enroll_verb_test.dart +++ b/tests/at_functional_test/test/enroll_verb_test.dart @@ -769,6 +769,18 @@ void main() { otp = otp.replaceAll('data:', '').trim(); }); + Future getNewOtp() async { + socketConnection2 = + await secure_socket_connection(firstAtsignServer, firstAtsignPort); + socket_listener(socketConnection2!); + await prepare(socketConnection2!, firstAtsign); + await socket_writer(socketConnection2!, 'otp:get'); + String otp = (await read()).replaceFirst('data:', ''); + otp = otp.replaceFirst('\n', ''); + await socketConnection2?.close(); + return otp; + } + test( 'A test to verify exception is thrown when request exceed the configured limit', () async { @@ -805,11 +817,13 @@ void main() { jsonDecode((await read()).replaceAll('data:', '')); expect(enrollmentResponse['status'], 'pending'); expect(enrollmentResponse['enrollmentId'], isNotNull); + otp = await getNewOtp(); enrollRequest = 'enroll:request:{"appName":"wavi","deviceName":"pixel","namespaces":{"wavi":"rw"},"otp":"$otp","apkamPublicKey":"${pkamPublicKeyMap[firstAtsign]!}"}\n'; await socket_writer(unAuthenticatedConnection, enrollRequest); enrollmentResponse = await read() ..replaceAll('error:', ''); + print(enrollmentResponse); expect( enrollmentResponse.contains( 'Enrollment requests have exceeded the limit within the specified time frame'), @@ -832,6 +846,7 @@ void main() { jsonDecode((await read()).replaceAll('data:', '')); expect(enrollmentResponse['status'], 'pending'); expect(enrollmentResponse['enrollmentId'], isNotNull); + otp = await getNewOtp(); enrollRequest = 'enroll:request:{"appName":"wavi","deviceName":"pixel","namespaces":{"wavi":"rw"},"otp":"$otp","apkamPublicKey":"${pkamPublicKeyMap[firstAtsign]!}"}\n'; await socket_writer(unAuthenticatedConnection, enrollRequest); @@ -844,6 +859,7 @@ void main() { SecureSocket secondUnAuthenticatedConnection2 = await secure_socket_connection(firstAtsignServer, firstAtsignPort); socket_listener(secondUnAuthenticatedConnection2); + otp = await getNewOtp(); enrollRequest = 'enroll:request:{"appName":"wavi","deviceName":"pixel","namespaces":{"wavi":"rw"},"otp":"$otp","apkamPublicKey":"${pkamPublicKeyMap[firstAtsign]!}"}\n'; await socket_writer(secondUnAuthenticatedConnection2, enrollRequest); From 1bcace89c2689243b0f5b3a169f6116a0ac51a98 Mon Sep 17 00:00:00 2001 From: Srie Teja Date: Fri, 6 Oct 2023 03:36:13 +0530 Subject: [PATCH 6/8] test: fix formatting issues causing tests to fail --- packages/at_secondary_server/config/config.yaml | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/at_secondary_server/config/config.yaml b/packages/at_secondary_server/config/config.yaml index 4b27f99e4..6af01548d 100644 --- a/packages/at_secondary_server/config/config.yaml +++ b/packages/at_secondary_server/config/config.yaml @@ -9,7 +9,7 @@ root_server: # Default logger settings log: - level: INFO + level: FINER # The atProtocol security configurations. security: @@ -17,9 +17,9 @@ security: # When [useTLS] is set to true, the secondary server starts in secure mode. On setting [useTLS] to true, the [certificateChainLocation] and # [privateKeyLocation] should be populated with the path's to respective certificates. useTLS: true - certificateChainLocation: 'certs/fullchain.pem' - privateKeyLocation: 'certs/privkey.pem' - trustedCertificateLocation: '/etc/cacert/cacert.pem' + certificateChainLocation: '/home/srie/Desktop/work/at_server/tools/build_virtual_environment/ve_base/contents/atsign/secondary/base/certs/fullchain.pem' + privateKeyLocation: '/home/srie/Desktop/work/at_server/tools/build_virtual_environment/ve_base/contents/atsign/secondary/base/certs/privkey.pem' + trustedCertificateLocation: '/home/srie/Desktop/work/at_server/tools/build_virtual_environment/ve_base/contents/atsign/secondary/base/certs/cacert.pem' clientCertificateRequired: true # The atProtocol storage configurations From 846c9ee47dde07f3afe1aa29584fe6ba7f88ac68 Mon Sep 17 00:00:00 2001 From: Srie Teja Date: Fri, 6 Oct 2023 03:41:49 +0530 Subject: [PATCH 7/8] test: fix formatting issues causing tests to fail --- .../test/all_verbs_test.dart | 123 +++++++++--------- 1 file changed, 62 insertions(+), 61 deletions(-) diff --git a/tests/at_functional_test/test/all_verbs_test.dart b/tests/at_functional_test/test/all_verbs_test.dart index 9411d166a..1e163213a 100644 --- a/tests/at_functional_test/test/all_verbs_test.dart +++ b/tests/at_functional_test/test/all_verbs_test.dart @@ -105,69 +105,70 @@ void main() async { //THE FOLLOWING TESTS ONLY WORK WHEN IN TESTING MODE test('config verb test set-reset-print operation', () async { - //the below block of code sets the commit log compaction freq to 4 - await socket_writer( - socketFirstAtsign!, 'config:set:commitLogCompactionFrequencyMins=4'); - var response = await read(); - print('config verb response $response'); - expect(response, contains('data:ok')); - - //this resets the commit log compaction freq previously set to 4 to default value - await socket_writer( - socketFirstAtsign!, 'config:reset:commitLogCompactionFrequencyMins'); - //await Future.delayed(Duration(seconds: 2)); - response = await read(); - print('config verb response $response'); - expect(response, contains('data:ok')); - - // this ensures that the reset actually works and the current value is 18 as per the config - // at tools/build_virtual_environment/ve_base/contents/atsign/secondary/base/config/config.yaml - await socket_writer( - socketFirstAtsign!, 'config:print:commitLogCompactionFrequencyMins'); - //await Future.delayed(Duration(seconds: 2)); - response = (await read()).trim(); - print('config verb response [$response]'); - // TODO gkc 20230219 It's two values because we used to set to 30 but now we're setting to 18. - // TODO gkc 20230219 We can remove the 'or' once build_virtual_environment is merged to trunk - expect((response == 'data:18' || response == 'data:30'), true); + //the below block of code sets the commit log compaction freq to 4 + await socket_writer( + socketFirstAtsign!, 'config:set:commitLogCompactionFrequencyMins=4'); + var response = await read(); + print('config verb response $response'); + expect(response, contains('data:ok')); + + //this resets the commit log compaction freq previously set to 4 to default value + await socket_writer( + socketFirstAtsign!, 'config:reset:commitLogCompactionFrequencyMins'); + //await Future.delayed(Duration(seconds: 2)); + response = await read(); + print('config verb response $response'); + expect(response, contains('data:ok')); + + // this ensures that the reset actually works and the current value is 18 as per the config + // at tools/build_virtual_environment/ve_base/contents/atsign/secondary/base/config/config.yaml + await socket_writer( + socketFirstAtsign!, 'config:print:commitLogCompactionFrequencyMins'); + //await Future.delayed(Duration(seconds: 2)); + response = (await read()).trim(); + print('config verb response [$response]'); + // TODO gkc 20230219 It's two values because we used to set to 30 but now we're setting to 18. + // TODO gkc 20230219 We can remove the 'or' once build_virtual_environment is merged to trunk + expect((response == 'data:18' || response == 'data:30'), true); }); test('config verb test set-print', () async { - //the block of code below sets max notification retries to 25 - await socket_writer( - socketFirstAtsign!, 'config:set:maxNotificationRetries=25'); - var response = await read(); - print('config verb response $response'); - expect(response, contains('data:ok')); - - //the block of code below verifies that the max notification retries is set to 25 - await socket_writer( - socketFirstAtsign!, 'config:print:maxNotificationRetries'); - await Future.delayed(Duration(seconds: 2)); - response = await read(); - print('config verb response $response'); - expect(response, contains('data:25')); - - test('fetch and validate otp', ()async{ - await socket_writer(socketFirstAtsign!, 'otp:get'); - String otp = (await read()).replaceFirst('data:', ''); - expect(otp.length, 6); - // validate otp - await socket_writer(socketFirstAtsign!, 'otp:validate:$otp'); - expect(await read(), 'data:valid'); - }); - - test('ensure otp is invalidated after use', ()async{ - await socket_writer(socketFirstAtsign!, 'otp:get'); - String otp = (await read()).replaceFirst('data:', ''); - expect(otp.length, 6); - // validate otp attempt #1 should be valid - await socket_writer(socketFirstAtsign!, 'otp:validate:$otp'); - expect(await read(), 'data:valid'); - // validate otp attempt #2 should be invalid - await socket_writer(socketFirstAtsign!, 'otp:validate:$otp'); - expect(await read(), 'data:invalid'); - }); + //the block of code below sets max notification retries to 25 + await socket_writer( + socketFirstAtsign!, 'config:set:maxNotificationRetries=25'); + var response = await read(); + print('config verb response $response'); + expect(response, contains('data:ok')); + + //the block of code below verifies that the max notification retries is set to 25 + await socket_writer( + socketFirstAtsign!, 'config:print:maxNotificationRetries'); + await Future.delayed(Duration(seconds: 2)); + response = await read(); + print('config verb response $response'); + expect(response, contains('data:25')); + }); + + test('fetch and validate otp', () async { + await socket_writer(socketFirstAtsign!, 'otp:get'); + String otp = (await read()).replaceFirst('data:', ''); + otp = otp.replaceFirst('\n', ''); + expect(otp.length, 6); + // validate otp + await socket_writer(socketFirstAtsign!, 'otp:validate:$otp'); + expect(await read(), 'data:valid\n'); + }); + + test('ensure otp is invalidated after use', () async { + await socket_writer(socketFirstAtsign!, 'otp:get'); + String otp = (await read()).replaceFirst('data:', ''); + otp = otp.replaceFirst('\n', ''); + // validate otp attempt #1 should be valid + await socket_writer(socketFirstAtsign!, 'otp:validate:$otp'); + expect(await read(), 'data:valid\n'); + // validate otp attempt #2 should be invalid + await socket_writer(socketFirstAtsign!, 'otp:validate:$otp'); + expect(await read(), 'data:invalid\n'); }); tearDown(() { @@ -175,4 +176,4 @@ void main() async { clear(); socketFirstAtsign!.destroy(); }); -} \ No newline at end of file +} From 3ff7e0a81bddaf31bf5cd7f001797eb8180e3dcf Mon Sep 17 00:00:00 2001 From: Srie Teja Date: Fri, 6 Oct 2023 03:43:41 +0530 Subject: [PATCH 8/8] revert: 1bcace89c2689243b0f5b3a169f6116a0ac51a98 --- packages/at_secondary_server/config/config.yaml | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/at_secondary_server/config/config.yaml b/packages/at_secondary_server/config/config.yaml index 6af01548d..4b27f99e4 100644 --- a/packages/at_secondary_server/config/config.yaml +++ b/packages/at_secondary_server/config/config.yaml @@ -9,7 +9,7 @@ root_server: # Default logger settings log: - level: FINER + level: INFO # The atProtocol security configurations. security: @@ -17,9 +17,9 @@ security: # When [useTLS] is set to true, the secondary server starts in secure mode. On setting [useTLS] to true, the [certificateChainLocation] and # [privateKeyLocation] should be populated with the path's to respective certificates. useTLS: true - certificateChainLocation: '/home/srie/Desktop/work/at_server/tools/build_virtual_environment/ve_base/contents/atsign/secondary/base/certs/fullchain.pem' - privateKeyLocation: '/home/srie/Desktop/work/at_server/tools/build_virtual_environment/ve_base/contents/atsign/secondary/base/certs/privkey.pem' - trustedCertificateLocation: '/home/srie/Desktop/work/at_server/tools/build_virtual_environment/ve_base/contents/atsign/secondary/base/certs/cacert.pem' + certificateChainLocation: 'certs/fullchain.pem' + privateKeyLocation: 'certs/privkey.pem' + trustedCertificateLocation: '/etc/cacert/cacert.pem' clientCertificateRequired: true # The atProtocol storage configurations