Allow re-importing the client TLS certifcate. #1743

pattivacek · 2020-08-11T14:25:57Z

We already allow it for the root CA.

codecov-commenter · 2020-08-11T14:42:25Z

Codecov Report

Merging #1743 into master will decrease coverage by 3.57%.
The diff coverage is 51.10%.

@@            Coverage Diff             @@
##           master    #1743      +/-   ##
==========================================
- Coverage   84.98%   81.40%   -3.58%     
==========================================
  Files         178      178              
  Lines       12019    12561     +542     
==========================================
+ Hits        10214    10225      +11     
- Misses       1805     2336     +531

Impacted Files	Coverage Δ
src/libaktualizr/crypto/crypto.h	`61.11% <ø> (ø)`
src/libaktualizr/storage/invstorage.h	`100.00% <ø> (ø)`
src/libaktualizr/crypto/crypto.cc	`53.14% <43.67%> (-30.97%)`	⬇️
src/libaktualizr/storage/invstorage.cc	`65.25% <61.70%> (-23.34%)`	⬇️
src/cert_provider/main.cc	`33.87% <65.30%> (-29.85%)`	⬇️
src/libaktualizr/crypto/keymanager.cc	`70.74% <100.00%> (-18.74%)`	⬇️
src/libaktualizr/storage/sqlstorage_base.h	`60.00% <0.00%> (-40.00%)`	⬇️
... and 4 more

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 6c61fc6...dcad65b. Read the comment docs.

zabbal

LGTM.

pattivacek · 2020-08-13T09:47:40Z

LGTM.

Thanks. Unfortunately I just tested it in a more realistic use case and it isn't working. Still investigating. For now, we should wait on merging.

zabbal · 2020-08-13T10:13:08Z

Is there unit-test for the previous version (root CA re-import)?

pattivacek · 2020-08-13T10:55:06Z

Unfortunately I just tested it in a more realistic use case and it isn't working. Still investigating. For now, we should wait on merging.

False alarm, it was just because trying to reimport the key, too, which I haven't allowed here. For just the cert, it works fine.

Is there unit-test for the previous version (root CA re-import)?

Yep, the same test that I edited to test the new functionality.

pattivacek · 2020-08-14T07:18:42Z

I've updated this, tested it more thoroughly, removed the do-not-merge tag, and in the process (automatically) dismissed @zabbal's review. This is now ready for review again.

zabbal

Looks good.

A little side note: the importUpdateSimple() is defined and used only in invstorage.cc - do we really need to expose it in public API? Can it be marked static instead?

pattivacek · 2020-08-14T08:35:45Z

do we really need to expose it in public API?

Doesn't appear to be part of the public API (include/libaktualizr) to me, but maybe you mean something different? The function is private.

Can it be marked static instead?

I tried that recently (unrelated to this) but I wasn't able to make it work.

zabbal · 2020-08-14T09:09:01Z

Nevermind then - most likely that's smth "lost in translation" while trying to apply my C experience to C++ world.

We already allow it for the root CA. Signed-off-by: Patrick Vacek <patrickvacek@gmail.com>

Signed-off-by: Patrick Vacek <patrickvacek@gmail.com>

This enables the shared credential hack to help test cert replacement. Note that the relevant parameter is --certificate-cn because that's how it's used for the proper fleet CA method. Signed-off-by: Patrick Vacek <patrickvacek@gmail.com>

This logic is now only in once place (in the Crypto class) and the KeyManager function just uses that directly. Signed-off-by: Patrick Vacek <patrickvacek@gmail.com>

Also refactor to use RSA_generate_key_ex instead of the deprecated RSA_generate_key. Signed-off-by: Patrick Vacek <patrickvacek@gmail.com>

Signed-off-by: Patrick Vacek <patrickvacek@gmail.com>

pattivacek · 2020-08-21T14:28:19Z

I did a bunch of refactoring in order to be able to generate certificates so I could appropriately test the situation where a new certificate has a different device ID, which we've decided is not allowed.

zabbal

Looks pretty neat overall.

src/libaktualizr/crypto/crypto.cc

src/libaktualizr/crypto/crypto.h

src/libaktualizr/crypto/keymanager.cc

src/libaktualizr/crypto/crypto.cc

Raigi

OTF tests have passed:
test_06_replace_device_certificate_using_fleet_root_ca
test_07_invalid_certificate_not_accepted_by_backend
test_08_device_certificate_with_new_device_id_not_allowed

…functions. This allows for much easier reuse of the specific parts. Also switch to exceptions instead of printing and returning false. Signed-off-by: Patrick Vacek <patrickvacek@gmail.com>

This should only be used for testing, so it prints a message to tell you that. Signed-off-by: Patrick Vacek <patrickvacek@gmail.com>

Expand the test to check the negative case (with a different device ID) as well. Signed-off-by: Patrick Vacek <patrickvacek@gmail.com>

Signed-off-by: Patrick Vacek <patrickvacek@gmail.com>

zabbal · 2020-08-24T16:40:22Z

@Raigi is this still good with latest changes?

pattivacek · 2020-08-25T15:54:13Z

Raigi is this still good with latest changes?

She's away this week, but very little changed from the version she was testing. I think this is safe to merge.

pattivacek requested review from zabbal, xaviergully, kbushgit and MatthiasBerchner August 11, 2020 14:25

zabbal previously approved these changes Aug 13, 2020

