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.

Sign up for GitHub

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

Jump to bottom

Calling into thim agent to get collateral. #149

Draft
wants to merge 8 commits into
base: master
Choose a base branch
from
Draft

Calling into thim agent to get collateral. #149

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
45d7779
Calling into thim agent to get collateral.
msft-gumunjal Sep 9, 2021
0e3688b
Updating linux curl request.
msft-gumunjal Sep 9, 2021
a8e56e8
Sending cert fetch request to THIM agent using IMDS path.
msft-gumunjal Oct 19, 2021
d7c727c
Resolving comments from last iteration.
msft-gumunjal Oct 27, 2021
dcc249b
Including a JSON parser to parse headers and certificate passed by TH…
msft-gumunjal Oct 29, 2021
81fb0bc
Updating function signatures to make linux compatible.
msft-gumunjal Oct 29, 2021
734a3e4
Updating linux side code.
msft-gumunjal Nov 1, 2021
6e56c7b
Updating logging
msft-gumunjal Nov 9, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions src/Linux/curl_easy.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Copy link

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

@msft-gumunjal msft-gumunjal Oct 26, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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.

{
std::unique_ptr<curl_easy> easy(new curl_easy);

Expand All @@ -92,7 +92,12 @@ std::unique_ptr<curl_easy> curl_easy::create(const std::string& url, const std::

if (p_body != nullptr && !p_body->empty())
{
easy->set_opt_or_throw(CURLOPT_CUSTOMREQUEST, "GET");
if (httpVerb == L"POST") {
msft-gumunjal marked this conversation as resolved.
Show resolved Hide resolved
easy->set_opt_or_throw(CURLOPT_POST, 1L);
}
else {
Copy link

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

easy->set_opt_or_throw(CURLOPT_HTTPGET, 1L);
}
easy->set_opt_or_throw(CURLOPT_COPYPOSTFIELDS, p_body->c_str());
}

Expand Down
5 changes: 4 additions & 1 deletion src/Linux/curl_easy.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,10 @@ class curl_easy
char function[128]{};
};

static std::unique_ptr<curl_easy> create(const std::string& url, const std::string* const p_body);
static std::unique_ptr<curl_easy> create(
const std::string& url,
const std::string* const p_body,
LPCWSTR httpVerb = L"GET");

~curl_easy();

Expand Down
14 changes: 11 additions & 3 deletions src/UnitTest/test_quote_prov.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -458,7 +458,7 @@ static void GetCrlTest()
{
// This is the CRL DP used by Intel for leaf certs
static const char* TEST_CRL_URL =
"https://api.trustedservices.intel.com/sgx/certification/v1/"
"https://api.trustedservices.intel.com/sgx/certification/v3/"
"pckcrl?ca=processor";

sgx_ql_get_revocation_info_params_t params = {
Expand Down Expand Up @@ -927,7 +927,11 @@ void SetupEnvironment(std::string version)
#if defined __LINUX__
setenv(
"AZDCAP_BASE_CERT_URL",
"https://global.acccache.azure.net/sgx/certificates",
"https://global.acccache.azure.net/sgx/certificates/",
1);
setenv(
"AZDCAP_THIM_AGENT_URL",
"http://169.254.169.254/metadata/THIM/sgx/getCerts?",
1);
setenv("AZDCAP_CLIENT_ID", "AzureDCAPTestsLinux", 1);
if (!version.empty())
Expand All @@ -943,7 +947,10 @@ void SetupEnvironment(std::string version)
}
EXPECT_TRUE(SetEnvironmentVariableA(
"AZDCAP_BASE_CERT_URL",
"https://global.acccache.azure.net/sgx/certificates"));
"https://global.acccache.azure.net/sgx/certificates/"));
EXPECT_TRUE(SetEnvironmentVariableA(
"AZDCAP_THIM_AGENT_URL",
"http://169.254.169.254/metadata/THIM/sgx/getCerts?"));
EXPECT_TRUE(
SetEnvironmentVariableA("AZDCAP_CLIENT_ID", "AzureDCAPTestsWindows"));
#endif
Expand Down Expand Up @@ -997,6 +1004,7 @@ TEST(testQuoteProv, quoteProviderTestsV2DataFromService)
// Get the data from the service
//
SetupEnvironment("v2");

