Pass Backend #62
Pass Backend #62
Conversation
MagicRB
force-pushed
the
magic_rb/#55-pass-backend
branch
4 times, most recently
from
6432a1d to
52de877
Compare
|
Ok, think that's. So this is a weird one, or rather weird 3. It's actually 3 PRs in one. I'll split them up before merge, but we can review them now together, since they all developed together, triplets so to speak. Anyway. PR list:
Ok, so that's all. The last part I'm not happy with is |
|
Oh and I have no idea why it doesn't build. None at all, it builds on my machine:tm: |
MagicRB
force-pushed
the
magic_rb/#55-pass-backend
branch
from
52de877 to
77286c3
Compare
lib/Backend/Debug.hs
Outdated
|
|
||
| data DebugBackend = | ||
| DebugBackend | ||
| { dSubType :: T.Text |
There was a problem hiding this comment.
You have explicit Text import, so you don't need to write T.Text.
There was a problem hiding this comment.
Thought i got them, Ill grep for any T.Text and BS.Bytestring stragglers
There was a problem hiding this comment.
should be all of them, i think
lib/Effect/Fs.hs
Outdated
| _listDirectoryRec dirPath = | ||
| _listDirectory dirPath | ||
| >>= mapM \case Left f -> pure $ NodeRec $ Left f | ||
| Right d -> _listDirectoryRec d | ||
| <&> NodeRec . Right . Directory |
There was a problem hiding this comment.
Due to BlockArguments you can write smth like this
_listDirectoryRec dirPath = do
list <- _listDirectory dirPath
forM list \case
Left f -> pure $ NodeRec $ Left f
Right d ->
_listDirectoryRec d <&> NodeRec . Right . Directory
lib/Entry/Pass.hs
Outdated
| rightToMaybe :: Either a b -> Maybe b | ||
| rightToMaybe = \case | ||
| Left _ -> Nothing | ||
| Right b -> Just b |
lib/Entry/Pass.hs
Outdated
| pure $ E.newEntry entryPath dateModified | ||
| & E.fields .~ fields | ||
| & E.tags .~ tags | ||
| & E.masterField .~ (masterField >>= rightToMaybe . E.newFieldKey) |
There was a problem hiding this comment.
We're losing information about errors here (and in some other places).
There was a problem hiding this comment.
We can't really get errors out, since prisms don't support errors and all. That's my fault, it needs a refactor.
lib/Entry/Pass.hs
Outdated
| let parseLine | ||
| :: Maybe String | ||
| -> Parser T.Text | ||
| parseLine label = | ||
| do | ||
| x <- takeWhileP label (/='\n') | ||
| try newline | ||
| pure x | ||
| let parsePair | ||
| :: Parser (T.Text, T.Text) | ||
| parsePair = do | ||
| key <- takeWhileP (Just "key") (/='=') | ||
| char '=' | ||
| value <- takeWhileP (Just "value") (/='\n') | ||
| char '\n' <|> pure '\n' | ||
| pure (key, value) |
There was a problem hiding this comment.
I think these functions could be in where block
MagicRB
force-pushed
the
magic_rb/#55-pass-backend
branch
from
77286c3 to
304691f
Compare
|
I've gone through most, thanks for the input. I've noticed that mostly you see more combinators that could be used to make the code simpler. I guess that comes with experience. |
MagicRB
force-pushed
the
magic_rb/#55-pass-backend
branch
from
304691f to
da7170b
Compare
Signed-off-by: Magic_RB <[email protected]>
MagicRB
force-pushed
the
magic_rb/#55-pass-backend
branch
from
da7170b to
32e0702
Compare
| @@ -2,11 +2,11 @@ | |||
| # | |||
There was a problem hiding this comment.
Please, remember to rollback this config when you are done
|
|
||
|
|
There was a problem hiding this comment.
According to styleguide we must add one blank line between top-level definitions.
lib/Effect/Fs.hs
Outdated
| Right b -> Right $ T.unpack b | ||
| stringToPath :: String -> Path |
@MagicRB It's because you edited the We've since added support for hpack + stack, so now we should edit the We have a CI step to validate that the stack and cabal files are in sync (see here), but the check seems to be faulty... the step succeeded in this PR's pipeline and it shouldn't 🤔 I'll look into this. |
| Left e -> throw $ OtherError (show e & T.pack) | ||
| Right (Just _) -> pure () | ||
| Right Nothing -> throw . OtherError $ | ||
| "You must first initialize the password store at: " <> T.pack storeDir |
There was a problem hiding this comment.
Good idea, that's a good check! 👏
lib/Backend/Pass.hs
Outdated
| where tToFPath = Just . T.unpack | ||
| fPathToT :: Maybe String -> Maybe Text | ||
| fPathToT a = a <&> T.pack |
There was a problem hiding this comment.
Keep the indentation to a minimum.
From the styleguide:
Indent the
wherekeyword with 2 spaces and the definitions within the
whereclause with 2 more spaces
where
tToFPath = Just . T.unpack
fPathToT :: Maybe String -> Maybe Text
fPathToT a = a <&> T.packPlease look for other similar cases (e.g. in debugCodec, pbListSecrets, etc)
There was a problem hiding this comment.
think i got them all, ill regex it, yup got them all
lib/Backend/Pass.hs
Outdated
| PassBackend | ||
| <$> backendNameCodec "name" Toml..= pbName | ||
| <*> Toml.string "store_dir" Toml..= pbStoreDir | ||
| <*> Toml.dimatch fPathToT tToFPath (Toml.text "pass_exe") Toml..= pbPassExe |
There was a problem hiding this comment.
Instead of using Toml.text and then mapping to/contramapping from String, we can just use Toml.string.
lib/Backend/Pass.hs
Outdated
| >>= (\case Left e -> | ||
| if | isDoesNotExistError e -> pure Nothing | ||
| | True -> throw $ OtherError (T.pack $ show e) | ||
| Right v -> pure $ Just v) |
There was a problem hiding this comment.
Please use descriptive variable names instead of v/a/etc.
From the code style:
You should not use short names like
n,sk,f, unless their meaning is
clear from the context (function name, types, other variables, etc.).
lib/Effect/Fs.hs
Outdated
| pathToString :: Path -> Either FsError String | ||
| pathToString path = | ||
| case decodeUtf8' path of | ||
| Left a -> Left $ FEInvalidPath path |
There was a problem hiding this comment.
Looks like the UnicodeException in the Left should also be included in FEInvalidPath
|
|
||
| type Node f d = Either (File f) (Directory d) | ||
| type Node' a = Node a a | ||
| type Path = ByteString |
There was a problem hiding this comment.
Why not use the FilePath alias from the prelude?
I get that technically a unix filepath can be any byte sequence, it doesn't have to be unicode. And the prelude's type FilePath = String assumes the path only contains unicode characters.
So type Path = ByteString is more correct. However, we're internally converting those bytestrings to strings anyway (with pathToString), so we might as well just use FilePath from the prelude.
| deriving stock (Show) | ||
| makeLensesWith abbreviatedFields ''PassEntry | ||
|
|
||
| passFieldPrism :: Prism' PassField (E.FieldKey, E.Field) |
There was a problem hiding this comment.
Like we talked on Slack, prisms shouldn't really be used for parsing because it's impossible to know why parsing failed.
| deriving stock (Show) | ||
| makeLensesWith abbreviatedFields ''PassField | ||
|
|
||
| data PassEntry = |
There was a problem hiding this comment.
Why are these intermediate data structures needed?
In order to write an Entry to pass, we're converting Entry -> PassEntry -> PassKv -> Text
There was a problem hiding this comment.
I plan to remedy that globally, in the Prism rework. I want to merge Vault's format and pass' format. They're both KV, just pass uses a flat structure, vault a nested one. Names are different but that can be generalized too.
There was a problem hiding this comment.
I plan to remedy that globally, in the Prism rework
Can you please create an issue for that? Please detail what will be done.
I want to merge Vault's format and pass' format. They're both KV, just pass uses a flat structure, vault a nested one. Names are different but that can be generalized too.
Can you expand a bit on this?
If I'm understanding correctly, that would justify the need for PassKv, but I still don't see the need for the PassEntry intermediate data structure (I haven't looked at this part of the code in detail yet, so I might just be missing something).
There was a problem hiding this comment.
If I'm understanding correctly, that would justify the need for PassKv, but I still don't see the need for the PassEntry intermediate data structure (I haven't looked at this part of the code in detail yet, so I might just be missing something).
PassEntry exists, because the code is ugly and confusing and I thought that if I was trying to convert from a to Text and stuff into HashMap Text Text the code would be completely uncomprehensible.
Can you please create an issue for that? Please detail what will be done.
sure
|
#62 (comment) idk why but i cant seem to reply to this, so doing it here. |
lib/Effect/Fs.hs
Outdated
|
|
||
| data FsEffect m a where | ||
| NodeExists :: Path -> FsEffect m (Maybe (Node' ())) | ||
| GetNode :: Path -> FsEffect m (Maybe (Node' Path)) |
There was a problem hiding this comment.
_getNode doesn't seem to be a "primitive operation", it's more like a "helper" wrapper over _nodeExists. So I don't think we need to have it here, as a constructor of FsEffect. We can define it simply as:
getNode :: Member FsEffect r => Path -> Sem r (Maybe (Node' Path))
getNode path = do
mNode <- nodeExists path
pure
$ mNode <&> bimap
(const (File path))
(const (Directory path))Same thing for _listDirectoryRec, it's just a higher-level helper that builds on top of _listDirectory
There was a problem hiding this comment.
Actually, getNode and listDirectoryRec aren't used anywhere, so we should delete them.
There was a problem hiding this comment.
getNode returning Path is actually an error on my part, i just fit the types together. I meant for it to return a ByteString or a [Path] depending on whether it's a dir or file, but I'll yeet it. The idea keeping these in was to build up a small library that I planned to factor out in my own free time.
lib/Effect/Fs.hs
Outdated
| data FsEffect m a where | ||
| NodeExists :: Path -> FsEffect m (Maybe (Node' ())) | ||
| GetNode :: Path -> FsEffect m (Maybe (Node' Path)) | ||
| ListDirectory :: Directory Path -> FsEffect m [Node' ByteString] |
There was a problem hiding this comment.
Shouldn't ListDirectory return a Maybe, and Nothing when the given path doesn't exist?
lib/Backend/Pass.hs
Outdated
| verifyPassStore storeDir | ||
|
|
||
| let fpath = storeDir <> (path & build & fmt) | ||
| contents <- runError (fromException @IOException $ D.listDirectory fpath) |
There was a problem hiding this comment.
Shouldn't we be using listDirectory from the FsEffect here?
|
|
||
| case exitCode of | ||
| ExitSuccess -> | ||
| pure $ T.decodeUtf8 (BS.toStrict stdout) ^? passTextPrism . E.entry |
There was a problem hiding this comment.
Here, if decoding fails, we're returning Nothing.
But that's wrong - Nothing, in this context, means that the entry doesn't exist.
If it does exist but, for some reason, parsing failed, then we should throw an error.
|
|
||
| case exitCode of | ||
| ExitSuccess -> pure () | ||
| ExitFailure _e -> throw $ OtherError (T.decodeUtf8 $ BS.toStrict stderr) |
There was a problem hiding this comment.
_eWe shouldn't swallow the exit code, we should report that as well.
Same thing in pbWriteSecret and pbReadSecret
| -- | ||
| -- SPDX-License-Identifier: MPL-2.0 | ||
|
|
||
| module Backend.Debug |
There was a problem hiding this comment.
I'm not sure I agree with this approach...
It's essentially a very limited type of logging that can only log the input/output of BackendEffect operations. It cannot log what's happening inside the BackendEffect implementation, it can't log what's happening in the high-level Commands module, etc.
IMO, we should have a --verbose/-v switch that turns on logging throughout the entire codebase, wherever it may be useful. Plus, this wouldn't require the user to change their config file.
We could have a LogEffect and two interpreters: one that logs to stdout and another that does nothing. In main, depending on whether the -v switch was used, we choose which interpreter to use.
There was a problem hiding this comment.
This can be enabled easily by any other method, i wrote the debug backend because i was going nuts over not understanding what happening. We can reuse the impl later for a proper -v mode
| -- ReadNode nodePath -> undefined | ||
| -- CreateNode nodePath -> undefined | ||
|
|
||
| _nodeExists |
There was a problem hiding this comment.
We shouldn't prefix function names with _, it has a special meaning to GHC.
Prefixing with _ disables GHC's "unused definition" warnings/errors.
| @@ -22,6 +22,8 @@ library | |||
| Backend | |||
There was a problem hiding this comment.
We have a battery of integration/golden tests in tests/golden.
We should run those tests against the pass backend as well.
Those tests are run by make bats, which in turn calls the scripts/run-bats-tests.sh script.
At the moment, that script:
- spins up 2 vault instances
- runs the bats tests
- the tests are hardcoded to use
tests/golden/default-config.toml, which contains the config for those 2 vault instances
- the tests are hardcoded to use
- kills the 2 vault instances
We should modify the script such that:
- sets up 2
passinstances - exports
COFFER_CONFIG="pass-config.toml" - runs the tests
- cleans up the 2
passinstances - spins up 2 vault instances
- exports
COFFER_CONFIG="vault-config.toml" - runs the tests again
- kills the 2 vault instances
There was a problem hiding this comment.
I've found 2 bugs so far:
Creating an entry with a multiline field content succeeds (as in, it creates a file /tmp/pass-store/dir/entry4 on disk), but then coffer view says it doesn't exist.
$ coffer create /dir/entry4 --field "user=$(echo -e "first\nsecond")"
[SUCCESS] Entry created at '/dir/entry4'.
$ coffer view /dir
[ERROR] Entry or directory not found at '/dir'.
Haven't looked too much into it, but I suspect the parsing is failing. And the parser failure is being masked by the other bug I mentioned in another comment; we're returning Nothing in pbReadSecret when decoding fails.
This may or may not be a bug, maybe I'm doing something wrong? I don't know. But setting up a new pass instance and then running coffer / fails for me. Running coffer view with any path other than / works though:
$ export PASSWORD_STORE_DIR='/tmp/pass-store'
$ pass init [email protected]
mkdir: created directory '/tmp/pass-store/'
Password store initialized for [email protected]
$ coffer /
ListSecrets: Path {unPath = []}
out: Just [".gp"]
OtherError "Internal error:\nBackend returned a secret that is not a valid entry or directory name.\nGot: '.gp'.\n"
Signed-off-by: Magic_RB <[email protected]>
Signed-off-by: Magic_RB [email protected]
Description
Add a pass backend calling the
passCLI program directly. As discussed in #55Related issue(s)
Directly discusses #55.
✅ Checklist for your Pull Request
Related changes (conditional)
silently reappearing again.
of Public Contracts policy.
and
Stylistic guide (mandatory)