Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pass Backend #62

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions coffer.cabal
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@ library
Backend
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. spins up 2 vault instances
  2. runs the bats tests
  3. kills the 2 vault instances

We should modify the script such that:

  1. sets up 2 pass instances
  2. exports COFFER_CONFIG="pass-config.toml"
  3. runs the tests
  4. cleans up the 2 pass instances
  5. spins up 2 vault instances
  6. exports COFFER_CONFIG="vault-config.toml"
  7. runs the tests again
  8. kills the 2 vault instances

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"

Backend.Commands
Backend.Interpreter
Backend.Pass
Backend.Debug
Backend.Vault.Kv
Backend.Vault.Kv.Internal
BackendName
Expand All @@ -35,6 +37,8 @@ library
Config
Entry
Entry.Json
Entry.Pass
Effect.Fs
Error
other-modules:
Paths_coffer
Expand Down Expand Up @@ -95,9 +99,12 @@ library
aeson
, ansi-terminal
, base >=4.14.3.0 && <5
, bytestring
, containers
, directory
, extra
, fmt
, filepath
, hashable
, http-client
, http-client-tls
Expand All @@ -117,6 +124,7 @@ library
, tomland
, unordered-containers
, validation-selective
, typed-process
default-language: Haskell2010

executable coffer
Expand Down
12 changes: 6 additions & 6 deletions config.toml
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,11 @@
#
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please, remember to rollback this config when you are done

# SPDX-License-Identifier: MPL-2.0

main_backend = "vault-local"
main_backend = "pass"

