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

Remade attestation NONE to work in all cases #1942

Merged
merged 19 commits into from
Nov 1, 2022
Merged

Conversation

Jeffery-Wasty
Copy link
Contributor

@Jeffery-Wasty Jeffery-Wasty commented Oct 27, 2022

Our original implementation of attestation NONE did work, but as was later shown, not for all cases, and thus a fix was needed. This new NONE enclave provider is a copy of our HGS/VSM enclave provider with attestation validation stripped, and response validation stripped to only what is needed.

SQLServerNoneEnclaveProvider:

  • Removed the URL parameter as its never used, removed the catch as a result as IOException is then never thrown
  • Removed validation for NONE. After receiving the response, when NONE is selected, the driver does not attempt to verify the response for attestation (enclave details, certificate, etc.) and only computes the shared secret from server's DH public key.
  • All that needs to be sent to the server is the enclave type, the challenge, and encrypted ECDH. Subsequently remove anything related to NONCE or attestation URL
  • Changed how response is handled. We're only concerned with: total size, session info size and session info, and the server enclave ECDH key, used to validate the enclave session.

EnclavePackageTest

  • Added a condition for the second test here (testing a null attestation URL) to not be tested with NONE, as no URL is allowed for NONE, while the test expects a failure.

@Jeffery-Wasty Jeffery-Wasty self-assigned this Oct 27, 2022
@Jeffery-Wasty Jeffery-Wasty added the Under Review Used for pull requests under review label Oct 27, 2022
@Jeffery-Wasty Jeffery-Wasty added this to the 12.1.0 milestone Oct 27, 2022
lilgreenbird
lilgreenbird previously approved these changes Oct 27, 2022
@Jeffery-Wasty Jeffery-Wasty changed the title Fix attestation for NONE as it's now broken Changes for attestation NONE to work in all cases Oct 27, 2022
@Jeffery-Wasty Jeffery-Wasty changed the title Changes for attestation NONE to work in all cases Remade attestation NONE to work in all cases Oct 27, 2022
tkyc
tkyc previously approved these changes Oct 27, 2022
David-Engel
David-Engel previously approved these changes Oct 27, 2022
@Jeffery-Wasty Jeffery-Wasty dismissed stale reviews from David-Engel, tkyc, and lilgreenbird via ab8e54d October 28, 2022 18:08
@Jeffery-Wasty Jeffery-Wasty merged commit a2bcb60 into main Nov 1, 2022
@Jeffery-Wasty Jeffery-Wasty removed the Under Review Used for pull requests under review label Nov 1, 2022
@Jeffery-Wasty Jeffery-Wasty deleted the attestationNoneFix branch December 21, 2022 22:36
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.

4 participants