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

[fix] some minor fixes #201

Merged
merged 6 commits into from
Jun 10, 2024
Merged
Show file tree
Hide file tree
Changes from 3 commits
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
2 changes: 2 additions & 0 deletions cabal.project
Original file line number Diff line number Diff line change
Expand Up @@ -4,3 +4,5 @@ package hsec-core
package hsec-tools
package cvss
package osv

test-show-details: direct
30 changes: 13 additions & 17 deletions code/hsec-tools/app/Main.hs
Original file line number Diff line number Diff line change
@@ -1,13 +1,14 @@
{-# LANGUAGE LambdaCase #-}
{-# LANGUAGE OverloadedStrings #-}
{-# LANGUAGE BlockArguments #-}

module Main where

import Control.Monad (forM_, join, void, when)
import Control.Monad.Trans.Except (runExceptT, ExceptT (ExceptT), withExceptT, throwE)
import Control.Monad.IO.Class (liftIO)
import qualified Data.ByteString.Lazy as L
import Data.Maybe (fromMaybe)
import Data.Foldable (for_)
import Data.Functor ((<&>))
import Data.List (intercalate, isPrefixOf)
import Distribution.Parsec (eitherParsec)
import Distribution.Types.VersionRange (VersionRange, anyVersion)
Expand All @@ -19,6 +20,7 @@ import Validation (Validation(..))
import qualified Data.Aeson
import qualified Data.Text as T
import qualified Data.Text.IO as T
import Control.Exception (Exception(displayException))
import Options.Applicative

import Security.Advisories
Expand Down Expand Up @@ -153,24 +155,18 @@ withAdvisory :: (Maybe FilePath -> Advisory -> IO ()) -> Maybe FilePath -> IO ()
withAdvisory go file = do
input <- maybe T.getContents T.readFile file

oob <- ($ emptyOutOfBandAttributes) <$> case file of
Nothing -> pure id
Just path ->
getAdvisoryGitInfo path <&> \case
Left _ -> id
Right gitInfo -> \oob -> oob
{ oobPublished = Just (firstAppearanceCommitDate gitInfo)
, oobModified = Just (lastModificationCommitDate gitInfo)
}
oob <- runExceptT case file of
Nothing -> throwE StdInHasNoOOB
Just path -> withExceptT GitHasNoOOB do
gitInfo <- ExceptT $ liftIO $ getAdvisoryGitInfo path
blackheaven marked this conversation as resolved.
Show resolved Hide resolved
pure OutOfBandAttributes
{ oobPublished = firstAppearanceCommitDate gitInfo
, oobModified = lastModificationCommitDate gitInfo
}

case parseAdvisory NoOverrides oob input of
Left e -> do
T.hPutStrLn stderr $
case e of
MarkdownError _ explanation -> "Markdown parsing error:\n" <> explanation
MarkdownFormatError explanation -> "Markdown structure error:\n" <> explanation
TomlError _ explanation -> "Couldn't parse front matter as TOML:\n" <> explanation
AdvisoryError _ explanation -> "Advisory structure error:\n" <> explanation
hPutStrLn stderr (displayException e)
exitFailure
Right advisory -> do
go file advisory
Expand Down
3 changes: 2 additions & 1 deletion code/hsec-tools/hsec-tools.cabal
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
cabal-version: 2.4
name: hsec-tools
version: 0.1.0.0
version: 0.2.0.0

-- A short (one-line) description of the package.
synopsis:
Expand Down Expand Up @@ -90,6 +90,7 @@ executable hsec-tools
, hsec-tools
, optparse-applicative >=0.17 && <0.19
, text >=1.2 && <3
, transformers
, validation-selective >=0.1 && <1

hs-source-dirs: app
Expand Down
29 changes: 14 additions & 15 deletions code/hsec-tools/src/Security/Advisories/Filesystem.hs
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
{-# LANGUAGE LambdaCase #-}

{-# LANGUAGE BlockArguments #-}
{-|

Helpers for the /security-advisories/ file system.
Expand All @@ -25,9 +24,7 @@ module Security.Advisories.Filesystem
) where

import Control.Applicative (liftA2)
import Data.Bifunctor (bimap)
import Data.Foldable (fold)
import Data.Functor ((<&>))
import Data.Semigroup (Max(Max, getMax))
import Data.Traversable (for)

Expand All @@ -37,11 +34,13 @@ import qualified Data.Text.IO as T
import System.FilePath ((</>), takeBaseName)
import System.Directory (doesDirectoryExist, pathIsSymbolicLink)
import System.Directory.PathWalk
import Validation (Validation, eitherToValidation)
import Validation (Validation (..))

import Security.Advisories (Advisory, AttributeOverridePolicy (NoOverrides), OutOfBandAttributes (..), ParseAdvisoryError, emptyOutOfBandAttributes, parseAdvisory)
import Security.Advisories (Advisory, AttributeOverridePolicy (NoOverrides), OutOfBandAttributes (..), ParseAdvisoryError, parseAdvisory)
import Security.Advisories.Core.HsecId (HsecId, parseHsecId, placeholder)
import Security.Advisories.Git(firstAppearanceCommitDate, getAdvisoryGitInfo, lastModificationCommitDate)
import Control.Monad.Except (runExceptT, ExceptT (ExceptT), withExceptT)
import Security.Advisories.Parse (OOBError(GitHasNoOOB))


dirNameAdvisories :: FilePath
Expand Down Expand Up @@ -128,16 +127,16 @@ listAdvisories root =
if isSym
then return $ pure []
else do
oob <-
liftIO (getAdvisoryGitInfo advisoryPath) <&> \case
Left _ -> emptyOutOfBandAttributes
Right gitInfo ->
emptyOutOfBandAttributes
{ oobPublished = Just (firstAppearanceCommitDate gitInfo),
oobModified = Just (lastModificationCommitDate gitInfo)
}
oob <- runExceptT $ withExceptT GitHasNoOOB do
gitInfo <- ExceptT $ liftIO $ getAdvisoryGitInfo advisoryPath
pure OutOfBandAttributes
{ oobPublished = firstAppearanceCommitDate gitInfo
, oobModified = lastModificationCommitDate gitInfo
}
fileContent <- liftIO $ T.readFile advisoryPath
return $ eitherToValidation $ bimap return return $ parseAdvisory NoOverrides oob fileContent
pure
$ either (Failure . (: [])) (Success . (: []))
$ parseAdvisory NoOverrides oob fileContent

-- | Get names (not paths) of subdirectories of the given directory
-- (one level). There's no monoidal, interruptible variant of
Expand Down
11 changes: 8 additions & 3 deletions code/hsec-tools/src/Security/Advisories/Git.hs
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
{-# LANGUAGE LambdaCase #-}
{-# LANGUAGE DerivingStrategies #-}

{-|

Expand All @@ -18,11 +19,12 @@ module Security.Advisories.Git

import Data.Char (isSpace)
import Data.List (dropWhileEnd)
import Data.Time (ZonedTime)
import Data.Time (ZonedTime, utcToZonedTime, utc)
import Data.Time.Format.ISO8601 (iso8601ParseM)
import System.Exit (ExitCode(ExitSuccess))
import System.FilePath (splitFileName)
import System.Process (readProcessWithExitCode)
import Control.Applicative ((<|>))

data AdvisoryGitInfo = AdvisoryGitInfo
{ firstAppearanceCommitDate :: ZonedTime
Expand All @@ -32,7 +34,7 @@ data AdvisoryGitInfo = AdvisoryGitInfo
data GitError
= GitProcessError ExitCode String String -- ^ exit code, stdout and stderr
| GitTimeParseError String -- ^ unable to parse this input as a datetime
deriving (Show)
deriving stock (Eq, Ord, Show)

explainGitError :: GitError -> String
explainGitError = \case
Expand Down Expand Up @@ -117,4 +119,7 @@ getAdvisoryGitInfo path = do
-- the same as `ExitFailure`
pure . Left $ GitProcessError status stdout stderr
where
parseTime s = maybe (Left $ GitTimeParseError s) Right $ iso8601ParseM s
parseTime :: String -> Either GitError ZonedTime
parseTime s = maybe (Left $ GitTimeParseError s) Right $
iso8601ParseM s
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should force the format at git level: https://stackoverflow.com/a/7651782

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 tried to pass --date=local and it didn't work. This is in general not really ideal, we shouldn't depend on the git installation if git itself doesn't guarantee a stable interface to these things and/ or this is not documented.

There's also haskell/time#257 which I don't know whether it's the case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://github.com/MangoIV/cabal-audit/actions/runs/9321254777/job/25659857545

here's a run. the build step is Test git which uses the date format local and it still outputs a non-zoned iso8601. I wouldn't have done the separate parse try without trying that first.

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 assume (read "guess") that it is valid to output a non-zoned iso8601 even with local if the computer is itself in UTC which is why I'm not sure whether parsing an unzoned time into a UTCTime should just work.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's do this, it seems to be the lesser bad solution we have.

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 confused, setting it in git is a non-solution because it doesn't work. We have to do it like this, no?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm confused, I thought that, requiring a ZonedTime from git wasn't working, requiring a UTCTime doesn't work either?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh but then we always lose zone information, what does that improve? should we do #203 then?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@frasertweedale was talking of a flag few minutes ago (at ZuriHac), maybe we can have a try

<|> utcToZonedTime utc <$> iso8601ParseM s
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure it does preserve the date

Copy link
Contributor Author

@MangoIV MangoIV Jun 7, 2024

Choose a reason for hiding this comment

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

what do you mean?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I just validated that the osv files are not changed by this PR with:

mkdir osvs
for i in $(find advisories/ -name "*.md" | grep -v reserved); do cabal run exe:hsec-tools -- osv $i > osvs/$(basename $i); done

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's not that obvious, we get a time and day without time zone, assuming UTC seems audacious.

IMO, we should rely only on UTCTime, enforcing it in the git command.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh I see, yes I'm on UTC. Then I remove my approval. Perhaps we could just parse an epoch timestamp if possible?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hsec-tools already does this several times (assuming UTC when there's not enough information) That's why I opened the issue about removing ZonedTime from hsec-tools. It makes stuff harder as well. It is however the correct time to assume, it's not wrong, it's just not semantically as nice. See #203 cc @frasertweedale who commented on #203

82 changes: 58 additions & 24 deletions code/hsec-tools/src/Security/Advisories/Parse.hs
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,16 @@
{-# LANGUAGE DeriveGeneric #-}
{-# LANGUAGE DerivingVia #-}
{-# LANGUAGE GeneralizedNewtypeDeriving #-}
{-# LANGUAGE LambdaCase #-}
{-# LANGUAGE MultiParamTypeClasses #-}
{-# LANGUAGE OverloadedStrings #-}
{-# OPTIONS_GHC -Wno-orphans #-}
module Security.Advisories.Parse
( parseAdvisory
, OOB
, OOBError (..)
, OutOfBandAttributes(..)
, emptyOutOfBandAttributes
, displayOOBError
, AttributeOverridePolicy(..)
, ParseAdvisoryError(..)
)
Expand Down Expand Up @@ -47,6 +50,14 @@ import Security.Advisories.Core.HsecId
import Security.Advisories.Core.Advisory
import Security.OSV (Reference(..), ReferenceType, referenceTypes)
import qualified Security.CVSS as CVSS
import Control.Exception (Exception(displayException))
import Security.Advisories.Git (GitError, explainGitError)

-- | if there are no out of band attributes, attach a reason why that's the case
--
-- @since 0.2.0.0
type OOB = Either OOBError OutOfBandAttributes

-- | A source of attributes supplied out of band from the advisory
-- content. Values provided out of band are treated according to
-- the 'AttributeOverridePolicy'.
Expand All @@ -56,17 +67,11 @@ import qualified Security.CVSS as CVSS
-- set particular fields.
--
data OutOfBandAttributes = OutOfBandAttributes
{ oobModified :: Maybe ZonedTime
, oobPublished :: Maybe ZonedTime
{ oobModified :: ZonedTime
, oobPublished :: ZonedTime
}
deriving (Show)

emptyOutOfBandAttributes :: OutOfBandAttributes
emptyOutOfBandAttributes = OutOfBandAttributes
{ oobModified = Nothing
, oobPublished = Nothing
}

data AttributeOverridePolicy
= PreferInBand
| PreferOutOfBand
Expand All @@ -80,12 +85,34 @@ data ParseAdvisoryError
| AdvisoryError [Toml.MatchMessage Toml.Position] T.Text
deriving stock (Eq, Show, Generic)

-- | @since 0.2.0.0
instance Exception ParseAdvisoryError where
displayException = T.unpack . \case
MarkdownError _ explanation -> "Markdown parsing error:\n" <> explanation
MarkdownFormatError explanation -> "Markdown structure error:\n" <> explanation
TomlError _ explanation -> "Couldn't parse front matter as TOML:\n" <> explanation
AdvisoryError _ explanation -> "Advisory structure error:\n" <> explanation

-- | errors that may occur while ingesting oob data
--
-- @since 0.2.0.0
data OOBError
= StdInHasNoOOB -- ^ we obtain the advisory via stdin and can hence not parse git history
| GitHasNoOOB GitError -- ^ processing oob info via git failed
deriving stock (Eq, Show, Generic)

displayOOBError :: OOBError -> String
displayOOBError = \case
StdInHasNoOOB -> "stdin doesn't provide out of band information"
GitHasNoOOB gitErr -> "no out of band information obtained with git error:\n"
<> explainGitError gitErr

-- | The main parsing function. 'OutOfBandAttributes' are handled
-- according to the 'AttributeOverridePolicy'.
--
parseAdvisory
:: AttributeOverridePolicy
-> OutOfBandAttributes
-> OOB
-> T.Text -- ^ input (CommonMark with TOML header)
-> Either ParseAdvisoryError Advisory
parseAdvisory policy attrs raw = do
Expand Down Expand Up @@ -145,7 +172,7 @@ parseAdvisory policy attrs raw = do
mkPretty ctr pretty x = ctr x $ pretty x

parseAdvisoryTable
:: OutOfBandAttributes
:: OOB
-> AttributeOverridePolicy
-> Pandoc -- ^ parsed document (without frontmatter)
-> T.Text -- ^ summary
Expand All @@ -158,13 +185,14 @@ parseAdvisoryTable oob policy doc summary details html tab =
do fm <- Toml.fromValue (Toml.Table' Toml.startPos tab)
published <-
mergeOobMandatory policy
(oobPublished oob)
(oobPublished <$> oob)
displayOOBError
"advisory.date"
(amdPublished (frontMatterAdvisory fm))
modified <-
fromMaybe published <$>
mergeOobOptional policy
(oobPublished oob)
(oobPublished <$> oob)
"advisory.modified"
(amdModified (frontMatterAdvisory fm))
pure Advisory
Expand Down Expand Up @@ -520,41 +548,47 @@ instance Toml.ToValue CVSS.CVSS where
mergeOob
:: MonadFail m
=> AttributeOverridePolicy
-> Maybe a -- ^ out-of-band value
-> Either e a -- ^ out-of-band value
-> String -- ^ key
-> Maybe a -- ^ in-band-value
-> m b -- ^ when key and out-of-band value absent
-> (e -> m b) -- ^ when key and out-of-band value absent
-> (a -> m b) -- ^ when value present
-> m b
mergeOob policy oob k ib absent present = do
case (oob, ib) of
(Just l, Just r) -> case policy of
(Right l, Just r) -> case policy of
NoOverrides -> fail ("illegal out of band override: " ++ k)
PreferOutOfBand -> present l
PreferInBand -> present r
(Just a, Nothing) -> present a
(Nothing, Just a) -> present a
(Nothing, Nothing) -> absent
(Right a, Nothing) -> present a
(Left _, Just a) -> present a
(Left e, Nothing) -> absent e

mergeOobOptional
:: MonadFail m
=> AttributeOverridePolicy
-> Maybe a -- ^ out-of-band value
-> Either e a -- ^ out-of-band value
-> String -- ^ key
-> Maybe a -- ^ in-band-value
-> m (Maybe a)
mergeOobOptional policy oob k ib =
mergeOob policy oob k ib (pure Nothing) (pure . Just)
mergeOob policy oob k ib (const $ pure Nothing) (pure . Just)

mergeOobMandatory
:: MonadFail m
=> AttributeOverridePolicy
-> Maybe a -- ^ out-of-band value
-> Either e a -- ^ out-of-band value
-> (e -> String) -- ^ how to display information about a missing out of band value
-> String -- ^ key
-> Maybe a -- ^ in-band value
-> m a
mergeOobMandatory policy oob k ib =
mergeOob policy oob k ib (fail ("missing mandatory key: " ++ k)) pure
mergeOobMandatory policy eoob doob k ib =
mergeOob policy eoob k ib everythingFailed pure
where
everythingFailed e = fail $ unlines
[ "while trying to lookup mandatory key " <> show k <> ":"
, doob e
]

-- | A solution to an awkward problem: how to delete the TOML
-- block. We parse into this type to get the source range of
Expand Down
13 changes: 6 additions & 7 deletions code/hsec-tools/test/Spec.hs
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import qualified Data.Text.Lazy.Encoding as LText
import Data.Time.Calendar.OrdinalDate (fromOrdinalDate)
import Data.Time.LocalTime
import System.Directory (listDirectory)
import Test.Tasty
import Test.Tasty (defaultMain, testGroup, TestTree)
import Test.Tasty.Golden (goldenVsString)
import Text.Pretty.Simple (pShowNoColor)

Expand Down Expand Up @@ -42,12 +42,11 @@ doGoldenTest fp = goldenVsString fp (fp <> ".golden") (LText.encodeUtf8 <$> doCh
doCheck = do
input <- T.readFile fp
let fakeDate = ZonedTime (LocalTime (fromOrdinalDate 1970 0) midnight) utc
attr =
emptyOutOfBandAttributes
{ oobPublished = Just fakeDate
, oobModified = Just fakeDate
}
res = parseAdvisory NoOverrides attr input
attr = OutOfBandAttributes
{ oobPublished = fakeDate
, oobModified = fakeDate
}
res = parseAdvisory NoOverrides (Right attr) input
osvExport = case res of
Right adv ->
let osv = OSV.convert adv
Expand Down