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.

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

Support for authentication with client certificates #656

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 2 additions & 0 deletions src/adu_types/inc/aduc/adu_types.h
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ typedef enum tagADUC_AuthType
ADUC_AuthType_SASToken = 1,
ADUC_AuthType_SASCert = 2,
ADUC_AuthType_NestedEdgeCert = 3,
ADUC_AuthType_X509 = 4,
} ADUC_AuthType;

/**
Expand All @@ -72,6 +73,7 @@ typedef struct tagConnectionInfo
char* certificateString; /**< x509 certificate in PEM format for the IoTHubClient to be used for authentication*/
char* opensslEngine; /**< identifier for the OpenSSL Engine used for the certificate in certificateString*/
char* opensslPrivateKey; /**< x509 private key in PEM format for the IoTHubClient to be used for authentication */
char* clientCertificateString; /**< x509 certificate in PEM format for the IoTHubClient to be used for authentication*/
} ADUC_ConnectionInfo;

/**
Expand Down
3 changes: 3 additions & 0 deletions src/adu_types/src/adu_types.c
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,9 @@ void ADUC_ConnectionInfo_DeAlloc(ADUC_ConnectionInfo* info)
free(info->opensslPrivateKey);
info->opensslPrivateKey = NULL;

free(info->clientCertificateString);
info->clientCertificateString = NULL;

info->authType = ADUC_AuthType_NotSet;
info->connType = ADUC_ConnType_NotSet;
}
Expand Down
10 changes: 8 additions & 2 deletions src/agent/src/health_management.c
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,9 @@ bool GetConnectionInfoFromIdentityService(ADUC_ConnectionInfo* info);
*
* @return true if connection info can be obtained
*/
bool GetConnectionInfoFromConnectionString(ADUC_ConnectionInfo* info, const char* connectionString);
bool GetConnectionInfoFromConnectionString(
ADUC_ConnectionInfo* info, const char* connectionString,
const char* const x509Cert, const char* const x509PrivateKey, const char* const x509CaCert);

/**
* @brief Checks whether we can obtain a device or module connection string.
Expand Down Expand Up @@ -95,7 +97,11 @@ bool IsConnectionInfoValid(const ADUC_LaunchArguments* launchArgs, const ADUC_Co
}
else if (strcmp(agent->connectionType, "string") == 0)
{
validInfo = GetConnectionInfoFromConnectionString(&info, agent->connectionData);
validInfo = GetConnectionInfoFromConnectionString(&info, agent->connectionData, NULL, NULL, NULL);
}
else if (strcmp(agent->connectionType, "X509") == 0)
{
validInfo = GetConnectionInfoFromConnectionString(&info, agent->connectionData, agent->x509Cert, agent->x509PrivateKey, agent->x509CaCert);
}
else
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,9 @@ ADUC_ConnType GetConnTypeFromConnectionString(const char* connectionString);
*
* @return true if connection info can be obtained
*/
bool GetConnectionInfoFromConnectionString(ADUC_ConnectionInfo* info, const char* connectionString);
bool GetConnectionInfoFromConnectionString(
ADUC_ConnectionInfo* info, const char* connectionString,
const char* const x509Cert, const char* const x509PrivateKey, const char* const x509CaCert);

