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

feat(cert): Create a Certificate signing request using tedge cert create-csr #2783

Merged
merged 6 commits into from
Mar 22, 2024

Conversation

Ruadhri17
Copy link
Contributor

@Ruadhri17 Ruadhri17 commented Mar 15, 2024

Proposed changes

This PR is introducing new tedge cert command: create-csr along with new config option device.csr_path. This allow to create a certificate signing request which later can be used to create device certificate using CA. The Syntax for the command is following:

# Create a new private key (if it does not already exist)
tedge cert create-csr --device-id mydevice001

# Re-use the existing device-id from an already existing certificate
tedge cert create-csr

Command will reuse existing public certificate to use its CommonName.

Moreover, the command will reuse private key if it already exists. Same goes for tedge cert create.

Types of changes

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Improvement (general improvements like code refactoring that doesn't explicitly fix a bug or add any new functionality)
  • Documentation Update (if none of the other choices apply)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Paste Link to the issue

Checklist

  • I have read the CONTRIBUTING doc
  • I have signed the CLA (in all commits with git commit -s)
  • I ran cargo fmt as mentioned in CODING_GUIDELINES
  • I used cargo clippy as mentioned in CODING_GUIDELINES
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)

Further comments

Copy link
Contributor

github-actions bot commented Mar 15, 2024

Robot Results

✅ Passed ❌ Failed ⏭️ Skipped Total Pass % ⏱️ Duration
424 0 3 424 100 0s

@Ruadhri17 Ruadhri17 changed the title Create a Certificate signing request using tedge cert create-csr feat(cli): Create a Certificate signing request using tedge cert create-csr Mar 15, 2024
@Ruadhri17 Ruadhri17 changed the title feat(cli): Create a Certificate signing request using tedge cert create-csr feat(cert): Create a Certificate signing request using tedge cert create-csr Mar 15, 2024
Copy link

codecov bot commented Mar 15, 2024

Codecov Report

Attention: Patch coverage is 81.94444% with 52 lines in your changes are missing coverage. Please review.

Project coverage is 76.6%. Comparing base (19773d6) to head (281fe69).
Report is 13 commits behind head on main.

❗ Current head 281fe69 differs from pull request most recent head 1395167. Consider uploading reports for the commit 1395167 to get more accurate results

Additional details and impacted files
Files Coverage Δ
crates/core/tedge/src/cli/certificate/error.rs 33.3% <ø> (ø)
crates/common/tedge_utils/src/file.rs 67.6% <90.0%> (-0.3%) ⬇️
crates/core/tedge/src/cli/certificate/renew.rs 86.2% <66.6%> (+9.3%) ⬆️
crates/common/certificate/src/lib.rs 89.4% <95.4%> (+1.6%) ⬆️
.../tedge_config/src/tedge_config_cli/tedge_config.rs 27.8% <54.5%> (-52.4%) ⬇️
crates/core/tedge/src/cli/certificate/cli.rs 43.7% <30.0%> (-6.3%) ⬇️
...rates/core/tedge/src/cli/certificate/create_csr.rs 86.5% <86.5%> (ø)
crates/core/tedge/src/cli/certificate/create.rs 82.9% <58.3%> (-6.0%) ⬇️

... and 206 files with indirect coverage changes

@reubenmiller
Copy link
Contributor

@didier-wenzek Do you think it would make sense if there was also an option to override where to write the csr file to? Reasoning is that the CSR is generally something that has to be triggered by another system (e.g. service which is doing the request to a PKI API and the service needs to know how to access the resulting csr file...and having to do a tedge config get csr.path involves having to know too much about tedge)

For example:

tedge cert csr --output-file ./mycsr

Or even writing the csr to stdout?

tedge cert csr > ./mycsr

Copy link
Contributor

@reubenmiller reubenmiller left a comment

Choose a reason for hiding this comment

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

Given that this will be used by users, can you please add a system test?

Example test cases:

  • Generate CSR using the device-id from an existing certificate
  • Generate CSR without an existing certificate (requiring the user to specify the --device-id option)

You can validate the CSR by using openssl to makes sure it is valid and that it is generated from the expected private key.

@didier-wenzek
Copy link
Contributor

@didier-wenzek Do you think it would make sense if there was also an option to override where to write the csr file to? Reasoning is that the CSR is generally something that has to be triggered by another system (e.g. service which is doing the request to a PKI API and the service needs to know how to access the resulting csr file...and having to do a tedge config get csr.path involves having to know too much about tedge)

For example:

tedge cert csr --output-file ./mycsr

Or even writing the csr to stdout?

tedge cert csr > ./mycsr

Why not.

That said, to interact with a CA what we provide now might not be enough to get a device certificate and is definitely not enough to get certificates for local entities. Indeed, we will have to provide not only a CN, but also critical infos (Subject, Subject Alternative Name, Key Usage, Extended Key Usage).

So we will also need to provide a CSR template on the command line as well as template examples for each kind of entity (the main device, the MQTT broker, tedge_agent, tedge_mapper, child devices ...).

Copy link
Contributor

@didier-wenzek didier-wenzek left a comment

Choose a reason for hiding this comment

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

This is working fine as described. However, can you share the on-disk writing logic between the commands to create a self-signed certificate and a CSR. Indeed, this logic contains several security-critical steps that is would be better keep in one place.

crates/core/tedge/src/cli/certificate/create.rs Outdated Show resolved Hide resolved
crates/core/tedge/src/cli/certificate/create_csr.rs Outdated Show resolved Hide resolved
crates/core/tedge/src/cli/certificate/create_csr.rs Outdated Show resolved Hide resolved
crates/core/tedge/src/cli/certificate/create_csr.rs Outdated Show resolved Hide resolved
crates/core/tedge/src/cli/certificate/create_csr.rs Outdated Show resolved Hide resolved
docs/src/operate/security/certificate-signing-request.md Outdated Show resolved Hide resolved
Copy link
Contributor

@didier-wenzek didier-wenzek left a comment

Choose a reason for hiding this comment

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

Approved. Thank you.

Copy link
Contributor

@reubenmiller reubenmiller left a comment

Choose a reason for hiding this comment

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

Still a few minor points to address (mostly usability things)

Copy link
Contributor

@reubenmiller reubenmiller left a comment

Choose a reason for hiding this comment

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

Approved. Nice addition

@Ruadhri17 Ruadhri17 added this pull request to the merge queue Mar 22, 2024
Merged via the queue into thin-edge:main with commit 6d3ae41 Mar 22, 2024
30 checks passed
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 this pull request may close these issues.

3 participants