fix: Commit log special key bug #2115

murali-shris · 2024-10-08T16:48:05Z

Fixes #2114

- What I did

fixed a bug in _specialKey method in commit log. Due to this bug, lastCommitSequenceNumber was incorrectly returned when regex is passed.
- How I did it
modified a condition to check public encryption key from "public:" to "public:publickey"
match only reserved shared key (@bob:shared_key@alice, shared_key.bob@alice) in _specialKey
removed pkam signature and signing private key from _specialKey check
added unit tests for getEntries and lastSequenceNumber
fixed an issue in commit log test by replacing commitLog.getBox().add(doesn't update commitLog cache) with commitLog.add
renamed method names to reflect their purpose
- How to verify it
tests should pass
fix: test persistence at_client_sdk#1414

Dependent at_commons PR:
atsign-foundation/at_libraries#694

gkc

Thanks @murali-shris

Noticed another bug while I was reviewing that we should also fix via this PR
All of this code could really do with some explanation of what's going on and why - especially what is the purpose of _acceptKey?

gkc · 2024-10-09T08:38:20Z

packages/at_persistence_secondary_server/lib/src/log/commitlog/commit_log_keystore.dart

@@ -220,7 +220,7 @@ class CommitLogKeyStore extends BaseCommitLogKeyStore {

  bool _isSpecialKey(String atKey) {
    return atKey.contains(AtConstants.atEncryptionSharedKey) ||


this contains AtConstants.atEncryptionSharedKey check is also a bug. Why are we treating keys that contain "shared_key" as special?

And if we are treating them as special for good reason, we should specifically only match shared_key\..*@this_atservers_at_sign or @some_other_atsign:shared_key@this_servers_at_sign (precise regexes are in at_commons in Regexes._reservedKeysWithAtsignSuffix) ... currently this code would also match keys like

"@bob:key1.shared_keys.buzz@alice"

@murali-shris I don't see a good explanation for why we are not treating the two reserved shared_key variants as 'special' which (as I understand it) would result in those keys not being synced?

Oh - ignore that last comment. I understand it now. This ensures that those keys are synced, regardless of whether they match the enrollment namespace or not.

murali-shris · 2024-10-10T09:53:46Z

Thanks @murali-shris

Noticed another bug while I was reviewing that we should also fix via this PR

All of this code could really do with some explanation of what's going on and why - especially what is the purpose of _acceptKey?

These are my findings on _acceptKey.

Initially the method name was _isRegexMatches which will match the key with regex and if there is match will be included in the result of getChanges(commitId, regex) method. _isRegexMatches method also had special keys which will not match the regex(shared key, pkam signature, signing private key, public encryption key)
minor refactoring was done to move the special keys check to _isSpecialKey. _isRegexMatches was changed to _acceptKey. _acceptKey invoked _isRegexMatches and _isSpecialKey
After enrollment feature, new check was added to match key name space with enrolled namespace.
So _accept key does this functionality - matches regex or checks if a key is special key and does namespace check(if enrolledNamespace is passed).
I will try removing the signing private key and pkam signature from the implementation and test. These keys should remain on the server and need not be synced.

…n-foundation/at_server into commit_log_special_key_bug

gkc

@murali-shris thanks for adding comments explaining the purpose of this code which is to ensure that we are syncing none of the stuff that should not be synced (i.e. anything in namespaces the client does not have access to) and all of the stuff that should be synced (i.e. anything in namespaces the client does have access to, plus other things that all clients should have access to)

As I understand it now, therefore, shouldn't we also, in _isSpecialKey, match everything which (a) is public (b) has no namespace? This would explain why the code had a startsWith 'public:' match, although that was incorrect since it should be 'starts with public:' and 'does not contain a period'

murali-shris · 2024-10-15T15:31:20Z

@murali-shris thanks for adding comments explaining the purpose of this code which is to ensure that we are syncing none of the stuff that should not be synced (i.e. anything in namespaces the client does not have access to) and all of the stuff that should be synced (i.e. anything in namespaces the client does have access to, plus other things that all clients should have access to)

As I understand it now, therefore, shouldn't we also, in _isSpecialKey, match everything which (a) is public (b) has no namespace? This would explain why the code had a startsWith 'public:' match, although that was incorrect since it should be 'starts with public:' and 'does not contain a period'

Scenario 2) and 4) are the ones you meant. correct ?

1. _acceptKey(public:phone@alice, .*) 
_isNamespaceAuthorised -  true (no namespace passed)
_isRegexMatches - true
_isSpecialKey - false
_acceptKey - true


2. _acceptKey(public:phone@alice, ‘.buzz’) 
_isNamespaceAuthorised -  true (no namespace passed)
_isRegexMatches - false
_isSpecialKey - false // must be true
_acceptKey - false // must be true


3. _acceptKey(public:phone@alice, .*, [buzz, rw]) 

_isNamespaceAuthorised -  true (key doesn’t have namespace)
_isRegexMatches - true
_isSpecialKey - false
_acceptKey - true


4. _acceptKey(public:phone@alice, ‘.buzz’, [buzz, rw]) 
_isNamespaceAuthorised - true (key doesn’t have namespace)
_isRegexMatches - false
_isSpecialKey - false // must be true
_acceptKey - false //must be true

There are currently no tests to check whether public: keys without namespace are returned. I will add them

gkc · 2024-10-15T17:31:19Z

@murali-shris yes that's correct. Please can you also come up with better names for these functions - i.e. names which are more reflective of their purpose? Future maintainers will thank you!

murali-shris · 2024-10-16T11:15:51Z

@murali-shris yes that's correct. Please can you also come up with better names for these functions - i.e. names which are more reflective of their purpose? Future maintainers will thank you!

I have renamed the methods. Please review whether the methods names are self explanatory.

gkc · 2024-10-16T11:47:45Z

packages/at_persistence_secondary_server/lib/src/log/commitlog/commit_log_keystore.dart

@@ -217,16 +218,17 @@ class CommitLogKeyStore extends BaseCommitLogKeyStore {
    return false;
  }

-  bool _isRegexMatches(String atKey, String regex) {
+  bool _doesKeyMatchRegex(String atKey, String regex) {


I'd suggest _keyMatchesRegex

gkc · 2024-10-16T11:48:46Z

packages/at_persistence_secondary_server/lib/src/log/commitlog/commit_log_keystore.dart

-  bool _isSpecialKey(String atKey) {
+  /// match only reserved keys and public keys without namespace which have to be synced from server to client, if regex doesn't match
+  /// e.g @bob:shared_key@alice, shared_key.bob@alice, public:publickey@alice, public:phone@alice
+  bool _shouldKeyBeIncludedInSyncIfNoRegexMatch(String atKey) {


I'd suggest _alwaysIncludeInSync

gkc · 2024-10-16T11:49:59Z

packages/at_persistence_secondary_server/test/commit_log_test.dart

+        expect(commitEntriesMap.containsKey('@bob:shared_key@alice'), true);
+        expect(commitEntriesMap.containsKey('shared_key.bob@alice'), true);
+        expect(
+            commitEntriesMap.containsKey('bob:test_shared_key@alice'), false);


gkc · 2024-10-16T11:51:34Z

@murali-shris yes that's correct. Please can you also come up with better names for these functions - i.e. names which are more reflective of their purpose? Future maintainers will thank you!

I have renamed the methods. Please review whether the methods names are self explanatory.

This reads so much better, thank you. I've suggested two changes

…n-foundation/at_server into commit_log_special_key_bug

gkc

LGTM. Thank you @murali-shris

murali-shris added 2 commits October 8, 2024 22:15

fix: special key bug fix

3ea9883

fix: pubspec change to test persistence in server

e42136a

murali-shris changed the title ~~Commit log special key bug~~ fix: Commit log special key bug Oct 8, 2024

murali-shris added 2 commits October 8, 2024 22:36

fix: sync unit test assertion change

9ddbacb

fix: changes to publish persistence

1a09a6d

murali-shris marked this pull request as ready for review October 9, 2024 06:11

Merge branch 'trunk' into commit_log_special_key_bug

ee4edd8

murali-shris requested review from sitaram-kalluri and gkc October 9, 2024 06:55

gkc requested changes Oct 9, 2024

View reviewed changes

gkc mentioned this pull request Oct 9, 2024

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

Merged

This was referenced Oct 14, 2024

commit log keystore _isSpecialKey returns incorrect result for public keys #2114

Closed

ability for sync verb handler to skip delete commit entries to be sent to client #2097

Closed

murali-shris and others added 7 commits October 15, 2024 11:50

fix: add at_commons dependency overrides

719ab2b

Merge branch 'commit_log_special_key_bug' of https://github.com/atsig…

494d497

…n-foundation/at_server into commit_log_special_key_bug

fix: add at_commons dependency overrides

011c9f0

fix: added test and doc for _acceptKey

17190dd

Merge branch 'trunk' into commit_log_special_key_bug

0411139

fix: modified pubspec and change

498c51c

Merge branch 'commit_log_special_key_bug' of https://github.com/atsig…

f1df357

…n-foundation/at_server into commit_log_special_key_bug

murali-shris requested a review from gkc October 15, 2024 08:24

gkc requested changes Oct 15, 2024

View reviewed changes

murali-shris and others added 2 commits October 16, 2024 13:28

fix: added test for public key without namespace

69ec2c2

Merge branch 'trunk' into commit_log_special_key_bug

c22e236

murali-shris requested a review from gkc October 16, 2024 11:15

gkc requested changes Oct 16, 2024

View reviewed changes

murali-shris added 2 commits October 16, 2024 17:46

fix: rename methods

3ea497f

Merge branch 'commit_log_special_key_bug' of https://github.com/atsig…

e670ee8

…n-foundation/at_server into commit_log_special_key_bug

gkc approved these changes Oct 16, 2024

View reviewed changes

murali-shris requested a review from gkc October 16, 2024 12:27

gkc merged commit 8338b6d into trunk Oct 16, 2024
26 checks passed

gkc deleted the commit_log_special_key_bug branch October 16, 2024 13:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Commit log special key bug #2115

fix: Commit log special key bug #2115

murali-shris commented Oct 8, 2024 •

edited

Loading

gkc left a comment

gkc Oct 9, 2024 •

edited by murali-shris

Loading

gkc Oct 15, 2024

gkc Oct 15, 2024

murali-shris commented Oct 10, 2024

gkc left a comment

murali-shris commented Oct 15, 2024

gkc commented Oct 15, 2024

murali-shris commented Oct 16, 2024

gkc Oct 16, 2024

gkc Oct 16, 2024

gkc Oct 16, 2024

gkc commented Oct 16, 2024

gkc left a comment

		@@ -220,7 +220,7 @@ class CommitLogKeyStore extends BaseCommitLogKeyStore {

		bool _isSpecialKey(String atKey) {
		return atKey.contains(AtConstants.atEncryptionSharedKey) \|\|

fix: Commit log special key bug #2115

fix: Commit log special key bug #2115

Conversation

murali-shris commented Oct 8, 2024 • edited Loading

gkc left a comment

Choose a reason for hiding this comment

gkc Oct 9, 2024 • edited by murali-shris Loading

Choose a reason for hiding this comment

gkc Oct 15, 2024

Choose a reason for hiding this comment

gkc Oct 15, 2024

Choose a reason for hiding this comment

murali-shris commented Oct 10, 2024

gkc left a comment

Choose a reason for hiding this comment

murali-shris commented Oct 15, 2024

gkc commented Oct 15, 2024

murali-shris commented Oct 16, 2024

gkc Oct 16, 2024

Choose a reason for hiding this comment

gkc Oct 16, 2024

Choose a reason for hiding this comment

gkc Oct 16, 2024

Choose a reason for hiding this comment

gkc commented Oct 16, 2024

gkc left a comment

Choose a reason for hiding this comment

murali-shris commented Oct 8, 2024 •

edited

Loading

gkc Oct 9, 2024 •

edited by murali-shris

Loading