regression: honor the store-owner encoding written up to 1.20 - #95
Open
r4nc0r wants to merge 2 commits into
Open
regression: honor the store-owner encoding written up to 1.20#95r4nc0r wants to merge 2 commits into
r4nc0r wants to merge 2 commits into
Conversation
Store ownership has been encoded three ways in the IPM_SUBTREE ACL: GROMOXSTOREOWNER (0x2000) from 1.3 up to 1.20, FOLDEROWNER alone in d8557b3, and STOREACCESS (0x700) since bb64403. Each generation read back what it wrote, so the change only becomes visible on upgrade, and no migration exists for the rows the first one left behind. gromox grants store ownership from a literal 0x2000 on its own, because get_mbox_perm ors the stored rights in verbatim in addition to deriving ownership from FOLDEROWNER on IPM_SUBTREE. Those old grants are thus still in force, while STOREACCESS matches none of their bits: - getUserStoreAccess omits them, so the list of a shared mailbox comes up empty although the users still have access and can open it - deleteUserStoreAccess clears 0x700 only, so a revoke reports success and leaves ownership in place - setUserStoreAccessMulti likewise cannot revoke by omission Match on GROMOXSTOREOWNER as well when listing, clear it when revoking, and drop it when granting so a row is normalized on first write. Since setFolderMember(s) deletes a row once no rights remain, revoking a grant that carries nothing else now removes the entry outright.
STOREACCESS is the right mask to write, but the wrong one to match on: gromox derives store ownership from FOLDEROWNER on IPM_SUBTREE, so an entry carrying only FOLDERCONTACT or FOLDERVISIBLE was reported as a store owner while having no such rights. A folder ACL set by a client routinely has that shape, which is why the list could show entries that could not be removed through it. Match FOLDEROWNER (or the legacy bit) instead, so the list says what gromox would answer. This covers the over-reporting half of grommunio#93. The remaining part of that report, a client-granted entry keeping its working rights on IPM_SUBTREE after the store-owner grant is revoked, is deliberately left alone here: those bits were not granted through this endpoint, and dropping them is a separate decision.
Member
|
we can do an automated one-time replacement, cf. grommunio/gromox@8e0dd96 |
Member
|
All the tests (e.g. Setting 0x600 is not strictly needed for the store superuser bit. 0x600 is mimicing gromox's |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The problem
After upgrading to 1.21 the store-owner list of every shared mailbox comes up empty, while the users concerned still have store ownership as far as gromox is concerned and can still open the mailbox. Removing and re-adding through the UI appears to work but leaves the old grant in place.
storeAccesshas encoded "store owner" in the IPM_SUBTREE ACL three different ways, each generation reading back exactly what it wrote, so the change is invisible until an upgrade:GROMOXSTOREOWNER=0x2000FOLDEROWNER=0x100STOREACCESS=FOLDEROWNER|FOLDERCONTACT|FOLDERVISIBLE=0x700No migration exists for the rows the first generation left behind, and gromox grants store ownership from a literal
0x2000on its own:get_mbox_perm(exch/exmdb/store.cpp) ors the stored rights in verbatim, in addition to deriving ownership fromFOLDEROWNERon IPM_SUBTREE. Those grants are therefore still in force, andSTOREACCESSmatches none of their bits. Concretely, for an ACE of0x3800:getUserStoreAccessomits it, so an administrator auditing a shared mailbox is told nobody has access when somebody does;deleteUserStoreAccessclears0x700only, so a revoke returns success and changes nothing;setUserStoreAccessMulticannot revoke by omission either, for the same reason.Reproduced on gromox
3.10.25.m1692b65, one grantee onIPM_SUBTREEof a shared mailbox, read back withgrommunio-admin exmdb <mailbox> folder permissions 0x9:0x3800 (freebusysimple,freebusydetailed,storeowner)0x3f000x3800The delta is exactly
0x700; the0x3800residue is never touched and the row is never removed.The change
Two commits, separable.
1. Honor the store-owner encoding written up to 1.20. Match
GROMOXSTOREOWNERas well when listing, clear it when revoking, and drop it when granting so a row is normalized on first write. BecausesetFolderMember/setFolderMembersalready delete a row once no rights remain, revoking a grant that carries nothing else now removes the entry outright rather than leaving a husk.2. List store owners by
folderowner, not by anySTOREACCESSbit.STOREACCESSis the right mask to write but the wrong one to match on: an entry carrying onlyFOLDERCONTACTorFOLDERVISIBLEwas reported as a store owner despite having no such rights, which is the shape a client-set folder ACL routinely has. This is the over-reporting half of #93. The rest of that report, a client-granted entry keeping its working rights on IPM_SUBTREE after the store-owner grant is revoked, is deliberately left alone: those bits were not granted through this endpoint, and dropping them is a separate decision.Masks after the change: writes use
STOREACCESS, listing usesSTOREOWNER_EFFECTIVE(FOLDEROWNER|GROMOXSTOREOWNER, i.e. what gromox itself would answer), revoking usesSTOREACCESS_ANY(STOREACCESS|GROMOXSTOREOWNER).Testing
pyexmdbis not available in my environment, so the endpoints were not exercised against a live store. What was checked instead: the ACL primitives were reimplemented fromlibexmdbppsrc/queries.cpp(setFolderMember,setFolderMembers) and the masks imported from the patchedtools/constants.py, then the invariant "listed bygetUserStoreAccessif and only if gromox considers the user a store owner" was asserted over the call sequences this patch actually emits:0x3800, listed?0x38000x1f000x18000x2000+ revoke0x700+ revoke0x18000x1f000x7fb0x7fb0x6000x6000x4010x401The two rows that disagreed before the change are the legacy ones, and
0x600is the row that commit 2 fixes. A live confirmation on a real store would still be worth having before release.Note on affected deployments
Because the first encoding shipped in 1.3, any installation upgraded from <=1.20 to >=1.21 is affected for every store owner granted before the upgrade. This patch makes those grants visible and revocable again, and normalizes each row the first time it is written, but it does not rewrite rows on its own. Affected rows can be listed with:
Anything printing
storeownerwithoutfolderownerwas written by <=1.20. Note thatgrommunio-admin user <mailbox> storeowner listused the same filter as the API and so could not be used to find these either; it is fixed by the same commits.A related gap this patch does not address:
UserSecondaryStoresis maintained only by the current endpoints, so mailboxes whose owners were granted under the old encoding are likely missing rows there as well.