ASSERT_TRUE(RunQuoteProviderTests());
ASSERT_TRUE(GetQveIdentityTest());

Expand Down
9 changes: 5 additions & 4 deletions src/Windows/curl_easy.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,8 @@
///////////////////////////////////////////////////////////////////////////////
// Constants
///////////////////////////////////////////////////////////////////////////////
static constexpr int maximum_retries = 5;
static constexpr int initial_retry_delay_ms = 20;
static constexpr int maximum_retries = 3;
static constexpr int initial_retry_delay_ms = 2000;
static constexpr WCHAR content_type_header[] =
L"Content-Type: application/json";

Expand Down Expand Up @@ -128,7 +128,8 @@ std::wstring UnicodeStringFromUtf8String(_In_ const std::string& ansiString)
///////////////////////////////////////////////////////////////////////////////
std::unique_ptr<curl_easy> curl_easy::create(
const std::string& url,
const std::string* const p_body)
const std::string* const p_body,
LPCWSTR httpVerb)
{
struct make_unique_enabler : public curl_easy
{
Expand Down Expand Up @@ -191,7 +192,7 @@ std::unique_ptr<curl_easy> curl_easy::create(

curl->request.reset(WinHttpOpenRequest(
curl->connectionHandle.get(),
L"GET",
httpVerb,
urlToRetrieve.c_str(),
nullptr,
WINHTTP_NO_REFERER,
Expand Down
3 changes: 2 additions & 1 deletion src/Windows/curl_easy.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,8 @@ class curl_easy
};
static std::unique_ptr<curl_easy> create(
const std::string& url,
const std::string* const p_body);
const std::string* const p_body,
LPCWSTR httpVerb = L"GET");

~curl_easy();

Expand Down
156 changes: 116 additions & 40 deletions src/dcap_provider.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Copy link

@sidse-msft sidse-msft Sep 13, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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";

Copy link

@sidse-msft sidse-msft Sep 13, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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.


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

Choose a reason for hiding this comment

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

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={}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

@Shivr-ctrl Shivr-ctrl Oct 20, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

The new URL is available now.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hey guys -- I see we've changed from certificates to "certification" -- in the last correspondence I read I saw
image
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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No change needed, certificates is fine here.

static std::string cert_base_url = DEFAULT_CERT_URL;

static char DEFAULT_CLIENT_ID[] = "production_client";
static char DEFAULT_THIM_AGENT_URL[] =
"http://169.254.169.254/metadata/THIM/sgx/getCerts?";
msft-gumunjal marked this conversation as resolved.
Show resolved Hide resolved
static std::string thim_agent_base_url = DEFAULT_THIM_AGENT_URL;

static char DEFAULT_CLIENT_ID[] = "Azure DCAP_client";
msft-gumunjal marked this conversation as resolved.
Show resolved Hide resolved
static std::string prod_client_id = DEFAULT_CLIENT_ID;

static char DEFAULT_COLLATERAL_VERSION[] = "v3";
Expand Down Expand Up @@ -135,6 +138,25 @@ static std::string get_collateral_version()
}
}

static std::string get_agent_base_url()
Copy link

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

{
std::string env_base_url = get_env_variable(ENV_AZDCAP_THIM_AGENT_URL);

if (env_base_url.empty())
{
log(SGX_QL_LOG_WARNING,
"Using default THIM Agent base cert URL '%s'.",
thim_agent_base_url.c_str());
return thim_agent_base_url;
}

log(SGX_QL_LOG_INFO,
"Using %s envvar for base cert URL, set to '%s'.",
ENV_AZDCAP_THIM_AGENT_URL,
env_base_url.c_str());
return env_base_url;
}

static std::string get_base_url()
{
std::string env_base_url = get_env_variable(ENV_AZDCAP_BASE_URL);
Expand Down Expand Up @@ -456,10 +478,9 @@ void safe_cast(input_t in, output_t* out)
*out = static_cast<output_t>(in);
}

//
// Build up the URL needed to fetch specific certificate information.
//
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)
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

