storage/cloud: create user scoped "from client" file upload for easy imports (fka make 'nodelocal' safe for multi-user clusters)

[dt]

Right now nodelocal simply maps to the external IO dir, but since all access is done as the server process, this means there is no user-level access control the way there is with, say, s3 where the user can bring their own tokens.

The current system is useful in some cases but to be more generally useful in multi-user deployments, we need to make it user-aware: it should be a grantable privilege to interact with nodelocal files at all and users should only be able to interact with their own files. We potentially could also want quotas on nodelocal usage, though until table usage has quotas it is a moot point (you could just store your files in a blob columns).

[adityamaru]

Notes after talking with @dt and @miretskiy and exploring relevant code.

Using nodelocal upload along with local_storage has two significant issues which we can think off:

1. Assume a situation in which a user uploads a local file via nodelocal upload and proceeds to import data from that file stored on local_storage. A malicious user without INSERT privileges can tamper with the file once it is on the nodes local_storage and meddle with the data inserted into the table.

2. We provide encryption-at-rest (as an enterprise feature). This means that once the file is imported it is encrypted on disk, but it is in plaintext form while stored on the node's local_storage. This is not secure for a customer looking for end to end encryption.

To address these concerns we feel that instead of incrementally making nodelocal upload user aware, it makes sense to use the current building blocks to build an alternate upload/download command (name TBD).

The new command will reuse much of the logic to transfer a local file over pgwire, as well as the existing inter-node blob communication service. The main differences are highlighted below:

Instead of using local_storage which writes to ExternalIODir we will make a new storage abstraction (upload/download_storage) that will direct reads/writes to a user scoped subdirectory of the chosen store directory. The file path could look something like $STORE_DIR/$USER/. For temp storage we currently use either the store specified using --temp-dir, or the path of the first encrypted store, or that of the first store if there is no encryption-at-rest. This seems like a pattern we could use to choose the user directory location.

Another difference between local_storage and upload/download_storage will be how we interact with the files. While local_storage performs IO via unencrypted system calls, upload/download_storage can use storage.fs. This FS interface is implemented by the engine (RocksDB/Pebble) linked to the store we make the directory on. From what I can tell, temp has the flexibility of allowing the user to specify an alternate store via --temp-dir because it creates a temp engine linked to that store path. In our case, we probably want to use one of the engines linked to the main store paths specified via --store, and therefore cannot allow this.

While nodelocal upload can only be run as admin, upload/download will be usable by non-admin users as well. A user will only be able to access the subdirectory with the name matching their username. Logic preventing users from escaping from this directory and attempting to access other user directories will need to be added.

The eventual goal of developing upload/download_storage is to:

• Support import table test (id INT primary key) CSV DATA test.csv from a cockroach sql shell where test.csv is a local file. This single step import (instead of upload + import) makes for better user experience.

• Add an upload command to store local files in a user scoped directory on the node.

• Add a download command to fetch backup files from the user scoped directory and copy it to a local destination.

Outstanding questions:

• What will the garbage collection semantics of the user directory be?
• Why will a user use local_storage anymore?
• Will we limit the disk space this user directory can use?
• UX discussions with @mwang1026.

[adityamaru]

Notes after UX discussion with @mwang1026 and @dt.

I believe we have a consensus on the importance of developing a new user scoped, encrypted storage, and so I will give my 2¢ on some of the pending technical/UX questions below:

Streaming directly from laptop to table vs via ephemeral, on-disk storage

• The concern of a long-running import job getting interrupted every time the laptop goes to sleep, or the connection flakes is a red flag if we are designing something which could be a user's first interaction with CockroachDB. I also imagine that in this slower, direct import case we would have to give the user a visual cue that progress is being made.
• Having a staging area to upload/download files to/from is also more in line with the inter-node communication service we have from the nodelocal project.
• There were concerns that the upload size must be less than the disk space of a particular node. If user scoped storage is not really a consideration for enterprise customers then this may be less of a red flag. In my opinion, it is also fair to expect the user to upload files to the node with sufficient disk space - else we can return a not enough space error?

Where will this staging area live?

