search

uktrade / sqlite-s3vfs

Python writable virtual filesystem for SQLite on S3
MIT License
121 stars 9 forks source link

Advertise correct permissions in xAccess #2

Closed simonwo closed 2 years ago

simonwo commented 3 years ago

xAccess is meant to advise SQLite whether it can read or write to a file depending on the permissions it asks for. At the moment it is only capable of telling it whether the file already exists.

xAccess should hook into the AWS ACL stuff and work out whether files are readable or writable as well.

michalc commented 3 years ago

I am tempted to do this a different way... even maybe "lie" in xAccess and fail later on. It's all based on my own experience, but...

... I don't think I've ever used ACLs to control access to a bucket/parts of a bucket, but always through IAM or bucket policies. Also, if locking down access in a bucket, I've never granted the ability to read an ACL, so I suspect there are too many cases where anything ACL-based would fail anyway.

So I think probably the best we can do is make sure 403s etc bubble up in some appropriate way, and make sure it's documented what permissions are needed on what object?

michalc commented 3 years ago

Actually, at least in the tests, it looks like the xAccess method isn't even called for the main database file (but only for the journal / wal)...

simonwo commented 3 years ago

Yeah I put ACLs but perhaps that’s not right. I guess really it’s “we should make xAccess work however appropriate”. Maybe something around attempting to read/write a test file and catching any 403s?

Do the tests cover any case of reading from an existing db with a new connection obj? I could imagine that might stimulate some usage of xAccess.

michalc commented 3 years ago

Maybe something around attempting to read/write a test file and catching any 403s?

It crossed my mind as well... but I'm torn if it's worth it. A test file working doesn't mean that access to the other files would work, and then has the possibility of leaving the bucket a bit messy (say DELETE fails and leaves the test file), so there are more cases to deal with.

Do the tests cover any case of reading from an existing db with a new connection obj? I could imagine that might stimulate some usage of xAccess.

They do! https://github.com/uktrade/sqlite-s3vfs/blob/1006f2d95da432ceb283641950563fa157c7426e/test_sqlite_s3vfs.py#L98 and I still couldn't see it querying xAccess to see it the file exists. It just calls xRead on the main file, which returns 0 bytes if nothing exists.

michalc commented 2 years ago

I'm going to close this... I think the current behaviour, although imperfect, has the nice property that it's simple. There are too many edge cases of AWS permissions to address them all, and without a specific issue to address, tricky to know which are the most important in the real world

(Of course if a specific issue crops up/is thought to be likely to crop up, feel free to re-open)