Calling into thim agent to get collateral. #149

msft-gumunjal · 2021-09-09T23:23:30Z

Adding call to thim agent for Get cert collateral.
Updating the API call to THIM service to use the new noauth API introduced in the service.

Testing:

Unit tests pass on Windows.

sidse-msft

First pass through code. I haven't looked at every file yet; wanted to get some of these resolved before doing another pass.

sidse-msft · 2021-09-13T13:58:04Z

src/Linux/curl_easy.cpp

@@ -70,7 +70,7 @@ char const* curl_easy::error::what() const noexcept
 ///////////////////////////////////////////////////////////////////////////////
 // curl_easy implementation
 ///////////////////////////////////////////////////////////////////////////////
-std::unique_ptr<curl_easy> curl_easy::create(const std::string& url, const std::string* const p_body)
+std::unique_ptr<curl_easy> curl_easy::create(const std::string& url, const std::string* const p_body, LPCWSTR httpVerb))


Just curious, why use LPCWSTR instead of const std::string& ?

The main reason for this file is to keep the structure consistent with the other curl_easy.cpp file(in src/Windows). The variable is used by "WinHttpOpenRequest" in src/Windows/curl_easy.cpp file.

This file is Linux specific. Other parameters are std::string, so not appreciating the need for consistency with the windows version

This parameter is used by "WinHttpOpenRequest" and the data type is supposed to be "LPCWSTR". Either I can pass a string to the function and then convert it inside the function or just pass the desired data type. I chose the latter one because I thought that's the better out of the two. I can update if required.

src/Linux/curl_easy.cpp

sidse-msft · 2021-09-13T13:58:42Z

src/Linux/curl_easy.cpp

+        if (httpVerb == L"POST") {
+            easy->set_opt_or_throw(CURLOPT_POST, 1L);
+        }
+        else {


I don't know if we have a style checker integrated into the VS solution. If we do and it passes, then ignore this comment. The convention I see in other parts of the file is to throw opening curly braces on the line after the if/else statement. We should probably keep doing that to stay consistent.

I dont think we have a style checker. Let me try to add one.

sidse-msft · 2021-09-13T14:05:36Z

src/dcap_provider.cpp

 static char DEFAULT_CERT_URL[] =
-    "https://global.acccache.azure.net/sgx/certificates";
+    "https://global.acccache.azure.net/sgx/certificates/";


Once THIM Service deploys new controller to align with Intel, this should be updated. The final URL will be like:
https://global.acccache.azure.net/sgx/certification/v3/pckcert?qeid={}&cpusvn={}&pcesvn={}&pceid={}&encrypted_ppid={}

Agreed! I coded this based on the current prod code. Will update this

The new URL is available now.

Hey guys -- I see we've changed from certificates to "certification" -- in the last correspondence I read I saw

The ask was Thim agent to use "certificates" like THIM Service did.
-- Do I need to make a change to thim agent to stay consistent here then? I wasn't aware of this. Apologies if I missed something

No change needed, certificates is fine here.

src/dcap_provider.cpp

src/dcap_provider.h

src/dcap_provider.cpp

sidse-msft · 2021-09-13T14:21:06Z

src/dcap_provider.cpp

@@ -54,12 +54,15 @@ static const std::map<std::string, std::string> default_values = {
 // New API version used to request PEM encoded CRLs
 constexpr char API_VERSION_LEGACY[] = "api-version=2018-10-01-preview";
 constexpr char API_VERSION[] = "api-version=2020-02-12-preview";


We have a new API version for the THIM Agent APIs. However, I want to confirm -- what format CRL does Azure DCAP want now? It says 'New API version used to request PEM encoded CRLs', but I recall Shiv saying the new versions of DCAP expect DER, so I'm a bit confused.

Suggested change

constexpr char API_VERSION[] = "api-version=2020-02-12-preview";

constexpr char API_VERSION[] = "api-version=2021-07-22-preview";

Scratch that. I think what you might want to do is:
constexpr char API_VERSION_THIM_AGENT[] = "api-version=2021-07-22-preview";

You can use the new API version for the GetCert calls, and the existing API versions for the other collateral.

sidse-msft · 2021-09-13T14:24:21Z

src/dcap_provider.cpp

@@ -135,6 +138,25 @@ static std::string get_collateral_version()
    }
 }

+static std::string get_agent_base_url()


Is this function necessary? It made sense with THIM cloud service because we could test different cloud endpoints (e.g. Europe vs. Americas). But THIM Agent will always be hosted at the same local IP address.

I think this will be useful for dev box testing with IMDS and THIM agent running on the dev box.

src/dcap_provider.cpp

szutshi-msft · 2021-10-26T17:45:21Z

src/dcap_provider.cpp

 static char DEFAULT_CERT_URL[] =
