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

Implement admin POST method to change controller signing certificate in runtime #118

Open
milan-zededa opened this issue Oct 3, 2023 · 4 comments

Comments

@milan-zededa
Copy link
Contributor

milan-zededa commented Oct 3, 2023

Recently, we have seen many issues where EVE didn't properly handle a change in the controller signing certificate. It would be therefore beneficial to prepare an eden test in which the signing certificate used by Adam is changed after device is onboarded. This can be combined with poor network connectivity (which can be modeled with eden), etc. (conditions that triggered aforementioned issues).

However, even though there is a POST method declared for /certs (to presumably change the controller certificate), it is not implemented. Handler apiv2.certs does not check the request method type and always behaves as GET method, returning the set of installed certificates.

Edit: As pointed out by @giggsoff, instead of using the mentioned /certs endpoint, which is actually used between EVE and Adam, we should add a new POST method under the Admin handler, that could be then used from Eden CLI to update the controller certificate in runtime.

CC @eriknordmark @rouming

@giggsoff
Copy link
Collaborator

giggsoff commented Oct 3, 2023

The endpoint you point onto responsible for communication between Adam and EVE-OS (described here). It is quite strange that there are POST handler there, but I think that endpoints for communication between admin (Eden?) and Adam should be separated (probably into admin handler).

Small question: as I can see currently signing certificate configured using command line option, which points onto file from Eden. Will the re-creation of the file (and probably re-start of Adam) be enough in the workflow you want to check? Do we really need to implement any handler?

@eriknordmark
Copy link
Contributor

I guess one additional question is whether some POST should take the identical payload as what's returned by the GET, which is the full set of controller certificates (signing, encryption, plus zero or more intermediate certs). Plus the questions from Petr about the file and command line option etc.

@milan-zededa milan-zededa changed the title Implement POST method for /certs Implement POST method to change controller signing certificate in runtime Oct 4, 2023
@milan-zededa
Copy link
Contributor Author

milan-zededa commented Oct 4, 2023

Will the re-creation of the file (and probably re-start of Adam) be enough in the workflow you want to check? Do we really need to implement any handler?

@giggsoff Wouldn't a restart of Adam lead to a loss of same state information (device UUID, device config, etc.)?

@milan-zededa milan-zededa changed the title Implement POST method to change controller signing certificate in runtime Implement admin POST method to change controller signing certificate in runtime Oct 4, 2023
@giggsoff
Copy link
Collaborator

giggsoff commented Oct 4, 2023

Will the re-creation of the file (and probably re-start of Adam) be enough in the workflow you want to check? Do we really need to implement any handler?

@giggsoff Wouldn't a restart of Adam lead to a loss of same state information (device UUID, device config, etc.)?

It is true, we have the driver with no persistent state, it is memory one. But I hope nobody use it in production (or how we can name it). We have file driver and redis driver as well, both of them recover after restart. Eden uses redis one with persistent redis.

Potentially we can avoid restart of Adam, assume the next request will read novel certificate file.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants