Pass Backend #62

MagicRB · 2022-04-05T19:11:38Z

Signed-off-by: Magic_RB [email protected]

Description

Add a pass backend calling the pass CLI program directly. As discussed in #55

Related issue(s)

Directly discusses #55.

✅ Checklist for your Pull Request

Related changes (conditional)

Tests
- If I added new functionality, I added tests covering it.
- If I fixed a bug, I added a regression test to prevent the bug from
  silently reappearing again.

Documentation
- I checked whether I should update the docs and did so if necessary:
  - README
  - Haddock

Public contracts
- Any modifications of public contracts comply with the Evolution
  of Public Contracts policy.
- I added an entry to the changelog if my changes are visible to the users
  and
- provided a migration guide for breaking changes if possible

Stylistic guide (mandatory)

My commits comply with the policy used in Serokell.
My code complies with the style guide.

MagicRB · 2022-04-18T20:36:51Z

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:

the actual backend, Backend/Pass.hs and Entry/Pass.hs
the supporting debug backend, very useful thing, Backend/Debug.hs
and finally an experiment. I needed a nice FS api and i wanted to try something more type bound, it's not as special as i wanted but it works and we can use it instead of IO, Effect/Fs.hs

Ok, so that's all. The last part I'm not happy with is Entry/Pass.hs, it's a parsing spaghetti hell as is common with marshalling code, but that in particular is hellish. I'm not really certain what to do with it, the format itself is a little thrown together since we're trying to fit into pass' standards. But, actually, it's similar to Vault, so maybe we could unify those formats. The main issue with the code right now, is the "modern art" of chaining functions and making little "pipelines". That would be solved by breaking the pipeline up into parts and giving those parts meaningful names. That's all.

MagicRB · 2022-04-18T20:38:04Z

Oh and I have no idea why it doesn't build. None at all, it builds on my machine:tm:

DK318 · 2022-04-19T14:10:14Z

lib/Backend/Debug.hs

+
+data DebugBackend =
+  DebugBackend
+  { dSubType :: T.Text


You have explicit Text import, so you don't need to write T.Text.

Thought i got them, Ill grep for any T.Text and BS.Bytestring stragglers

should be all of them, i think

lib/Backend/Debug.hs

DK318 · 2022-04-19T15:03:08Z

lib/Effect/Fs.hs

+_listDirectoryRec dirPath =
+  _listDirectory dirPath
+    >>= mapM \case Left f -> pure $ NodeRec $ Left f
+                   Right d -> _listDirectoryRec d
+                              <&> NodeRec . Right . Directory


Please, use do-notation + forM

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

DK318 · 2022-04-19T15:05:19Z

lib/Entry/Pass.hs

+rightToMaybe :: Either a b -> Maybe b
+rightToMaybe = \case
+  Left _ -> Nothing
+  Right b -> Just b


See eitherToMaybe

DK318 · 2022-04-19T15:08:46Z

lib/Entry/Pass.hs

+            pure $ E.newEntry entryPath dateModified
+              & E.fields .~ fields
+              & E.tags .~ tags
+              & E.masterField .~ (masterField >>= rightToMaybe . E.newFieldKey)


We're losing information about errors here (and in some other places).

We can't really get errors out, since prisms don't support errors and all. That's my fault, it needs a refactor.

lib/Backend/Pass.hs

DK318 · 2022-04-19T15:13:18Z

lib/Entry/Pass.hs

+                  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)


I think these functions could be in where block

MagicRB · 2022-04-20T05:35:34Z

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.

Signed-off-by: Magic_RB <[email protected]>

DK318

Please, don't squash your commits after review. It's really hard to rereview without seeing diff after the first review

DK318 · 2022-04-21T22:49:05Z

config.toml

@@ -1,12 +1,12 @@
 # SPDX-FileCopyrightText: 2022 Serokell <https://serokell.io>
 #


Please, remember to rollback this config when you are done

DK318 · 2022-04-21T22:51:19Z

lib/Backend/Pass.hs

+
+

According to styleguide we must add one blank line between top-level definitions.

DK318 · 2022-04-21T22:52:17Z

lib/Effect/Fs.hs

