Skip to content
This repository was archived by the owner on Aug 18, 2020. It is now read-only.

[CBR-470] Ensure 600 file permission on generated x509 certificates and keys #3773

Merged
merged 2 commits into from
Oct 30, 2018

Conversation

KtorZ
Copy link
Contributor

@KtorZ KtorZ commented Oct 22, 2018

Description

It is of the utmost importance that those files aren't readable by anyone but the user generating them.
Files are generated upon start and used to secure the communication between cardano-sl and its clients (e.g. Daedalus)

Note that, we do assume that users are able to secure their own environment from potential adversary services,
so this is really about preventing one user to have access to certificates of another user.

Linked issue

[CBR-470]

Type of change

  • 🐞 Bug fix (non-breaking change which fixes an issue)
  • 🛠 New feature (non-breaking change which adds functionality)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)
  • 🏭 Refactoring that does not change existing functionality but does improve things like code readability, structure etc
  • 🔨 New or improved tests for existing code
  • ⛑ git-flow chore (backport, hotfix, etc)

Developer checklist

  • I have read the style guide document, and my code follows the code style of this project.
  • If my code deals with exceptions, it follows the guidelines.
  • I have updated any documentation accordingly, if needed. Documentation changes can be reflected in opening a PR on cardanodocs.com, amending the inline Haddock comments, any relevant README file or one of the document listed in the docs directory.
  • CHANGELOG entry has been added and is linked to the correct PR on GitHub.

Testing checklist

  • I have added tests to cover my changes.
  • All new and existing tests passed.

QA Steps

$ stack exec -- cardano-x509-certificates --server-out-dir /tmp/tls --ca-out-dir /tmp/tls --clients-out-dir /tmp/tls -k dev -c lib/configuration.yaml
$ ls -lh /tmp/tls
total 108K
-rw------- 1 ktorz ktorz 1,2K oct.  22 17:20 ca.crt
-rw------- 1 ktorz ktorz 1,7K oct.  22 17:20 ca.key
-rw------- 1 ktorz ktorz 2,9K oct.  22 17:20 ca.pem
-rw------- 1 ktorz ktorz 1,2K oct.  22 17:20 client.crt
-rw------- 1 ktorz ktorz 1,7K oct.  22 17:20 client.key
-rw------- 1 ktorz ktorz 2,9K oct.  22 17:20 client.pem
-rw------- 1 ktorz ktorz 1,3K oct.  22 17:20 server.crt
-rw------- 1 ktorz ktorz 1,7K oct.  22 17:20 server.key
-rw------- 1 ktorz ktorz 3,0K oct.  22 17:20 server.pem

Screenshots (if available)

@KtorZ KtorZ self-assigned this Oct 22, 2018
@KtorZ KtorZ requested a review from disassembler October 22, 2018 15:25
@amias-iohk amias-iohk self-requested a review October 24, 2018 15:53
Copy link
Contributor

@dcoutts dcoutts left a comment

Choose a reason for hiding this comment

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

We can do this portably without the unix library:

I stole this function I wrote years ago for the Cabal lib. It also has the advantage of writing the file atomically.

-- | Writes a file atomically, and with private file permissions.
--
-- The file is either written successfully or an IO exception is raised and
-- the original file is left unchanged.
--
-- On unix systems the file permissions are 600, i.e. user read and write,
-- but no others.
--
-- On windows it is not possible to delete a file that is open by a process.
-- This case will give an IO exception but the atomic property is not affected.
--
writeFileAtomicPrivate :: FilePath -> BS.ByteString -> IO ()
writeFileAtomicPrivate targetPath content = do
  let (targetDir, targetFile) = splitFileName targetPath
  Exception.bracketOnError
    (openBinaryTempFile targetDir $ targetFile <.> "tmp")
    (\(tmpPath, handle) -> hClose handle >> removeFile tmpPath)
    (\(tmpPath, handle) -> do
        BS.hPut handle content
        hClose handle
        renameFile tmpPath targetPath)

…nd keys

It is of the utmost importance that those files aren't readable by anyone but the user generating them.
Files are generated upon start and used to secure the communication between cardano-sl and its clients (e.g. Daedalus)

Note that, we do assume that users are able to secure their own environment from potential adversary services,
so this is really about preventing one user to have access to certificates of another user.
@KtorZ KtorZ force-pushed the KtorZ/CBR-470/ensure-mode-600-x509 branch from 9f6fd9c to 310b94d Compare October 25, 2018 12:53
@KtorZ
Copy link
Contributor Author

KtorZ commented Oct 25, 2018

@dcoutts Went for your nice trick :)
Works fine -- as expected -- on unix.

$ ls -lh tls/
total 108K
-rw------- 1 ktorz ktorz 1,2K oct.  25 14:52 ca.crt
-rw------- 1 ktorz ktorz 1,7K oct.  25 14:52 ca.key
-rw------- 1 ktorz ktorz 2,9K oct.  25 14:52 ca.pem
-rw------- 1 ktorz ktorz 1,2K oct.  25 14:52 client.crt
-rw------- 1 ktorz ktorz 1,7K oct.  25 14:52 client.key
-rw------- 1 ktorz ktorz 2,9K oct.  25 14:52 client.pem
-rw------- 1 ktorz ktorz 1,3K oct.  25 14:52 server.crt
-rw------- 1 ktorz ktorz 1,7K oct.  25 14:52 server.key
-rw------- 1 ktorz ktorz 3,0K oct.  25 14:52 server.pem

Copy link
Contributor

@rvl rvl left a comment

Choose a reason for hiding this comment

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

The normal way this is done is with umask.
Is the 600 file permission a result of using openBinaryTempFile?

@KtorZ
Copy link
Contributor Author

KtorZ commented Oct 25, 2018

Yes. I've double-checked in the source code actually:

https://hackage.haskell.org/package/base-4.12.0.0/docs/src/System.IO.html#openBinaryTempFile

-- | Like 'openTempFile', but opens the file in binary mode. See 'openBinaryFile' for more comments.
openBinaryTempFile :: FilePath -> String -> IO (FilePath, Handle)
openBinaryTempFile tmp_dir template
    = openTempFile' "openBinaryTempFile" tmp_dir template True 0o600

A bit ad-hoc, but if it buys us some extra "security" on windows as well, why not.

@amias-iohk
Copy link
Contributor

This looks like it fixes the permissions to lock them down a bit further.
there is a test failure here but it is known to be broken for reasons not related to this code or build.

lets merge it

Copy link
Contributor

@amias-iohk amias-iohk left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@dcoutts dcoutts left a comment

Choose a reason for hiding this comment

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

LGTM.

@KtorZ KtorZ changed the base branch from develop to release/2.0.0 October 30, 2018 10:43
@KtorZ
Copy link
Contributor Author

KtorZ commented Oct 30, 2018

@rvl

This was an oversight and should have landed in develop before the cut-off.
I've warned QA it won't be in RC1 but should make it in RC2.

May I merge that in the release branch now or should I wait a bit?

@disassembler disassembler merged commit e84ca6c into release/2.0.0 Oct 30, 2018
@KtorZ KtorZ mentioned this pull request Oct 30, 2018
12 tasks
@disassembler disassembler deleted the KtorZ/CBR-470/ensure-mode-600-x509 branch October 30, 2018 14:16
@KtorZ KtorZ mentioned this pull request Jan 4, 2019
12 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants