feat: Include certificate roots and certificate policy in GroupContext - WPB-1188 #346

augustocdias · 2023-06-29T13:28:59Z

PR Submission Checklist for internal contributors

The PR Title
- conforms to the style of semantic commits messages¹ supported in Wire's Github Workflow²
- contains a reference JIRA issue number like SQPIT-764
- answers the question: If merged, this PR will: ... ³
The PR Description
- is free of optional paragraphs and you have filled the relevant parts to the best of your ability

What's new in this PR?

Include certificate roots and certificate policy in GroupContext and expose to API

References

https://sparkbox.com/foundry/semantic_commit_messages
https://github.com/wireapp/.github#usage
E.g. feat(conversation-list): Sort conversations by most emojis in the title #SQPIT-764.

codecov-commenter · 2023-06-29T13:58:00Z

Codecov Report

Merging #346 (dcb154d) into develop (7c2d982) will decrease coverage by 1.53%.
The diff coverage is 6.43%.

@@             Coverage Diff             @@
##           develop     #346      +/-   ##
===========================================
- Coverage    72.98%   71.45%   -1.53%     
===========================================
  Files           68       70       +2     
  Lines        11348    11608     +260     
===========================================
+ Hits          8282     8295      +13     
- Misses        3066     3313     +247

Impacted Files	Coverage Δ
crypto/src/error.rs	`0.00% <ø> (ø)`
crypto/src/mls/conversation/decrypt.rs	`69.52% <0.00%> (-0.22%)`	⬇️
crypto/src/mls/conversation/handshake.rs	`69.35% <ø> (ø)`
crypto/src/mls/credential/mod.rs	`87.96% <ø> (ø)`
crypto/src/test_utils/mod.rs	`99.00% <ø> (ø)`
crypto/src/test_utils/x509.rs	`0.00% <0.00%> (ø)`
crypto/src/mls/credential/trust_anchor.rs	`2.27% <2.27%> (ø)`
crypto/src/mls/conversation/config.rs	`76.36% <83.33%> (+1.36%)`	⬆️
crypto/src/mls/conversation/mod.rs	`83.67% <100.00%> (ø)`
crypto/src/mls/external_commit.rs	`85.67% <100.00%> (ø)`

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7c2d982...dcb154d. Read the comment docs.

crypto-ffi/bindings/js/CoreCrypto.ts

Cargo.toml

crypto-ffi/src/wasm.rs

crypto/src/mls/credential/trust_anchor.rs

crypto-ffi/bindings/js/CoreCrypto.ts

beltram · 2023-07-10T10:16:06Z

crypto-ffi/bindings/js/CoreCrypto.ts

@@ -953,6 +974,45 @@ export class CoreCrypto {
        ));
    }

