refactor: Untangle certificate request, creation and renewal#3268
Merged
didier-wenzek merged 6 commits intothin-edge:mainfrom Nov 28, 2024
Merged
Conversation
Codecov ReportAttention: Patch coverage is Additional details and impacted files📢 Thoughts on this report? Let us know! |
Contributor
Robot Results
|
didier-wenzek
commented
Nov 27, 2024
| params.not_before = not_before; | ||
| params.not_after = not_after; | ||
|
|
||
| params.is_ca = rcgen::IsCa::Ca(rcgen::BasicConstraints::Unconstrained); // IsCa::SelfSignedOnly is rejected by C8Y |
Contributor
Author
There was a problem hiding this comment.
This is the change failing the system tests.
I forgot to inject this back to self-signed certificate creation.
ddff773 to
950ebcb
Compare
Bravo555
reviewed
Nov 27, 2024
Comment on lines
16
to
30
| /// Create self-signed device certificate and signing request | ||
| pub struct CreateCertCmd { | ||
| /// The device identifier | ||
| pub id: String, | ||
|
|
||
| /// The path where the device certificate will be stored | ||
| /// The path where the device certificate / request will be stored | ||
| pub cert_path: Utf8PathBuf, | ||
|
|
||
| /// The path where the device private key will be stored | ||
| pub key_path: Utf8PathBuf, | ||
|
|
||
| /// The path where the device CSR file will be stored | ||
| pub csr_path: Option<Utf8PathBuf>, | ||
|
|
||
| /// The component that is configured to host the MQTT bridge logic | ||
| pub bridge_location: BridgeLocation, | ||
| /// The owner of the private key | ||
| pub user: String, | ||
| pub group: String, | ||
| } |
Member
There was a problem hiding this comment.
question: if CreateCertCmd creates certificate signing requests, what does the CreateCsrCmd do?
Is this a leftover of some previous changes, or am I not understanding something?
Contributor
Author
There was a problem hiding this comment.
Oops. This is a leftover. CreateCertCmd is now only used to create self signed certificate.
This now confusing comment has been added during an intermediate step and has to be removed.
Bravo555
approved these changes
Nov 27, 2024
albinsuresh
approved these changes
Nov 28, 2024
| let cert = KeyCertPair::new_selfsigned_certificate(config, &id, &previous_key)?; | ||
|
|
||
| create_cmd.renew_test_certificate(config) | ||
| override_public_key(cert_path, cert.certificate_pem_string()?) |
Contributor
There was a problem hiding this comment.
The explicit removal of the certificate in line. 40 seems redundant now, because of this "override" mechanism. But not harm either.
Contributor
Author
There was a problem hiding this comment.
I thought the same but it is not. Indeed, a certificate is in practice write protected (0o444).
Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>
Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>
This field was used to hack the CreateCertCmd making it creating a CSR instead of a self-signed certificate. Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>
Previously, this user was derived from the bridge type introducing an unrelated domain, even if in practice thin-edge mainly uses certificates to authenticate the bridge. Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>
Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>
Two fields were used to pass a device id to a CSR command, one being only used if the other was not suitable. Now the device id is provided by the caller. Signed-off-by: Didier Wenzek <didier.wenzek@free.fr>
0de8244 to
88bcaad
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Proposed changes
Prepare the integration with Cumulocity CA, by putting apart the code to create self-signed certificate, to create signing request and to renew a certificate.
These different tasks was implemented by a single structure using misc flags to drive checks and file generation.
Now each PEM operation is control by a specific flow.
Types of changes
Paste Link to the issue
#3248
Checklist
cargo fmtas mentioned in CODING_GUIDELINEScargo clippyas mentioned in CODING_GUIDELINESFurther comments