Taking a leaf from the temp dir book, on server startup we can create a users directory using the first encrypted store spec or the first spec by default. This will ensure that the data is encrypted if encryption-at-rest is enabled. The first time a user attempts interact with this directory, we create a username sub-directory. Inter-node communication can utilize the existing blob client service.

When/how will this directory be cleaned up?

Broadly I see three sources of files in this staging area:

• Files uploaded by the user.
• Backup files to be downloaded/restored from by the user.
• Files that will be uploaded under-the-hood to support the CLI import command.

In the first two cases, I do not believe we should proactively delete the files as the user has initiated the process of getting them onto the node. In the third case, the files should be deleted on import completion (both success and failure) because the goal of the CLI import command is to get the data into the specified table and not on disk. Some of the suggestions on how we will keep track of this metadata were to create a system table or simply write to a txt file stored in the user-specific directory with the relevant metadata (this is what temp dir does). A worker can use this information to perform the relevant cleanup.

How will user access be restricted?

Additional conversations will be required to narrow down on the URI we require users to specify when uploading, downloading, backing up, or importing to their directories, but at a high level, username directories under the global users/, will only be accessible by users with that username. The admin user might be granted the privilege to access all the user directories.

Roughly the development process should be very similar to the one mentioned in the nodelocal blog post:

• Develop the user scoped storage which will interact with disk via storage.FS.
• Support IMPORT/BACKUP/RESTORE from the user directory.
• Support upload/download to/from user directory.
• Support magical CLI import command.

I'd love to get comments on these broad design areas (and ones that I have missed)!

[adityamaru]

More notes after a discussion with @bdarnell and @dt.

Adding a new user storage into CRDB comes with a host of additional semantics we have to design solutions for, such as:

• Persisting files over node restarts
• User privileges
• Deletion semantics
• Storage quota

The surface area of this change is very large for the target use case which is top-of-the-funnel non-production users trying to import local files/dumps into a cluster.

With the hypothesis that we are not designing for large scale imports, another solution is to upload files to user-specific tables. More specifically when a user triggers an upload/import from the CLI we would create a table tied to that user with primary key filename, byte-offset, and an additional bytes-payload column. Each row would represent a chunk of the uploaded file, where the chunk size could be ~4MB. Having a single table with all uploaded files per user, as opposed to a single table for each upload by a user, allows us to nuke all the uploaded files with a simple DROP.... By default, this table could be created in the default node db. Whether we would have the user specify the database (or maybe even the table) in the URI to which they upload is a UX decision.

It is important to understand the downsides of this approach:

• Write amplification: We are writing to a table during upload, only to read from that table and write to another table during import.
• Raft: The table will be replicated just like any ordinary table, which consequently means we will have to go through Raft.
• Performance: The reasons stated above will have serious performance implications for big imports, but to reiterate that is not the use case we are targeting.

Now for the upsides:

• User access: We get grantable privileges for free by virtue of reading/writing from a table. This was one of the major stumbling blocks for nodelocal.
• Encryption: If the cluster has encryption-at-rest we get it for free.
• Persisting files: Since the table will be replicated (like any other user table), the data will be persisted across node restarts and thus we will be able to resume import jobs.
• Deletion semantics: While we will still need to decide when to clean up the table, doing so would be a simple DROP... statement on the user table.

As an aside, we already allow users to create a table and insert a column of bytes into it. The new solution is more in line with this, we are telling the user "we could do this for you, and use it for an import".

There are still some product-related questions which @miretskiy raised:
The new solution reduces the scope of this user storage to only upload/import. It would be an anti-pattern to backup your data into this user table which in turn should be backed up (infinite backup recursion?). Even with imports, we will have to have a limit on the file size which can be uploaded via this mechanism. If this size we agree on is not substantial could we stick it in the job protobuf and just run an import? In essence, are we over-designing for the target use case?

[mwang1026]

I'm a little concerned that the complexity of these solutions is >> the value it'll bring. A "can't upload files bigger than X" criteria seems like poor UX as well--it all but tells the user that it's a "for testing only" feature. It feels to me that we're overthinking this a bit. If the file is big and they're worried about timeouts there IS a workaround that is very straightforward--upload file to something that won't go to sleep! And if they're looking for something quick and dirty for small-ish files on their laptop maybe streaming is sufficient and they're OK with having to retry of the connection fails or computer goes to sleep.

