SNOW-1902244: Workflow identity federation attestations #871

sfc-gh-jszczerbinski · 2025-04-09T11:49:38Z

Adds Workflow identity federation attestations for AWS, Azure, GCP and OIDC

sfc-gh-mkubik · 2025-04-17T12:31:35Z

cpp/AwsAttestation.cpp

+      }
+
+      auto request = Aws::Http::CreateHttpRequest(
+          Aws::String("https://sts.us-west-2.amazonaws.com/?Action=GetCallerIdentity&Version=2011-06-15"),


should it always be us-west-2?

sfc-gh-mkubik · 2025-04-17T12:37:34Z

cpp/AwsAttestation.cpp

+      return Attestation{AttestationType::AWS, base64};
+    }
+
+


🧐 random empty lines

sfc-gh-mkubik · 2025-04-17T12:53:00Z

cpp/AzureAttestation.cpp

+      std::string err = picojson::parse(json, response_body);
+      if (!err.empty()) {
+        CXX_LOG_ERROR("Error parsing Azure response: %s", err.c_str());
+        return Attestation{AttestationType::AZURE, ""};


does boost::none mean error connecting to csp while Attestation with empty credentials means csp side issue?

This should return boost::none here, it's a refactor leftover.

sfc-gh-mkubik · 2025-04-17T12:57:31Z

cpp/AzureAttestation.cpp

+    }
+
+    boost::url AzureAttestationConfig::getRequestURL() const {
+      boost::urls::url url = boost::url("http://169.254.169.254/metadata/identity/oauth2/token");


Could we hide this ip behind some constant like CSP_METADATA_SERVICE_IP to make it less scary?

Yes, but this well known ip. I think the code is more readable this way as this IP is well known.

sfc-gh-mkubik · 2025-04-17T13:18:27Z

cpp/WifAttestation.cpp

+      boost::optional<Attestation> createOIDCAttestation(const AttestationConfig& config) {
+        if (!config.token) {
+          CXX_LOG_WARN("No token provided for OIDC attestation.");
+          return {};


sometimes we return {} and sometimes boost:none, let's commit to one option

Changing to boost::none for clarity

codecov · 2025-04-17T13:59:46Z

Codecov Report

Attention: Patch coverage is 37.29508% with 153 lines in your changes missing coverage. Please review.

Project coverage is 78.10%. Comparing base (68b2501) to head (d94ed3e).

Files with missing lines	Patch %	Lines
cpp/AwsAttestation.cpp	0.00%	32 Missing ⚠️
cpp/http/HttpClient.cpp	9.37%	29 Missing ⚠️
cpp/AzureAttestation.cpp	62.50%	27 Missing ⚠️
cpp/WifAttestation.cpp	36.66%	19 Missing ⚠️
cpp/AWSUtils.cpp	44.82%	16 Missing ⚠️
cpp/GcpAttestation.cpp	62.96%	10 Missing ⚠️
include/snowflake/HttpClient.hpp	9.09%	10 Missing ⚠️
include/snowflake/WifAttestation.hpp	0.00%	10 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #871      +/-   ##
==========================================
- Coverage   80.61%   78.10%   -2.52%     
==========================================
  Files         117      125       +8     
  Lines       10463    10683     +220     
==========================================
- Hits         8435     8344      -91     
- Misses       2028     2339     +311

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

sfc-gh-dstempniak · 2025-04-17T16:13:29Z

cpp/AwsAttestation.cpp

+      }
+
+      std::string region = getAwsRegion();
+      std::string host = std::string("sts.") + getAwsRegion() + ".amazonaws.com";


Don't we want to use SnowflakeS3Client::getDomainSuffixForRegionalUrl here?

sfc-gh-dstempniak · 2025-04-17T16:14:51Z

cpp/AwsAttestation.cpp

+      );
+
+      request->SetHeaderValue("Host", host);
+      request->SetHeaderValue("X-Snowflake-Audience", "snowflakecomputing.com");


What about .cn?

sfc-gh-dstempniak · 2025-04-17T16:20:26Z

cpp/AwsAttestation.cpp

+      std::string json = picojson::value(obj).serialize(true);
+      std::string base64;
+      Util::Base64::encodePadding(json.begin(), json.end(), std::back_inserter(base64));
+      Aws::ShutdownAPI(options);


Maybe it would be better to integrate aws init and shutdown with awsdk_init struct?

sfc-gh-dstempniak · 2025-04-17T16:39:59Z

cpp/http/HttpClient.cpp

+        CXX_LOG_INFO("Running request: %s", req.url.c_str());
+        CXX_LOG_INFO("Method: %s", HttpRequest::methodToString(req.method).c_str());
+        for (const auto &h: req.headers) {
+          CXX_LOG_INFO("Header: %s: %s", h.first.c_str(), h.second.c_str());


Is it OK to log all headers on INFO level? What about secrets?

Those were development prints i forgot to delete.

sfc-gh-dstempniak · 2025-04-17T16:51:05Z

cpp/http/HttpClient.cpp

+
+    private:
+      static size_t write(void *ptr, size_t size, size_t nmemb, HttpResponse *response) {
+        CXX_LOG_INFO("Writing %d bytes", (int) (size * nmemb));


It's rather debug or trace level.

sfc-gh-dstempniak · 2025-04-17T16:58:09Z

cpp/http/HttpClient.cpp

+    private:
+      static size_t write(void *ptr, size_t size, size_t nmemb, HttpResponse *response) {
+        CXX_LOG_INFO("Writing %d bytes", (int) (size * nmemb));
+        response->m_buffer.insert(response->m_buffer.end(), (char *) ptr, (char *) ptr + size * nmemb);


I think HttpResponse should be responsible for writing to m_buffer. Maybe whole method write should be there as well?

This write method is implementation specific (for curl) if I used other http library it would not exist. I would like to keep it within a curl client implementation.

sfc-gh-dstempniak · 2025-04-17T17:08:18Z

include/snowflake/HttpClient.hpp

+namespace Snowflake {
+  namespace Client {
+    struct HttpResponse {
+      long code;


m_code? or buffer

sfc-gh-dstempniak · 2025-04-17T17:14:55Z

tests/test_create_wif_attestation.cpp

+int main()
+{
+  const struct CMUnitTest tests[] = {
+#ifndef _WIN32


Why? A comment would be nice.

sfc-gh-jszczerbinski force-pushed the SNOW-1902244-odbc-csp-in-driver branch from 74fa2a6 to 3b1aba1 Compare April 11, 2025 08:55

sfc-gh-jszczerbinski marked this pull request as ready for review April 15, 2025 09:07

sfc-gh-jszczerbinski requested a review from a team as a code owner April 15, 2025 09:07

sfc-gh-jszczerbinski force-pushed the SNOW-1902244-odbc-csp-in-driver branch from eca7ee8 to 5c4618c Compare April 16, 2025 08:14

sfc-gh-jszczerbinski changed the title ~~Workflow identity federation attestations~~ SNOW-1902244: Workflow identity federation attestations Apr 16, 2025

sfc-gh-jszczerbinski added 2 commits April 17, 2025 10:17

AWorkflow identity federation attestations

908c46d

Fix windows build

3593e97

sfc-gh-jszczerbinski force-pushed the SNOW-1902244-odbc-csp-in-driver branch from 82f9e4b to 3593e97 Compare April 17, 2025 08:19

sfc-gh-jszczerbinski added 4 commits April 17, 2025 10:38

Fix azure tests

cf8c4f2

Fix warnings

6cd596e

Fix

9871c39

Fix

167e5ff

sfc-gh-mkubik approved these changes Apr 17, 2025

View reviewed changes

After review fixes

63e0dde

sfc-gh-dstempniak reviewed Apr 17, 2025

View reviewed changes

sfc-gh-jszczerbinski added 2 commits April 18, 2025 10:29

After review fixes

f8be8ef

Check for multiple issuers

cdaee0e

sfc-gh-dheyman approved these changes Apr 18, 2025

View reviewed changes

sfc-gh-jszczerbinski added 2 commits April 18, 2025 14:41

After review fixes

d3246d3

Merge branch 'master' into SNOW-1902244-odbc-csp-in-driver

d94ed3e

sfc-gh-jszczerbinski merged commit 478b08b into master Apr 22, 2025
47 of 51 checks passed

sfc-gh-jszczerbinski deleted the SNOW-1902244-odbc-csp-in-driver branch April 22, 2025 08:33

github-actions bot locked and limited conversation to collaborators Apr 22, 2025

SNOW-1902244: Workflow identity federation attestations #871

SNOW-1902244: Workflow identity federation attestations #871

Uh oh!

Conversation

sfc-gh-jszczerbinski commented Apr 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

codecov bot commented Apr 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

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!

Choose a reason for hiding this comment

Uh oh!

sfc-gh-jszczerbinski Apr 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

sfc-gh-jszczerbinski commented Apr 9, 2025 •

edited

Loading

codecov bot commented Apr 17, 2025 •

edited

Loading

sfc-gh-jszczerbinski Apr 18, 2025 •

edited

Loading