{
const std::string qe_id =
format_as_hex_string(pck_cert_id.p_qe3_id, pck_cert_id.qe3_id_size);
Expand All @@ -473,28 +494,34 @@ static std::string build_pck_cert_url(const sgx_ql_pck_cert_id_t& pck_cert_id)

const std::string pce_id =
format_as_big_endian_hex_string(pck_cert_id.pce_id);

std::string version = get_collateral_version();
std::stringstream pck_cert_url;
pck_cert_url << get_base_url();

collateral_url->thim_service_url << get_base_url();
collateral_url->thim_service_url << "noauth/pckcertificate?";
collateral_url->thim_agent_url << get_agent_base_url();

if (!version.empty())
{
pck_cert_url << '/';
pck_cert_url << version;
collateral_url->thim_service_url << "version=" << version << '&';
collateral_url->thim_agent_url << "TEEVersionType=" << version << '&';
}
pck_cert_url << '/' << qe_id;
pck_cert_url << '/' << cpu_svn;
pck_cert_url << '/' << pce_svn;
pck_cert_url << '/' << pce_id;
pck_cert_url << '?';
collateral_url->thim_service_url << "qeid=" << qe_id << '&';
collateral_url->thim_service_url << "cpusvn=" << cpu_svn << '&';
collateral_url->thim_service_url << "pcesvn=" << pce_svn << '&';
collateral_url->thim_service_url << "pceid=" << pce_id << '&';

collateral_url->thim_agent_url << "qeid=" << qe_id << '&';
collateral_url->thim_agent_url << "cpusvn=" << cpu_svn << '&';
collateral_url->thim_agent_url << "pcesvn=" << pce_svn << '&';
collateral_url->thim_agent_url << "pceid=" << pce_id;

std::string client_id = get_client_id();

if (!client_id.empty())
{
pck_cert_url << "clientid=" << client_id << '&';
collateral_url->thim_service_url << "clientid=" << client_id << '&';
}
pck_cert_url << API_VERSION_LEGACY;
return pck_cert_url.str();
collateral_url->thim_service_url << API_VERSION_LEGACY;
}

//
Expand Down Expand Up @@ -897,31 +924,80 @@ extern "C" quote3_error_t sgx_ql_get_quote_config(

try
{
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);
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

std::string cert_url = collateral_url.thim_service_url.str();
std::string thim_agent_url = collateral_url.thim_agent_url.str();
std::unique_ptr<curl_easy> curl;
bool thim_agent_success = true;
msft-gumunjal marked this conversation as resolved.
Show resolved Hide resolved

try
{
curl = curl_easy::create(thim_agent_url, NULL);
log(SGX_QL_LOG_INFO,
"Fetching quote config from cache: '%s'.",
cert_url.c_str());
"Fetching quote config from THIM agent server: '%s'.",
thim_agent_url.c_str());
curl->set_headers(headers::default_values);
curl->perform();
}
catch (const std::bad_alloc&)
{
log_message(SGX_QL_LOG_ERROR, "Out of memory thrown");
thim_agent_success = false;
return SGX_QL_ERROR_OUT_OF_MEMORY;
}
catch (const std::runtime_error& error)
{
log(SGX_QL_LOG_WARNING,
"Runtime exception thrown, error: %s",
error.what());
thim_agent_success = false;
// Swallow adding file to cache. Library can
// operate without caching
// return SGX_QL_ERROR_UNEXPECTED;
msft-gumunjal marked this conversation as resolved.
Show resolved Hide resolved
}
catch (const std::exception& error)
{
log(SGX_QL_LOG_ERROR,
"Unknown exception thrown, error: %s",
error.what());
thim_agent_success = false;
//return SGX_QL_ERROR_UNEXPECTED;
}
catch (const curl_easy::error& error)
{
log(SGX_QL_LOG_ERROR,
"error thrown, error code: %x: %s",
error.code,
error.what());
thim_agent_success = false;
}
if (!thim_agent_success) {
if (auto cache_hit = try_cache_get(cert_url))
{
log(SGX_QL_LOG_INFO,
"Fetching quote config from cache: '%s'.",
cert_url.c_str());

*pp_quote_config =
(sgx_ql_config_t*)(new uint8_t[cache_hit->size()]);
memcpy(*pp_quote_config, cache_hit->data(), cache_hit->size());
*pp_quote_config =
(sgx_ql_config_t*)(new uint8_t[cache_hit->size()]);
memcpy(*pp_quote_config, cache_hit->data(), cache_hit->size());

// re-aligning the p_cert_data pointer
(*pp_quote_config)->p_cert_data =
(uint8_t*)(*pp_quote_config) + sizeof(sgx_ql_config_t);
// re-aligning the p_cert_data pointer
(*pp_quote_config)->p_cert_data =
(uint8_t*)(*pp_quote_config) + sizeof(sgx_ql_config_t);

return SGX_QL_SUCCESS;
return SGX_QL_SUCCESS;
}

const std::string eppid_json = build_eppid_json(*p_pck_cert_id);
curl = curl_easy::create(cert_url, &eppid_json, L"POST");
log(SGX_QL_LOG_INFO,
"Fetching quote config from remote server: '%s'.",
cert_url.c_str());
curl->set_headers(headers::default_values);
curl->perform();
}

