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: Integration with Cumulocity CA #3259

Draft
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

didier-wenzek
Copy link
Contributor

@didier-wenzek didier-wenzek commented Nov 21, 2024

Proposed changes

  • Impl tedge cert download c8y
  • Impl tedge cert renew c8y
  • Enhance tedge config with setting for certificate validity period.
  • Enhance tedge cert show to display number of days before expiration.
  • Make sure the current certificate is not erased on renewal

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

#3248

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

@didier-wenzek didier-wenzek added feature Change request theme:c8y Theme: Cumulocity related topics theme:certificates Theme: Device certificate topics labels Nov 21, 2024
)
}
}
warning!("Will retry in 5 seconds");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

One needs to loop only when it makes senses to retry: i.e. the c8y endpoint has been reached but the device has not been registered yet.

Question: does Cumulocity return a specific HTTP status when the device is not registered?

Copy link
Contributor

@reubenmiller reubenmiller Feb 27, 2025

Choose a reason for hiding this comment

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

No I don't think any other response is returned as it would give away information.

Sorry I didn't see that this comment was from last year!

@didier-wenzek didier-wenzek force-pushed the feat/integration-with-cumulocity-ca branch from 844dc34 to 284c8ee Compare November 22, 2024 10:16
@didier-wenzek didier-wenzek force-pushed the feat/integration-with-cumulocity-ca branch from 284c8ee to fba8ed6 Compare November 22, 2024 16:00
Copy link
Contributor

github-actions bot commented Nov 28, 2024

Robot Results

✅ Passed ❌ Failed ⏭️ Skipped Total Pass % ⏱️ Duration
570 0 3 570 100 1h35m28.294394s

Comment on lines 138 to 173
// For unknown reasons, the base64 encoding differs
// when compared with the value computed using `openssl pkcs7 -print_certs`
let openssl_x509 = r#"
-----BEGIN CERTIFICATE-----
MIIBeTCCASCgAwIBAgIGAZU9kiLNMAoGCCqGSM49BAMCMEIxFjAUBgNVBAYTDVVu
aXRlZCBTdGF0ZXMxEzARBgNVBAoTCkN1bXVsb2NpdHkxEzARBgNVBAMTCm1hbmFn
ZW1lbnQwHhcNMjUwMjI1MTQ0NTQyWhcNMjYwMjI0MDk0MTQ0WjBGMRowGAYDVQQD
DBFkaWRpZXItZGV2aWNlLTAwMTESMBAGA1UECgwJVGhpbiBFZGdlMRQwEgYDVQQL
DAtUZXN0IERldmljZTBZMBMGByqGSM49AgEGCCqGSM49AwEHA0IABPBKM0T/sAlk
S2tdbYI/YtIuVyXAPKHhjAeanAOGdMKb5nF55cFRxQBFwVc612bEyx630KAxCXUW
PDbnl0hKmqIwCgYIKoZIzj0EAwIDRwAwRAIgJqlgFbLOsNTfohUS3I592UMDRtb0
cTenAfPZf0sCCXUCIAPSdprLGJ4fRFUTW8m10PeIIkevMs0zcR9/aA06G7Mk
-----END CERTIFICATE-----
"#
.to_string();

// However, the certificate contents match
let openssl_cert = PemCertificate::from_pem_string(&openssl_x509).unwrap();
assert_eq!(cert.subject().unwrap(), openssl_cert.subject().unwrap());
assert_eq!(cert.issuer().unwrap(), openssl_cert.issuer().unwrap());
assert_eq!(
cert.not_before().unwrap(),
openssl_cert.not_before().unwrap()
);
assert_eq!(cert.not_after().unwrap(), openssl_cert.not_after().unwrap());

// With an exception on the thumbprint
// assert_eq!(cert.thumbprint().unwrap(), openssl_cert.thumbprint().unwrap());
assert_eq!(
cert.thumbprint().unwrap(),
"A392A53D3D4263A32C779D06CDDE13A847F272B6".to_string()
);
assert_eq!(
openssl_cert.thumbprint().unwrap(),
"9C68C7EC9A860366FB8D2697C53B2543D9EA525C".to_string()
);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can this be the root cause preventing c8y to accept the extracted certificate?

Copy link
Contributor

@reubenmiller reubenmiller Mar 1, 2025

Choose a reason for hiding this comment

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

After doing a deep dive into the differences in the certificates I've found exactly where the difference is coming from:

I have a working example with refactored code (using rasn/rasm-csm over the higher level cryptographic-message-syntax crate) where the thumbprint is the same as the one computed by openssl. The code can be found on my temp branch which uses a fork of the x509-certificate crate to avoid the "problem".

The problem originates from this part in the x509-certificate where it does the PEM encoding of the Der formatted value when handing the AlgorithmIdentifier and its handling of the optional parameters. The cryptographic-message-syntax crate suffers from the same problem as it uses the same x509-certificate crate.

The x509-certificate crate purposefully uses the ASN.1 NULL value when an AlgorithmIdentifier object does not have any parameters associated with it.

There is a comment in the code of x509-certificate explaining the motivation of using a ASN.1 NULL tag for (despite the ASN.1 spec clearly stating that the AlgorithmIdentifier parameter object is OPTIONAL).

fn encoded_values(&self, mode: Mode) -> impl Values + '_ {
    // parameters is strictly OPTIONAL, which means we can omit it completely.
    // However, it is common to see this field encoded as NULL and some
    // parsers seem to insist the NULL be there or else they refuse to
    // parse the ASN.1. So we ensure this field is always set.
    let captured = if let Some(params) = self.parameters.as_ref() {
        params.clone()
    } else {
        AlgorithmParameter(Captured::from_values(mode, ().encode_as(Tag::NULL)))
    };

    encode::sequence((self.algorithm.clone().encode(), captured))
}

Other libraries like openssl, and golang x509 libraries do not use ASN.1 NULL values if the AlgorithmIdentifier Parameters are empty, so this results in the PEM encoded x509 certificates having a different thumbprint when compared to the any Rust code which uses the 509-certificate crate.

Whilst this may not be a bug as the certificate should still be functionally equal, the difference in
thumbprints can give the impression that the cert is functionality different, and maybe this is causing the connection problem to Cumulocity (like you said), but I haven't verified this just yet.

Copy link
Contributor

Choose a reason for hiding this comment

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

And I've found an easier solution for encoding the x509 cert from DER into the PEM format by using the existing dependency to the pem crate. Now no custom forks are needed and it adds very little crate dependencies (in comparison with the x509-certificate and cryptographic-message-syntax crates.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Change request theme:c8y Theme: Cumulocity related topics theme:certificates Theme: Device certificate topics
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants