Moved attestation factory back to static method on attestation class … #3682

LarryOsterman · 2022-05-27T20:13:40Z

…and return a concrete type not a pointer

Pull Request Checklist

Please leverage this checklist as a reminder to address commonly occurring feedback when submitting a pull request to make sure your PR can be reviewed quickly:

See the detailed list in the contributing guide.

…and return a concrete type not a pointer

LarryOsterman · 2022-05-27T21:59:46Z

/azp run prepare-pipelines

azure-pipelines · 2022-05-27T21:59:55Z

Azure Pipelines successfully started running 1 pipeline(s).

LarryOsterman · 2022-05-28T00:37:12Z

Tagging @JeffreyRichter and @antkmsft since we were talking about this PR yesterday.

ahsonkhan

Some minor feedback and questions.

samples/integration/vcpkg-all-smoke/src/main.cpp

ahsonkhan · 2022-05-31T16:52:48Z

...ation/azure-security-attestation/inc/azure/attestation/attestation_administration_client.hpp

          m_pipeline(attestationClient.m_pipeline),
-          m_tokenValidationOptions(attestationClient.m_tokenValidationOptions){};
+          m_tokenValidationOptions(attestationClient.m_tokenValidationOptions),
+          m_attestationSigners(attestationClient.m_attestationSigners){};


Breaking change? If so, add CL entry.

All this does is copy a member variable in the copy constructor, is that breaking?

Isn't this adding a new mandatory parameter, that wasn't there before?

...ation/azure-security-attestation/inc/azure/attestation/attestation_administration_client.hpp

sdk/attestation/azure-security-attestation/inc/azure/attestation/attestation_client.hpp

sdk/attestation/azure-security-attestation/samples/policy/set_sealed_policy.cpp

...attestation/azure-security-attestation/test/ut/recordings/TpmAttestationTests.AttestTpm.json

ahsonkhan · 2022-05-31T17:08:11Z

sdk/attestation/azure-security-attestation/CHANGELOG.md

 - `AddIsolatedModeCertificatesOptions` becomes `AddIsolatedModeCertificateOptions`
 - `RemoveIsolatedModeCertificatesOptions` becomes `RemoveIsolatedModeCertificateOptions`
 - Renamed `AttestEnclaveOptions` to `AttestSgxEnclaveOptions` and `AttestOpenEnclaveOptions`.
- Split out `AttestationClient::Create` into its own factory class `AttestationClientFactory`.


Didn't we also remove RetrieveResponseValidationCollateral in the previous PR or was that always private? I don't see it being called in the CL breaking change list.

Removed in the previous CR.

What do you mean?
It was there in the beta.2 release correct, and the method no longer exists now? That's a breaking change change worth calling out in the changelog, but I don't see it listed in the beta.3 CL.

If it was added and removed during this release, then ignore my comment :)

Looks like it was always private. We might want to update the docs on public API that reference the method still, because the end user can't call it:

https://github.com/Azure/azure-sdk-for-cpp/search?q=RetrieveResponseValidationCollateral

azure-sdk-for-cpp/sdk/attestation/azure-security-attestation/inc/azure/attestation/attestation_administration_client.hpp

Line 334 in 0fd0267