+    Right b -> Right $ T.unpack b
+stringToPath :: String -> Path


dcastro · 2022-04-24T10:14:03Z

Oh and I have no idea why it doesn't build. None at all, it builds on my machine

@MagicRB It's because you edited the coffer.cabal file directly.

We've since added support for hpack + stack, so now we should edit the package.yaml file instead. And then use ./scripts/generate-cabal-files.sh to update coffer.cabal, cabal.project, cabal.project.freeze, etc.

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.

dcastro · 2022-04-24T10:36:45Z

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.

Ok, found the issue, fixed it here: #77

dcastro · 2022-04-24T13:41:19Z

lib/Backend/Pass.hs

+    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


Good idea, that's a good check! 👏

dcastro · 2022-04-24T13:43:32Z

lib/Backend/Pass.hs

+  where tToFPath = Just . T.unpack
+        fPathToT :: Maybe String -> Maybe Text
+        fPathToT a = a <&> T.pack


Keep the indentation to a minimum.

From the styleguide:

Indent the where keyword with 2 spaces and the definitions within the
where clause with 2 more spaces

where tToFPath = Just . T.unpack fPathToT :: Maybe String -> Maybe Text fPathToT a = a <&> T.pack

Please look for other similar cases (e.g. in debugCodec, pbListSecrets, etc)

think i got them all, ill regex it, yup got them all

lib/Backend/Pass.hs

dcastro · 2022-04-24T13:51:27Z

lib/Backend/Pass.hs

+  PassBackend
+  <$> backendNameCodec "name" Toml..= pbName
+  <*> Toml.string "store_dir" Toml..= pbStoreDir
+  <*> Toml.dimatch fPathToT tToFPath (Toml.text "pass_exe") Toml..= pbPassExe


Instead of using Toml.text and then mapping to/contramapping from String, we can just use Toml.string.

dcastro · 2022-04-24T14:56:33Z

lib/Backend/Pass.hs

+              >>= (\case Left e ->
+                           if | isDoesNotExistError e -> pure Nothing
+                              | True -> throw $ OtherError (T.pack $ show e)
+                         Right v -> pure $ Just v)


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.).

dcastro · 2022-04-25T07:52:39Z

lib/Effect/Fs.hs

+pathToString :: Path -> Either FsError String
+pathToString path =
+  case decodeUtf8' path of
+    Left a -> Left $ FEInvalidPath path


Looks like the UnicodeException in the Left should also be included in FEInvalidPath

dcastro · 2022-04-25T07:57:57Z

lib/Effect/Fs.hs

+
+type Node f d = Either (File f) (Directory d)
+type Node' a = Node a a
+type Path = ByteString


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.

dcastro · 2022-04-25T07:59:26Z

lib/Entry/Pass.hs

+  deriving stock (Show)
+makeLensesWith abbreviatedFields ''PassEntry
+
+passFieldPrism :: Prism' PassField (E.FieldKey, E.Field)


Like we talked on Slack, prisms shouldn't really be used for parsing because it's impossible to know why parsing failed.

See comment below.

dcastro · 2022-04-25T08:04:54Z

lib/Entry/Pass.hs

+  deriving stock (Show)
+makeLensesWith abbreviatedFields ''PassField
+
+data PassEntry =


Why are these intermediate data structures needed?

In order to write an Entry to pass, we're converting Entry -> PassEntry -> PassKv -> Text

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.

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).

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

lib/Effect/Fs.hs

MagicRB · 2022-04-25T09:18:58Z

#62 (comment) idk why but i cant seem to reply to this, so doing it here. stringToPath is fine, we're not doing pathToString anywhere, which would be an issue.

dcastro · 2022-04-25T09:54:44Z

lib/Effect/Fs.hs

