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

Using hackage security #412

Closed
wants to merge 15 commits into from
Closed

Using hackage security #412

wants to merge 15 commits into from

Conversation

edsko
Copy link
Contributor

@edsko edsko commented Aug 25, 2015

This PR makes a few improvements to the hackage-mirror client, and adds instructions to the README for setting up the security infrastructure.

@edsko
Copy link
Contributor Author

edsko commented Aug 25, 2015

(Travis fails because the server is started without the required security files in place.)

@ocharles
Copy link
Contributor

There is a merge conflict here, fixed in https://github.com/ocharles/hackage-server (master).

@edsko
Copy link
Contributor Author

edsko commented Dec 25, 2015

I will create a new PR that fixes the tests, and then rebase this stuff on that.

@edsko
Copy link
Contributor Author

edsko commented Dec 30, 2015

I've rebased this PR on master. However, THIS IS WIP. I've now added example keys to the server so that people can get a Hackage server going without first having to generate keys etc. However, this now all relies on the latest hackage-security library, which isn't released yet. I'd release it, but it in turn relies on the latest tar library, which @dcoutts hasn't released yet and he's on holidays. The tests will also still fail on this branch, even if it did compile, for an annoying reason which I will fix as well.

In short: working on this, will indicate when it's ready to be merged.

@edsko edsko mentioned this pull request Dec 30, 2015
@edsko
Copy link
Contributor Author

edsko commented Jan 1, 2016

Ok guys. This is done! Tests pass! They won't yet on Travis because it relies on yet-unreleased versions of tar and hackage-security, but once they are released we can restart the Travis job for this PR and it should be green 👊 💥 Happy new year :)

An overview of the commits in this PR:

  • ffa205e is Fix the build #459
  • 6c20397 adds example keys to the repo, along with corresponding TUF files in datafiles/TUF. This means the server can run on Travis and also makes it easier for people to get their own instances up and running.
  • bc2bd8b just updates the code base to use hackage-security-0.5.
  • fdbc895 , 1c44e50 , 8dba414 and c2d6fe2 are changes to the mirror client. These are already in use on the server, where we were using a branch even though it had not been merged yet with HEAD here. (The commits are mostly unchanged, minor changes here and there to allow for the newer hackage-security library.)
  • 16bf051 just adds instructions to the README on how to set up the TUF infrastructure, and explains the situation with the example keys.
  • b507a09 replaces the use of the (slow) SHA package with the (much faster) cryptohash package, just like we have done in the hackage-security. Although the Digest SHA256 type from the SHA package was used as part of the server state, this change does not need a state migration because the the binary instance is fully backwards compatible (this is explained in detail in the commit message).
  • 24feca8 then adds a new field securityLastUpdate into the SecurityState. This does require a state migration, but a simple one: it just sets the securityLastUpdatefield to 'Nothing'. This is a very small commit so that this migration can easily be audited separately.
  • The remainder of the commits (67ab723 , 6247827 and 6f47c2b) then finally make use of this new information to avoid unnecessarily updating the snapshot and timestamp. This means that simply restarting the server, or importing a backup, does not automatically create a new snapshot/timestamp, and that means that the server backup roundtrips now pass.

@edsko
Copy link
Contributor Author

edsko commented Jan 2, 2016

I have a snapshot of the Hackage server state on my local machine (updated yesterday). I intended to run the server (using the same commit as on the live server right now) on this, then update to my PR and test the migration. However, I cannot run the server on my snapshot:

hackage-server: Guessing public URI as http://server.local:8080
(you can override with the --base-uri= flag)
hackage-server: state/db/PackagesState/checkpoints-0000000013.log: invalid
argument

I don't know what this means. I tried asking the server to create a checkpoint and updated my local snapshot after that, but that didn't help either. It's a little worrying that this is not working, makes me wonder about the integrity of the server state on the live server. (I tried running my local server on a Hackage snapshot in the past and it worked back then.)

edsko added 15 commits January 6, 2016 10:58
This should make it easier for people to experiment with the server, and will
also facilitate continous integration.
(rather than using the mirrors from mirrors.json)
In two places the mirror client was using openTempFile and then renaming that
temp file to become a permanent file; use
openBinaryTempFileWithDefaultPermissions instead.
Although some SHA types made it into the server state, swapping these two
packages does not require a DB migration because they are fully binary
compatible (as well as compatible in their base16-encoded representations, of
course):

