A secret vault storage with AES encryption using a certificate #56

Skarlso · 2018-07-24T10:56:10Z

Deals with #51.

TODO

The certificate needs to be obtained and used
Front-end work
- Create a view under settings which let's the admin add key/value pairs and then save it
Write Test for vault handler
Write documentation for the Vault

codecov-io · 2018-07-24T11:02:20Z

Codecov Report

Merging #56 into master will increase coverage by 2.66%.
The diff coverage is 71.5%.

@@            Coverage Diff             @@
##           master      #56      +/-   ##
==========================================
+ Coverage   53.61%   56.27%   +2.66%     
==========================================
  Files          14       16       +2     
  Lines        1218     1418     +200     
==========================================
+ Hits          653      798     +145     
- Misses        496      527      +31     
- Partials       69       93      +24

Impacted Files	Coverage Δ
handlers/handler.go	`84.5% <100%> (+0.92%)`	⬆️
handlers/vault.go	`59.37% <59.37%> (ø)`
security/vault.go	`76.51% <76.51%> (ø)`
security/ca.go	`76.23% <0%> (+1.98%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3d8da40...5b28828. Read the comment docs.

Skarlso · 2018-07-24T11:21:50Z

security/vault_test.go

+	}
+	defer os.Remove(vaultName)
+	v.Cert = []byte("test")
+	v.OpenVault()


This isn't needed here.

Skarlso · 2018-07-24T20:22:05Z

@michelvocks The view of the vault. :)

Skarlso · 2018-07-25T05:26:27Z

security/vault.go

+}
+
+// OpenVault decrypts the contents of the vault and fills up a map of data to work with.
+func (v *Vault) OpenVault() error {


Maybe call this LoadVault or GetVaultContent because it feels weird to use it in the handler.

Skarlso · 2018-07-25T05:27:04Z

security/vault.go

+}
+
+// CloseVault encrypts data passed to the vault in a k/v format and saves it to the vault file.
+func (v *Vault) CloseVault() error {


This definitely shloud be called SaveVault rather.

Skarlso · 2018-07-25T05:32:09Z

handlers/vault.go

+		gaia.Cfg.Logger.Error("error initializing vault", "error", err.Error())
+		return c.String(http.StatusInternalServerError, err.Error())
+	}
+	v.Add(s.Key, []byte(s.Value))


I'm missing an open here so this is overwriting whatever is in the vault at the moment. Which brings me to rename the functions to make it more apparent how this needs to be used.

Skarlso · 2018-07-25T05:41:18Z

I need to write docs for the vault.

Skarlso · 2018-07-25T05:58:12Z

Working secret list.

michelvocks · 2018-07-25T09:10:46Z

Is the value shown as plain text? 😧
I think we should hide that.

Skarlso · 2018-07-25T13:12:30Z

@michelvocks for sure. It's for testing purposes only. :)

Skarlso · 2018-07-25T19:43:51Z

@michelvocks masked :)

…d user. Also fixed the possibility to have an empty vault.

Skarlso · 2018-07-25T21:37:51Z

Dooooooneee!

Vault empty:

Vault with secret:

Vault editing secret:

Vault deleting secret:

Skarlso · 2018-07-26T04:58:55Z

Ah, I need to write docs for this.

michelvocks

Good Job @Skarlso 🤗 Just a few remarks with things I do not understand 🤣

michelvocks · 2018-07-26T04:43:12Z

frontend/client/store/modules/menu/index.js

@@ -29,6 +29,14 @@ const state = {
        icon: 'fa-cogs'
      },
      component: lazyLoading('settings', true)
+    },


Move vault up so it's over Settings in the menu list. Don't know why, but I'm used to find Settings at the last menu entry 🤗

michelvocks · 2018-07-26T04:50:59Z

frontend/client/views/vault/index.vue

+    },
+
+    changeSecret () {
+      // pre-validate


it might make sense to put the validation steps into another method so we can avoid some lines in both changeSecret and addSecret 😄

@michelvocks I suck at Javascript. How do I do that? :D

michelvocks · 2018-07-26T04:52:56Z

frontend/client/views/vault/index.vue

+            message: 'Secret has been successful changed.',
+            type: 'success'
+          })
+          this.selectSecret.newvalue = ''


you call close below which sets selectSecret to {}. Is this still needed?

didn't know that, thanks! :)

michelvocks · 2018-07-26T04:53:49Z

frontend/client/views/vault/index.vue

+            message: 'Secret has been successfully added.',
+            type: 'success'
+          })
+          this.selectSecret.value = null


Same here. You call below close which already sets selectSecret

michelvocks · 2018-07-26T04:58:03Z

handlers/vault.go

+	s := new(updateSecret)
+	err := c.Bind(s)
+	if err != nil {
+		gaia.Cfg.Logger.Error("error reading secret", "error", err.Error())


This might get spammy in the logs when a user or another client is bored 😄. To be consistent with all other handlers, I think we should not log here.

michelvocks · 2018-07-26T05:01:20Z

handlers/vault.go

+		return c.String(http.StatusInternalServerError, err.Error())
+	}
+	gaia.Cfg.Logger.Info("secret successfully added")
+	return c.String(http.StatusOK, "secret successfully added")


To be honest, I'm not really good at HTTP Status codes but this should be Created, right? 😆

Ops, absolutely.

michelvocks · 2018-07-26T05:04:35Z

security/vault.go

+	}
+
+	// Setting up certificate key content
+	c, err := InitCA()


Don't create the CA here. It will be already created in the main.go file. You should pass this instance via Parameter to NewVault.

Yeah, but I'm always using NewVault, and Ca won't be something global, and it shouldn't be. A call to init, should just be idempotent, OR... be a singleton which still is a global 🤔 Not sure.

michelvocks · 2018-07-26T05:06:15Z

security/vault.go

+	if err != nil {
+		return nil, err
+	}
+	data, err := ioutil.ReadFile(c.caKeyPath)


You should not access the private variable caKeyPath here. Use the method GetCACertPath.

Ops, I mean to do that, huh.

michelvocks · 2018-07-26T05:09:03Z

security/vault.go

+// GetAll returns all keys and values in a copy of the internal data.
+func (v *Vault) GetAll() []string {
+	m := make([]string, 0)
+	for k := range v.data {


What happens when someone accesses Add and in the same time GetAll? Isn't this a race condition?

Yes, there should be a Read Lock here. :/ Thanks.

michelvocks · 2018-07-26T05:13:00Z

security/vault.go

+
+// Vault is a secret storage for data that gaia needs to store encrypted.
+type Vault struct {
+	Path string


Is it on purpose that Path and Cert are global available?

You mean exported? Hum, haven't really thought about it, but you are right. They aren't used directly, so they can be private for only internal use. Thanks. :)

Skarlso · 2018-07-26T16:24:24Z

handlers/vault.go

+}
+
+// UpdateSecret updates a secret using the vault.
+func UpdateSecret(c echo.Context) error {


I don't like this duplication. I'm going to pull it into SetSecret which updates or Creates.

Skarlso · 2018-07-27T04:47:35Z

README.rst

+Security
+--------
+
+See the Documentation located here: |security-docs|.


this isn't linking the way I thought it would :D

michelvocks · 2018-07-27T06:22:00Z

README.rst

@@ -132,6 +132,11 @@ Gaia will compile it and add it to it's store for later execution.

 Please find a bit more sophisticated example in our `go-example repo`_.

+Security
+--------


It should be = instead of -.

michelvocks · 2018-07-27T06:22:23Z

README.rst

+Security
+--------
+
+See the Documentation located here: |security-docs|.


`security-docs`_

michelvocks · 2018-07-27T06:25:07Z

README.rst

 .. |sh-pipeline-logs| image:: https://cdn.rawgit.com/michelvocks/6868118d0da06a422e69e453497eb30d/raw/51b4d6cbc3d86b1fe9531250db5456595423d9ec/pipeline_logs.png
    :alt: gaia pipeline logs screenshot
    :width: 650px

 .. |sh-settings| image:: https://cdn.rawgit.com/michelvocks/6868118d0da06a422e69e453497eb30d/raw/142a2969c4d27d4135ef8f96213bb166009fda1e/settings.png
    :alt: gaia settings screenshot
    :width: 650px
+.. |security-docs|


.. _`security-docs`: https://github.com/gaia-pipeline/gaia/blob/master/security/README.md

Skarlso · 2018-07-27T11:39:42Z

@michelvocks Battle tested the front-end with the Javascript modifications and all worked fine. Thanks for that. :)

Also updated the docs, everything should be cool now. Thanks for those fixes.

…ium.

Skarlso · 2018-07-28T07:50:37Z

@michelvocks A little change... I slept on this and realised that we might want to change the vault's storage medium at some point to a database, or I don't know what.

In that case, I created the VaultStorer interface here: https://github.com/gaia-pipeline/gaia/pull/56/files#diff-6d277307688fa8de2b326d79a993e6b4R25

This made me able to mock the vault to a simple []byte storage medium in the tests, which is awesome. I'm going to do this in #61 for others as well. And also added some more coverage when the storage fails to write or read which is also nice.

michelvocks · 2018-07-28T16:12:36Z

Awesome! 🤗
I think we shouldn't merge this PR as long as #24 isn't implemented. Currently, you can manage secrets but they are not useful. What do you think, @Skarlso?

Skarlso · 2018-07-28T19:19:21Z

I think that PR would be better off if this one is already in, since we could immediately utilise the functionality in #48. Which is blocked on the both of them. Also, I would rather start on the parameter stuff once this and #61 are in because they provide facilities and implementation details that could prove useful. Especially the Service Provider. In fact, I kind of want that in first, since it touches everything basically.

And if this is in, I can also make the vault part of the service provider.

At least that's what I'm thinking @michelvocks. :)

michelvocks · 2018-07-29T11:56:12Z

Good point. Could you please squash your commits? Makes the history a bit cleaner. 🤗

Can you also have a look at the following change? https://github.com/gaia-pipeline/gaia/pull/60/files#diff-1e72a85e5dbe49ee4428eae35d6f1ee6R207
Instead of using the PKCS#1 format we need to use the PKCS#8 format. Does it affect this PR?

Skarlso · 2018-07-29T12:33:08Z

It does, yeah. But not that much and I agree with you. I'll do the changes. Also git can squash commits. You can select the squash and merge when merging, but I can do that if you wish ofc. :-)

Also, in fact I shloud have been using #8... Lazy me. :-/

Skarlso · 2018-07-29T19:41:24Z

So, I completely misunderstood this comment, and thought about something completely different. :D yay, me.

So as I wrote in Slack, but I'm writing it here too, what's happening with the certificate is, that I'm deriving a 32 bit password from it by base64-ing the content, and chopping it into a aes.BlockSize'd slice.

This means that the vault doesn't care what is in there. I'm also applying some padding in case of testing. The IV is generated and then used upon decryption too. It's marshalled into the encrypted data.

All in all it should be pretty secure. And it's AES 256 so it should be safe from brute forcing.

Skarlso added 5 commits July 23, 2018 23:02

Hammering out the api.

4a25927

Changing the api.

e8f911b

More work on the api.

b42cbd1

Steps

c483488

Created the vault back-end.

5ed244f

Skarlso changed the title ] [WIP] A secret vault storage with AES encryption using a certificate Jul 24, 2018

Fixed the logger nil pointer.

87290ca

Skarlso commented Jul 24, 2018

View reviewed changes

security/vault_test.go Outdated

}

defer os.Remove(vaultName)

v.Cert = []byte("test")

v.OpenVault()

Copy link

Member Author

Skarlso Jul 24, 2018

Choose a reason for hiding this comment

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

This isn't needed here.

Skarlso added 4 commits July 24, 2018 13:59

Removed un-needed open.

4a29226

Started chunking on the web interface.

c0c26f4

Some linting.

3d44393

Fixed some values and names.

7d1a21b

Skarlso added 2 commits July 24, 2018 22:45

Making the end-point work.

65bbc62

Working on the display.

b3ab064

Skarlso commented Jul 25, 2018

View reviewed changes

Added missing open.

26f4fc1

Skarlso added 6 commits July 25, 2018 21:45

Done with managing secret view. Added secret field masking.

0313fd1

Not returning the values at all so they aren't even getting to the en…

a9c5a90

…d user. Also fixed the possibility to have an empty vault.

Merge branch 'master' into secure_vault_file

b79d97b

Merge branch 'master' into secure_vault_file

ddb9917

Front-end done and vault now is using the certificate for encryption.

2863cb7

Defaulting vault path to the data folder.

ad4ceb4

Skarlso changed the title ~~[WIP] A secret vault storage with AES encryption using a certificate~~ A secret vault storage with AES encryption using a certificate Jul 25, 2018

michelvocks reviewed Jul 26, 2018

View reviewed changes

addressed comments.

cbe5cac

Skarlso commented Jul 26, 2018

View reviewed changes

Skarlso added 2 commits July 26, 2018 21:36

Some small adjustments and creating the SetSecret endpoint.

c5f7710

Added readme, added tests, and fixed up some more comments.

0c12482

Skarlso force-pushed the secure_vault_file branch from 7c29152 to 0c12482 Compare July 27, 2018 04:26

Skarlso commented Jul 27, 2018

View reviewed changes

michelvocks reviewed Jul 27, 2018

View reviewed changes

Skarlso added 2 commits July 27, 2018 12:52

Readme style fixes.

71b4409

Battle tested the front-end.

b3bbba4

Added the VaultStorer interface to be able to extract the storage med…

452697d

…ium.

Added test testing default storer initialization.

5b28828

michelvocks merged commit 742944c into gaia-pipeline:master Jul 30, 2018

michelvocks mentioned this pull request Jul 30, 2018

Secret store for passwords, tokens, api keys and more #51

Closed

michelvocks mentioned this pull request Feb 15, 2019

Store credentials in Vault #173

Open

A secret vault storage with AES encryption using a certificate #56

A secret vault storage with AES encryption using a certificate #56

Conversation

Skarlso commented Jul 24, 2018 • edited Loading

TODO

codecov-io commented Jul 24, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Skarlso commented Jul 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Skarlso commented Jul 25, 2018

Skarlso commented Jul 25, 2018

michelvocks commented Jul 25, 2018 • edited Loading

Skarlso commented Jul 25, 2018

Skarlso commented Jul 25, 2018 • edited Loading

Skarlso commented Jul 25, 2018

Skarlso commented Jul 26, 2018

michelvocks left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Skarlso commented Jul 27, 2018

Skarlso commented Jul 28, 2018 • edited Loading

michelvocks commented Jul 28, 2018

Skarlso commented Jul 28, 2018

michelvocks commented Jul 29, 2018

Skarlso commented Jul 29, 2018 • edited Loading

Skarlso commented Jul 29, 2018

Skarlso commented Jul 24, 2018 •

edited

Loading

codecov-io commented Jul 24, 2018 •

edited

Loading

michelvocks commented Jul 25, 2018 •

edited

Loading

Skarlso commented Jul 25, 2018 •

edited

Loading

Skarlso commented Jul 28, 2018 •

edited

Loading

Skarlso commented Jul 29, 2018 •

edited

Loading