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

Fix multiple issues in current PR by crating V2 APIs #267

Merged
merged 6 commits into from
Sep 13, 2024

Conversation

danmamsft
Copy link
Contributor

@danmamsft danmamsft commented Aug 31, 2024

The current APIs have two major issues:

  1. the string returned back to caller doesn't include a length, so the caller cannot use memory safe str functions which requires length. This is caught by OneFuzz: https://dev.azure.com/microsoft/OS/_workitems/edit/53371227. The fix in V2 API is to return length as well
  2. the C string returned to caller (both data blob and error string) is not freed, causing memory leak. The fix is to let caller allocates the memory and pass in allocated buffer. If the buffer is not pre-allocated or is too small, the V2 API will return buffer too small with the expected size for caller to reallocate the proper size.

one improvement made to encrypt and decrypt API is made the algorithm as a parameter

Test:
I have modifies testMoc.exe in IgvmAgent (please see my draft code in branch user/dama/testwrapperfix in Igvmagent repo for reference).
Here are the test I have done:
SecurityLoginV2: tested happy path; wrong/empty login file path; invalid/empty group name
CrateKeyV2: tested happy path to create both AES and RSA key; invalid/empty group name
KeyvaultKeyDecryptDataV2: tested happy path; buffer too small case (caller call API twice to get decrypt working); invalid algorithm; invalid groupName/keyvaultName; empty input data
KeyvaultKeyEncryptDataV2: same test coverage as decrypt.Also tested Encrypt and decrypt together and made sure decrypted data matches original plaintext
KeyvaultKeyCreateOrUpdateV2: tested happy path to create AES key and RSA key; tested invalid algorithm;invalid/empty groupName/keyvaultName
KeyvaultGetPublicKeyV2: tested happy path; tested return key empty; invalid/empty groupName/keyvaultName/keyName

Tested the onefuzz repro, with the fix, original error went away.

Tested with replacing igvmagent in a haas cluster. TVM is stopping and starting with no issue.

wrapper/cpp/main.go Outdated Show resolved Hide resolved
wrapper/cpp/main.go Outdated Show resolved Hide resolved
wrapper/cpp/main.go Outdated Show resolved Hide resolved
SethBe
SethBe previously approved these changes Sep 9, 2024
Copy link
Contributor

@SethBe SethBe left a comment

Choose a reason for hiding this comment

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

:shipit:

@raghavendra-nataraj
Copy link
Contributor

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@raghavendra-nataraj
Copy link
Contributor

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

wrapper/cpp/main.go Outdated Show resolved Hide resolved
SethBe
SethBe previously approved these changes Sep 13, 2024
Copy link
Contributor

@SethBe SethBe left a comment

Choose a reason for hiding this comment

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

:shipit:

Copy link
Contributor

@SethBe SethBe left a comment

Choose a reason for hiding this comment

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

:shipit:

@raghavendra-nataraj
Copy link
Contributor

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@danmamsft danmamsft merged commit 547960b into main Sep 13, 2024
6 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