``` haskell
fromRight :: Either a b -> b
fromRight (Right b) = b

serializeOld :: Old.Digest Old.SHA256State -> BS.Strict.ByteString
serializeNew :: New.SHA256Digest           -> BS.Strict.ByteString

serializeOld = Serialize.encode
serializeNew = Serialize.encode

deserializeOld :: BS.Strict.ByteString -> Old.Digest Old.SHA256State
deserializeNew :: BS.Strict.ByteString -> New.SHA256Digest

deserializeOld = fromRight . Serialize.decode
deserializeNew = fromRight . Serialize.decode

showOld :: Old.Digest Old.SHA256State -> String
showNew :: New.SHA256Digest           -> String

showOld = show
showNew = show

readOld :: String -> Old.Digest Old.SHA256State
readNew :: String -> New.SHA256Digest

readOld = fromRight . readDigest
readNew = fromRight . readDigest

oldToNewSerialized :: Old.Digest Old.SHA256State -> New.SHA256Digest
oldToNewSerialized = undefined

test :: (Show a, Show b, Eq a)
     => (BS.Lazy.ByteString -> a) -> (a -> b) -> (b -> a) -> IO ()
test mk conv convBack = do
    print origHash
    print convHash
    print origHash'
    print $ origHash == origHash'
  where
    origHash  = mk "Input string"
    convHash  = conv origHash
    origHash' = convBack convHash

main :: IO ()
main = do
    -- old -> new -> old, through binary form
    test Old.sha256 (deserializeNew . serializeOld) (deserializeOld . serializeNew)

    -- new -> old -> new, through binary form
    test New.sha256 (deserializeOld . serializeNew) (deserializeNew . serializeOld)

    -- old -> new -> old, through base16 (string) form
    test Old.sha256 (readNew . showOld) (readOld . showNew)

    -- new -> old -> new, through base16 (string) form
    test New.sha256 (readOld . showNew) (readNew . showOld)
```

(This test is no longer in the code base because I've removed the dependency on
SHA completely, seems a bit silly to leave it in just so we can test this.)

This should improve efficiency of hash calculation for packages and for the
index significantly.
This will enable us to reproduce precisely the same TUF files after a server
restart or a backup import, which is essential for the backup roundtrip. This
commit _just_ introduces the new state; I've committed this separately from
actually making using of this state because this bit requires a (simple)
migration.
The SafeCopy instance will be necessary to return a TUFFile from an acid-state
transaction; the simplification is that we removed the expiry time. The latter
was originally intended for cache control, but we don't want to use expiry
times for that, so it's now redundant. Removing it allows for some
simplifications in the code, removes a possible runtime exception (not all
files _have_ an expiry time), and if we now introduce a SafeCopy instances it
makes sense to do it now or else we might have to introduce migrations later.

This is a bit of an annoying change, because `TUFFile` used to have a phantom
type argument indicating what kind of TUFFile it was (this type argument was
instantiated with the corresponding type from the hackage-security library).
Sadly, `deriveSafeCopy` is not very smart and when applied to `TUFFile` it
added a spurious `SafeCopy` requirement on the phantom type argument. To avoid
this problem, in this commit we remove the phantom type argument and instead
introduce a bunch of newtype wrappers.
.. in preparation for doing the TUF update in acid-state.
(unnecessarily, at least). The DB state now has enough information to recreate
the TUF files at any one point. We take advantage of this to make sure that if
the TUF cache is updating itself, we only increment the timestamp/snapshot
version if something actually changed, or the existing files are older than 1
hour.

The cabal tests (including backup roundtrips) now pass! Yay! \o/
The hackage-security library now uses Int54 instead of Int everywhere (for
annoying JSON reasons). In this commit I prove a SafeCopy instance for Int54
which translates to Int, so that we remain backwards binary compatible. I do
change to Int54 in a few datatypes, but only in datatypes that are not (yet)
serialized on the running server.
@edsko
Copy link
Contributor Author

edsko commented Jan 7, 2016

Note that I have not modified the generation of the source distribution; I don't know if we want to make sure that the example TUF files as part of that; possibly we do. I've opened a separate ticket #465 about this.

hvr added a commit that referenced this pull request Jan 10, 2016
@dcoutts
Copy link
Contributor

dcoutts commented Jan 11, 2016

Merged. 765ec00

@dcoutts dcoutts closed this Jan 11, 2016
@dcoutts dcoutts deleted the using-hackage-security branch January 11, 2016 10:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants