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.

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

Add initial support for storing user room keys #3098

Merged
merged 19 commits into from
Jun 12, 2023
Merged

Conversation

S7evinK
Copy link
Contributor

@S7evinK S7evinK commented Jun 2, 2023

No description provided.

@S7evinK S7evinK added the T-Enhancement New features, changes in functionality, improvements in performance, or user-facing enhancements. label Jun 2, 2023
@S7evinK S7evinK requested a review from a team as a code owner June 2, 2023 12:35
Copy link
Member

@kegsay kegsay left a comment

Choose a reason for hiding this comment

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

General shape is good but API could be made a bit kinder to the caller.

);
`

const insertUserRoomKeySQL = `INSERT INTO roomserver_user_room_keys (user_nid, room_nid, pseudo_id_key) VALUES ($1, $2, $3)`
Copy link
Member

Choose a reason for hiding this comment

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

Without an ON CONFLICT we'll get errors if we try to insert the same key twice surely?

@@ -1589,6 +1591,29 @@ func (d *Database) UpgradeRoom(ctx context.Context, oldRoomID, newRoomID, eventS
})
}

// InsertUserRoomKey inserts a new user room key for the given user and room.
// Returns an error if a database error occurred, also if the primary constraint was violated.
func (d *Database) InsertUserRoomKey(ctx context.Context, userNID types.EventStateKeyNID, roomNID types.RoomNID, key ed25519.PrivateKey) error {
Copy link
Member

Choose a reason for hiding this comment

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

This API shape isn't the most helpful and is racey.

The main scenario we are designing for is "I want to join a pseudo ID room, please ensure there is a key for me". If you concurrently call InsertUserRoomKey, the slower call will fail with an error, when in reality you just want to return the key that was previously inserted. I would probably change this to return the private key to use and make use of ON CONFLICT ... RETURNING like we do elsewhere in Dendrite.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In theory it shouldn't race when called through /join, since there can only be one join in progress.

// SelectUserRoomKey queries the user room key for a given user.
// Returns the key and an error.
// TODO: should we handle absent keys (sql.ErrNoRows) as non-fatal?
func (d *Database) SelectUserRoomKey(ctx context.Context, userNID types.EventStateKeyNID, roomNID types.RoomNID) (key ed25519.PrivateKey, err error) {
Copy link
Member

Choose a reason for hiding this comment

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

It's not entirely clear that this is for selecting the private key i.e because Alice wants to send a message and now you need to sign an event, as opposed to "here's a set of events you're about to return to the client, please pull out the user IDs for these sender keys". It's not clear because of comments like:

queries the user room key for a given user

It's not clear if this is the public part or private part.

TODO: should we handle absent keys (sql.ErrNoRows) as non-fatal?

Yes, and let the caller decide what to do if the user has no key (maybe it can make one).

@codecov
Copy link

codecov bot commented Jun 2, 2023

Codecov Report

Patch coverage: 72.99% and project coverage change: -0.24 ⚠️

Comparison is base (8ea1a11) 66.12% compared to head (f912b9c) 65.89%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3098      +/-   ##
==========================================
- Coverage   66.12%   65.89%   -0.24%     
==========================================
  Files         500      502       +2     
  Lines       54342    54579     +237     
==========================================
+ Hits        35936    35967      +31     
- Misses      14777    14972     +195     
- Partials     3629     3640      +11     
Flag Coverage Δ
unittests 50.02% <72.99%> (+0.08%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
roomserver/storage/tables/interface.go 81.48% <ø> (ø)
roomserver/types/types.go 70.27% <ø> (ø)
roomserver/storage/postgres/storage.go 35.29% <25.00%> (-0.51%) ⬇️
roomserver/storage/sqlite3/storage.go 40.10% <25.00%> (-0.66%) ⬇️
roomserver/storage/shared/storage.go 67.27% <64.76%> (-1.09%) ⬇️
roomserver/storage/sqlite3/user_room_keys_table.go 85.93% <85.93%> (ø)
...oomserver/storage/postgres/user_room_keys_table.go 88.46% <88.46%> (ø)

... and 41 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@S7evinK S7evinK requested a review from kegsay June 7, 2023 06:19
Copy link
Member

@kegsay kegsay left a comment

Choose a reason for hiding this comment

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

A few tweaks required, but this is looking good.

roomserver/storage/interface.go Show resolved Hide resolved
const insertUserRoomKeySQL = `INSERT INTO roomserver_user_room_keys (user_nid, room_nid, pseudo_id_key) VALUES ($1, $2, $3)`
const insertUserRoomPrivateKeySQL = `
INSERT INTO roomserver_user_room_keys (user_nid, room_nid, pseudo_id_key, pseudo_id_pub_key) VALUES ($1, $2, $3, $4)
ON CONFLICT ON CONSTRAINT roomserver_user_room_keys_pk DO UPDATE SET pseudo_id_key = roomserver_user_room_keys.pseudo_id_key
Copy link
Member

Choose a reason for hiding this comment

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

We probably want to DO NOTHING on conflict and return the old key. This is because if we already have a priv key, it's likely we've signed events using it, which means new events would be signed with a different key which would be bad and break the protocol.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

DO NOTHING doesn't work, as this returns no result. We need to update, even if this just sets the key to the existing one.

roomserver/storage/postgres/user_room_keys_table.go Outdated Show resolved Hide resolved
roomserver/storage/postgres/user_room_keys_table.go Outdated Show resolved Hide resolved
SelectUserRoomPrivateKey(ctx context.Context, txn *sql.Tx, userNID types.EventStateKeyNID, roomNID types.RoomNID) (ed25519.PrivateKey, error)
BulkSelectUserNIDs(ctx context.Context, txn *sql.Tx, senderKeys [][]byte) (map[string]types.EventStateKeyNID, error)
// BulkSelectUserNIDs selects all userIDs for the requested senderKeys. Returns a map from publicKey -> types.UserRoomKeyPair.
BulkSelectUserNIDs(ctx context.Context, txn *sql.Tx, senderKeys map[types.RoomNID][]ed25519.PublicKey) (map[string]types.UserRoomKeyPair, error)
Copy link
Member

Choose a reason for hiding this comment

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

Need to doc what happens if some keys don't exist in the table.

senders = append(senders, key)
}
}
rows, err := stmt.QueryContext(ctx, pq.Array(senders))
Copy link
Member

Choose a reason for hiding this comment

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

This isn't what I was expecting, as this code will give incorrect results in some cases. This code just queries on the set of sender keys, ignoring the room NID for each key. This means it's possible to query this function asking for the user NID in room A with sender key X, and get back the user NID for room B with sender key X.

We either need to WHERE room_nid to filter DB-side or we need to only add UserRoomKeyPair to result if the room NID was asked for in senderKeys.

}
// get the userIDs
nidMAP, seErr := d.EventStateKeys(ctx, nids)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
nidMAP, seErr := d.EventStateKeys(ctx, nids)
nidMap, seErr := d.EventStateKeys(ctx, nids)

