Skip to content

Commit

Permalink
fixup: add 'version' and 'oldestValidVersion' sslProfile fields, vari…
Browse files Browse the repository at this point in the history 
…ous name changes for clarity
  • Loading branch information
@kgiusti
kgiusti committed Aug 19, 2024
1 parent fc1f5f3 commit 8ff9acd
Show file tree
Hide file tree
Showing 4 changed files with 109 additions and 61 deletions.
7 changes: 7 additions & 0 deletions include/qpid/dispatch/tls.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,13 @@ struct qd_ssl2_profile_t {
* Full path to the file that contains the uid to display name mapping.
*/
char *uid_name_mapping_file;

/**
* version: Version assigned to the current configuration
* oldest_valid_version: Previous sslProfile updates with versions values < oldest_valid_version have expired.
*/
long version;
long oldest_valid_version;
};

/**
Expand Down
12 changes: 12 additions & 0 deletions python/skupper_router/management/skrouter.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -720,6 +720,18 @@
"description": "The absolute path to the file containing the unique id to display name mapping",
"create": true,
"update": true
},
"version": {
"type": "integer",
"description": "RESERVED FOR FUTURE USE",
"create": true,
"update": true
},
"oldestValidVersion": {
"type": "integer",
"description": "RESERVED FOR FUTURE USE",
"create": true,
"update": true
}
}
},
Expand Down
14 changes: 8 additions & 6 deletions src/tls/private.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -44,12 +44,13 @@ struct qd_tls_session_t {
pn_tls_t *pn_raw;
pn_ssl_t *pn_amqp;

void *user_context;
void *user_context;
qd_tls_session_on_secure_cb_t *on_secure_cb;

// copies from parent qd_tls_config_t to avoid extra locking:
// copies from parent qd_tls_config_t to avoid locking during I/O:
char *ssl_profile_name;
char *uid_format;
long version;

bool tls_error;
bool output_eos; // pn_tls_close_output() called
Expand All @@ -71,17 +72,18 @@ struct qd_tls_session_t {
*/
struct qd_tls_config_t {
DEQ_LINKS(qd_tls_config_t); // for parent qd_tls_context_t list
sys_mutex_t lock;
char *ssl_profile_name;
char *uid_format;
char *uid_format; // lock must be held

// only one of the following Proton pointers will be set based on whether this configuration is for a raw connection
// or an AMQP connection
pn_tls_config_t *pn_raw;
pn_ssl_domain_t *pn_amqp;
pn_tls_config_t *pn_raw; // lock must be held
pn_ssl_domain_t *pn_amqp; // lock must be held

qd_tls_type_t p_type;
sys_mutex_t lock; // must be held when accessing proton pn_amqp instance
sys_atomic_t ref_count;
long version; // lock must be held

bool authenticate_peer;
bool verify_hostname;
Expand Down
137 changes: 82 additions & 55 deletions src/tls/tls.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -56,12 +56,12 @@ static qd_tls_context_list_t context_list;


// Internals:
static qd_error_t _read_tls_config(qd_entity_t *entity, qd_ssl2_profile_t *config);
static void _cleanup_tls_config(qd_ssl2_profile_t *config);
static qd_error_t _read_tls_profile(qd_entity_t *entity, qd_ssl2_profile_t *profile);
static void _cleanup_tls_profile(qd_ssl2_profile_t *profile);
static qd_tls_context_t *_find_tls_context(const char *profile_name);
static void _tls_context_free(qd_tls_context_t *ctxt);
static qd_error_t _update_tls_config(qd_tls_config_t *tls_config, const qd_ssl2_profile_t *config);
static qd_error_t _validate_config(const qd_ssl2_profile_t *config, const char *profile_name, bool is_listener,
static qd_error_t _update_tls_config(qd_tls_config_t *tls_config, const qd_ssl2_profile_t *profile);
static qd_error_t _validate_config(const qd_ssl2_profile_t *profile, const char *profile_name, bool is_listener,
bool authenticate_peer);

// TODO: these should be moved somewhere public as they are called from multiple places
Expand Down Expand Up @@ -115,7 +115,7 @@ QD_EXPORT void *qd_tls_configure_ssl_profile(qd_dispatch_t *qd, qd_entity_t *ent
DEQ_INIT(tls_context->tls_configs);
tls_context->ssl_profile_name = name;

if (_read_tls_config(entity, &tls_context->profile) != QD_ERROR_NONE) {
if (_read_tls_profile(entity, &tls_context->profile) != QD_ERROR_NONE) {
qd_log(LOG_AGENT, QD_LOG_ERROR, "Unable to create sslProfile '%s': %s", name, qd_error_message());
_tls_context_free(tls_context);
return 0;
Expand Down Expand Up @@ -153,30 +153,30 @@ QD_EXPORT void *qd_tls_update_ssl_profile(qd_dispatch_t *qd, qd_entity_t *entity
ASSERT_MGMT_THREAD;

qd_tls_context_t *tls_context = (qd_tls_context_t *) impl;
qd_ssl2_profile_t new_config;
qd_ssl2_profile_t new_profile;

qd_error_clear();

assert(tls_context);
if (_read_tls_config(entity, &new_config) != QD_ERROR_NONE) {
if (_read_tls_profile(entity, &new_profile) != QD_ERROR_NONE) {
qd_log(LOG_AGENT, QD_LOG_ERROR, "Unable to update sslProfile '%s': %s", tls_context->ssl_profile_name, qd_error_message());
return 0;
}

qd_tls_config_t *config = DEQ_HEAD(tls_context->tls_configs);
while (config) {
if (_update_tls_config(config, &new_config) != QD_ERROR_NONE) {
if (_update_tls_config(config, &new_profile) != QD_ERROR_NONE) {
// There is a problem with the new configuration. Discard the change and return 0 to force the management
// operation to fail
qd_log(LOG_AGENT, QD_LOG_ERROR, "Failed to update sslProfile '%s': %s", tls_context->ssl_profile_name, qd_error_message());
_cleanup_tls_config(&new_config);
_cleanup_tls_profile(&new_profile);
return 0;
}
config = DEQ_NEXT(config);
}

_cleanup_tls_config(&tls_context->profile);
tls_context->profile = new_config;
_cleanup_tls_profile(&tls_context->profile);
tls_context->profile = new_profile;
qd_log(LOG_AGENT, QD_LOG_INFO, "Updated sslProfile %s ", tls_context->ssl_profile_name);
return impl;
}
Expand Down Expand Up @@ -245,6 +245,7 @@ qd_tls_config_t *qd_tls_config(const char *ssl_profile_name,

tls_config->ssl_profile_name = qd_strdup(ssl_profile_name);
tls_config->uid_format = CHECKED_STRDUP(tls_context->profile.uid_format);
tls_config->version = tls_context->profile.version;
tls_config->authenticate_peer = authenticate_peer;
tls_config->verify_hostname = verify_hostname;
tls_config->is_listener = is_listener;
Expand Down Expand Up @@ -305,6 +306,7 @@ qd_tls_session_t *qd_tls_session_raw(qd_tls_config_t *tls_config, const char *pe

sys_mutex_lock(&tls_config->lock);
tls_session->uid_format = CHECKED_STRDUP(tls_config->uid_format); // may be changed by mgmt thread
tls_session->version = tls_config->version; // may be changed by mgmt thread

// if ALPN needs to be configured it must be done on the config. The config lock needs to be held to prevent other
// threads that are starting streams on this same domin from using the wrong ALPN configuration. Once the session
Expand Down Expand Up @@ -380,6 +382,7 @@ qd_tls_session_t *qd_tls_session_amqp(qd_tls_config_t *tls_config, pn_transport_

sys_mutex_lock(&tls_config->lock);
tls_session->uid_format = CHECKED_STRDUP(tls_config->uid_format); // may be changed by mgmt thread
tls_session->version = tls_config->version; // may be changed by mgmt thread

tls_session->pn_amqp = pn_ssl(tport);
if (!tls_session->pn_amqp) {
Expand Down Expand Up @@ -544,6 +547,8 @@ qd_ssl2_profile_t *qd_tls_read_ssl_profile(const char *ssl_profile_name, qd_ssl2
profile->private_key_file = CHECKED_STRDUP(tls_context->profile.private_key_file);
profile->uid_name_mapping_file = CHECKED_STRDUP(tls_context->profile.uid_name_mapping_file);
profile->trusted_certificate_db = CHECKED_STRDUP(tls_context->profile.trusted_certificate_db);
profile->version = tls_context->profile.version;
profile->oldest_valid_version = tls_context->profile.oldest_valid_version;

return profile;
}
Expand All @@ -568,83 +573,101 @@ void qd_tls_cleanup_ssl_profile(qd_ssl2_profile_t *profile)
/**
* Read the sslProfile configuration record from entity and copy it into config
*/
static qd_error_t _read_tls_config(qd_entity_t *entity, qd_ssl2_profile_t *config)
static qd_error_t _read_tls_profile(qd_entity_t *entity, qd_ssl2_profile_t *profile)
{
ZERO(config);
ZERO(profile);

config->ciphers = qd_entity_opt_string(entity, "ciphers", 0);
char *name = 0;
name = qd_entity_opt_string(entity, "name", "<NONE>");
if (qd_error_code()) goto error;
config->protocols = qd_entity_opt_string(entity, "protocols", 0);

profile->ciphers = qd_entity_opt_string(entity, "ciphers", 0);
if (qd_error_code()) goto error;
profile->protocols = qd_entity_opt_string(entity, "protocols", 0);
if (qd_error_code()) goto error;
profile->trusted_certificate_db = qd_entity_opt_string(entity, "caCertFile", 0);
if (qd_error_code()) goto error;
config->trusted_certificate_db = qd_entity_opt_string(entity, "caCertFile", 0);
profile->certificate_file = qd_entity_opt_string(entity, "certFile", 0);
if (qd_error_code()) goto error;
config->certificate_file = qd_entity_opt_string(entity, "certFile", 0);
profile->private_key_file = qd_entity_opt_string(entity, "privateKeyFile", 0);
if (qd_error_code()) goto error;
config->private_key_file = qd_entity_opt_string(entity, "privateKeyFile", 0);
profile->password = qd_entity_opt_string(entity, "password", 0);
if (qd_error_code()) goto error;
config->password = qd_entity_opt_string(entity, "password", 0);
profile->uid_format = qd_entity_opt_string(entity, "uidFormat", 0);
if (qd_error_code()) goto error;
config->uid_format = qd_entity_opt_string(entity, "uidFormat", 0);
profile->uid_name_mapping_file = qd_entity_opt_string(entity, "uidNameMappingFile", 0);
if (qd_error_code()) goto error;
config->uid_name_mapping_file = qd_entity_opt_string(entity, "uidNameMappingFile", 0);
profile->version = qd_entity_opt_long(entity, "version", 0);
if (qd_error_code()) goto error;
profile->oldest_valid_version = qd_entity_opt_long(entity, "oldestValidVersion", 0);
if (qd_error_code()) goto error;

if (config->uid_format) {
if (!tls_private_validate_uid_format(config->uid_format)) {
char *name = qd_entity_opt_string(entity, "name", "UNKNOWN");
if (profile->uid_format) {
if (!tls_private_validate_uid_format(profile->uid_format)) {
// backward compatibility: this isn't treated as a hard error - the fallback behavior is to use the user
// name from the transport. I have no idea why that is the case but changing it to a hard error results in
// CI test failures so for now I go along to get along:
qd_log(LOG_AGENT, QD_LOG_ERROR, "Invalid format for uidFormat field in sslProfile '%s': %s",
name, config->uid_format);
free(name);
free(config->uid_format);
config->uid_format = 0;
name, profile->uid_format);
free(profile->uid_format);
profile->uid_format = 0;
}
}

if (config->password) {
if (profile->password) {
//
// Process the password to handle any modifications or lookups needed
//
char *actual_pass = 0;
bool is_file_path = 0;
qd_server_config_process_password(&actual_pass, config->password, &is_file_path, true);
qd_server_config_process_password(&actual_pass, profile->password, &is_file_path, true);
if (qd_error_code()) goto error;

if (actual_pass) {
if (is_file_path) {
qd_set_password_from_file(actual_pass, &config->password);
qd_set_password_from_file(actual_pass, &profile->password);
free(actual_pass);
}
else {
free(config->password);
config->password = actual_pass;
free(profile->password);
profile->password = actual_pass;
}
}
}

// simple validation of version fields:
if (profile->version < 0 || profile->oldest_valid_version < 0) {
qd_log(LOG_AGENT, QD_LOG_ERROR, "Negative version field values are invalid (sslProfile '%s')", name);
goto error;
}
if (profile->version < profile->oldest_valid_version) {
qd_log(LOG_AGENT, QD_LOG_ERROR, "version must be >= oldestValidVersion (sslProfile '%s')", name);
goto error;
}

free(name);
return QD_ERROR_NONE;

error:
_cleanup_tls_config(config);
free(name);
_cleanup_tls_profile(profile);
return qd_error_code();
}


/** Release the contents of a configuration instance
*/
static void _cleanup_tls_config(qd_ssl2_profile_t *config)
static void _cleanup_tls_profile(qd_ssl2_profile_t *profile)
{
free(config->ciphers);
free(config->protocols);
free(config->trusted_certificate_db);
free(config->certificate_file);
free(config->private_key_file);
free(config->password);
free(config->uid_format);
free(config->uid_name_mapping_file);
ZERO(config);
free(profile->ciphers);
free(profile->protocols);
free(profile->trusted_certificate_db);
free(profile->certificate_file);
free(profile->private_key_file);
free(profile->password);
free(profile->uid_format);
free(profile->uid_name_mapping_file);
ZERO(profile);
}


Expand All @@ -658,33 +681,33 @@ static void _cleanup_tls_config(qd_ssl2_profile_t *config)
* Note: this function blocks the caller while loading certificate files from the filesystem. It is *SLOW* - assume it
* will block for at least hundreds of milliseconds!
*/
static qd_error_t _update_tls_config(qd_tls_config_t *tls_config, const qd_ssl2_profile_t *config)
static qd_error_t _update_tls_config(qd_tls_config_t *tls_config, const qd_ssl2_profile_t *profile)
{
ASSERT_MGMT_THREAD; // runs too slow to block an I/O thread (see ISSUE-1572)

pn_tls_config_t *pn_raw_config = 0;
pn_ssl_domain_t *pn_amqp_config = 0;

if (_validate_config(config, tls_config->ssl_profile_name, tls_config->is_listener, tls_config->authenticate_peer) != QD_ERROR_NONE) {
if (_validate_config(profile, tls_config->ssl_profile_name, tls_config->is_listener, tls_config->authenticate_peer) != QD_ERROR_NONE) {
return qd_error_code(); // validate_config sets qd_error()
}

//
// Generate a new proton configuration using the updated configuration from the parent tls_context and the existing
// tls_config. Do not hold the config lock because loading certs takes a loooong time and we do not want to block
// tls_config. Do not hold the tls_config lock because loading certs takes a loooong time and we do not want to block
// new connections.
//

switch (tls_config->p_type) {
case QD_TLS_TYPE_PROTON_AMQP:
pn_amqp_config = tls_private_allocate_amqp_config(tls_config->ssl_profile_name, config, tls_config->is_listener,
pn_amqp_config = tls_private_allocate_amqp_config(tls_config->ssl_profile_name, profile, tls_config->is_listener,
tls_config->verify_hostname, tls_config->authenticate_peer);
if (!pn_amqp_config) {
return qd_error_code(); // allocation function set qd_error()
}
break;
case QD_TLS_TYPE_PROTON_RAW:
pn_raw_config = tls_private_allocate_raw_config(tls_config->ssl_profile_name, config, tls_config->is_listener,
pn_raw_config = tls_private_allocate_raw_config(tls_config->ssl_profile_name, profile, tls_config->is_listener,
tls_config->verify_hostname, tls_config->authenticate_peer);
if (!pn_raw_config) {
return qd_error_code(); // allocation function set qd_error()
Expand All @@ -708,6 +731,10 @@ static qd_error_t _update_tls_config(qd_tls_config_t *tls_config, const qd_ssl2_
tls_config->pn_amqp = pn_amqp_config;
pn_ssl_domain_free(old_amqp_config);
}
// And refresh any parameters that must be used when creating new sessions:
tls_config->version = profile->version;
free(tls_config->uid_format);
tls_config->uid_format = CHECKED_STRDUP(profile->uid_format);
sys_mutex_unlock(&tls_config->lock);

return QD_ERROR_NONE;
Expand Down Expand Up @@ -742,7 +769,7 @@ static void _tls_context_free(qd_tls_context_t *ctxt)
tls_config = DEQ_HEAD(ctxt->tls_configs);
}
free(ctxt->ssl_profile_name);
_cleanup_tls_config(&ctxt->profile);
_cleanup_tls_profile(&ctxt->profile);
free_qd_tls_context_t(ctxt);
}
}
Expand All @@ -751,27 +778,27 @@ static void _tls_context_free(qd_tls_context_t *ctxt)
/***
* Basic sanity checking that the sslProfile is valid
*/
static qd_error_t _validate_config(const qd_ssl2_profile_t *config, const char *ssl_profile_name, bool is_listener, bool authenticate_peer)
static qd_error_t _validate_config(const qd_ssl2_profile_t *profile, const char *ssl_profile_name, bool is_listener, bool authenticate_peer)
{
if (is_listener) {
// self identifying certificate is required for a listener:
if (!config->certificate_file) {
if (!profile->certificate_file) {
qd_error(QD_ERROR_CONFIG, "Listener requires a self-identifying certificate (sslProfile: %s)", ssl_profile_name);
return QD_ERROR_CONFIG;
}
if (authenticate_peer) {
if (!config->trusted_certificate_db) {
if (!profile->trusted_certificate_db) {
qd_error(QD_ERROR_CONFIG, "Listener requires a CA for peer authentication (sslProfile: %s)", ssl_profile_name);
return QD_ERROR_CONFIG;
}
}
} else if (!config->trusted_certificate_db) {
} else if (!profile->trusted_certificate_db) {
// CA must be provided for a connector:
qd_error(QD_ERROR_CONFIG, "Connector requires a CA certificate (sslProfile: %s)", ssl_profile_name);
return QD_ERROR_CONFIG;
}

if (config->certificate_file && !config->private_key_file) {
if (profile->certificate_file && !profile->private_key_file) {
// missing private key file
qd_error(QD_ERROR_CONFIG, "Missing Private Keyfile (sslProfile: %s)", ssl_profile_name);
return QD_ERROR_CONFIG;
Expand Down

0 comments on commit 8ff9acd

Please sign in to comment.