ZcashFoundation / zebra

Zcash - Financial Privacy in Rust 🦓
https://zfnd.org/zebra/
Apache License 2.0
409 stars 100 forks source link

Make the writing API of read-only databases inaccessible #8098

Open upbqdn opened 10 months ago

upbqdn commented 10 months ago

Motivation

PR #8079 adds support for opening the database in a read-only mode. However, the writing functionality still remains accessible in the API of the returned database. We should make it inaccessible for read-only databases. Note that this is not a security issue since attempting to write to a read-only database fails or panics, and the writes are not remotely triggerable.

Possible Solutions

One possible solution is having a type that implements all the read methods, but only implements the two write methods: write(batch) and spawn_format_change() when it is a read-write database. This can be implemented using a generic parameter that allows writing.

One possible implementation is adding an IsWriteable generic to the database and TypedColumnFamily, and only implementing TypedColumnFamily::for_writing() when the generic is ReadWriteDatabase.

After PR #8112, this should be implemented on TypedColumnFamily and WriteTypedBatch.

For example, the second generic parameter here is required to be DBWithThreadModeInner, we could do something similar with a ReadWriteDatabase unit struct: https://docs.rs/rocksdb/latest/rocksdb/struct.DBCommon.html#impl-DBCommon%3CT,+DBWithThreadModeInner%3E

And here are the methods that work regardless of the generic type (for us that would be read methods): https://docs.rs/rocksdb/latest/rocksdb/struct.DBCommon.html#impl-DBCommon%3CT,+D%3E

Credit to Teor for outlining the solutions.

Documentation

Document that secondary/read-only instances only read data that is in the database when it is opened. A specific method needs to be called to make secondary instances get more data: https://github.com/facebook/rocksdb/wiki/Read-only-and-Secondary-instances

Document that the supported way to get read-only access to the state from a separate process is RPCs, and within the same process is cloning a ReadStateService. This is because we have a non-finalized state containing blocks not in the database.

mpguerra commented 10 months ago

would this be a potential security issue for the blockchain scanner? If so we can add it to the list of tasks to do at some point after the MVP...

upbqdn commented 10 months ago

I don't think so because writing would fail or panic. I added this note to the description:

Note that this is not a security issue since attempting to write to a read-only database fails or panics, and the writes are not remotely triggerable.

teor2345 commented 10 months ago

It might also be helpful to document that the supported way to get read-only access to the state from a separate process is RPCs, and within the same process is cloning a ReadStateService. This is because we have a non-finalized state containing blocks not in the database.

I'll add this to the ticket.

teor2345 commented 10 months ago

We might want to think about how to do this with TypedColumnFamily and WriteTypedBatch after PR #8112.

One possible implementation is adding an IsWriteable generic to the database and TypedColumnFamily, and only implementing TypedColumnFamily::for_writing() when the generic is ReadWriteDatabase.

mpguerra commented 14 hours ago

Is this work relevant outside of the scanner work?