View reviewed changes

pattivacek added the do-not-merge-until-ready label Aug 13, 2020

pattivacek dismissed zabbal’s stale review via 3608f74 August 13, 2020 13:16

pattivacek force-pushed the feat/OTA-5178/re-import-client-cert branch from 1e4ec83 to 3608f74 Compare August 13, 2020 13:16

pattivacek removed the do-not-merge-until-ready label Aug 13, 2020

pattivacek force-pushed the feat/OTA-5178/re-import-client-cert branch 2 times, most recently from 5097ce1 to ece2479 Compare August 13, 2020 13:50

pattivacek requested a review from Raigi August 13, 2020 14:20

zabbal previously approved these changes Aug 14, 2020

View reviewed changes

Allow re-importing the client TLS certifcate and key.

b1db345

We already allow it for the root CA. Signed-off-by: Patrick Vacek <patrickvacek@gmail.com>

pattivacek dismissed zabbal’s stale review via 8c144ce August 21, 2020 13:21

pattivacek force-pushed the feat/OTA-5178/re-import-client-cert branch from ece2479 to 8c144ce Compare August 21, 2020 13:21

pattivacek requested a review from aodukha August 21, 2020 13:23

pattivacek force-pushed the feat/OTA-5178/re-import-client-cert branch 2 times, most recently from a300a58 to 04edbfb Compare August 21, 2020 13:26

pattivacek added 4 commits August 21, 2020 16:03

Print more import details to the log at debug level.

4858c9a

Signed-off-by: Patrick Vacek <patrickvacek@gmail.com>

Remove redundant common name parsing code.

090677d

This logic is now only in once place (in the Crypto class) and the KeyManager function just uses that directly. Signed-off-by: Patrick Vacek <patrickvacek@gmail.com>

Move cert generation code to Crypto class.

a20ac4c

Also refactor to use RSA_generate_key_ex instead of the deprecated RSA_generate_key. Signed-off-by: Patrick Vacek <patrickvacek@gmail.com>

Standardize StorageCommon test names.

9e86af0

Signed-off-by: Patrick Vacek <patrickvacek@gmail.com>

pattivacek force-pushed the feat/OTA-5178/re-import-client-cert branch from 04edbfb to 8bf559a Compare August 21, 2020 14:04

zabbal reviewed Aug 21, 2020

View reviewed changes

Raigi previously approved these changes Aug 21, 2020

View reviewed changes

pattivacek added 4 commits August 24, 2020 09:47

Refactor cert generation, signing, and serialization into a separate …

c43736e

…functions. This allows for much easier reuse of the specific parts. Also switch to exceptions instead of printing and returning false. Signed-off-by: Patrick Vacek <patrickvacek@gmail.com>

Add option to self-sign a generated certificate.

a3bcf82

This should only be used for testing, so it prints a message to tell you that. Signed-off-by: Patrick Vacek <patrickvacek@gmail.com>

Only re-import the client cert if the device ID is the same.

7e71c30

Expand the test to check the negative case (with a different device ID) as well. Signed-off-by: Patrick Vacek <patrickvacek@gmail.com>

Reuse Crypto::generateRSAKeyPairEVP() in generateCert().

dcad65b

Signed-off-by: Patrick Vacek <patrickvacek@gmail.com>

pattivacek dismissed Raigi’s stale review via dcad65b August 24, 2020 07:48

pattivacek force-pushed the feat/OTA-5178/re-import-client-cert branch from 8bf559a to dcad65b Compare August 24, 2020 07:48

zabbal approved these changes Aug 24, 2020

View reviewed changes

pattivacek merged commit 3d4d39e into master Aug 25, 2020

pattivacek deleted the feat/OTA-5178/re-import-client-cert branch August 25, 2020 15:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow re-importing the client TLS certifcate. #1743

Allow re-importing the client TLS certifcate. #1743

pattivacek commented Aug 11, 2020

codecov-commenter commented Aug 11, 2020 •

edited

Loading

zabbal left a comment

pattivacek commented Aug 13, 2020

zabbal commented Aug 13, 2020

pattivacek commented Aug 13, 2020

pattivacek commented Aug 14, 2020

zabbal left a comment

pattivacek commented Aug 14, 2020

zabbal commented Aug 14, 2020

pattivacek commented Aug 21, 2020

zabbal left a comment

Raigi left a comment

zabbal commented Aug 24, 2020

pattivacek commented Aug 25, 2020

Allow re-importing the client TLS certifcate. #1743

Allow re-importing the client TLS certifcate. #1743

Conversation

pattivacek commented Aug 11, 2020

codecov-commenter commented Aug 11, 2020 • edited Loading

Codecov Report

zabbal left a comment

Choose a reason for hiding this comment

pattivacek commented Aug 13, 2020

zabbal commented Aug 13, 2020

pattivacek commented Aug 13, 2020

pattivacek commented Aug 14, 2020

zabbal left a comment

Choose a reason for hiding this comment

pattivacek commented Aug 14, 2020

zabbal commented Aug 14, 2020

pattivacek commented Aug 21, 2020

zabbal left a comment

Choose a reason for hiding this comment

Raigi left a comment

Choose a reason for hiding this comment

zabbal commented Aug 24, 2020

pattivacek commented Aug 25, 2020

codecov-commenter commented Aug 11, 2020 •

edited

Loading