/**
* @brief Get the Connection Info from Identity Service
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@
#include <iothub.h>
#include <iothub_client_options.h>

#include <assert.h>

#ifdef ADUC_ALLOW_MQTT
# include <iothubtransportmqtt.h>
#endif
Expand Down Expand Up @@ -342,6 +344,14 @@ static bool ADUC_DeviceClient_Create(
Log_Error("Unable to set IotHub certificate, error=%d", iothubResult);
result = false;
}
else if (
connInfo->clientCertificateString != NULL && connInfo->authType == ADUC_AuthType_X509
&& (iothubResult = ClientHandle_SetOption(*outClientHandle, SU_OPTION_X509_CERT, connInfo->clientCertificateString))
!= IOTHUB_CLIENT_OK)
{
Log_Error("Unable to set client certificate, error=%d", iothubResult);
result = false;
}
else if (
shouldSetProxyOptions
&& ((iothubResult = ClientHandle_SetOption(*outClientHandle, OPTION_HTTP_PROXY, &proxyOptions))
Expand All @@ -351,7 +361,7 @@ static bool ADUC_DeviceClient_Create(
result = false;
}
else if (
connInfo->certificateString != NULL && connInfo->authType == ADUC_AuthType_NestedEdgeCert
connInfo->certificateString != NULL && (connInfo->authType == ADUC_AuthType_NestedEdgeCert || connInfo->authType == ADUC_AuthType_X509)
&& (iothubResult = ClientHandle_SetOption(*outClientHandle, OPTION_TRUSTED_CERT, connInfo->certificateString))
!= IOTHUB_CLIENT_OK)
{
Expand All @@ -367,7 +377,7 @@ static bool ADUC_DeviceClient_Create(
result = false;
}
else if (
connInfo->opensslPrivateKey != NULL && connInfo->authType == ADUC_AuthType_SASCert
connInfo->opensslPrivateKey != NULL && ( connInfo->authType == ADUC_AuthType_SASCert || connInfo->authType == ADUC_AuthType_X509 )
&& (iothubResult =
ClientHandle_SetOption(*outClientHandle, SU_OPTION_X509_PRIVATE_KEY, connInfo->opensslPrivateKey))
!= IOTHUB_CLIENT_OK)
Expand Down Expand Up @@ -479,7 +489,9 @@ ADUC_ConnType GetConnTypeFromConnectionString(const char* connectionString)
*
* @return true if connection info can be obtained
*/
bool GetConnectionInfoFromConnectionString(ADUC_ConnectionInfo* info, const char* connectionString)
bool GetConnectionInfoFromConnectionString(
ADUC_ConnectionInfo* info, const char* connectionString,
const char* const x509Cert, const char* const x509PrivateKey, const char* const x509CaCert)
{
bool succeeded = false;
const ADUC_ConfigInfo* config = NULL;
Expand Down Expand Up @@ -509,7 +521,28 @@ bool GetConnectionInfoFromConnectionString(ADUC_ConnectionInfo* info, const char
goto done;
}

info->authType = ADUC_AuthType_SASToken;
if (x509Cert)
{
assert(x509PrivateKey);
assert(x509CaCert);
info->authType = ADUC_AuthType_X509;
if (mallocAndStrcpy_s(&info->clientCertificateString, x509Cert) != 0)
{
goto done;
}
if (mallocAndStrcpy_s(&info->opensslPrivateKey, x509PrivateKey) != 0)
{
goto done;
}
if (mallocAndStrcpy_s(&info->certificateString, x509CaCert) != 0)
{
goto done;
}
}
else
{
info->authType = ADUC_AuthType_SASToken;
}

// Optional: The certificate string is needed for Edge Gateway connection.
config = ADUC_ConfigInfo_GetInstance();
Expand Down Expand Up @@ -609,7 +642,14 @@ bool GetAgentConfigInfo(ADUC_ConnectionInfo* info)
}
else if (strcmp(agent->connectionType, "string") == 0)
{
if (!GetConnectionInfoFromConnectionString(info, agent->connectionData))
if (!GetConnectionInfoFromConnectionString(info, agent->connectionData, NULL, NULL, NULL))
{
goto done;
}
}
else if (strcmp(agent->connectionType, "X509") == 0)
{
if (!GetConnectionInfoFromConnectionString(info, agent->connectionData, agent->x509Cert, agent->x509PrivateKey, agent->x509CaCert))
{
goto done;
}
Expand Down
6 changes: 6 additions & 0 deletions src/utils/config_utils/inc/aduc/config_utils.h
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,12 @@ typedef struct tagADUC_AgentInfo

char* connectionData; /**< the name in AIS principal (AIS); or the connectionString (connectionType string). */

char* x509Cert; // client certificate

char* x509PrivateKey; // private key to be used with the client certificate

char* x509CaCert; // ca certificate

char* manufacturer; /**< Device property manufacturer. */

char* model; /**< Device property model. */
Expand Down
112 changes: 112 additions & 0 deletions src/utils/config_utils/src/config_utils.c
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
#include <stdbool.h>
#include <stdlib.h>
#include <string.h>
#include <errno.h>

static pthread_mutex_t s_config_mutex = PTHREAD_MUTEX_INITIALIZER;

Expand Down Expand Up @@ -57,13 +58,87 @@ static const char* CONFIG_RUN_AS = "runas";
static const char* CONFIG_CONNECTION_SOURCE = "connectionSource";
static const char* CONFIG_CONNECTION_TYPE = "connectionType";
static const char* CONFIG_CONNECTION_DATA = "connectionData";
static const char* CONFIG_CONNECTION_X509_CERT_FILE_PATH = "connectionX509CertFilePath";
static const char* CONFIG_CONNECTION_X509_PRIVATE_KEY_FILE_PATH = "connectionX509PrivateKeyFilePath";
static const char* CONFIG_CONNECTION_X509_CA_CERT_FILE_PATH = "connectionX509CaCertFilePath";
static const char* CONFIG_ADDITIONAL_DEVICE_PROPERTIES = "additionalDeviceProperties";
static const char* CONFIG_AGENTS = "agents";

static const char* INVALID_OR_MISSING_FIELD_ERROR_FMT = "Invalid json - '%s' missing or incorrect";

static void ADUC_AgentInfo_Free(ADUC_AgentInfo* agent);


static char* ADUC_AgentInfo_Read_X509_File(const char* const x509_file_path) {
char* buffer = NULL;
long ftellResult = 0;
size_t bufferLength = 0;
size_t bytesRead = 0;
FILE* x509File = NULL;

x509File = fopen(x509_file_path, "rb");
if (x509File)
Copy link
Contributor

Choose a reason for hiding this comment

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

use explicit checks for NULL / non-NULL:

if (x509File != NULL)

{
if (fseek(x509File, 0, SEEK_END) == 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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:

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 (

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

ftellResult = ftell(x509File);
if (ftellResult != -1)
{
bufferLength = (size_t)ftellResult;
if (fseek(x509File, 0, SEEK_SET) == 0)
{
buffer = malloc(bufferLength);
if (buffer)
{
bytesRead = fread(buffer, 1, bufferLength, x509File);
if (bytesRead < bufferLength)
{
if (feof(x509File))
Copy link
Contributor

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

paren on next line

Log_Error("Error occurred while reading '%s' (bytesRead: %zu)!", x509_file_path, bytesRead);
}
else
{
Log_Error("Failed to read '%s' (bytesRead: %zu)!", x509_file_path, bytesRead);
}
free(buffer);
Copy link
Contributor

Choose a reason for hiding this comment

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

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

buffer = NULL;
}
}
else
{
Log_Error("Failed to get memory for buffer!");
}
}
else
{
Log_Error("Failed to seek begin of '%s'!", x509_file_path);
}
}
else
{
Log_Error("Failed to get file position ('%d')!", errno);
}
}
else
{
Log_Error("Failed to seek end of '%s'!", x509_file_path);
}

if(fclose (x509File) != 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Log_Warn("Failed to close '%s'!", x509_file_path);
Copy link
Contributor

Choose a reason for hiding this comment

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

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)

}
}
else
{
Log_Error("Failed to open '%s'!", x509_file_path);
}

return(buffer);
Copy link
Contributor

Choose a reason for hiding this comment

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

style: no parens for return

}