[[backend]]
type = "vault-kv"
name = "vault-local"
address = "localhost:8200"
mount = "secret"
token = "<vault token>"
type = "debug"
sub_type = "pass"
name = "pass"
store_dir = "/tmp/pass-store"
pass_exe = "pass"
1 change: 1 addition & 0 deletions flake.nix
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
cabal-install
haskell-language-server
haskellPackages.implicit-hie
stylish-haskell
];
buildInputs = with pkgs;
[ zlib
Expand Down
120 changes: 120 additions & 0 deletions lib/Backend/Debug.hs
Original file line number Diff line number Diff line change
@@ -0,0 +1,120 @@
-- SPDX-FileCopyrightText: 2022 Serokell <https://serokell.io>
--
-- SPDX-License-Identifier: MPL-2.0

module Backend.Debug
Copy link
Member

@dcastro dcastro Apr 25, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

( DebugBackend
, debugCodec
) where

import Backend
import Backends
import Coffer.Path
import Control.Lens
import Data.HashMap.Lazy qualified as HS
import Data.Text (Text)
import Data.Text qualified as T
import Entry (Entry)
import Polysemy
import Toml (TomlCodec, TomlEnv)
import Toml qualified
import Validation (Validation(Failure, Success))

data DebugBackend =
DebugBackend
{ dSubType :: Text
, dSubBackend :: SomeBackend
}
deriving stock (Show)

debugCodec :: TomlCodec DebugBackend
debugCodec = Toml.Codec input output
where input :: TomlEnv DebugBackend
input toml = case HS.lookup "sub_type" $ Toml.tomlPairs toml of
Just x ->
case Toml.backward Toml._Text x of
Right t ->
case supportedBackends t of
Right y ->
let newToml = toml { Toml.tomlPairs =
Toml.tomlPairs toml
& HS.delete "sub_type"
}
in
case y newToml of
Success b -> Success $ DebugBackend
{ dSubType = t
, dSubBackend = b
}
Failure e -> Failure e
Left e ->
Failure
[ Toml.BiMapError "type" e
]
Left e ->
Failure
[ Toml.BiMapError "type" e
]
Nothing ->
Failure
[ Toml.BiMapError "sub_type" $
Toml.ArbitraryError
"Debug backend doesn't have a `sub_type` key"
]
output :: DebugBackend -> Toml.TomlState DebugBackend
output debugBackend =
case dSubBackend debugBackend of
SomeBackend (be :: a) -> do
Toml.codecWrite (Toml.text "type") "debug"
Toml.codecWrite (Toml.text "sub_type") (dSubType debugBackend)
Toml.codecWrite (_codec @a) be
pure debugBackend

dbWriteSecret
:: Effects r => DebugBackend -> Entry -> Sem r ()
dbWriteSecret b entry = unSubBackend b $ \(SomeBackend backend) -> do
embed $ putStrLn ("WriteSecret: \n" <> show entry)
_writeSecret backend entry

dbReadSecret
:: Effects r => DebugBackend -> EntryPath -> Sem r (Maybe Entry)
dbReadSecret b path = unSubBackend b $ \(SomeBackend backend) -> do
embed $ putStrLn ("ReadSecret: " <> show path)
_readSecret backend path >>= showPass "out: "

dbListSecrets
:: Effects r => DebugBackend -> Path -> Sem r (Maybe [Text])
dbListSecrets b path = unSubBackend b $ \(SomeBackend backend) -> do
embed $ putStrLn ("ListSecrets: " <> show path)
_listSecrets backend path >>= showPass "out: "

dbDeleteSecret
:: Effects r => DebugBackend -> EntryPath -> Sem r ()
dbDeleteSecret b path = unSubBackend b $ \(SomeBackend backend) -> do
embed $ putStrLn ("DeleteSecret: " <> show path)
_deleteSecret backend path

unSubBackend
:: DebugBackend
-> (SomeBackend -> a)
-> a
unSubBackend b f = f (dSubBackend b)

showPass
:: ( Member (Embed IO) r
, Show a
)
=> Text -> a -> Sem r a
showPass txt a = do
let atxt = T.pack $ show a
embed $ putStrLn (T.unpack $ txt <> atxt)
pure a


instance Backend DebugBackend where
_name debugBackend = (\(SomeBackend x) -> _name x) $ dSubBackend debugBackend
_codec = debugCodec
_writeSecret = dbWriteSecret
_readSecret = dbReadSecret
_listSecrets = dbListSecrets
_deleteSecret = dbDeleteSecret
170 changes: 170 additions & 0 deletions lib/Backend/Pass.hs
Original file line number Diff line number Diff line change
@@ -0,0 +1,170 @@
-- SPDX-FileCopyrightText: 2022 Serokell <https://serokell.io>
--
-- SPDX-License-Identifier: MPL-2.0

module Backend.Pass
( PassBackend ) where
import Backend
import BackendName
import Coffer.Path
import Coffer.Path qualified as P
import Control.Exception (IOException)
import Control.Lens
import Data.ByteString.Lazy (ByteString)
import Data.ByteString.Lazy qualified as BS
import Data.Maybe
import Data.Text (Text)
import Data.Text qualified as T
import Data.Text.Encoding (encodeUtf8)
import Data.Text.Encoding qualified as T
import Effect.Fs
import Entry (Entry)
import Entry qualified as E
import Entry.Pass
import Error
import Fmt (build, fmt)
import Polysemy
import Polysemy.Error
import System.Directory qualified as D
import System.FilePath (makeRelative)
import System.IO.Error (isDoesNotExistError)
import System.Process.Typed
import Toml (TomlCodec)
import Toml qualified

data PassBackend =
PassBackend
{ pbName :: BackendName
, pbStoreDir :: FilePath
, pbPassExe :: Maybe FilePath
}
deriving stock (Show)

passCodec :: TomlCodec PassBackend
passCodec =
PassBackend
<$> backendNameCodec "name" Toml..= pbName
<*> Toml.string "store_dir" Toml..= pbStoreDir
<*> Toml.dimatch fPathToT tToFPath (Toml.text "pass_exe") Toml..= pbPassExe
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

resolved

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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



Comment on lines +49 to +50
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

verifyPassStore
:: Member (Error CofferError) r
=> Member (Embed IO) r
=> FilePath
-> Sem r ()
verifyPassStore storeDir =
res >>= \case
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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea, that's a good check! 👏

where
res = runError @FsError . runFsInIO $ do
nodeExists (stringToPath $ storeDir <> "/.gpg-id")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We shouldn't concatenate strings with <> to construct filepaths.

If the user-supplied storeDir happens to end with a /, e.g. /tmp/pass-store/, you'll get a double slash, e.g. /tmp/pass-store//.gpg-id.

You can use the </> operator which takes care of this for you and adds a backslash \ instead when running on Windows.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

resolved



wrapper
:: Effects r
=> PassBackend
-> [String]
-> Maybe (StreamSpec 'STInput ())
-> Sem r (ExitCode, ByteString, ByteString)
wrapper backend args input = do
let passExe = pbPassExe backend
let storeDir = pbStoreDir backend
verifyPassStore storeDir

proc (fromMaybe "pass" passExe) args
& case input of
Just a -> setStdin a
Nothing -> setStdin nullStream
& setEnv [("PASSWORD_STORE_DIR", storeDir)]
& readProcess



pbWriteSecret
:: Effects r => PassBackend -> Entry -> Sem r ()
pbWriteSecret backend entry = do
let input =
entry ^. re E.entry . re passTextPrism
& encodeUtf8
& BS.fromStrict

(exitCode, _stdout, stderr) <-
wrapper
backend
[ "insert"
, "-mf"
, entry ^. E.path & P.entryPathAsPath & build & fmt
]
(Just $ byteStringInput input)

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


pbReadSecret
:: Effects r => PassBackend -> EntryPath -> Sem r (Maybe Entry)
pbReadSecret backend path = do
(exitCode, stdout, stderr) <-
wrapper
backend
[ "show"
, path & P.entryPathAsPath & build & fmt
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to convert EntryPath to Path

Suggested change
, path & P.entryPathAsPath & build & fmt
, pretty path

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think i got all of them, resolved

]
Nothing

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

@dcastro dcastro Apr 25, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

ExitFailure 1 ->
pure Nothing
ExitFailure _e ->
throw $ OtherError (T.decodeUtf8 $ BS.toStrict stderr)

pbListSecrets
:: Effects r => PassBackend -> Path -> Sem r (Maybe [Text])
pbListSecrets backend path = do
let storeDir = pbStoreDir backend
verifyPassStore storeDir

let fpath = storeDir <> (path & build & fmt)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fmt . build = pretty

Suggested change
let fpath = storeDir <> (path & build & fmt)
let fpath = storeDir <> pretty path

I see you're using fmt . build in a few more places, please have a look at those as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

resolved i think

contents <- runError (fromException @IOException $ D.listDirectory fpath)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

>>= (\case Left e ->
if | isDoesNotExistError e -> pure Nothing
| True -> throw $ OtherError (T.pack $ show e)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use otherwise instead of True.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm getting conflicted recommendations, i read that on should use True not otherwise. Why one or the other?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's more semantic to use otherwise in guards

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i read that on should use True not otherwise

Where did you read that? 😮 Genuinely curious, I've never seen anyone prefer True over otherwise

I think it's more semantic to use otherwise in guards

Yeah

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

something something rebinding the otherwise from prelude i think?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need for MultiWayIf here.

You can have patterns with guards:

Suggested change
>>= (\case Left e ->
if | isDoesNotExistError e -> pure Nothing
| True -> throw $ OtherError (T.pack $ show e)
>>= (\case Left e
| isDoesNotExistError e -> pure Nothing
| True -> throw $ OtherError (T.pack $ show e)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

resolved

Right v -> pure $ Just v)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

<&> \a -> a <&> map (makeRelative fpath)

pure $ contents <&> map (T.dropEnd 4 . T.pack)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can simplify some of this.

Instead of:

  • wrapping all the filepaths in pure . Just (giving us a Sem r (Maybe [FilePath]))
  • and then later running <&> \a -> a <&> map to get at those FilePathss

We can just:

  • apply all transformations directly to [FilePath]
  • and then wrapping the result in pure . Just
Suggested change
contents <- runError (fromException @IOException $ D.listDirectory fpath)
>>= (\case Left e ->
if | isDoesNotExistError e -> pure Nothing
| True -> throw $ OtherError (T.pack $ show e)
Right v -> pure $ Just v)
<&> \a -> a <&> map (makeRelative fpath)
pure $ contents <&> map (T.dropEnd 4 . T.pack)
runError (fromException @IOException $ D.listDirectory fpath) >>= \case
Left e
| isDoesNotExistError e -> pure Nothing
| True -> throw $ OtherError $ T.pack $ show e
Right filepaths -> do
pure $ Just $ filepaths
<&> makeRelative fpath
<&> T.pack
<&> T.dropEnd 4

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

resolved


pbDeleteSecret
:: Effects r => PassBackend -> EntryPath -> Sem r ()
pbDeleteSecret backend path = do
(exitCode, _stdout, stderr) <-
wrapper
backend
[ "rm"
, "-f"
, path & P.entryPathAsPath & build & fmt
]
Nothing

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_e

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

Same thing in pbWriteSecret and pbReadSecret



instance Backend PassBackend where
_name kvBackend = pbName kvBackend
_codec = passCodec
_writeSecret = pbWriteSecret
_readSecret = pbReadSecret
_listSecrets = pbListSecrets
_deleteSecret = pbDeleteSecret
24 changes: 2 additions & 22 deletions lib/Backends.hs
Original file line number Diff line number Diff line change
Expand Up @@ -4,36 +4,16 @@

module Backends
( supportedBackends
, backendPackedCodec
) where

import Backend (Backend(..), SomeBackend(..))
import Backend.Pass
import Backend.Vault.Kv (VaultKvBackend)
import Data.HashMap.Strict qualified as HS
import Data.Text (Text)
import Toml (TomlCodec)
import Toml qualified
import Validation (Validation(Failure))

backendPackedCodec :: TomlCodec SomeBackend
backendPackedCodec = Toml.Codec input output
where
input :: Toml.TomlEnv SomeBackend
input toml =
case HS.lookup "type" $ Toml.tomlPairs toml of
Just t -> do
case Toml.backward Toml._Text t >>= supportedBackends of
Right c -> c toml
Left e -> Failure [ Toml.BiMapError "type" e ]
Nothing -> Failure
[ Toml.BiMapError "type" $ Toml.ArbitraryError
"Backend doesn't have a `type` key"
]
output (SomeBackend a) = do
SomeBackend <$> Toml.codecWrite _codec a
<* Toml.codecWrite (Toml.text "type") "vault"

supportedBackends
:: Text -> Either Toml.TomlBiMapError (Toml.TomlEnv SomeBackend)
supportedBackends "vault-kv" = Right $ fmap SomeBackend . Toml.codecRead (_codec @VaultKvBackend)
supportedBackends "pass" = Right $ fmap SomeBackend . Toml.codecRead (_codec @PassBackend)
supportedBackends _ = Left (Toml.ArbitraryError "Unknown backend type")
Loading