serokell / coffer

Multi-backend password store with multiple frontends
4 stars 2 forks source link

Make path's charset rules backend-specific #65

Closed MagicRB closed 2 years ago

MagicRB commented 2 years ago

Clarification and motivation

Currently coffer restricts all Entry paths to ['a'..'z'] ++ ['A'..'Z'] ++ ['0'..'9'] ++ "-_" while Unix/Linux allows for many more. This may cause issues with file based backends like pass. Rather than cheking globally, if a backend has limitations it should enforce them at its boundary.

Acceptance criteria

Either allow more chars or close as wontfix.

dcastro commented 2 years ago

I've never used pass but, supporting all valid unix/linux paths seems troublesome. IIRC, a unix path can be any byte sequence, doesn't even have to be unicode.

Do we want to support that? Or maybe something in the middle?

MagicRB commented 2 years ago

Maybe backends could define what charset they support, so if you try to copy from A to B when A uses chars B doesnt support you get a nice consistent error mesaage

dcastro commented 2 years ago

~Just something to keep in mind: I agree with backends having their own list of supported characters, but I think = should be forbidden regardless of the backend, because that character is used in coffer create.~

~Allowing it would make its syntax ambiguous, e.g.~

# Is the field name `a` or `a=b`?
coffer create /entry --field a=b=c

Nevermind, I was confusing path's charset with field name's charset.

For paths, we should reserve the character #, since we use it to qualify a path (e.g. backend#/path)

dcastro commented 2 years ago

I've edited the title of the PR to reflect what was discussed. To sum up:

Kariel-Myrr commented 2 years ago

HI! While thinking of how to make further validation on paths by backend I came with this solution: 1) Try implement it in mkQualifiedPath:

2) Checking when we're actually trying get path from backend (like Backend.Commnds.getEntryOrDir)

dcastro commented 2 years ago

Try implement it in mkQualifiedPath: it would force us to now for witch type of backend we're making path

mkQualifiedPath is used to parse the command-line options. At this point, you only have the backend name, not the backend type. The mapping "backend name -> backend type" is only known after parsing the config.toml file, and that happens after we parse the command-line options. So I don't think this is the way to go.

I suggest instead of storing Maybe BackendName but whole Backend

Why? mkQualifiedPath is used to parse paths from the command line and, as far as the CLI user is concerned, the backend name is optional.


Checking when we're actually trying get path from backend (like Backend.Commnds.getEntryOrDir)

I don't get this - if you're reading entries from a backend, then surely their paths are valid. We're more concerned with validating paths before writing to a backend.

Kariel-Myrr commented 2 years ago

You're totally right about QualifiedPath

Kariel-Myrr commented 2 years ago

I don't get this - if you're reading entries from a backend, then surely their paths are valid. We're more concerned with validating paths before writing to a backend.

Yeah, so I'm still not sure on which lvl I we have to validate: