Support for authentication with client certificates #656

D-r-P-3-p-p-3-r · 2024-09-12T11:09:19Z

Added possibility to authenticate via client certificates (including configuration of CA certificates).

…lient certificates.

D-r-P-3-p-p-3-r · 2024-09-12T11:15:24Z

@microsoft-github-policy-service agree company="Seadex GmbH"

jw-msft · 2024-12-06T18:32:09Z

Using EIS (edge identity service) to retrieve the x509 certs is already a supported way to use them for individual and group DPS enrollments. If I am interpreting correctly, what is the reasoning to support yet another way of doing it directly? Is it because you do not want to use EIS?

You can see here that we request the certificate from EIS unix domain server:

iot-hub-device-update/src/utils/eis_utils/src/eis_utils.c

Line 621 in 18e98a9

    
           EISErr certResult = RequestCertificateFromEIS(certId, timeoutMS, &certResponseStr);

Specifically, we are using https://azure.github.io/iot-identity-service/ with further details here: https://azure.github.io/iot-identity-service/api/identity-service.html

The reason device update agent uses EIS is for separating and offloading the concerns of securely handling the certicates where EIS can use a TPM or HSM, etc.

D-r-P-3-p-p-3-r · 2024-12-09T12:21:02Z

Using EIS (edge identity service) to retrieve the x509 certs is already a supported way to use them for individual and group DPS > enrollments. If I am interpreting correctly, what is the reasoning to support yet another way of doing it directly?

Yes.

Is it because you do not want to use EIS?

Yes, because it is rather heavy-weight for our environment.
Building DUA and all its dependencies for an ARM-based system with a custom Linux (ptxdist-baked) was already very time-consuming. It will be very time-consuming to maintain in the future as well and we'll have to repeat this step in parts for DUA updates. Not only might DUA change, but so might its dependencies.

If we do the same with EIS it's even more binaries to build, configure, and maintain.

Side note:
It wasn't a lot of big issues, but DUA wasn't even 32-bit compatible out of the box.

Also, the system is constrained in memory and flash storage. It's not a tiny embedded controller. But adding a bunch of services just to do certificate-based authentication (which works without them) is not justified.

The reason device update agent uses EIS is for separating and offloading the concerns of securely handling the certicates where > EIS can use a TPM or HSM, etc.

This is already implemented for our device and works just fine with OpenSSL out of the box. You don't point to a key file in the filesystem, but you use a "special" path to the key in the secure storage.

Thus, there is no benefit for us. But a lot of overhead. Especially, because the device software already uses the Azure IoT C SDK and uses the "direct way" to authenticate using the same key material. It works well and it's completely secure.

jw-msft

Looks good. A couple of minor comments.

jw-msft · 2025-01-08T18:53:34Z

src/utils/config_utils/src/config_utils.c

