Address two issues#461
Merged
Merged
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces a UtcTime enum to distinguish between reliable and unreliable (Last-Known-Good) clock states, updating certificate verification logic to skip NotBefore checks when the clock is unreliable. It also refines session expiration handling to allow ongoing exchanges to complete before a session is fully reclaimed, specifically addressing scenarios like the RemoveFabric command. Additionally, the SessionNotFound status code is updated to not require the reliability flag in secure channel communications, and various components are updated to utilize the new time-tracking API. I have no feedback to provide.
|
PR #461: Size comparison from b23a6ab to 3e7c20d Full report (8 builds for (core), dimmable-light, onoff-light, onoff-light-bt, speaker)
|
ivmarkov
force-pushed
the
fix-cert-validation-issue
branch
from
3e7c20d to
b4c53f8
Compare
ivmarkov
force-pushed
the
fix-cert-validation-issue
branch
from
b4c53f8 to
2fc1e65
Compare
|
PR #461: Size comparison from b23a6ab to 2fc1e65 Full report (8 builds for (core), dimmable-light, onoff-light, onoff-light-bt, speaker)
|
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
1 participant
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.
This PR is containing two commits, each addressing a separate bug:
Commit 1 (f5f7a0d)
The recently-introduced "expired"
Sessionstate is currently treated too "drastically".What I mean by that is that if we receive a message on an already active exchange (say, an innocent ACK), if that's for an expired session we'll claim that this session does not exist.
That's too drastic because e.g. - when the "remove fabric" command on the NOC cluster is invoked by a peer - our own command response to that command cannot complete, because - according to the above logic - we never receive the ACK reply ffrom the peer for our command response and therefore we keep re-trying the sending!
The fix is to only not allow expired sessions for new exchanges on that session, which are about to be created. For on-going exchanges, the fact that the session is expired is not considered a show-stopper anymore.
Commit 2 (3e7c20d)
This is addressing #459 .
The bug here is that if we don't have a reliable wall clock UTC timestamp, but only a "last known good UTC timestamp" - like the one we automatically get during
rs-matterbuild-time - we cannot use the lkgut value to check the validity start of a certificate because the certificate might actually be generated AFTER the build of rs-matter had happened.