+
+data FsEffect m a where
+  NodeExists :: Path -> FsEffect m (Maybe (Node' ()))
+  GetNode :: Path -> FsEffect m (Maybe (Node' Path))


_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

Actually, getNode and listDirectoryRec aren't used anywhere, so we should delete them.

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.

dcastro · 2022-04-25T11:48:32Z

lib/Effect/Fs.hs

+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]


Shouldn't ListDirectory return a Maybe, and Nothing when the given path doesn't exist?

dcastro · 2022-04-25T11:49:06Z

lib/Backend/Pass.hs

+  verifyPassStore storeDir
+
+  let fpath = storeDir <> (path & build & fmt)
+  contents <- runError (fromException @IOException $ D.listDirectory fpath)


Shouldn't we be using listDirectory from the FsEffect here?

dcastro · 2022-04-25T20:13:10Z

lib/Backend/Pass.hs

+
+  case exitCode of
+    ExitSuccess ->
+      pure $ T.decodeUtf8 (BS.toStrict stdout) ^? passTextPrism . E.entry


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.

dcastro · 2022-04-25T20:14:39Z

lib/Backend/Pass.hs

+
+  case exitCode of
+    ExitSuccess -> pure ()
+    ExitFailure _e -> throw $ OtherError (T.decodeUtf8 $ BS.toStrict stderr)


_e

We shouldn't swallow the exit code, we should report that as well.

Same thing in pbWriteSecret and pbReadSecret

dcastro · 2022-04-25T20:26:34Z

lib/Backend/Debug.hs

+--
+-- SPDX-License-Identifier: MPL-2.0
+
+module Backend.Debug


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.

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

dcastro · 2022-04-25T20:43:44Z

lib/Effect/Fs.hs

+    -- ReadNode nodePath -> undefined
+    -- CreateNode nodePath -> undefined
+
+_nodeExists


We shouldn't prefix function names with _, it has a special meaning to GHC.

Prefixing with _ disables GHC's "unused definition" warnings/errors.

dcastro · 2022-04-25T20:51:43Z

coffer.cabal


 library
  exposed-modules:
      Backend


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

kills the 2 vault instances

We should modify the script such that:

sets up 2 pass instances

exports COFFER_CONFIG="pass-config.toml"

runs the tests

cleans up the 2 pass instances

spins up 2 vault instances

exports COFFER_CONFIG="vault-config.toml"

runs the tests again

kills the 2 vault instances

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]>

MagicRB changed the title ~~WIP~~ Pass Backend Apr 18, 2022

MagicRB force-pushed the magic_rb/#55-pass-backend branch 4 times, most recently from 6432a1d to 52de877 Compare April 18, 2022 20:21

MagicRB marked this pull request as ready for review April 18, 2022 20:37

MagicRB force-pushed the magic_rb/#55-pass-backend branch from 52de877 to 77286c3 Compare April 18, 2022 20:38

DK318 requested changes Apr 19, 2022

View reviewed changes

MagicRB force-pushed the magic_rb/#55-pass-backend branch from 77286c3 to 304691f Compare April 20, 2022 05:34

MagicRB force-pushed the magic_rb/#55-pass-backend branch from 304691f to da7170b Compare April 20, 2022 05:37

WIP

32e0702

Signed-off-by: Magic_RB <[email protected]>

MagicRB force-pushed the magic_rb/#55-pass-backend branch from da7170b to 32e0702 Compare April 21, 2022 16:36

DK318 requested changes Apr 21, 2022

View reviewed changes

MagicRB mentioned this pull request Apr 22, 2022

[#74] Add proper README and project description #76

Merged

5 tasks

dcastro reviewed Apr 25, 2022

View reviewed changes

dcastro requested changes Apr 25, 2022

View reviewed changes

fixup! WIP

5a6bbeb

Signed-off-by: Magic_RB <[email protected]>

		@@ -1,12 +1,12 @@
		# SPDX-FileCopyrightText: 2022 Serokell <https://serokell.io>
		#

Pass Backend #62

Are you sure you want to change the base?

Pass Backend #62

Uh oh!

Conversation

MagicRB commented Apr 5, 2022

Description

Related issue(s)

✅ Checklist for your Pull Request

Related changes (conditional)

Stylistic guide (mandatory)

Uh oh!

MagicRB commented Apr 18, 2022

Uh oh!

MagicRB commented Apr 18, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MagicRB commented Apr 20, 2022

Uh oh!

DK318 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dcastro commented Apr 24, 2022

Uh oh!

dcastro commented Apr 24, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

MagicRB Apr 25, 2022 •

edited

Loading

dcastro Apr 25, 2022 •

edited

Loading