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

Change vault to use GCM with counter #178

Merged
merged 12 commits into from Mar 6, 2019

Conversation

Skarlso
Copy link
Member

@Skarlso Skarlso commented Mar 5, 2019

I need to increase the coverage a bit.

  • Test the counter being increased
  • Properly cover hex decode
  • Test some edge cases

Overall though... it works. :)

@codecov-io
Copy link

codecov-io commented Mar 5, 2019

Codecov Report

Merging #178 into master will increase coverage by <.01%.
The diff coverage is 73.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #178      +/-   ##
==========================================
+ Coverage   66.97%   66.98%   +<.01%     
==========================================
  Files          35       36       +1     
  Lines        2695     2732      +37     
==========================================
+ Hits         1805     1830      +25     
- Misses        659      665       +6     
- Partials      231      237       +6
Impacted Files Coverage Δ
security/legacy_vault.go 61.76% <61.76%> (ø)
security/vault.go 78.57% <82.92%> (+1.19%) ⬆️

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 2925ff9...5c587fe. Read the comment docs.

@Skarlso
Copy link
Member Author

Skarlso commented Mar 5, 2019

@Skarlso don't forget to initialize the counter if the file exists in init. Otherwise the counter will be overwritten by a save that was not preceeded by a load.

That is technically not possible right now from the Frontend but you never know... 🙂

@Skarlso
Copy link
Member Author

Skarlso commented Mar 5, 2019

Actually it doesn't matter. Since the vault is always overwritten. If anyone does a save accidentally before loading in all the secrets, they will basically wipe the vault with what they have in the data. This is by design because the vault is never stored unencrypted.

@Skarlso Skarlso changed the title [WIP] Change vault to use GCM with counter Change vault to use GCM with counter Mar 5, 2019
@Skarlso
Copy link
Member Author

Skarlso commented Mar 5, 2019

screenshot 2019-03-05 at 21 32 10

Also did a quick manual too. All seems to be working as expected. :)

@michelvocks
Copy link
Member

Cool. Thanks a lot @Skarlso
One question ahead: This is will break existing vaults, right?

@Skarlso
Copy link
Member Author

Skarlso commented Mar 6, 2019

Cool. Thanks a lot @Skarlso
One question ahead: This is will break existing vaults, right?

Oh! That's a very good notion. It will, yes. :/ Crap. I need to come up with a fallback to cycle out the old vault files. And after a while delete it.

@michelvocks
Copy link
Member

Oh! That's a very good notion. It will, yes. :/ Crap. I need to come up with a fallback to cycle out the old vault files. And after a while delete it.

I guess a migration step would be helpful here. So when Gaia is about to start, we check if the existing vault is in an old state and migrate it to the new state?

@Skarlso
Copy link
Member Author

Skarlso commented Mar 6, 2019

@michelvocks correct, that's my vision. It should be fairly easy to check since the new one is in a different format. If hex.Decode fails, I'll try with the previous method. If that works, I'm loading it in and converting it back to the new format. After a while, we can delete that functionality.

@Skarlso
Copy link
Member Author

Skarlso commented Mar 6, 2019

I just also realised that the "password" for the gaia vault is the cert, but what I thought I did is have a checksum or something of the whole content and use that. But instead I just used the content at a 32 mark. But I failed to skip the first line... so the password is always something like: -----BEGIN CERTIFICATE-----.... and like a couple more characters. 🤦‍♂️

This is not true. I hashed the whole thing.

@Skarlso
Copy link
Member Author

Skarlso commented Mar 6, 2019

Hopefully after everyone has taken the new version this will be much improved and the old vault files will be replaced. :)

@Skarlso
Copy link
Member Author

Skarlso commented Mar 6, 2019

Huh, okay, it's not that bad. This was a password for example ( this is a dummy one ):

LS0tLS1CRUdJTiBQUklWQVRFIEtFWS0tLS0tCk1JSUV2UUlCQURBTkJna3Foa2lHOXcwQkFRRUZBQVNDQktjd2dnU2pBZ0VBQW9JQkFRRFZDd3h2YXpyakliYXAKazRmUDBnRlRoWFMxeGFXTnVjd3A0N3cweE1DSDBtT2k1QjFKR2ZIbFJBSm1mUU9uNGpxekVtKzJuVHp3UW1SLwpFTjB3VjVNUFdnMWNMVGlTcERyRk45bm1rSlppWmlONU5ST0pwVVB3NHNveklrU0JsaTA1NVRTYU5FcmYyS0hGCjFseWxSNFp5TVYzMTN6UytoYjV3UGZ6YlNQcm1leTZmOGVZNmIxRm9TLzVjYVRHd2VveGs3NjRxUm02VVUyNVoKY2ZHYVRmcEkzS3NnTitWb3Q5cWpTZmRJN2REczlqUkNmN0JZQ1BobTBEZkg5ZTNKQkMvSHlSUXJieGdocGpxUQo4VmFuM2E4T0lXZVYvQVYxQkkyZ1RyemVDV0lBaGF0TkFmalJhRVZJcFRVKzFLSmMrYkdseFVLbGRxdmlHSUNDCllLRm54TFNUQWdNQkFBRUNnZ0VBSFM5Z1Nyc3oyLzI0V2s2OW9qaXd1ZEpraEtwSTNidUFQcFRXS1p4eWk2akUKd1lIaWlTc211ak93NkgxanpOSHZIS3ovNU5KeGtMQm51QWlGWktQNm4zWEVzc1gzSkErZmhYajdQdHk3M1VzRQp2UXdLV3licXdjc3Z6QVY3d1F6anNUUzNHaERqMlBxQ1h1blkwME9USlgyaDA1YjZVTWRkcVY2MGp3M1dZVkJxCnFRMDBNdVpXVi9yd0Y0R210MjhQQ1NuTXhvb0x2TkI2cEpsSDBxNjRwcndpSm9lL29FRGQ4eUZEQUlhT0VEdzkKNUU2YVBuUGZDa2o0cHUzZnJhSGRjR1lDeUxHc2lkUytFWi9oUHhEMVhML1owSy9TVTBvMDZqdy9XVjA1Nk9wMgplamNwdGNNWFJYS2RXeTkrYmxVazFubml3QkFF3i8dduh3hhfzc0t1bEFEQVFLQmdRRDFLQ3hlcTAyUnI4aTBjTVF3ClhhVllnQ0h6cURiNk5tckU3QUhBcS9vdU8zb0dWSDRhMXIvQXBWY3AyL2g4RFpZV0Z1dzdFMkVvU0NTOVFyQmsKMlVCNmgyekY0NzFMRTk5Skg5d1BMaFdEN0xPc3VBaXVuTnpwZ2t5c05ibUJXN2JKUXZXaUNvQmphM2d5SnlNbQpGNTg3dkVzS2tGUlF4aDEvRWxXZlRLME13UUtCZ1FEZWQwTWdrQ1NmTktCOE5PN29VSEdzeVpQS0g4YjJ0WCtFCllxbCtGMTlIWE9yYjdGbExndGUwSTdsQjA1Q2FUM3dBUlpFYXJWc3VjSzBiRHQyclNCMUUxMVNFSnRWTFlyWjIKL2RTVms5YUhBdkRoQU90VGorS0VWd0RTZ2ZqOHNwa3QrMG5DdElVVWJzVzEvdm5PNk93eENGMjZHOGpaTkJuQgpTSnpYM2NRU1V3S0JnQ1pjZmV6bVkwSHJ2cjAxZEEyWmFia2FlN1dUMmQ1M1MyZTdBbDh5eWZnWUNIVWJIWXgzCmxCUENDNHlhUmh5clI1UDNURW5HTTRySkZ5NmlVOVhFQlFublRRYitKdTJyazJIdTRWRml4YTBhQ2RkNkNLbkMKRS9OYUYwTlBPTkxjRmhNU0xqdUg1eVVuZUiuui3udhjdjdj33FPQ2lCOEhrVEFFSDIvSTRMOUJBb0dCQUwwSQppam01UWVVbVN0aEFBbUhWT1VLaFpydHhsUmM5MGtVanNQSTcyZkpCdWk5MS9jcDBPK1lPRlBVaVdOVkdoUStXCkRWNmx2NzBPY1lsMGdjdhfkjdshf88XdmZk9sdU5nQUF1UnNUUlBoMHp1MmFTaVJuSzQrdHk4UzRTVEtXem9Pckh3NDcKWU1uWnF0dFo1UlpvdXN4ZWo1UjZqMm45QWdYT2g3UDloNGpHSUQyUkFvR0FQQ3ZCcWlKY280dWRRc29zdXl0LwpaMGRPakY2c1NhaUFueDdXMiswYUtNUlpPaHRDaGxDcTBTRnZxVHZLQjdYOVV2RVZGU3lmR0VCT2F3aU5YV0R1ClBIZURCMURXY3FUb0RUS3FTNVY0ZHFTKzNaYU1IanZYMzlZaG5qTENTMGMzbm9mVldXK1BheTExY0M5aTh0VjEKT0lMT2JUMnRnYTJ0eE80SnhVcnMrTlU9Ci0tLS0tRU5EIFBSSVZBVEUgS0VZLS0tLS0KCAgICAgICAg=

So I think we are fine.. :D

@Skarlso
Copy link
Member Author

Skarlso commented Mar 6, 2019

Alright. This is now working with the fall back too. I tested it with an old encrypted vault and a new one. The old one was overwritten and replaced with a new vault file.

Copy link
Member

@michelvocks michelvocks left a comment

Choose a reason for hiding this comment

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

LGTM 😄 One small minor issue

@@ -82,8 +87,15 @@ func NewVault(ca CAAPI, storer VaultStorer) (*Vault, error) {
if err != nil {
return nil, err
}
if len(data) < 32 {
Copy link
Member

Choose a reason for hiding this comment

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

I guess we can use keySize here?

@Skarlso Skarlso merged commit 069a24b into gaia-pipeline:master Mar 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants