Store WebAuthn credential state as individual DB records and return as array in API #16
Conversation
This will be useful for storing WebAuthn credentials as individual records instead of in one blob. This way the `webauthnService` can receive a subspace of the KV store and operate on that with just credential IDs as keys, instead of having to remember to add the subspace prefix in every individual call.
| // Create a new `NamespacedKV` with the same leveldb backend, and the current | ||
| // prefix appended with `keyPrefix`. | ||
| // `n.Subspace(subpre).PutString(k)` is equivalent to `n.PutString(subpre + k)`. | ||
| func (n *NamespacedKV) Subspace(keyPrefix string) *NamespacedKV { | ||
| return NewNamespacedKV(n.db, n.prefix+keyPrefix) | ||
| } |
There was a problem hiding this comment.
This is interesting, but I think we're starting to mix up some concepts. In essence, the current LevelDB backend is a flat key-value store. Values are usually protobuf-marshalled message blobs. The keys are made up of one prefix byte, followed by a prefix-specific disambiguation, naming the actual thing being stored. See the explanations in lib/db/keyer.go.
The NamespacedKV seems to be an API abstraction on top of that, with convenience methods to put unstructured (i.e. usually scalar) values in the DB without specifying the prefix byte over and over (that's my understanding). And without having to define a protobuf message for each and every type of data.
The miscDB is a predefined space using the KeyTypeMiscData prefix byte (value 10). Other instances are the folder and device statistics, each with their own prefix byte values. As I understand it, the "namespace for miscellaneous metadata" (miscDB) is meant to store singular pieces of information, such as the dbVersion (of which there is just one entry).
So your construct of a "Subspace" within the miscDB basically solves the same problem that the NamespacedKV already does for the DB as a whole. Thus I believe it would be wiser to define a new key prefix byte value to provide the desired subspace. The next step would be a NewWebauthnCredentialsNamespace() function akin to NewMiscDataNamespace() that provides an object with the convenience API.
But since we know what an entry should look like, the NamespacedKV convenience API wouldn't even be useful, as you'd always put []byte values in there, resulting from the protobuf encoding of a certain fixed Go structure type. At which point you can skip the NamespacedKV construct altogether and just put your values in the DB directly using your own KeyTypeWebauthnCredential value as a common prefix, followed by the credential ID. Like we do for KeyTypeDevice and lots of other things.
Looking ahead to an SQL-based backend, one key type prefix would translate to one SQL table with a fixed definition of record fields, mapping to the protobuf structure fields. I don't know if that's the direction we'll be heading, as I didn't have much time to look at syncthing#9965 yet. But it would be one logical way to employ a relational DB as intended, instead of abusing it to hold protobuf blobs.
I hope this all makes sense to you. I am frankly still confused by the different DB access methods in the code base myself. But at the current development pace, syncthing#9175 may very well get to land after the SQLite stuff. So it makes sense to take the needed refactoring into account now. Really sorry to see this causing extra work and further delays. I'd rather have had this finished already and let Jakob worry about yet another SQL table during the SQLite migration. But as it stands, it will probably be on us to integrate with the new paradigms instead.
| defer s.credStateMut.RUnlock() | ||
|
|
||
| var state apiproto.WebauthnVolatileState | ||
| for _, cred := range slices.Concat(guiCfg.WebauthnCredentials, s.credentialsPendingRegistration) { |
There was a problem hiding this comment.
Should iterate through all entries in the DB, regardless of what the GUI config holds. Missing values for a specific credID can be handled easily on the client side, we don't need to send an empty dummy object.
What you could do though is filter out (and drop from the DB) any entry that is neither mentioned in the config, nor in the pending registrations.
| @@ -7,10 +7,11 @@ import "google/protobuf/timestamp.proto"; | |||
| // State that changes often but also is not security-critical, | |||
| // and therefore can be wiped and overwritten without much consequence if needed. | |||
| message WebauthnVolatileState { | |||
| map<string, WebauthnCredentialVolatileState> credentials = 1; // Keys are base64.RawURLEncoding.EncodeToString(credential ID) | |||
| repeated WebauthnCredentialState credentials = 1; | |||
There was a problem hiding this comment.
I'm not sure this extra indirection (outer object with only a .credentials attribute) gives us any added value?
But since we probably need it to properly define the API structure, why not separate into .pending and .active collections? Just a thought, we maybe don't gain much on the GUI side by the distinction.
There was a problem hiding this comment.
Agreed, I deleted it altogether at first. But it's needed in order to use sendProtobufJSON in the getCredentialsState API handler, because protojson.Marshal takes only proto.Message arguments. protojson is in turn needed because Protobuf sets the correct JSON name in the protobuf: tag but not the json: tag, as revealed by the new test I added to syncthing#9175 yesterday. So I needed to either add and use sendProtobufJSON or very verbosely copy all the values into local structs with the right json: tags, like the ones used for unmarshaling in the tests.
(Also it looks like you can't use a standalone slice or map as a proto.Message argument either, so the parent object is needed for that even if we wanted to return an unwrapped JSON array.)
@acolomb This meta-PR addresses this review comment: syncthing#9175 (comment)