const std::string eppid_json = build_eppid_json(*p_pck_cert_id);
const auto curl = curl_easy::create(cert_url, &eppid_json);
log(SGX_QL_LOG_INFO,
"Fetching quote config from remote server: '%s'.",
cert_url.c_str());
curl->set_headers(headers::default_values);
curl->perform();

// we better get TCB info and the cert chain, else we cannot provide the
// required data to the caller.
Expand Down Expand Up @@ -977,7 +1053,7 @@ extern "C" quote3_error_t sgx_ql_get_quote_config(
*curl, headers::CACHE_CONTROL, &cache_control);

auto retval = convert_to_intel_error(get_cache_header_operation);
if (retval == SGX_QL_SUCCESS)
if ((retval == SGX_QL_SUCCESS) and (!thim_agent_success))
msft-gumunjal marked this conversation as resolved.
Show resolved Hide resolved
{
time_t expiry;
if (get_cache_expiration_time(cache_control, cert_url, expiry))
Expand Down
12 changes: 12 additions & 0 deletions src/dcap_provider.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,9 @@
#define PLATFORM_QUOTE_PROVIDER_H

#include <stdint.h>
#include <sstream>

using namespace std;
msft-gumunjal marked this conversation as resolved.
Show resolved Hide resolved

/*****************************************************************************
* Data types and interfaces for getting platform revocation info. This
Expand Down Expand Up @@ -84,6 +87,15 @@ typedef sgx_plat_error_t (*sgx_get_qe_identity_info_t)(
typedef void (*sgx_free_qe_identity_info_t)(
sgx_qe_identity_info_t* p_qe_identity_info);

/*****************************************************************************
* Structure to pass THIM agent and THIM service URL
****************************************************************************/

typedef struct _collateral_fetch_url {
std::stringstream thim_agent_url; // URL to fetch collateral from the agent
std::stringstream thim_service_url; // URL to fetch collateral from the service
} collateral_fetch_url;

/*****************************************************************************
* Data types and interfaces for configuration the platform quote provider
* library.
Expand Down
1 change: 1 addition & 0 deletions src/environment.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
// modify or override these values as they can cause regressions in
// caching service behavior.
#define ENV_AZDCAP_BASE_URL "AZDCAP_BASE_CERT_URL"
#define ENV_AZDCAP_THIM_AGENT_URL "AZDCAP_THIM_AGENT_URL"
#define ENV_AZDCAP_CLIENT_ID "AZDCAP_CLIENT_ID"
#define ENV_AZDCAP_COLLATERAL_VER "AZDCAP_COLLATERAL_VERSION"
#define ENV_AZDCAP_DEBUG_LOG "AZDCAP_DEBUG_LOG_LEVEL"
Expand Down