-    "https://global.acccache.azure.net/sgx/certificates";
+    "https://global.acccache.azure.net/sgx/certificates/";


Hey guys -- I see we've changed from certificates to "certification" -- in the last correspondence I read I saw

The ask was Thim agent to use "certificates" like THIM Service did.
-- Do I need to make a change to thim agent to stay consistent here then? I wasn't aware of this. Apologies if I missed something

src/dcap_provider.cpp

szutshi-msft · 2021-10-26T17:52:12Z

src/dcap_provider.cpp

-        const std::string cert_url = build_pck_cert_url(*p_pck_cert_id);
-        if (auto cache_hit = try_cache_get(cert_url))
+        collateral_fetch_url collateral_url;
+        build_pck_cert_url(*p_pck_cert_id, &collateral_url);


Is there a reason this was done as a pointer instead of just as a reference?
You currently have it as:
build_pck_cert_url(..., &collateral_url) -- To satisfy ---- build_pck_cert_url(...., collateral_fetch_url* url)

Why not just use it as a reference?
build_pck_cert_url(..., collateral_url) -- To satisfy ---- build_pck_cert_url(...., collateral_fetch_url& url)

Your end result is the same but you don't end up having to worry about NULL issues and it seems like your goal is to update the existing variable here so my recommendation is to pass by reference.

This part of code was already there. I don't want to change whats already there, just adding functionality to call into THIM agent in this PR

I've added another comment above to indicate what I think you're adding in. Please let me know if I have some misunderstanding.

src/dcap_provider.cpp

szutshi-msft · 2021-10-26T19:43:11Z

src/dcap_provider.cpp

-static std::string build_pck_cert_url(const sgx_ql_pck_cert_id_t& pck_cert_id)
+static void build_pck_cert_url(
+    const sgx_ql_pck_cert_id_t& pck_cert_id,
+    collateral_fetch_url* collateral_url)


This is the part im referencing below. It doesn't seem legacy, it seems like you've introduced it?

…IM agent as a JSON response.

msft-gumunjal added 2 commits September 9, 2021 15:21

Calling into thim agent to get collateral.

45d7779

Updating linux curl request.

0e3688b

msft-gumunjal marked this pull request as draft September 9, 2021 23:27

sidse-msft suggested changes Sep 13, 2021

View reviewed changes

Sending cert fetch request to THIM agent using IMDS path.

a8e56e8

szutshi-msft requested changes Oct 26, 2021

View reviewed changes

szutshi-msft reviewed Oct 26, 2021

View reviewed changes

msft-gumunjal added 5 commits October 27, 2021 21:51

Resolving comments from last iteration.

d7c727c

Including a JSON parser to parse headers and certificate passed by TH…

dcc249b

…IM agent as a JSON response.

Updating function signatures to make linux compatible.

81fb0bc

Updating linux side code.

734a3e4

Updating logging

6e56c7b

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Calling into thim agent to get collateral. #149

Calling into thim agent to get collateral. #149

msft-gumunjal commented Sep 9, 2021

sidse-msft left a comment

sidse-msft Sep 13, 2021

msft-gumunjal Sep 14, 2021

Shivr-ctrl Oct 20, 2021

msft-gumunjal Oct 26, 2021 •

edited

Loading

sidse-msft Sep 13, 2021

msft-gumunjal Sep 14, 2021

sidse-msft Sep 13, 2021

msft-gumunjal Sep 13, 2021

Shivr-ctrl Oct 20, 2021 •

edited

Loading

szutshi-msft Oct 26, 2021

msft-gumunjal Oct 26, 2021

sidse-msft Sep 13, 2021 •

edited

Loading

sidse-msft Sep 13, 2021 •

edited

Loading

sidse-msft Sep 13, 2021

msft-gumunjal Sep 13, 2021

szutshi-msft Oct 26, 2021

szutshi-msft Oct 26, 2021

msft-gumunjal Oct 26, 2021

szutshi-msft Oct 26, 2021

szutshi-msft Oct 26, 2021

	constexpr char API_VERSION[] = "api-version=2020-02-12-preview";
	constexpr char API_VERSION[] = "api-version=2021-07-22-preview";

Calling into thim agent to get collateral. #149

Are you sure you want to change the base?

Calling into thim agent to get collateral. #149

Conversation

msft-gumunjal commented Sep 9, 2021

sidse-msft left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msft-gumunjal Oct 26, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Shivr-ctrl Oct 20, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sidse-msft Sep 13, 2021 • edited Loading

Choose a reason for hiding this comment

sidse-msft Sep 13, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msft-gumunjal Oct 26, 2021 •

edited

Loading

Shivr-ctrl Oct 20, 2021 •

edited

Loading

sidse-msft Sep 13, 2021 •

edited

Loading

sidse-msft Sep 13, 2021 •

edited

Loading