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

libsql: attach databases from other namespaces as readonly #784

Merged
merged 10 commits into from
Feb 14, 2024
Merged

Conversation

psarna
Copy link
Collaborator

@psarna psarna commented Dec 13, 2023

With this proof-of-concept patch, other namespaces hosted on the same sqld machine can now be attached in readonly mode, so that users can read from other databases when connected to a particular one.

Turned off by default, can be turned on with admin API by setting allow_attach = true:

curl -H 'Content-Type: application/json' \
    -d '{"block_reads": false, "block_writes": false,"allow_attach": true}' \
    http://localhost:9090/v1/namespaces/default/config

The expected usage is to ATTACH namespace-name AS namespace-name, without any paths or flags, e.g.

ATTACH my_other_ns AS my_other_ns; SELECT * FROM my_other_ns;

Note that turso db shell sends separate statements in separate streams, so ATTACH will only be in effect if you put it in the same line.

Fixes TURSO-90.

@psarna
Copy link
Collaborator Author

psarna commented Dec 13, 2023

(transplant of libsql/sqld#770, work in very heavy progress)

@psarna psarna force-pushed the attachpoc branch 3 times, most recently from 71c5a77 to f712661 Compare December 14, 2023 13:19
@psarna psarna marked this pull request as ready for review December 14, 2023 13:23
Copy link
Collaborator

@penberg penberg left a comment

Choose a reason for hiding this comment

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

Looks good to me, but let's ping @MarinPostma and @LucioFranco.

@glommer
Copy link
Contributor

glommer commented Dec 19, 2023

the shell thing is unfortunate. Is there a path to fix this ?

@psarna
Copy link
Collaborator Author

psarna commented Dec 19, 2023

the shell thing is unfortunate. Is there a path to fix this ?
I assume the issue is in our Go code, which treats each sent line as a separate db connection - so that when you send a

BEGIN

, and then, in the next line,

COMMIT

, it will not finish the transaction, and instead give you a SQLite error: cannot commit - no transaction is active, because both statements where sent as separate database connections. Instead, I suppose the shell should establish a single connection and send all statements within that single connection. @haaawk does it match the Golang impl?

@psarna
Copy link
Collaborator Author

psarna commented Jan 2, 2024

Enabled attaching databases with aliases, e.g. ATTACH very-long-db-name-in-form-of-a-uuid AS nice_short_name. We still have an unsolved issue that the SQL user needs to know the aforementioned very-long-db-name-in-form-of-a-uuid in order to attach anything via SQL, but it's a start.

@psarna
Copy link
Collaborator Author

psarna commented Jan 2, 2024

also, I need to fortify this interface against attaching system files as dbs, even in readonly mode, pretty sure it's possible right now if somebody tries to ATTACH "../../../../../etc/passwd AS exploit"

@psarna
Copy link
Collaborator Author

psarna commented Jan 2, 2024

ok, didn't manage to break it after all, dots and slashes need to be quoted, and that quote lands in the file path as well, so it's not that easy to jailbreak

@psarna
Copy link
Collaborator Author

psarna commented Jan 2, 2024

snap, now I realized there's a mirror issue... if the db name contains characters like -, it needs to be quoted in libsql and it won't work.... let me try and fix both

@psarna
Copy link
Collaborator Author

psarna commented Jan 2, 2024

k done

@psarna psarna force-pushed the attachpoc branch 5 times, most recently from b15de01 to e6e1de5 Compare January 15, 2024 09:59
@psarna
Copy link
Collaborator Author

psarna commented Jan 15, 2024

Rebased on top of the new per-namespace jwt_key code

Copy link
Contributor

@MarinPostma MarinPostma left a comment

Choose a reason for hiding this comment

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

This will not be supported on the platform, I guess that'll be addressed as a followup?

Comment on lines 140 to 213
// STEP 4: try attaching a database
{
let foo = Database::open_remote_with_connector(
"http://foo.primary:8080",
"",
TurmoilConnector,
)?;
let foo_conn = foo.connect()?;

foo_conn.execute("attach foo as foo", ()).await.unwrap_err();
}

// STEP 5: update config to allow attaching databases
client
.post(
"http://primary:9090/v1/namespaces/foo/config",
json!({
"block_reads": false,
"block_writes": false,
"allow_attach": true,
}),
)
.await?;

{
let foo = Database::open_remote_with_connector(
"http://foo.primary:8080",
"",
TurmoilConnector,
)?;
let foo_conn = foo.connect()?;

foo_conn
.execute_batch("attach foo as foo; select * from foo.sqlite_master")
.await
.unwrap();
}

Copy link
Contributor

Choose a reason for hiding this comment

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

let's move that to it's own test

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

k, moved

@MarinPostma
Copy link
Contributor

The fix for the shell would be to ideally use websockets, so that you can have a long live connection. With HTTP you, you'll have a 10sec idle connection timeout that needs to be worked around. Either way, you have to ensure that all statements go to the same stream

With this proof-of-concept patch, other namespaces hosted
on the same sqld machine can now be attached in readonly mode,
so that users can read from other databases when connected
to a particular one.
Default is false, which means connections are blocked from attaching
databases. If allowed, colocated databases can be attached in readonly
mode.

Example:
→  attach another as another; select * from another.sqlite_master;
TYPE      NAME     TBL NAME     ROOTPAGE     SQL
table     t3       t3           2            CREATE TABLE t3(id)
psarna and others added 7 commits February 6, 2024 11:32
We're going to need it, because the internal database names in sqld
are uuids, and we don't expect users to know or use them.
In libsql-server, raw db names are uuids that need to be quoted,
so that needs to be supported in the ATTACH layer.
As a bonus, "names" that are actually file system paths are refused
to prevent abuse.
@haaawk haaawk added this pull request to the merge queue Feb 14, 2024
Merged via the queue into main with commit eb7dadd Feb 14, 2024
12 checks passed
@haaawk haaawk deleted the attachpoc branch February 14, 2024 10:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants