Add upload certificate command to adam #927

europaul · 2023-11-21T10:49:42Z

A new CLI option is added: on eden adam upload-cert the provided certificate is uploaded to /admin/certs via an HTTP POST

NOTE: the new EDEN test needs to be run with an EVE version that includes this fix and with the Adam version that includes this change.

Closes #896

giggsoff · 2023-11-22T01:19:04Z

It looks like the last part of the puzzle before moving to tests is to have some function, that will re-sign all config objects derived from the signing certificate here, at least modify ControllerCertHash and ContextId, of CipherContexts correct?

europaul · 2023-11-22T14:10:45Z

@giggsoff wouldn't it be enough just to change Eden's path to signing certificate to the new certificate, so it only affects the newly issued expectation objects?

Also just fyi - rn I'm not planning to change the signing key, just the certificate.

giggsoff · 2023-11-22T14:36:33Z

Well, I am not sure will EVE-OS work fine with old certificate in CipherContexts and new certificate in Adam (attestation). I think we need some action to re-calculate old CipherContexts as we will potentially have a test with deploying of some app and check how it works after modification of certificate.

I see, but ControllerCertHash in CipherContext is about cert, not key.

europaul · 2023-11-24T15:32:45Z

@giggsoff I tried the following:

start eden and eve
eden pod deploy -n nginx1 docker://nginx
generate a new signing certificate from the same signing key (every parameter is the same except the validity period)
eden adam upload-cert --cert-file ~/.eden/certs/signing-new.pem
eden pod deploy -n nginx2 docker://nginx
check the logs and eden pod ps - both applications are running fine and I didn't see anything suspicious

I kept the old signing cert and key here. Tbh I don't fully understand yet how the Expectation structures and package is used. My understanding now is that they are used to build the config that is sent to Adam to be later relayed to EVE. But that would mean that in this case we are signing the structures with the old key, while we sign the message (envelope) with the new key. This likely doesn't make the difference since the key stays the same in my scenario - only the certificate changes.

Please tell me if I'm missing something.

giggsoff · 2023-11-24T17:43:35Z

Can you please check apps with any userdata (e.g. you made test with #918)? Inside of expect we try to encrypt two types of data: credentials of datastores and userdata. Also it would be great to check the restart of EVE-OS (and re-attestatation process).

europaul · 2023-11-25T19:35:14Z

@giggsoff I went on a roller-coaster ride through the code and found out that

we don't use the signing certificate to generate the symmetric key for the encryption, we only use the signing key
BUT
we use the hash of the signing certificate to generate IV for AES CFB, so this can mess the decryption up
BUT
we also send that IV with the cipher text in the cipher block, so decrypting should actually not be a problem...

I'm not yet sure where EVE gets cipher context from (does it come with every message or independently of the messages), but I will check. I will also do the tests that you suggested 👍

But also I just thought maybe it's a good idea not to re-sign the old configs after changing the signing certificate - if EVE cannot deal with that this might just show a flaw in our design, since we the cloud might change the certificates frequently to ensure forward secrecy and we cannot expect it to re-sign all configs since the beginning of time. What do you think?

uncleDecart · 2023-11-27T12:15:37Z

But also I just thought maybe it's a good idea not to re-sign the old configs after changing the signing certificate - if EVE cannot deal with that this might just show a flaw in our design, since we the cloud might change the certificates frequently to ensure forward secrecy and we cannot expect it to re-sign all configs since the beginning of time. What do you think?

@europaul but that means if I have any previous certificate I can use it to sign configuration and send it, is it not? So potentially if I get any old certificate I can use it to change configuration, which sounds not secure. Or am I missing something?

europaul · 2023-11-27T12:22:11Z

But also I just thought maybe it's a good idea not to re-sign the old configs after changing the signing certificate - if EVE cannot deal with that this might just show a flaw in our design, since we the cloud might change the certificates frequently to ensure forward secrecy and we cannot expect it to re-sign all configs since the beginning of time. What do you think?

@europaul but that means if I have any previous certificate I can use it to sign configuration and send it, is it not? So potentially if I get any old certificate I can use it to change configuration, which sounds not secure. Or am I missing something?

@uncleDecart you'll need the controllers (old) private key to sign the configuration, the certificate is meant to be public and available to everybody - it's just our choice if we choose to remember the old certificates in EVE or not.
Or are you talking about the case when controller's private key has been compromised? In this case EVE would need some signal from the controller that the corresponding certificate should not be trusted anymore, e.g. a Certificate Revocation List (CRL). Idk however if EVE supports CRLs...

europaul · 2023-11-29T14:09:34Z

@giggsoff you were right, I re-did the same test with applications with userdata and reboot like you suggested - after reboot EVE cannot decipher the userdata from the first application, because it is missing the old certificate that is required in the old cipherContext. The reason is that EVE only stores the latest signing certificate from the controller in persistent storage.

pkg/openevec/adam.go

pkg/controller/controller.go

cmd/certs.go

pkg/controller/functions.go

pkg/expect/encrypt.go

milan-zededa · 2023-12-18T10:02:28Z

tests/eclient/testdata/ctrl_cert_change.txt

+test eden.app.test -test.v -timewait 20m RUNNING eclient1
+
+# generate new controller certificate
+eden utils gen-ctrl-cert -o /home/paul/.eden/certs/signing-new.pem


You seem to have absolute paths here referring to your home folder. Use eden.root variable instead to store it somewhere under ./dist directory.

This is embarrassing 🤦

I wanted to put the certificate next to the the old signing cert, which is in $HOME/.eden/certs, so I created a new config variable eden.cert-dir for it now.

I think it would be better to put the certificate under eden.tests. This is a temporary directory used by running tests.

pkg/openevec/adam.go

milan-zededa · 2024-01-08T17:17:22Z

@europaul LGTM but lets wait for the adam side to be merged: lf-edge/adam#120
Then you will be able to update the adam version and put your test into the test suite.
How long does the test take to execute? If it is fairly quick, we can put into the "Smoke tests" suite.

tests/eclient/testdata/ctrl_cert_change.txt

europaul · 2024-01-09T16:41:46Z

@europaul LGTM but lets wait for the adam side to be merged: lf-edge/adam#120 Then you will be able to update the adam version and put your test into the test suite. How long does the test take to execute? If it is fairly quick, we can put into the "Smoke tests" suite.

@milan-zededa on my system the test takes around 4 mins to complete

cmd/edenUtils.go

pkg/controller/adam/adam.go

pkg/utils/cipher.go

europaul · 2024-01-17T18:39:57Z

Adam used to get it's certificates and keys through envs. To unify the UX I switched this mechanism to getting paths to files with certificates and keys through the CLI parameters only. This way it doesn't require a POST API anymore.

This PR needs lf-edge/adam#123 to work.

Tell me how you like this solution!

P.S.: I will fix the commits later once we agree on the code.

deitch · 2024-01-17T19:36:17Z

Adam used to get it's certificates and keys through envs. To unify the UX I switched this mechanism to getting paths to files with certificates and keys through the CLI parameters only. This way it doesn't require a POST API anymore.

To be clear, those path-based CLI flags already existed in Adam, correct? You are just taking advantage of them?

And Adam already reads those files with each request, rather than cache them in memory, so there is no need to tell Adam to reload the files, correct?

This PR needs lf-edge/adam#123 to work.

Does it? I think lf-edge/adam#123 just gets rid of something that was confusing there. But this should not be blocked by it (unless I misunderstood)?

Tell me how you like this solution!

Like!

europaul · 2024-01-18T10:07:31Z

To be clear, those path-based CLI flags already existed in Adam, correct? You are just taking advantage of them?

Does it? I think lf-edge/adam#123 just gets rid of something that was confusing there. But this should not be blocked by it (unless I misunderstood)?

yes, the flags are already part of Adam, so no changes in Adam are required. The referenced commit is more of a clean-up.

And Adam already reads those files with each request, rather than cache them in memory, so there is no need to tell Adam to reload the files, correct?

yes, that is the case for the signing certificate which is relevant for this test

milan-zededa

LGTM, you can add the test into the "smoke" test suite: https://github.com/lf-edge/eden/blob/master/tests/workflow/smoke.tests.txt

europaul · 2024-01-18T18:24:01Z

added the test to the smoke test suite. ready to merge!

giggsoff

Great work! Thank you for your patience.
Few comments below.

pkg/utils/cryptoConfig.go

pkg/utils/x509.go

pkg/utils/cipher.go

pkg/openevec/adam.go

pkg/expect/encrypt.go

pkg/openevec/adam.go

uncleDecart · 2024-01-22T10:37:43Z

once lf-edge/adam#120 is merged and we pull latest adam we can merge this

europaul · 2024-01-22T13:00:34Z

@giggsoff @uncleDecart thanks for the great comments!

once lf-edge/adam#120 is merged and we pull latest adam we can merge this

lf-edge/adam#120 was superseded by lf-edge/adam#123, so I'm gonna close it. However the current PR doesn't require any changes in Adam and can be merged independently

deitch · 2024-01-23T08:52:53Z

Some yetus complains, and some eden tests failing. Are those failures because of this, because of the other changes, or transient?

europaul · 2024-01-23T09:30:05Z

Some yetus complains, and some eden tests failing. Are those failures because of this, because of the other changes, or transient?

I'd say they fail, because Eden workflows still run with an older EVE version. This PR is here to fix it

And the yetus complains are in accordance to what we discussed above.

uncleDecart · 2024-01-24T12:10:33Z

@europaul can you rebase to master so that we can rerun tests?

uncleDecart

Running tests

To unify the UX and make sure that the certs and keys can be changed during the runtime of Adam while preserving them through Adam's restart we move from setting certs and keys through envs to cli args. Signed-off-by: Paul Gaiduk <[email protected]>

A new CLI option is added: on `eden adam change-signing-cert` the provided certificate is used to overwrite the current signing certificate file and re-encrypt the encrypted objects in config. Signed-off-by: Paul Gaiduk <[email protected]>

Signed-off-by: Paul Gaiduk <[email protected]>

uncleDecart · 2024-03-05T13:11:32Z

There's a failure on rebooting device on ZFS filesystem. Problem is persistent and also occurred before, need to investigate zfs failure before merging this

The test does the following: 1. deploy first app with userdata to EVE 2. create new signing certificate for controller 3. update controller's certificate and resign old config 4. deploy second app with userdata 5. reboot EVE 6. deploy third app with userdata 7. make sure all apps are running correclty Signed-off-by: Paul Gaiduk <[email protected]>

europaul · 2024-03-05T15:04:39Z

There's a failure on rebooting device on ZFS filesystem. Problem is persistent and also occurred before, need to investigate zfs failure before merging this

I removed the test for the app being in HALTED state after the reboot, because it's is sometimes skipped and the app goes from HALTING to INITIAL. This fixed the tests. We still check for the app to be RUNNING after the reboot.

@uncleDecart this PR is ready to be merged

europaul force-pushed the add-ctrl-sign-cert-test branch from bca0d6d to 7af8df6 Compare November 21, 2023 10:58

europaul force-pushed the add-ctrl-sign-cert-test branch from 7af8df6 to 1de4ce9 Compare December 7, 2023 15:32

europaul marked this pull request as ready for review December 14, 2023 15:29

europaul requested a review from uncleDecart as a code owner December 14, 2023 15:29

milan-zededa reviewed Dec 14, 2023

View reviewed changes

pkg/openevec/adam.go Show resolved Hide resolved

uncleDecart reviewed Dec 15, 2023

View reviewed changes

pkg/controller/controller.go Outdated Show resolved Hide resolved

europaul force-pushed the add-ctrl-sign-cert-test branch 2 times, most recently from 266987f to 371b3d7 Compare December 15, 2023 17:44

milan-zededa reviewed Dec 18, 2023

View reviewed changes

milan-zededa mentioned this pull request Dec 19, 2023

Import EVE API from the new repo (commit from Paul) #935

Merged

europaul force-pushed the add-ctrl-sign-cert-test branch from 371b3d7 to 8fb7358 Compare January 2, 2024 18:08

europaul commented Jan 3, 2024

View reviewed changes

pkg/openevec/adam.go Outdated Show resolved Hide resolved

europaul force-pushed the add-ctrl-sign-cert-test branch from 8fb7358 to 17cb2b3 Compare January 4, 2024 14:15

milan-zededa mentioned this pull request Jan 8, 2024

Admin post certificate lf-edge/adam#120

Closed

giggsoff reviewed Jan 8, 2024

View reviewed changes

tests/eclient/testdata/ctrl_cert_change.txt Outdated Show resolved Hide resolved

giggsoff reviewed Jan 8, 2024

View reviewed changes

tests/eclient/testdata/ctrl_cert_change.txt Show resolved Hide resolved

europaul force-pushed the add-ctrl-sign-cert-test branch 2 times, most recently from c457e6c to fc2f5dc Compare January 9, 2024 16:31

uncleDecart approved these changes Jan 16, 2024

View reviewed changes

cmd/edenUtils.go Outdated Show resolved Hide resolved

pkg/controller/adam/adam.go Show resolved Hide resolved

pkg/utils/cipher.go Outdated Show resolved Hide resolved

europaul force-pushed the add-ctrl-sign-cert-test branch from fc2f5dc to f0b9831 Compare January 16, 2024 09:54

giggsoff mentioned this pull request Jan 17, 2024

Move from env params to cli params only lf-edge/adam#123

Merged

milan-zededa approved these changes Jan 18, 2024

View reviewed changes

europaul force-pushed the add-ctrl-sign-cert-test branch 2 times, most recently from 79fa91d to abb1ae6 Compare January 18, 2024 18:19

giggsoff approved these changes Jan 18, 2024

View reviewed changes

pkg/utils/cryptoConfig.go Outdated Show resolved Hide resolved

pkg/utils/x509.go Outdated Show resolved Hide resolved

pkg/utils/cipher.go Show resolved Hide resolved

pkg/openevec/adam.go Outdated Show resolved Hide resolved

pkg/expect/encrypt.go Outdated Show resolved Hide resolved

uncleDecart reviewed Jan 22, 2024

View reviewed changes

pkg/openevec/adam.go Show resolved Hide resolved

europaul force-pushed the add-ctrl-sign-cert-test branch from abb1ae6 to cf7cc33 Compare January 22, 2024 12:58

europaul force-pushed the add-ctrl-sign-cert-test branch from cf7cc33 to 1d0232b Compare January 24, 2024 12:38

uncleDecart approved these changes Jan 31, 2024

View reviewed changes

europaul added 3 commits March 4, 2024 14:27

Add change certificate command to adam

dca80e4

A new CLI option is added: on `eden adam change-signing-cert` the provided certificate is used to overwrite the current signing certificate file and re-encrypt the encrypted objects in config. Signed-off-by: Paul Gaiduk <[email protected]>

Add new command to generate a new signing certificate for the controller

58c76f5

Signed-off-by: Paul Gaiduk <[email protected]>

europaul force-pushed the add-ctrl-sign-cert-test branch 2 times, most recently from 9440875 to c476acc Compare March 5, 2024 11:21

europaul force-pushed the add-ctrl-sign-cert-test branch from c476acc to 6290f1b Compare March 5, 2024 13:27

uncleDecart merged commit 4fbc73c into lf-edge:master Mar 5, 2024
11 of 18 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add upload certificate command to adam #927

Add upload certificate command to adam #927

europaul commented Nov 21, 2023 •

edited

Loading

giggsoff commented Nov 22, 2023

europaul commented Nov 22, 2023 •

edited

Loading

giggsoff commented Nov 22, 2023

europaul commented Nov 24, 2023 •

edited

Loading

giggsoff commented Nov 24, 2023 •

edited

Loading

europaul commented Nov 25, 2023 •

edited

Loading

uncleDecart commented Nov 27, 2023

europaul commented Nov 27, 2023

europaul commented Nov 29, 2023 •

edited

Loading

milan-zededa Dec 18, 2023

europaul Jan 2, 2024

milan-zededa Jan 4, 2024

europaul Jan 4, 2024

milan-zededa commented Jan 8, 2024

europaul commented Jan 9, 2024

europaul commented Jan 17, 2024 •

edited

Loading

deitch commented Jan 17, 2024

europaul commented Jan 18, 2024

milan-zededa left a comment

europaul commented Jan 18, 2024

giggsoff left a comment

uncleDecart commented Jan 22, 2024

europaul commented Jan 22, 2024

deitch commented Jan 23, 2024

europaul commented Jan 23, 2024 •

edited

Loading

uncleDecart commented Jan 24, 2024

uncleDecart left a comment

uncleDecart commented Mar 5, 2024

europaul commented Mar 5, 2024

Add upload certificate command to adam #927

Add upload certificate command to adam #927

Conversation

europaul commented Nov 21, 2023 • edited Loading

giggsoff commented Nov 22, 2023

europaul commented Nov 22, 2023 • edited Loading

giggsoff commented Nov 22, 2023

europaul commented Nov 24, 2023 • edited Loading

giggsoff commented Nov 24, 2023 • edited Loading

europaul commented Nov 25, 2023 • edited Loading

uncleDecart commented Nov 27, 2023

europaul commented Nov 27, 2023

europaul commented Nov 29, 2023 • edited Loading

milan-zededa Dec 18, 2023

Choose a reason for hiding this comment

europaul Jan 2, 2024

Choose a reason for hiding this comment

milan-zededa Jan 4, 2024

Choose a reason for hiding this comment

europaul Jan 4, 2024

Choose a reason for hiding this comment

milan-zededa commented Jan 8, 2024

europaul commented Jan 9, 2024

europaul commented Jan 17, 2024 • edited Loading

deitch commented Jan 17, 2024

europaul commented Jan 18, 2024

milan-zededa left a comment

Choose a reason for hiding this comment

europaul commented Jan 18, 2024

giggsoff left a comment

Choose a reason for hiding this comment

uncleDecart commented Jan 22, 2024

europaul commented Jan 22, 2024

deitch commented Jan 23, 2024

europaul commented Jan 23, 2024 • edited Loading

uncleDecart commented Jan 24, 2024

uncleDecart left a comment

Choose a reason for hiding this comment

uncleDecart commented Mar 5, 2024

europaul commented Mar 5, 2024

europaul commented Nov 21, 2023 •

edited

Loading

europaul commented Nov 22, 2023 •

edited

Loading

europaul commented Nov 24, 2023 •

edited

Loading

giggsoff commented Nov 24, 2023 •

edited

Loading

europaul commented Nov 25, 2023 •

edited

Loading

europaul commented Nov 29, 2023 •

edited

Loading

europaul commented Jan 17, 2024 •

edited

Loading

europaul commented Jan 23, 2024 •

edited

Loading