Skip to content
New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

fix: Update the error message and add skip commit for enrollment key deletion #2117

Merged
merged 7 commits into from
Oct 10, 2024
Merged

fix: Update the error message and add skip commit for enrollment key deletion #2117

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
c63e297
fix: Update the error message and add skip commit for enrollment key …
sitaram-kalluri Oct 9, 2024
3e4229a
fix: Update the error message in functional test
sitaram-kalluri Oct 9, 2024
2bb14ec
Merge branch 'trunk' into update_enrollment_error_messages
sitaram-kalluri Oct 9, 2024
918850b
fix: Add unit tests to verify commit log state for various enrollment…
sitaram-kalluri Oct 9, 2024
253f45b
fix: Remove commit log check for public key on deny enrollment operation
sitaram-kalluri Oct 9, 2024
2278f21
fix: Set skipCommit to true for apkamPublicKey
sitaram-kalluri Oct 10, 2024
6029647
Merge branch 'trunk' into update_enrollment_error_messages
sitaram-kalluri Oct 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions packages/at_secondary_server/lib/src/enroll/enrollment_manager.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,4 +79,18 @@ class EnrollmentManager {
String enrollmentKey = buildEnrollmentKey(enrollmentId);
await _keyStore.put(enrollmentKey, atData, skipCommit: true);
}

/// Deletes the enrollment key from the keystore.
///
/// This method generates an enrollment key using the provided enrollmentId and
/// removes the enrollment key from the keystore. The skipCommit parameter is
/// set to true to prevent this deletion from being logged in the commit log,
/// ensuring it is not synced to the clients.
///
/// Parameters:
/// - [enrollmentId]: The ID associated with the enrollment.
Future<void> remove(String enrollmentId) async {
String enrollmentKey = buildEnrollmentKey(enrollmentId);
await _keyStore.remove(enrollmentKey, skipCommit: true);
}
}
19 changes: 9 additions & 10 deletions packages/at_secondary_server/lib/src/verb/handler/enroll_verb_handler.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,7 @@ class EnrollVerbHandler extends AbstractVerbHandler {
enrollVerbParams, currentAtSign, response);
return;
case 'delete':
await _deleteDeniedEnrollment(
await _deleteEnrollment(
enrollVerbParams, currentAtSign, responseJson, response);
break;
}
Expand Down Expand Up @@ -255,7 +255,8 @@ class EnrollVerbHandler extends AbstractVerbHandler {
// 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(AtConstants.atPkamPublicKey,
AtData()..data = enrollParams.apkamPublicKey!);
AtData()..data = enrollParams.apkamPublicKey!,
skipCommit: true);
enrollData = AtData()..data = jsonEncode(enrollmentValue.toJson());
} else {
enrollmentValue.encryptedAPKAMSymmetricKey =
Expand Down Expand Up @@ -359,7 +360,7 @@ class EnrollVerbHandler extends AbstractVerbHandler {
'public:${enrollDataStoreValue.appName}.${enrollDataStoreValue.deviceName}.pkam.$pkamNamespace.__public_keys$currentAtSign';
var valueJson = {'apkamPublicKey': enrollDataStoreValue.apkamPublicKey};
var atData = AtData()..data = jsonEncode(valueJson);
await keyStore.put(apkamPublicKeyInKeyStore, atData);
await keyStore.put(apkamPublicKeyInKeyStore, atData, skipCommit: true);
await _storeEncryptionKeys(
enrollmentIdFromParams!, enrollParams, currentAtSign);
}
Expand Down Expand Up @@ -561,7 +562,7 @@ class EnrollVerbHandler extends AbstractVerbHandler {
!(EnrollmentStatus.denied == enrollStatus ||
EnrollmentStatus.revoked == enrollStatus)) {
throw AtEnrollmentException(
'Cannot delete ${enrollStatus.name} enrollments. Only denied enrollments can be deleted');
'Cannot delete ${enrollStatus.name} enrollments. Only denied and revoked enrollments can be deleted');
}
if (operation == 'unrevoke' && EnrollmentStatus.revoked != enrollStatus) {
throw AtEnrollmentException(
Expand Down Expand Up @@ -733,8 +734,8 @@ class EnrollVerbHandler extends AbstractVerbHandler {
return delayForInvalidOTPSeries.last;
}

Future<void> _deleteDeniedEnrollment(EnrollParams? enrollParams,
String atSign, Map responseJson, response) async {
Future<void> _deleteEnrollment(EnrollParams? enrollParams, String atSign,
Map responseJson, response) async {
// Note: The enrollmentId is verified for the null check in the _validateParams methods.
// Therefore, when control comes here, enrollmentId will not be null.
EnrollDataStoreValue enrollValue = await AtSecondaryServerImpl.getInstance()
Expand All @@ -750,7 +751,6 @@ class EnrollVerbHandler extends AbstractVerbHandler {
return;
}

// ensures only denied entries can be deleted
try {
_verifyEnrollmentStateBeforeAction(
EnrollOperationEnum.delete.name, enrollmentStatus);
Expand All @@ -759,10 +759,9 @@ class EnrollVerbHandler extends AbstractVerbHandler {
'Failed to delete enrollment id: ${enrollParams.enrollmentId} | Cause: ${e.message}');
}

String enrollmentKeyToDelete = AtSecondaryServerImpl.getInstance()
await AtSecondaryServerImpl.getInstance()
.enrollmentManager
.buildEnrollmentKey(enrollParams.enrollmentId!);
await keyStore.remove(enrollmentKeyToDelete);
.remove(enrollParams.enrollmentId!);

responseJson['enrollmentId'] = enrollParams.enrollmentId;
responseJson['status'] = 'deleted';
Expand Down
209 changes: 206 additions & 3 deletions packages/at_secondary_server/test/enroll_verb_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import 'package:at_persistence_secondary_server/at_persistence_secondary_server.
import 'package:at_secondary/src/connection/inbound/inbound_connection_metadata.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/enroll/enrollment_manager.dart';
import 'package:at_secondary/src/server/at_secondary_config.dart';
import 'package:at_secondary/src/utils/handler_util.dart';
import 'package:at_secondary/src/verb/handler/delete_verb_handler.dart';
Expand Down Expand Up @@ -634,8 +635,6 @@ void main() {
Iterator iterator =
(secondaryKeyStore.commitLog as AtCommitLog).getEntries(-1);
iterator.moveNext();
expect(iterator.current.key,
'public:wavi.mydevice.pkam.__pkams.__public_keys@alice');
expect(iterator.moveNext(), false);
});
tearDown(() async => await verbTestsTearDown());
Expand Down Expand Up @@ -2036,8 +2035,212 @@ void main() {
response, enrollVerbParams, inboundConnection),
throwsA(predicate((e) =>
e.toString() ==
'Exception: Failed to delete enrollment id: 345345345141 | Cause: Cannot delete approved enrollments. Only denied enrollments can be deleted')));
'Exception: Failed to delete enrollment id: 345345345141 | Cause: Cannot delete approved enrollments. Only denied and revoked enrollments can be deleted')));
});
tearDown(() async => await verbTestsTearDown());
});

group(
'A group of tests to validate the commit log state when performing enrollment operations',
() {
setUp(() async {
await verbTestsSetUp();
});

test(
'A test to verify commit log state during create approve revoke and delete an enrollment request',
() async {
Response response = Response();
// OTP Verb
inboundConnection.metaData.isAuthenticated = true;
inboundConnection.metaData.sessionID = 'dummy_session';
HashMap<String, String?> otpVerbParams =
getVerbParam(VerbSyntax.otp, 'otp:get');
OtpVerbHandler otpVerbHandler = OtpVerbHandler(secondaryKeyStore);
await otpVerbHandler.processVerb(
response, otpVerbParams, inboundConnection);
String otp = response.data!;

// 1. Create an enrollment request
String enrollmentRequest =
'enroll:request:{"appName":"wavi","deviceName":"mydevice"'
',"namespaces":{"buzz":"r"},"otp":"$otp"'
',"apkamPublicKey":"lorem_apkam"'
',"encryptedAPKAMSymmetricKey": "ipsum_apkam"}';
HashMap<String, String?> enrollmentRequestVerbParams =
getVerbParam(VerbSyntax.enroll, enrollmentRequest);
inboundConnection.metaData.isAuthenticated = false;
EnrollVerbHandler enrollVerbHandler =
EnrollVerbHandler(secondaryKeyStore);
await enrollVerbHandler.processVerb(
response, enrollmentRequestVerbParams, inboundConnection);
String enrollmentId = jsonDecode(response.data!)['enrollmentId'];

String enrollmentKey =
EnrollmentManager(secondaryKeyStore).buildEnrollmentKey(enrollmentId);

// Verify key is created in the secondary keystore.
AtData? atData = await secondaryKeyStore.get(enrollmentKey);
expect(atData!.data!.isNotEmpty, true);
var enrollmentDataMap = jsonDecode(atData.data!);
expect(enrollmentDataMap['appName'], 'wavi');
expect(enrollmentDataMap['deviceName'], 'mydevice');
expect(enrollmentDataMap['namespaces'], {'buzz': 'r'});
expect(enrollmentDataMap['apkamPublicKey'], 'lorem_apkam');

AtCommitLog? atCommitLog =
await AtCommitLogManagerImpl.getInstance().getCommitLog(alice);
var itr = atCommitLog?.getEntries(-1);
// Since there are no entries in commit log, iterator.moveNext() returns false.
expect(itr!.moveNext(), false);

// 2. Approve an enrollment and verify enrollmentKey is not stored in the commit log.
String approveEnrollment =
'enroll:approve:{"enrollmentId":"$enrollmentId","encryptedDefaultEncryptionPrivateKey": "dummy_encrypted_default_encryption_private_key","encryptedDefaultSelfEncryptionKey":"dummy_encrypted_default_self_encryption_key"}';
HashMap<String, String?> approveEnrollmentVerbParams =
getVerbParam(VerbSyntax.enroll, approveEnrollment);
inboundConnection.metaData.isAuthenticated = true;
enrollVerbHandler = EnrollVerbHandler(secondaryKeyStore);
await enrollVerbHandler.processVerb(
response, approveEnrollmentVerbParams, inboundConnection);
expect(jsonDecode(response.data!)['status'], 'approved');

atCommitLog =
await AtCommitLogManagerImpl.getInstance().getCommitLog(alice);
itr = atCommitLog?.getEntries(-1);
// Ensure there are no other keys in the commit log.
expect(itr!.moveNext(), false);

// 3. Revoke an enrollment and verify the commit log state.
enrollmentRequest = 'enroll:revoke:{"enrollmentId":"$enrollmentId"}';
HashMap<String, String?> revokeEnrollmentVerbParams =
getVerbParam(VerbSyntax.enroll, enrollmentRequest);
inboundConnection.metaData.isAuthenticated = true;
inboundConnection.metaData.sessionID = 'dummy_session';
response = Response();
enrollVerbHandler = EnrollVerbHandler(secondaryKeyStore);
await enrollVerbHandler.processVerb(
response, revokeEnrollmentVerbParams, inboundConnection);
expect(jsonDecode(response.data!)['status'], 'revoked');

atCommitLog =
await AtCommitLogManagerImpl.getInstance().getCommitLog(alice);
itr = atCommitLog?.getEntries(-1);
// Ensure there are no other keys in the commit log.
expect(itr!.moveNext(), false);

// 4. Delete an enrollment request.
enrollmentRequest = 'enroll:delete:{"enrollmentId":"$enrollmentId"}';
HashMap<String, String?> verbParams =
getVerbParam(VerbSyntax.enroll, enrollmentRequest);
inboundConnection.metaData.isAuthenticated = true;
inboundConnection.metaData.sessionID = 'dummy_session';
response = Response();
enrollVerbHandler = EnrollVerbHandler(secondaryKeyStore);
await enrollVerbHandler.processVerb(
response, verbParams, inboundConnection);
expect(jsonDecode(response.data!)['status'], 'deleted');

atCommitLog =
await AtCommitLogManagerImpl.getInstance().getCommitLog(alice);
itr = atCommitLog?.getEntries(-1);
// Since there are no entries in commit log, iterator.moveNext() returns false.
// Ensure there are no other keys in the commit log.
expect(itr!.moveNext(), false);

// Verify key is deleted in the secondary keystore.
expect(() async => await secondaryKeyStore.get(enrollmentKey),
throwsA(predicate((dynamic e) => e is KeyNotFoundException)));
});

test(
'A test to verify commit log state during create deny and delete an enrollment request',
() async {
Response response = Response();
// OTP Verb
inboundConnection.metaData.isAuthenticated = true;
inboundConnection.metaData.sessionID = 'dummy_session';
HashMap<String, String?> otpVerbParams =
getVerbParam(VerbSyntax.otp, 'otp:get');
OtpVerbHandler otpVerbHandler = OtpVerbHandler(secondaryKeyStore);
await otpVerbHandler.processVerb(
response, otpVerbParams, inboundConnection);
String otp = response.data!;

// 1. Create an enrollment request
String enrollmentRequest =
'enroll:request:{"appName":"wavi","deviceName":"mydevice"'
',"namespaces":{"buzz":"r"},"otp":"$otp"'
',"apkamPublicKey":"lorem_apkam"'
',"encryptedAPKAMSymmetricKey": "ipsum_apkam"}';
HashMap<String, String?> enrollmentRequestVerbParams =
getVerbParam(VerbSyntax.enroll, enrollmentRequest);
inboundConnection.metaData.isAuthenticated = false;
EnrollVerbHandler enrollVerbHandler =
EnrollVerbHandler(secondaryKeyStore);
await enrollVerbHandler.processVerb(
response, enrollmentRequestVerbParams, inboundConnection);
String enrollmentId = jsonDecode(response.data!)['enrollmentId'];

String enrollmentKey =
EnrollmentManager(secondaryKeyStore).buildEnrollmentKey(enrollmentId);

// Verify key is created in the secondary keystore.
AtData? atData = await secondaryKeyStore.get(enrollmentKey);
expect(atData!.data!.isNotEmpty, true);
var enrollmentDataMap = jsonDecode(atData.data!);
expect(enrollmentDataMap['appName'], 'wavi');
expect(enrollmentDataMap['deviceName'], 'mydevice');
expect(enrollmentDataMap['namespaces'], {'buzz': 'r'});
expect(enrollmentDataMap['apkamPublicKey'], 'lorem_apkam');

AtCommitLog? atCommitLog =
await AtCommitLogManagerImpl.getInstance().getCommitLog(alice);
var itr = atCommitLog?.getEntries(-1);
// Since there are no entries in commit log, iterator.moveNext() returns false.
expect(itr!.moveNext(), false);

// 2. Deny an enrollment and verify the commit log state.
enrollmentRequest = 'enroll:deny:{"enrollmentId":"$enrollmentId"}';
HashMap<String, String?> denyEnrollmentVerbParams =
getVerbParam(VerbSyntax.enroll, enrollmentRequest);
inboundConnection.metaData.isAuthenticated = true;
inboundConnection.metaData.sessionID = 'dummy_session';
response = Response();
enrollVerbHandler = EnrollVerbHandler(secondaryKeyStore);
await enrollVerbHandler.processVerb(
response, denyEnrollmentVerbParams, inboundConnection);
expect(jsonDecode(response.data!)['status'], 'denied');

atCommitLog =
await AtCommitLogManagerImpl.getInstance().getCommitLog(alice);
itr = atCommitLog?.getEntries(-1);
// Since there are no entries in commit log, iterator.moveNext() returns false.
expect(itr!.moveNext(), false);
Copy link
Contributor

Choose a reason for hiding this comment

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

The EnrollVerbHandler is not requesting to skip the commit when storing this key. So in theory this test is required - but I believe the commit log is not currently being updated because of this #2115

So - we should probably explicitly change the EnrollVerbHandler so that it skips the commit log when storing this public key also, since these are not things that need to be synced to clients and, indeed, currently are not synced - but would start syncing if #2115 were merged

Copy link
Contributor

Choose a reason for hiding this comment

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

Specifically add skipCommit: true to the calls to keyStore.put which are being made on lines 257 and 362 in enroll_verb_handler.dart

Copy link
Member Author

Choose a reason for hiding this comment

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

  • Set skipCommit to true and modified unit and functional tests accordingly.
  • Also did a round of manual testing with at_onboarding_cli to approve the enrollment request and use the new apkam keys authenticate the connection.


// 3. Delete an enrollment request.
enrollmentRequest = 'enroll:delete:{"enrollmentId":"$enrollmentId"}';
HashMap<String, String?> verbParams =
getVerbParam(VerbSyntax.enroll, enrollmentRequest);
inboundConnection.metaData.isAuthenticated = true;
inboundConnection.metaData.sessionID = 'dummy_session';
response = Response();
enrollVerbHandler = EnrollVerbHandler(secondaryKeyStore);
await enrollVerbHandler.processVerb(
response, verbParams, inboundConnection);
expect(jsonDecode(response.data!)['status'], 'deleted');

atCommitLog =
await AtCommitLogManagerImpl.getInstance().getCommitLog(alice);
itr = atCommitLog?.getEntries(-1);
// Since there are no entries in commit log, iterator.moveNext() returns false.
expect(itr!.moveNext(), false);

// Verify key is deleted in the secondary keystore.
expect(() async => await secondaryKeyStore.get(enrollmentKey),
throwsA(predicate((dynamic e) => e is KeyNotFoundException)));
});

tearDown(() async => await verbTestsTearDown());
});
}
4 changes: 2 additions & 2 deletions tests/at_functional_test/test/enroll_verb_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1629,7 +1629,7 @@ void main() {
jsonDecodedResponse['errorDescription'],
'Internal server exception : Failed to delete enrollment id: '
'$enrollmentId | Cause: Cannot delete approved enrollments. '
'Only denied enrollments can be deleted');
'Only denied and revoked enrollments can be deleted');
});

test('negative test - delete an pending enrollment', () async {
Expand Down Expand Up @@ -1675,7 +1675,7 @@ void main() {
jsonDecodedResponse['errorDescription'],
'Internal server exception : Failed to delete enrollment id: '
'$enrollmentId | Cause: Cannot delete pending enrollments. '
'Only denied enrollments can be deleted');
'Only denied and revoked enrollments can be deleted');
});

test(
Expand Down
6 changes: 3 additions & 3 deletions tests/at_functional_test/test/sync_verb_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,7 @@ void main() {
expect(
int.parse(
jsonDecode(statsResponse.replaceAll('data:', ''))[0]['value']),
lastCommitIdBeforeUpdate + 4);
lastCommitIdBeforeUpdate + 3);
Copy link
Member Author

Choose a reason for hiding this comment

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

Since akamPublicKeys are not logged into the commit log, reduced value to 3.

await authenticatedSocket.close();

await authenticatedSocket.initiateConnectionWithListener(
Expand All @@ -135,11 +135,11 @@ void main() {
expect(syncResponseList.length, 2);
expect(syncResponseList[0]['atKey'],
'$secondAtSign:phone-$randomString.wavi$firstAtSign');
expect(syncResponseList[0]['commitId'], lastCommitIdBeforeUpdate + 2);
expect(syncResponseList[0]['commitId'], lastCommitIdBeforeUpdate + 1);

expect(syncResponseList[1]['atKey'],
'$secondAtSign:phone-$randomString.buzz$firstAtSign');
expect(syncResponseList[1]['commitId'], lastCommitIdBeforeUpdate + 3);
expect(syncResponseList[1]['commitId'], lastCommitIdBeforeUpdate + 2);
});
});

Expand Down