+    if (x509File)
+    {
+        if (fseek(x509File, 0, SEEK_END) == 0) {
+            bufferLength = (size_t)ftell(x509File);


Somewhat pedantic but how about handling when ftell returns -1 and Log_Error the errno. I suppose it would handle mostly the off chance when x509File is not a regular file (such as ESPIPE)

jw-msft · 2025-01-08T19:07:38Z

src/utils/config_utils/src/config_utils.c

+                    bytesRead = fread(buffer, 1, bufferLength, x509File);
+                    if (bytesRead < bufferLength)
+                    {
+                        Log_Error("Failed to read '%s'!", x509_file_path);


could eof and error be distinguished and also log the return value from bytesRead? Something like this:

if (bytesRead < bufferLength) { if (feof(x509File)) { Log_Error("Unexpected end of file while reading '%s', bytesRead: %zu", x509_file_path, bytesRead); } else if (ferror(x509File)) { Log_Error("Error occurred while reading '%s', bytesRead: %zu", x509_file_path, bytesRead); } free(buffer); buffer = NULL; }

D-r-P-3-p-p-3-r · 2025-01-10T12:52:12Z

Updated the PR with the suggest changes.

jw-msft

mostly style comments, some of which can be fixed by running clang-format.

jw-msft · 2025-01-15T21:01:27Z

src/utils/config_utils/src/config_utils.c

+    if (connection_x509_cert_file_path || connection_x509_private_key_file_path || connection_x509_ca_cert_file_path) {
+        if(connection_x509_cert_file_path && connection_x509_private_key_file_path && connection_x509_ca_cert_file_path) {
+            agent->x509Cert = ADUC_AgentInfo_Read_X509_File(connection_x509_cert_file_path);
+            if (!agent->x509Cert)


the code base uses explicit conditions regarding check for NULL values, so please change to:

if (agent->x509Cert == NULL)

The same for lines 221 and 226.

jw-msft · 2025-01-15T21:04:10Z

src/utils/config_utils/src/config_utils.c

+    FILE* x509File = NULL;
+
+    x509File = fopen(x509_file_path, "rb");
+    if (x509File)


use explicit checks for NULL / non-NULL:

if (x509File != NULL)

jw-msft · 2025-01-15T21:08:07Z

src/utils/config_utils/src/config_utils.c

+                        bytesRead = fread(buffer, 1, bufferLength, x509File);
+                        if (bytesRead < bufferLength)
+                        {
+                            if (feof(x509File))


Line 95 here has an if-statement that is 7 levels deep.
Let's keep the nesting of if-statements shallow by using the following pattern that:

guarantees usually just 1-level of nesting, and usually at most 2-3 nesting depth in practice

Centralizes all clean-up code after 'done' label that in general avoids doing so in multiple nested error branches (not the case here but in general it lowers the cognitive load), similar in function to Zig or Odin's defer statement (or c++ RAII) but can control exactly when it happens:

bool success = false; ... if (x509File == NULL) { Log_Error("Failed to open '%s'!", x509_file_path); goto done; } if (fseek(x509File, 0, SEEK_END) != 0) { Log_Error("Failed to seek end of '%s'!", x509_file_path); goto done; } ... ... success = true; done: if (!success) { free(buffer); buffer = NULL; } if (x509File != NULL) { fclose(x509File); } return buffer; }

jw-msft · 2025-01-15T21:12:23Z

src/utils/config_utils/src/config_utils.c

+    x509File = fopen(x509_file_path, "rb");
+    if (x509File)
+    {
+        if (fseek(x509File, 0, SEEK_END) == 0) {


Style consistency issue: Opening brace should be on next line

Touch every file in the current set of commits, git add them to git staging, and then run scripts/clang-format.sh. see:

iot-hub-device-update/scripts/clang-format.sh

Line 10 in 0f041bf

if [[ $0 != "${BASH_SOURCE[0]}" ]]; then

Longer-term, we need to add clang-format.sh (and some other things) to the git pre-commit hook. Currently, the existing git pre-commit hook only runs sh-format.sh (

iot-hub-device-update/scripts/githooks/pre-commit.sh

Line 29 in 0f041bf

if ! "$GITROOT/scripts/sh-format.sh" -v; then

)

jw-msft · 2025-01-15T21:12:39Z

src/utils/config_utils/src/config_utils.c

+        Log_Error("Failed to open '%s'!", x509_file_path);
+    }
+
+    return(buffer);


style: no parens for return

jw-msft · 2025-01-15T21:13:06Z

src/utils/config_utils/src/config_utils.c

+            Log_Error("Failed to seek end of '%s'!", x509_file_path);
+        }
+
+        if(fclose (x509File) != 0) {


style: no space between function name and arg list (should be caught by clang-format)

jw-msft · 2025-01-15T21:13:49Z

src/utils/config_utils/src/config_utils.c

+                            {
+                                Log_Error("Unexpected end of file while reading '%s' (bytesRead: %zu)!", x509_file_path, bytesRead);
+                            }
+                            else if (ferror(x509File)) {


paren on next line

jw-msft · 2025-01-15T21:23:01Z

src/utils/config_utils/src/config_utils.c

+        }
+
+        if(fclose (x509File) != 0) {
+            Log_Warn("Failed to close '%s'!", x509_file_path);


omit. no need to log when fclose fails as it's not really actionable and the log trace might not even be able to flush to disk in that case (things are in a pretty bad state)

jw-msft · 2025-01-15T21:25:41Z

src/utils/config_utils/src/config_utils.c

+            Log_Error("Failed to seek end of '%s'!", x509_file_path);
+        }
+
+        if(fclose (x509File) != 0) {


would prefer this to be in centralized cleanup area, such as after 'done:' label (along with free(buffer) if in error condition).

jw-msft · 2025-01-15T21:34:17Z

src/utils/config_utils/src/config_utils.c

+                            {
+                                Log_Error("Failed to read '%s' (bytesRead: %zu)!", x509_file_path, bytesRead);
+                            }
+                            free(buffer);


for future maintainability, this should be done unconditionally at the end, if in "not succeeded" state.

D-r-P-3-p-p-3-r added 2 commits September 12, 2024 13:05

Support for auth type 'client certificate'

aea7b50

Add support for configuration of CA certificates in connection with c…

41ec48b

…lient certificates.

jw-msft reviewed Jan 8, 2025

View reviewed changes

D-r-P-3-p-p-3-r force-pushed the feature/pr_client_certificates branch from 9ed2796 to 41ec48b Compare January 10, 2025 12:44

Improvements for ADUC_AgentInfo_Read_X509_File

538cc2d

jw-msft reviewed Jan 15, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support for authentication with client certificates #656

Support for authentication with client certificates #656

D-r-P-3-p-p-3-r commented Sep 12, 2024

D-r-P-3-p-p-3-r commented Sep 12, 2024 •

edited

Loading

jw-msft commented Dec 6, 2024

D-r-P-3-p-p-3-r commented Dec 9, 2024

jw-msft left a comment

jw-msft Jan 8, 2025

D-r-P-3-p-p-3-r Jan 10, 2025

jw-msft Jan 8, 2025

D-r-P-3-p-p-3-r Jan 10, 2025

D-r-P-3-p-p-3-r commented Jan 10, 2025 •

edited

Loading

jw-msft left a comment

jw-msft Jan 15, 2025

jw-msft Jan 15, 2025

jw-msft Jan 15, 2025

jw-msft Jan 15, 2025

jw-msft Jan 15, 2025

jw-msft Jan 15, 2025

jw-msft Jan 15, 2025

jw-msft Jan 15, 2025

jw-msft Jan 15, 2025

jw-msft Jan 15, 2025

Support for authentication with client certificates #656

Are you sure you want to change the base?

Support for authentication with client certificates #656

Conversation

D-r-P-3-p-p-3-r commented Sep 12, 2024

D-r-P-3-p-p-3-r commented Sep 12, 2024 • edited Loading

jw-msft commented Dec 6, 2024

D-r-P-3-p-p-3-r commented Dec 9, 2024

jw-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

Choose a reason for hiding this comment

D-r-P-3-p-p-3-r commented Jan 10, 2025 • edited Loading

jw-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

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

D-r-P-3-p-p-3-r commented Sep 12, 2024 •

edited

Loading

D-r-P-3-p-p-3-r commented Jan 10, 2025 •

edited

Loading