/**
* @brief Initializes an ADUC_AgentInfo object
* @param agent the agent to be initialized
Expand Down Expand Up @@ -94,6 +169,9 @@ static bool ADUC_AgentInfo_Init(ADUC_AgentInfo* agent, const JSON_Object* agent_
JSON_Object* connection_source = json_object_get_object(agent_obj, CONFIG_CONNECTION_SOURCE);
const char* connection_type = NULL;
const char* connection_data = NULL;
const char* connection_x509_cert_file_path = NULL;
const char* connection_x509_private_key_file_path = NULL;
const char* connection_x509_ca_cert_file_path = NULL;

if (connection_source == NULL)
{
Expand All @@ -102,6 +180,9 @@ static bool ADUC_AgentInfo_Init(ADUC_AgentInfo* agent, const JSON_Object* agent_

connection_type = json_object_get_string(connection_source, CONFIG_CONNECTION_TYPE);
connection_data = json_object_get_string(connection_source, CONFIG_CONNECTION_DATA);
connection_x509_cert_file_path = json_object_get_string(connection_source, CONFIG_CONNECTION_X509_CERT_FILE_PATH);
connection_x509_private_key_file_path = json_object_get_string(connection_source, CONFIG_CONNECTION_X509_PRIVATE_KEY_FILE_PATH);
connection_x509_ca_cert_file_path = json_object_get_string(connection_source, CONFIG_CONNECTION_X509_CA_CERT_FILE_PATH);

// As these fields are mandatory, if any of the fields doesn't exist, the agent will fail to be constructed.
if (name == NULL || runas == NULL || connection_type == NULL || connection_data == NULL || manufacturer == NULL
Expand Down Expand Up @@ -130,6 +211,28 @@ static bool ADUC_AgentInfo_Init(ADUC_AgentInfo* agent, const JSON_Object* agent_
goto done;
}

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)
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

{
goto done;
}
agent->x509PrivateKey = ADUC_AgentInfo_Read_X509_File(connection_x509_private_key_file_path);
if (!agent->x509PrivateKey)
{
goto done;
}
agent->x509CaCert = ADUC_AgentInfo_Read_X509_File(connection_x509_ca_cert_file_path);
if (!agent->x509CaCert)
{
goto done;
}
} else {
Log_Error("Incomplete X509 client certificate configuration! Cert file path or private key file path is missing.");
}
}

if (mallocAndStrcpy_s(&(agent->manufacturer), manufacturer) != 0)
{
goto done;
Expand Down Expand Up @@ -173,6 +276,15 @@ static void ADUC_AgentInfo_Free(ADUC_AgentInfo* agent)
free(agent->connectionData);
agent->connectionData = NULL;

free(agent->x509Cert);
agent->x509Cert = NULL;

free(agent->x509PrivateKey);
agent->x509PrivateKey = NULL;

free(agent->x509CaCert);
agent->x509CaCert = NULL;

free(agent->manufacturer);
agent->manufacturer = NULL;

Expand Down