void RetrieveResponseValidationCollateral(

It was public for 1.0.0.beta-1, and was removed for either 1.0.0.beta-2 or 1.0.0.beta-3 (not sure which).

Could you show me the public API documentation for this method? I thought I had searched for all of them and removed them, but it's possible I missed a couple.

There are a couple places (I linked the GitHub search of the codebase above), here's an example:

azure-sdk-for-cpp/sdk/attestation/azure-security-attestation/inc/azure/attestation/attestation_administration_client.hpp

Lines 98 to 105 in ebe084b

* @note \b Note: The RetrieveResponseValidationCollateral API \b MUST be called before the

* GetAttestationPolicy API is called to retrieve the information needed to validate the

* result returned by the service.

*/

Response<Models::AttestationToken<std::string>> GetAttestationPolicy(

Models::AttestationType const& attestationType,

GetPolicyOptions const& options = GetPolicyOptions{},

Azure::Core::Context const& context = Azure::Core::Context{}) const;

LarryOsterman · 2022-05-31T19:05:33Z

/azp run cpp - attestation

azure-pipelines · 2022-05-31T19:05:51Z

Azure Pipelines successfully started running 1 pipeline(s).

LarryOsterman · 2022-05-31T20:40:13Z

/azp run cpp - attestation

azure-pipelines · 2022-05-31T20:40:30Z

Azure Pipelines successfully started running 1 pipeline(s).

LarryOsterman · 2022-05-31T21:38:48Z

/azp run cpp - attestation

azure-pipelines · 2022-05-31T21:39:05Z

Azure Pipelines successfully started running 1 pipeline(s).

...ation/azure-security-attestation/inc/azure/attestation/attestation_administration_client.hpp

sdk/attestation/azure-security-attestation/inc/azure/attestation/attestation_client.hpp

...ation/azure-security-attestation/inc/azure/attestation/attestation_administration_client.hpp

…ooses the ability to use 'auto'

sdk/attestation/azure-security-attestation/CHANGELOG.md

sdk/attestation/azure-security-attestation/inc/azure/attestation/attestation_client.hpp

Co-authored-by: Anton Kolesnyk <41349689+antkmsft@users.noreply.github.com>

…om/LarryOsterman/azure-sdk-for-cpp into larryo/attestation_create_by_value

LarryOsterman · 2022-06-01T18:30:56Z

/check-enforcer evaluate

ahsonkhan · 2022-06-02T18:49:31Z

...ation/azure-security-attestation/inc/azure/attestation/attestation_administration_client.hpp

+     * @param options The options to customize the client behavior.
+     * @return The newly created client.
+     */
+    static AttestationAdministrationClient CreateConcrete(


Remove these if no longer needed.

LarryOsterman added 2 commits May 27, 2022 13:07

Moved attestation factory back to static method on attestation class …

2c1a53e

…and return a concrete type not a pointer

Fixed factory in readme file

938cd4b

LarryOsterman requested review from RickWinter, ahsonkhan, anilba06, antkmsft, gearama, gkostal, olkroshk and vhvb1989 as code owners May 27, 2022 20:13

LarryOsterman added 4 commits May 27, 2022 13:50

Updated recordings for TPM attestation test

4edf95a

Fixed unit tests

51a8541

clang-format

db81845

clang-format

016fd6f

LarryOsterman requested a review from JeffreyRichter May 28, 2022 00:36

JeffreyRichter approved these changes May 29, 2022

View reviewed changes

ahsonkhan assigned LarryOsterman May 31, 2022

ahsonkhan added the Azure.Core label May 31, 2022

ahsonkhan reviewed May 31, 2022

View reviewed changes

LarryOsterman added 2 commits May 31, 2022 11:32

Factory class returns both pointers and concrete classes

b327f04

Doxygen and clang-format

8043d27

Added constructor tests

2db50e7

LarryOsterman added 3 commits May 31, 2022 13:55

Updated recordings

723c241

Added recordings for creation tests

027654d

clang_format

c579e4f

antkmsft requested changes May 31, 2022

View reviewed changes

...ation/azure-security-attestation/inc/azure/attestation/attestation_administration_client.hpp Outdated Show resolved Hide resolved

antkmsft reviewed May 31, 2022

View reviewed changes

sdk/attestation/azure-security-attestation/inc/azure/attestation/attestation_client.hpp Outdated Show resolved Hide resolved

antkmsft reviewed May 31, 2022

View reviewed changes

...ation/azure-security-attestation/inc/azure/attestation/attestation_administration_client.hpp Outdated Show resolved Hide resolved

LarryOsterman added 2 commits May 31, 2022 16:08

Removed ClientCreator and only create concrete types (ClientCreator l…

16490c8

…ooses the ability to use 'auto'

Updated changelog and readme

3459711

LarryOsterman enabled auto-merge (squash) May 31, 2022 23:36

antkmsft reviewed May 31, 2022

View reviewed changes

sdk/attestation/azure-security-attestation/CHANGELOG.md Outdated Show resolved Hide resolved

antkmsft approved these changes Jun 1, 2022

View reviewed changes

vhvb1989 reviewed Jun 1, 2022

View reviewed changes

sdk/attestation/azure-security-attestation/inc/azure/attestation/attestation_client.hpp Show resolved Hide resolved

LarryOsterman and others added 3 commits May 31, 2022 17:30

Update sdk/attestation/azure-security-attestation/CHANGELOG.md

862692f

Co-authored-by: Anton Kolesnyk <41349689+antkmsft@users.noreply.github.com>

Trivial change to kick pipelines

adb9370

Merge branch 'larryo/attestation_create_by_value' of https://github.c…

4e6f598

…om/LarryOsterman/azure-sdk-for-cpp into larryo/attestation_create_by_value

LarryOsterman merged commit 0fd0267 into Azure:main Jun 1, 2022

LarryOsterman deleted the larryo/attestation_create_by_value branch June 1, 2022 18:34

ahsonkhan reviewed Jun 2, 2022

View reviewed changes

LarryOsterman mentioned this pull request Jul 5, 2022

Update feature/websockets to match main. (#3767) #3795

Closed

14 tasks

	* @note \b Note: The RetrieveResponseValidationCollateral API \b MUST be called before the
	* GetAttestationPolicy API is called to retrieve the information needed to validate the
	* result returned by the service.
	*/
	Response<Models::AttestationToken<std::string>> GetAttestationPolicy(
	Models::AttestationType const& attestationType,
	GetPolicyOptions const& options = GetPolicyOptions{},
	Azure::Core::Context const& context = Azure::Core::Context{}) const;

Moved attestation factory back to static method on attestation class … #3682

Moved attestation factory back to static method on attestation class … #3682

Uh oh!

Conversation

LarryOsterman commented May 27, 2022

Pull Request Checklist

Uh oh!

LarryOsterman commented May 27, 2022

Uh oh!

azure-pipelines bot commented May 27, 2022

Uh oh!

LarryOsterman commented May 28, 2022

Uh oh!

ahsonkhan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LarryOsterman commented May 31, 2022

Uh oh!

azure-pipelines bot commented May 31, 2022

Uh oh!

LarryOsterman commented May 31, 2022

Uh oh!

azure-pipelines bot commented May 31, 2022

Uh oh!

LarryOsterman commented May 31, 2022

Uh oh!

azure-pipelines bot commented May 31, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

LarryOsterman commented Jun 1, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants