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

355 otar decrypt status #360

Merged
merged 8 commits into from
Dec 5, 2024
Merged

355 otar decrypt status #360

merged 8 commits into from
Dec 5, 2024

Conversation

dccutrig
Copy link
Contributor

@dccutrig dccutrig commented Dec 4, 2024

No description provided.

@dccutrig dccutrig self-assigned this Dec 4, 2024
…ctive with invalid settings to decrypt call fails for libgcrypt and error bubbles up.
@dccutrig dccutrig marked this pull request as ready for review December 4, 2024 21:54
@codecov-commenter
Copy link

codecov-commenter commented Dec 4, 2024

Codecov Report

Attention: Patch coverage is 95.18072% with 4 lines in your changes missing coverage. Please review.

Project coverage is 84.12%. Comparing base (a3f2e78) to head (e782c72).
Report is 50 commits behind head on dev.

Files with missing lines Patch % Lines
src/core/crypto_key_mgmt.c 63.63% 2 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##              dev     #360      +/-   ##
==========================================
+ Coverage   84.04%   84.12%   +0.07%     
==========================================
  Files          83       81       -2     
  Lines       23507    21719    -1788     
  Branches     1783     1757      -26     
==========================================
- Hits        19757    18271    -1486     
+ Misses       3075     2765     -310     
- Partials      675      683       +8     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@@ -1974,7 +1974,7 @@ int32_t Crypto_TC_ProcessSecurity_Cam(uint8_t *ingest, int *len_ingest, TC_t *tc
else
{
// Some Magic here to log that an inappropriate SA was attempted to be used for EP
status = CRYPTO_LIB_ERR_SPI_INDEX_OOB; // TODO: Do we want a different error code for this?
status = CRYPTO_LIB_ERR_SDLS_EP_WRONG_SPI; // TODO: Do we want a different error code for this?
Copy link
Member

Choose a reason for hiding this comment

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

Probably a good call on changing this error code to something more specific. Should we remove that comment now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rjbrown2 There is some weirdness here where we log an error status on 1978, then reset the final status to success. Could you ping me on that if you know/understand why it looks like that?

Copy link
Member

Choose a reason for hiding this comment

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

That is done that way so that we log that we attempted to process a PDU, but did not because it wasn't the appropriate SA. We log the error so that users would know what happened (without giving away too much other information), and then return successfully.

Copy link
Member

Choose a reason for hiding this comment

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

We didn't set and return any failures here, as that would have required a lot more manipulation, and Donnie and I decided that this would be the best method for now without a big discussion...unless you disagree...then hack away.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense. Sounds like by doing this and bubbling the error up it causes a lot of issues. I'll open a separate issue to track this.

@dccutrig dccutrig merged commit 86992f1 into dev Dec 5, 2024
10 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.

5 participants