+    /**
+     * Updates the trust anchors for a conversation


Give a bit more context about the validation rules: e.g. fails if you try to remove/add the same anchor etc...
When should a client call this method ? => when a federation event occurs etc..

beltram · 2023-07-10T10:17:10Z

crypto-ffi/src/generic.rs

        Ok(cfg)
    }
 }

+impl From<PerDomainTrustAnchor> for core_crypto::prelude::PerDomainTrustAnchor {
+    fn from(wasm_cfg: PerDomainTrustAnchor) -> Self {


rename wasm_cfg

beltram · 2023-07-10T10:30:31Z

crypto/src/mls/credential/trust_anchor.rs

+
+pub(crate) fn extract_domain_name(certificate: &Certificate) -> CryptoResult<String> {
+    for attr in certificate.tbs_certificate.subject.0.iter().flat_map(|n| n.0.iter()) {
+        // according to the RFC implementations must be prepared to receive the domain component


Which RFC ? This Object Identifier seems to be reserved for LDAPs.
I think you should remove this

https://datatracker.ietf.org/doc/html/rfc5280#section-4.1.2.4

In addition, implementations of this specification MUST be prepared
to receive the domainComponent attribute, as defined in [RFC4519].
The Domain Name System (DNS) provides a hierarchical resource
labeling system.

beltram · 2023-07-10T11:47:59Z

crypto/src/mls/credential/trust_anchor.rs

+            .unwrap_or_else(|| Ok(Vec::new()))?;
+
+        // check if all to remove exists
+        if remove_domain_names


You could simplify this by comparing anchors length before & after retain on L212

beltram · 2023-07-10T11:53:06Z

crypto/src/mls/credential/trust_anchor.rs

+        // check if any new chain is already in the group's context
+        if anchors.iter().any(|a| {
+            add_trust_anchors
+                .iter()
+                .any(|n| n.intermediate_certificate_chain == a.intermediate_certificate_chain)
+        }) {
+            return Err(CryptoError::DuplicateCertificateChain);
+        }


You could factorize this check with this one a few lines below:

// check for duplicate anchors to be added if anchors .iter() .any(|a| add_trust_anchors.iter().any(|n| a.domain_name == n.domain_name)) { return Err(CryptoError::DuplicateDomainName); }

That way you only iterate once the lists. Use a try_for_each()

beltram · 2023-07-10T11:56:07Z

crypto/src/mls/credential/trust_anchor.rs

+
+    /// see [MlsCentral::update_trust_anchors_from_conversation]
+    #[cfg_attr(test, crate::durable)]
+    pub(crate) async fn update_trust_anchors(


You should really extract the validation part of this method into a dedicated method. This way we can reuse it if/when we allow to do the same in a GCE proposal

beltram · 2023-07-10T11:57:15Z

crypto/src/test_utils/central.rs

@@ -432,6 +435,20 @@ impl MlsCentral {
    pub async fn count_credentials_in_keystore(&self) -> usize {
        self.mls_backend.key_store().count::<MlsCredential>().await.unwrap()
    }
+
+    pub async fn add_per_domain_trust_anchor_unwchecked(


beltram · 2023-07-10T11:57:31Z

crypto/src/test_utils/central.rs

+            .collect()
+    }
+
+    pub async fn add_per_domain_trust_anchor_unwchecked(


crypto-ffi/bindings/kt/main/com/wire/crypto/client/MLSClient.kt

crypto-ffi/bindings/swift/Sources/CoreCrypto/CoreCrypto.swift

crypto-ffi/src/CoreCrypto.udl

crypto-ffi/src/generic.rs

OtaK

Small nit to fix but LGTM

OtaK · 2023-07-24T07:20:10Z

crypto-ffi/src/CoreCrypto.udl

@@ -179,6 +185,14 @@ enum CryptoError {
    "ClearingPendingCommitError",
    "SelfCommitIgnored",
    "UnmergedPendingGroup",
+   "X509CertDerError",


Nit: indenting is wrong here (3 spaces, should be 4)

…t - WPB-1188 Co-authored-by: Mathieu Amiot <[email protected]> Co-authored-by: beltram <[email protected]>

augustocdias commented Jul 6, 2023

View reviewed changes

crypto-ffi/bindings/js/CoreCrypto.ts Show resolved Hide resolved

OtaK added the enhancement New feature or request label Jul 6, 2023

OtaK assigned augustocdias Jul 6, 2023

augustocdias force-pushed the feat/wpb-1188 branch 2 times, most recently from eacaf14 to c2cf701 Compare July 10, 2023 08:14

augustocdias requested a review from beltram July 10, 2023 08:14

augustocdias marked this pull request as ready for review July 10, 2023 08:14

OtaK suggested changes Jul 10, 2023

View reviewed changes

Cargo.toml Outdated Show resolved Hide resolved

crypto-ffi/src/wasm.rs Outdated Show resolved Hide resolved

crypto-ffi/src/wasm.rs Outdated Show resolved Hide resolved

crypto/src/mls/credential/trust_anchor.rs Show resolved Hide resolved

beltram suggested changes Jul 10, 2023

View reviewed changes

augustocdias requested review from OtaK and beltram July 10, 2023 12:00

augustocdias force-pushed the feat/wpb-1188 branch from 1b6c431 to 0678a8f Compare July 19, 2023 13:50

augustocdias requested review from beltram and removed request for beltram July 19, 2023 13:53

OtaK suggested changes Jul 20, 2023

View reviewed changes

augustocdias force-pushed the feat/wpb-1188 branch 2 times, most recently from 0cdf8a1 to a98f804 Compare July 21, 2023 06:44

augustocdias requested a review from OtaK July 21, 2023 08:20

augustocdias force-pushed the feat/wpb-1188 branch 2 times, most recently from add3cc9 to 9b87dce Compare July 24, 2023 06:46

OtaK approved these changes Jul 24, 2023

View reviewed changes

beltram approved these changes Jul 24, 2023

View reviewed changes

augustocdias force-pushed the feat/wpb-1188 branch from 9b87dce to e81b4b9 Compare July 24, 2023 11:56

feat: Include certificate roots and certificate policy in GroupContex…

945d073

…t - WPB-1188 Co-authored-by: Mathieu Amiot <[email protected]> Co-authored-by: beltram <[email protected]>

augustocdias force-pushed the feat/wpb-1188 branch from e81b4b9 to 945d073 Compare July 24, 2023 11:56

augustocdias merged commit 2ef9892 into develop Jul 24, 2023
21 checks passed

augustocdias deleted the feat/wpb-1188 branch July 24, 2023 11:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Include certificate roots and certificate policy in GroupContext - WPB-1188 #346

feat: Include certificate roots and certificate policy in GroupContext - WPB-1188 #346

augustocdias commented Jun 29, 2023

codecov-commenter commented Jun 29, 2023 •

edited

Loading

beltram Jul 10, 2023

beltram Jul 10, 2023

beltram Jul 10, 2023

augustocdias Jul 10, 2023

beltram Jul 10, 2023

beltram Jul 10, 2023

beltram Jul 10, 2023

beltram Jul 10, 2023

beltram Jul 10, 2023

OtaK left a comment

OtaK Jul 24, 2023

feat: Include certificate roots and certificate policy in GroupContext - WPB-1188 #346

feat: Include certificate roots and certificate policy in GroupContext - WPB-1188 #346

Conversation

augustocdias commented Jun 29, 2023

PR Submission Checklist for internal contributors

What's new in this PR?

References

codecov-commenter commented Jun 29, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OtaK left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Jun 29, 2023 •

edited

Loading