For the Postgres and Mysql parallels, do they also falter if the connection fails? If not what is the behavior for them?

[miretskiy]

We already have pretty poor UX experience if a user attempts to upload large single file;
Having (possibly separate) tool which makes it clear that this import is for initial setup of a testing cluster -- quick and dirty way to get going withsmall amount of data, would actually be an improvement, imo.

[adityamaru]

maybe streaming is sufficient

Ben had voiced concerns about exposing a gRPC connection to the client. It is common for DBs to only have a SQL access. It would add additional security concerns about ensuring that only an authorized user can read files off their client. David gave a worst-case scenario where a malicious client A could pull (import) private docs over an existing connection to the cluster with client B, via a crafted URI. Streaming is also technically more complex than the table based solution.

[dt]

(obviously we'd code-up checks in the client to the paths the server requests it send to avoid that case, but it's an example of the added security surface area we'd need to consider).

A nice thing about just stuffing bytes in a table is that it also means we get to use our existing security controls "for free" -- since it is just a table, existing grants and access control just work, existing space accounting / monitoring / visibility just works (i.e. the space used by these files would be reflected on dbs/tables pages), durability/replication/availability all work. It certainly has some appeal.

[miretskiy]

There are clear benefits with the table based approach; no doubt. However, if we are going to impose fairly low limits on the size of those files, then my question is: can we take this small file (say ~4MB) and put it directly inside import job specification? There are, of course, negatives w/ this (namely, the file size limit has to be fairly small, increase in proto buf size in the jobs table); but there are also clear benefits too (namely, we don't really have to develop much of anything, and, upon job completion, we can even erase those bytes from the payload).

…

On Wed, Jun 17, 2020 at 3:19 PM David Taylor ***@***.***> wrote: (obviously we'd code-up checks in the client to the paths the server requests it send to avoid that case, but it's an example of the added security surface area we'd need to consider). A nice thing about just stuffing bytes in a table is that it also means we get to use our existing security controls "for free" -- since it is just a table, existing grants and access control just work, existing space accounting / monitoring / visibility just works (i.e. the space used by these files would be reflected on dbs/tables pages), durability/replication/availability all work. It certainly has some appeal. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#47211 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ANA4FVCUUUEPRFCVKAFUCUDRXEJLBANCNFSM4MEDRLPQ> .

[mwang1026]

Makes sense re: complexity of solutions and gRPC concerns with streaming. No objections from me on implementation with the "intermediate" table with byte blobs. What's a high level estimate for the effort to accomplish this feature?

re: the downsides Ben mentioned, are they mitigated by a lower replication factor on this special table?

We already have pretty poor UX experience if a user attempts to upload large single file;
Having (possibly separate) tool which makes it clear that this import is for initial setup of a testing cluster -- quick and dirty way to get going withsmall amount of data, would actually be an improvement, imo.

Agreed. The point I'm trying to make is if someone is trying to migrate from a production DB, they're probably OK to use cloud storage, http sink, etc. to host the files to import.

re: the small use case it's definitely valuable to have a simpler option--the question in my mind is do we have explicit size limitations (not my favorite) or just stern warnings that this is best used for files smaller than X for testing.

To help make a decision here, I need a better understanding of what happens if the file IS big?

So a few open questions still:

• Would we explicitly limit import size or just issue a warning? What's the impact of using this feature with a huge file? What is huge?
• "TTL" of that data
• Naming of the feature
• Security, permissions, access control (probably want to vet with Aaron)

[dt]

a lower replication factor on this special table

Indeed, we'd love to do this (and could see it being useful in IMPORT more broadly too), but right now loss of quorum is a very, very bad thing -- unavailable ranges will cause various systems will get stuck forever and make a cluster sad. We need KV to build our more robust recovery from loss of quorum tools before we can safely run at lower replication factors in subsets of the keyspace.

[mwang1026]

Future work:

• list, delete
• "one shot upload"

[mwang1026]

Closing per #51222