@S7evinK S7evinK merged commit 832ccc3 into main Jun 12, 2023
@S7evinK S7evinK deleted the s7evink/userroomkeystable branch June 12, 2023 10:45
S7evinK pushed a commit that referenced this pull request Feb 28, 2024
Bumps [nokogiri](https://github.com/sparklemotion/nokogiri) from 1.14.3
to 1.16.2.
<details>
<summary>Release notes</summary>
<p><em>Sourced from <a
href="https://github.com/sparklemotion/nokogiri/releases">nokogiri's
releases</a>.</em></p>
<blockquote>
<h2>v1.16.2 / 2024-02-04</h2>
<h3>Security</h3>
<ul>
<li>[CRuby] Vendored libxml2 is updated to address CVE-2024-25062. See
<a
href="https://github.com/sparklemotion/nokogiri/security/advisories/GHSA-xc9x-jj77-9p9j">GHSA-xc9x-jj77-9p9j</a>
for more information.</li>
</ul>
<h3>Dependencies</h3>
<ul>
<li>[CRuby] Vendored libxml2 is updated to <a
href="https://gitlab.gnome.org/GNOME/libxml2/-/releases/v2.12.5">v2.12.5</a>
from v2.12.4. (<a
href="https://github.com/flavorjones"><code>@​flavorjones</code></a>)</li>
</ul>
<hr />
<p>sha256 checksums:</p>

<pre><code>69ba15d2a2498324489ed63850997f0b8f684260114ea81116d3082f16551d2d
nokogiri-1.16.2-aarch64-linux.gem
6a05ce42e3587a40cf8936ece0beaa5d32922254215d2e8cf9ad40588bb42e57
nokogiri-1.16.2-arm-linux.gem
c957226c8e36b31be6a3afb8602e2128282bf8b40ea51016c4cd21aa2608d3f8
nokogiri-1.16.2-arm64-darwin.gem
122652bfc338cd8a54a692ac035e245e41fd3b8283299202ca26e7a7d50db310
nokogiri-1.16.2-java.gem
7344b5072ca69fc5bedb61cb01a3b765b93a27aae5a2a845c2ba7200e4345074
nokogiri-1.16.2-x64-mingw-ucrt.gem
a2a5e184a424111a0d5b77947986484920ad708009c667f061e8d02035c562dd
nokogiri-1.16.2-x64-mingw32.gem
833efddeb51a6c2c9f6356295623c2b2e0d50050d468695c59bd929162953323
nokogiri-1.16.2-x86-linux.gem
e67fc0418dffaff9dc8b1dc65f0605282c3fee9488832d0223b620b4319e0b53
nokogiri-1.16.2-x86-mingw32.gem
5def799e5f139f21a79d7cf71172313a7b6fb0e4b2a31ab9bd5d4ad305994539
nokogiri-1.16.2-x86_64-darwin.gem
5b146240ac6ec6c40fd4367623e74442bca45a542bd3282b1d4d18b07b8e5dfe
nokogiri-1.16.2-x86_64-linux.gem
68922ee5cde27497d995c46f2821957bae961947644eed2822d173daf7567f9c
nokogiri-1.16.2.gem
</code></pre>
<h2>v1.16.1 / 2024-02-03</h2>
<h3>Dependencies</h3>
<ul>
<li>[CRuby] Vendored libxml2 is updated to <a
href="https://gitlab.gnome.org/GNOME/libxml2/-/releases/v2.12.4">v2.12.4</a>
from v2.12.3. (<a
href="https://github.com/flavorjones"><code>@​flavorjones</code></a>)</li>
</ul>
<h3>Fixed</h3>
<ul>
<li>[CRuby] <code>XML::Reader</code> defaults the encoding to UTF-8 if
it's not specified in either the document or as a method parameter.
Previously non-ASCII characters were serialized as NCRs in this case. <a
href="https://redirect.github.com/sparklemotion/nokogiri/issues/2891">#2891</a>
(<a
href="https://github.com/flavorjones"><code>@​flavorjones</code></a>)</li>
<li>[CRuby] Restored support for compilation by GCC versions earlier
than 4.6, which was broken in v1.15.0 (540e9aee). <a
href="https://redirect.github.com/sparklemotion/nokogiri/issues/3090">#3090</a>
(<a
href="https://github.com/adfoster-r7"><code>@​adfoster-r7</code></a>)</li>
<li>[CRuby] Patched upstream libxml2 to allow parsing HTML5 in the
context of a namespaced node (e.g., foreign content like MathML).
[#3112, <a
href="https://redirect.github.com/sparklemotion/nokogiri/issues/3116">#3116</a>]
(<a
href="https://github.com/flavorjones"><code>@​flavorjones</code></a>)</li>
<li>[CRuby] Fixed a small memory leak in libgumbo (HTML5 parser) when
the maximum tree depth limit is hit. [#3098, <a
href="https://redirect.github.com/sparklemotion/nokogiri/issues/3100">#3100</a>]
(<a
href="https://github.com/stevecheckoway"><code>@​stevecheckoway</code></a>)</li>
</ul>
<hr />
<p>sha256 checksums:</p>

<pre><code>a541f35e5b9798a0c97300f9ee18f4217da2a2945a6d5499e4123b9018f9cafc
nokogiri-1.16.1-aarch64-linux.gem
6b82affd195000ab2f9c36cc08744ec2d2fcf6d8da88d59a2db67e83211f7c69
nokogiri-1.16.1-arm-linux.gem
&lt;/tr&gt;&lt;/table&gt; 
</code></pre>
</blockquote>
<p>... (truncated)</p>
</details>
<details>
<summary>Changelog</summary>
<p><em>Sourced from <a
href="https://github.com/sparklemotion/nokogiri/blob/main/CHANGELOG.md">nokogiri's
changelog</a>.</em></p>
<blockquote>
<h2>v1.16.2 / 2024-02-04</h2>
<h3>Security</h3>
<ul>
<li>[CRuby] Vendored libxml2 is updated to address CVE-2024-25062. See
<a
href="https://github.com/sparklemotion/nokogiri/security/advisories/GHSA-xc9x-jj77-9p9j">GHSA-xc9x-jj77-9p9j</a>
for more information.</li>
</ul>
<h3>Dependencies</h3>
<ul>
<li>[CRuby] Vendored libxml2 is updated to <a
href="https://gitlab.gnome.org/GNOME/libxml2/-/releases/v2.12.5">v2.12.5</a>
from v2.12.4. (<a
href="https://github.com/flavorjones"><code>@​flavorjones</code></a>)</li>
</ul>
<h2>v1.16.1 / 2024-02-03</h2>
<h3>Dependencies</h3>
<ul>
<li>[CRuby] Vendored libxml2 is updated to <a
href="https://gitlab.gnome.org/GNOME/libxml2/-/releases/v2.12.4">v2.12.4</a>
from v2.12.3. (<a
href="https://github.com/flavorjones"><code>@​flavorjones</code></a>)</li>
</ul>
<h3>Fixed</h3>
<ul>
<li>[CRuby] <code>XML::Reader</code> defaults the encoding to UTF-8 if
it's not specified in either the document or as a method parameter.
Previously non-ASCII characters were serialized as NCRs in this case. <a
href="https://redirect.github.com/sparklemotion/nokogiri/issues/2891">#2891</a>
(<a
href="https://github.com/flavorjones"><code>@​flavorjones</code></a>)</li>
<li>[CRuby] Restored support for compilation by GCC versions earlier
than 4.6, which was broken in v1.15.0 (540e9aee). <a
href="https://redirect.github.com/sparklemotion/nokogiri/issues/3090">#3090</a>
(<a
href="https://github.com/adfoster-r7"><code>@​adfoster-r7</code></a>)</li>
<li>[CRuby] Patched upstream libxml2 to allow parsing HTML5 in the
context of a namespaced node (e.g., foreign content like MathML).
[#3112, <a
href="https://redirect.github.com/sparklemotion/nokogiri/issues/3116">#3116</a>]
(<a
href="https://github.com/flavorjones"><code>@​flavorjones</code></a>)</li>
<li>[CRuby] Fixed a small memory leak in libgumbo (HTML5 parser) when
the maximum tree depth limit is hit. [#3098, <a
href="https://redirect.github.com/sparklemotion/nokogiri/issues/3100">#3100</a>]
(<a
href="https://github.com/stevecheckoway"><code>@​stevecheckoway</code></a>)</li>
</ul>
<h2>v1.16.0 / 2023-12-27</h2>
<h3>Notable Changes</h3>
<h4>Ruby</h4>
<p>This release introduces native gem support for Ruby 3.3.</p>
<p>This release ends support for Ruby 2.7, for which <a
href="https://www.ruby-lang.org/en/downloads/branches/">upstream support
ended 2023-03-31</a>.</p>
<h4>Pattern matching</h4>
<p>This version marks <em>official support</em> for the pattern matching
API in <code>XML::Attr</code>, <code>XML::Document</code>,
<code>XML::DocumentFragment</code>, <code>XML::Namespace</code>,
<code>XML::Node</code>, and <code>XML::NodeSet</code> (and their
subclasses), originally introduced as an experimental feature in
v1.14.0. (<a
href="https://github.com/flavorjones"><code>@​flavorjones</code></a>)</p>
<p>Documentation on what can be matched:</p>
<ul>
<li><a
href="https://nokogiri.org/rdoc/Nokogiri/XML/Attr.html?h=deconstruct#method-i-deconstruct_keys"><code>XML::Attr#deconstruct_keys</code></a></li>
<li><a
href="https://nokogiri.org/rdoc/Nokogiri/XML/Document.html?h=deconstruct#method-i-deconstruct_keys"><code>XML::Document#deconstruct_keys</code></a></li>
<li><a
href="https://nokogiri.org/rdoc/Nokogiri/XML/Namespace.html?h=deconstruct+namespace#method-i-deconstruct_keys"><code>XML::Namespace#deconstruct_keys</code></a></li>
<li><a
href="https://nokogiri.org/rdoc/Nokogiri/XML/Node.html?h=deconstruct#method-i-deconstruct_keys"><code>XML::Node#deconstruct_keys</code></a></li>
<li><a
href="https://nokogiri.org/rdoc/Nokogiri/XML/DocumentFragment.html?h=deconstruct#method-i-deconstruct"><code>XML::DocumentFragment#deconstruct</code></a></li>
<li><a
href="https://nokogiri.org/rdoc/Nokogiri/XML/NodeSet.html?h=deconstruct#method-i-deconstruct"><code>XML::NodeSet#deconstruct</code></a></li>
</ul>
<!-- raw HTML omitted -->
</blockquote>
<p>... (truncated)</p>
</details>
<details>
<summary>Commits</summary>
<ul>
<li><a
href="https://github.com/sparklemotion/nokogiri/commit/673756fdd69d1036874b7d7250cc38a51fd4d7b8"><code>673756f</code></a>
version bump to v1.16.2</li>
<li><a
href="https://github.com/sparklemotion/nokogiri/commit/74ffd67a8efb9972657e5c4625fd8419bbccbe06"><code>74ffd67</code></a>
dep: update libxml to 2.12.5 (branch v1.16.x) (<a
href="https://redirect.github.com/sparklemotion/nokogiri/issues/3122">#3122</a>)</li>
<li><a
href="https://github.com/sparklemotion/nokogiri/commit/0d4018dc7009580659c101fc41efb3babcfec229"><code>0d4018d</code></a>
dep: update libxml2 to v2.12.5</li>
<li><a
href="https://github.com/sparklemotion/nokogiri/commit/f33a25f4378df33912ebc6b4ebc0f9e8e80ddfa8"><code>f33a25f</code></a>
dep: remove patch from <a
href="https://redirect.github.com/sparklemotion/nokogiri/issues/3112">#3112</a>
which has been released upstream</li>
<li><a
href="https://github.com/sparklemotion/nokogiri/commit/e99416896a182bc520a7940bbe286ec33597ab2b"><code>e994168</code></a>
version bump to v1.16.1</li>
<li><a
href="https://github.com/sparklemotion/nokogiri/commit/77ea2f228c20e79c848ca2906813ea5b5010281b"><code>77ea2f2</code></a>
dev: add files to manifest ignore list</li>
<li><a
href="https://github.com/sparklemotion/nokogiri/commit/756f27c6b7a23294d84bdcca5e03a639d0dd7421"><code>756f27c</code></a>
build(deps): bump actions/{download,upload}-artifact from 3 to 4</li>
<li><a
href="https://github.com/sparklemotion/nokogiri/commit/464f8d41eb73ca9c6dae0b366afcf5f4e8bff342"><code>464f8d4</code></a>
.gitignore: clangd-related files</li>
<li><a
href="https://github.com/sparklemotion/nokogiri/commit/2beeb960691df28dd5ebf828192c65b60250670f"><code>2beeb96</code></a>
doc: update CHANGELOG</li>
<li><a
href="https://github.com/sparklemotion/nokogiri/commit/a26536d7a41fd40c52940e165bb5a4f6b4c39662"><code>a26536d</code></a>
fix: apply upstream patch for in-context parsing (<a
href="https://redirect.github.com/sparklemotion/nokogiri/issues/3116">#3116</a>)</li>
<li>Additional commits viewable in <a
href="https://github.com/sparklemotion/nokogiri/compare/v1.14.3...v1.16.2">compare
view</a></li>
</ul>
</details>
<br />


[![Dependabot compatibility
score](https://dependabot-badges.githubapp.com/badges/compatibility_score?dependency-name=nokogiri&package-manager=bundler&previous-version=1.14.3&new-version=1.16.2)](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores)

Dependabot will resolve any conflicts with this PR as long as you don't
alter it yourself. You can also trigger a rebase manually by commenting
`@dependabot rebase`.

[//]: # (dependabot-automerge-start)
[//]: # (dependabot-automerge-end)

---

<details>
<summary>Dependabot commands and options</summary>
<br />

You can trigger Dependabot actions by commenting on this PR:
- `@dependabot rebase` will rebase this PR
- `@dependabot recreate` will recreate this PR, overwriting any edits
that have been made to it
- `@dependabot merge` will merge this PR after your CI passes on it
- `@dependabot squash and merge` will squash and merge this PR after
your CI passes on it
- `@dependabot cancel merge` will cancel a previously requested merge
and block automerging
- `@dependabot reopen` will reopen this PR if it is closed
- `@dependabot close` will close this PR and stop Dependabot recreating
it. You can achieve the same result by closing it manually
- `@dependabot show <dependency name> ignore conditions` will show all
of the ignore conditions of the specified dependency
- `@dependabot ignore this major version` will close this PR and stop
Dependabot creating any more for this major version (unless you reopen
the PR or upgrade to it yourself)
- `@dependabot ignore this minor version` will close this PR and stop
Dependabot creating any more for this minor version (unless you reopen
the PR or upgrade to it yourself)
- `@dependabot ignore this dependency` will close this PR and stop
Dependabot creating any more for this dependency (unless you reopen the
PR or upgrade to it yourself)
You can disable automated security fix PRs for this repo from the
[Security Alerts
page](https://github.com/matrix-org/dendrite/network/alerts).

</details>

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

[skip ci]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-Enhancement New features, changes in functionality, improvements in performance, or user-facing enhancements.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants