[GitHub] [qpid-dispatch] ChugR commented on a change in pull request #472: Dispatch 1288 1 - policy for outbound connectors
ChugR commented on a change in pull request #472: Dispatch 1288 1 - policy for outbound connectors URL: https://github.com/apache/qpid-dispatch/pull/472#discussion_r268183135 ## File path: src/policy.c ## @@ -1110,6 +1152,62 @@ void qd_policy_amqp_open(qd_connection_t *qd_conn) { } +void qd_policy_amqp_open_connector(qd_connection_t *qd_conn) { Review comment: I'm leaving the name as-is. It looks good at the call site in container.c. There are two hard problems in software engineering: cache invalidation, naming things, and off-by-one errors. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: dev-unsubscr...@qpid.apache.org For additional commands, e-mail: dev-h...@qpid.apache.org
[GitHub] [qpid-dispatch] ChugR commented on a change in pull request #472: Dispatch 1288 1 - policy for outbound connectors
ChugR commented on a change in pull request #472: Dispatch 1288 1 - policy for outbound connectors URL: https://github.com/apache/qpid-dispatch/pull/472#discussion_r268181900 ## File path: src/policy.c ## @@ -404,18 +409,47 @@ bool qd_policy_open_lookup_user( } else { qd_log(policy->log_source, QD_LOG_DEBUG, "Internal: lookup_user: lookup_user"); } +Py_XDECREF(module); } -if (!res) { -if (module) { -Py_XDECREF(module); -} -qd_python_unlock(lock_state); -return false; -} +qd_python_unlock(lock_state); -// if (name_buf[0]) { -// Go get the named settings +qd_log(policy->log_source, + QD_LOG_TRACE, + "[%"PRIu64"]: ALLOW AMQP Open lookup_user: %s, rhost: %s, vhost: %s, connection: %s. Usergroup: '%s'%s", + conn_id, username, hostip, vhost, conn_name, name_buf, (res ? "" : " Internal error.")); +} +return res; +} + + +/** Fetch policy settings for a vhost/group + * A vhost database user group name has been returned by qd_policy_open_lookup_user + * or by some configuration value. Access the vhost database for that group and + * extract the run-time settings. + * @param[in] policy pointer to policy + * @param[in] username authenticated user name (for logging) + * @param[in] hostip numeric host ip address (for logging) + * @param[in] vhost vhost name + * @param[in] conn_name connection name for tracking (for logging) + * @param[in] name_buf group name that holds the settings of interest + * @param[in] conn_id connection id for log tracking (for logging) + * @param[out] settings pointer to settings object to be filled with policy values + **/ +bool qd_policy_open_fetch_settings( +qd_policy_t *policy, +const char *username, +const char *hostip, +const char *vhost, +const char *conn_name, +const char *name_buf, +uint64_tconn_id, Review comment: Fixed at 7baea. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: dev-unsubscr...@qpid.apache.org For additional commands, e-mail: dev-h...@qpid.apache.org
[GitHub] [qpid-dispatch] ChugR commented on a change in pull request #472: Dispatch 1288 1 - policy for outbound connectors
ChugR commented on a change in pull request #472: Dispatch 1288 1 - policy for outbound connectors URL: https://github.com/apache/qpid-dispatch/pull/472#discussion_r268181596 ## File path: src/policy.c ## @@ -1110,6 +1152,62 @@ void qd_policy_amqp_open(qd_connection_t *qd_conn) { } +void qd_policy_amqp_open_connector(qd_connection_t *qd_conn) { +pn_connection_t *conn = qd_connection_pn(qd_conn); +qd_dispatch_t *qd = qd_server_dispatch(qd_conn->server); +qd_policy_t *policy = qd->policy; +bool connection_allowed = true; + +if (policy->enableVhostPolicy && +(!qd_conn->role || !strcmp(qd_conn->role, "normal") || !strcmp(qd_conn->role, "route-container"))) { +// Open connection or not based on policy. +const char *hostip = qd_connection_remote_ip(qd_conn); +const char *conn_name = qd_connection_name(qd_conn); +uint32_t conn_id = qd_conn->connection_id; + +qd_connector_t *connector = qd_connection_connector(qd_conn); +const char *policy_vhost = qd_connector_policy_vhost(connector); + +if (!qd_conn->policy_settings) { +qd_conn->policy_settings = NEW(qd_policy_settings_t); // TODO: memory pool for settings +ZERO(qd_conn->policy_settings); +} + +if (strlen(policy_vhost) > 0) { +// This connector connection is controlled by policy. +if (qd_policy_open_fetch_settings(policy, qd_conn->user_id, hostip, policy_vhost, conn_name, +POLICY_VHOST_GROUP, conn_id, +qd_conn->policy_settings)) { +// It's too late to apply transport policy settings as the local +// AMQP Open has already been sent. +// TODO: Apply transport max_frame and channel_max to outgoing Review comment: Resolved at 7baea. In outbound connections all the transport settings are from the connector and none come from policy. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: dev-unsubscr...@qpid.apache.org For additional commands, e-mail: dev-h...@qpid.apache.org
[GitHub] [qpid-dispatch] ChugR commented on a change in pull request #472: Dispatch 1288 1 - policy for outbound connectors
ChugR commented on a change in pull request #472: Dispatch 1288 1 - policy for outbound connectors URL: https://github.com/apache/qpid-dispatch/pull/472#discussion_r268180966 ## File path: src/policy.c ## @@ -404,18 +409,47 @@ bool qd_policy_open_lookup_user( } else { qd_log(policy->log_source, QD_LOG_DEBUG, "Internal: lookup_user: lookup_user"); } +Py_XDECREF(module); } -if (!res) { -if (module) { -Py_XDECREF(module); -} -qd_python_unlock(lock_state); -return false; -} +qd_python_unlock(lock_state); -// if (name_buf[0]) { -// Go get the named settings +qd_log(policy->log_source, + QD_LOG_TRACE, + "[%"PRIu64"]: ALLOW AMQP Open lookup_user: %s, rhost: %s, vhost: %s, connection: %s. Usergroup: '%s'%s", + conn_id, username, hostip, vhost, conn_name, name_buf, (res ? "" : " Internal error.")); +} +return res; +} + + +/** Fetch policy settings for a vhost/group + * A vhost database user group name has been returned by qd_policy_open_lookup_user + * or by some configuration value. Access the vhost database for that group and + * extract the run-time settings. + * @param[in] policy pointer to policy + * @param[in] username authenticated user name (for logging) + * @param[in] hostip numeric host ip address (for logging) + * @param[in] vhost vhost name + * @param[in] conn_name connection name for tracking (for logging) + * @param[in] name_buf group name that holds the settings of interest + * @param[in] conn_id connection id for log tracking (for logging) + * @param[out] settings pointer to settings object to be filled with policy values + **/ +bool qd_policy_open_fetch_settings( +qd_policy_t *policy, +const char *username, +const char *hostip, +const char *vhost, +const char *conn_name, +const char *name_buf, +uint64_tconn_id, +qd_policy_settings_t *settings) +{ +bool res = false; +qd_python_lock_state_t lock_state = qd_python_lock(); +PyObject *module = PyImport_ImportModule("qpid_dispatch_internal.policy.policy_manager"); +if (module) { res = false; Review comment: fixed at 7baea This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: dev-unsubscr...@qpid.apache.org For additional commands, e-mail: dev-h...@qpid.apache.org
[GitHub] [qpid-dispatch] ChugR commented on a change in pull request #472: Dispatch 1288 1 - policy for outbound connectors
ChugR commented on a change in pull request #472: Dispatch 1288 1 - policy for outbound connectors URL: https://github.com/apache/qpid-dispatch/pull/472#discussion_r267894583 ## File path: src/policy.c ## @@ -425,34 +459,39 @@ bool qd_policy_open_lookup_user( (PyObject *)policy->py_policy_manager, vhost, name_buf, upolicy); if (result2) { -settings->maxFrameSize = qd_entity_opt_long((qd_entity_t*)upolicy, "maxFrameSize", 0); -settings->maxSessionWindow = qd_entity_opt_long((qd_entity_t*)upolicy, "maxSessionWindow", 0); -settings->maxSessions = qd_entity_opt_long((qd_entity_t*)upolicy, "maxSessions", 0); -settings->maxSenders = qd_entity_opt_long((qd_entity_t*)upolicy, "maxSenders", 0); -settings->maxReceivers = qd_entity_opt_long((qd_entity_t*)upolicy, "maxReceivers", 0); -if (!settings->allowAnonymousSender) { //don't override if enabled by authz plugin -settings->allowAnonymousSender = qd_entity_opt_bool((qd_entity_t*)upolicy, "allowAnonymousSender", false); -} -if (!settings->allowDynamicSource) { //don't override if enabled by authz plugin -settings->allowDynamicSource = qd_entity_opt_bool((qd_entity_t*)upolicy, "allowDynamicSource", false); +int truthy = PyObject_IsTrue(result2); +if (truthy) { +settings->maxFrameSize = qd_entity_opt_long((qd_entity_t*)upolicy, "maxFrameSize", 0); Review comment: This function is shared by the incoming connection and outgoing connection handlers. The incoming connection handler needs it. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: dev-unsubscr...@qpid.apache.org For additional commands, e-mail: dev-h...@qpid.apache.org
[GitHub] [qpid-dispatch] ChugR commented on a change in pull request #472: Dispatch 1288 1 - policy for outbound connectors
ChugR commented on a change in pull request #472: Dispatch 1288 1 - policy for outbound connectors URL: https://github.com/apache/qpid-dispatch/pull/472#discussion_r267893514 ## File path: src/policy.c ## @@ -404,18 +409,47 @@ bool qd_policy_open_lookup_user( } else { qd_log(policy->log_source, QD_LOG_DEBUG, "Internal: lookup_user: lookup_user"); } +Py_XDECREF(module); } -if (!res) { -if (module) { -Py_XDECREF(module); -} -qd_python_unlock(lock_state); -return false; -} +qd_python_unlock(lock_state); -// if (name_buf[0]) { -// Go get the named settings +qd_log(policy->log_source, + QD_LOG_TRACE, + "[%"PRIu64"]: ALLOW AMQP Open lookup_user: %s, rhost: %s, vhost: %s, connection: %s. Usergroup: '%s'%s", + conn_id, username, hostip, vhost, conn_name, name_buf, (res ? "" : " Internal error.")); +} +return res; +} + + +/** Fetch policy settings for a vhost/group + * A vhost database user group name has been returned by qd_policy_open_lookup_user + * or by some configuration value. Access the vhost database for that group and + * extract the run-time settings. + * @param[in] policy pointer to policy + * @param[in] username authenticated user name (for logging) + * @param[in] hostip numeric host ip address (for logging) + * @param[in] vhost vhost name + * @param[in] conn_name connection name for tracking (for logging) + * @param[in] name_buf group name that holds the settings of interest + * @param[in] conn_id connection id for log tracking (for logging) + * @param[out] settings pointer to settings object to be filled with policy values + **/ +bool qd_policy_open_fetch_settings( +qd_policy_t *policy, +const char *username, +const char *hostip, +const char *vhost, +const char *conn_name, +const char *name_buf, +uint64_tconn_id, +qd_policy_settings_t *settings) +{ +bool res = false; +qd_python_lock_state_t lock_state = qd_python_lock(); Review comment: [DISPATCH-1298](https://issues.apache.org/jira/browse/DISPATCH-1298) This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: dev-unsubscr...@qpid.apache.org For additional commands, e-mail: dev-h...@qpid.apache.org
[GitHub] [qpid-dispatch] ChugR commented on a change in pull request #472: Dispatch 1288 1 - policy for outbound connectors
ChugR commented on a change in pull request #472: Dispatch 1288 1 - policy for outbound connectors URL: https://github.com/apache/qpid-dispatch/pull/472#discussion_r267883009 ## File path: src/policy.c ## @@ -1083,15 +1119,21 @@ void qd_policy_amqp_open(qd_connection_t *qd_conn) { } if (qd_policy_open_lookup_user(policy, qd_conn->user_id, hostip, vhost, conn_name, - settings_name, SETTINGS_NAME_SIZE, conn_id, - qd_conn->policy_settings) && + settings_name, SETTINGS_NAME_SIZE, conn_id) && settings_name[0]) { // This connection is allowed by policy. // Apply transport policy settings -if (qd_conn->policy_settings->maxFrameSize > 0) -pn_transport_set_max_frame(pn_trans, qd_conn->policy_settings->maxFrameSize); -if (qd_conn->policy_settings->maxSessions > 0) -pn_transport_set_channel_max(pn_trans, qd_conn->policy_settings->maxSessions - 1); +if (qd_policy_open_fetch_settings(policy, qd_conn->user_id, hostip, vhost, conn_name, +settings_name, conn_id, +qd_conn->policy_settings)) { +if (qd_conn->policy_settings->maxFrameSize > 0) +pn_transport_set_max_frame(pn_trans, qd_conn->policy_settings->maxFrameSize); +if (qd_conn->policy_settings->maxSessions > 0) +pn_transport_set_channel_max(pn_trans, qd_conn->policy_settings->maxSessions - 1); +} else { +// failed to fetch settings +connection_allowed = false; Review comment: The common pattern in dispatch C code is if any qd_entity_get call fails then the entity in question is failed. Same goes for connector connection policy: if the policy does not load then that connection is denied. Pre-loading the policy is a separate issue. Suppose there was a preload that failed then all the connections using that failed preload must fail as well. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: dev-unsubscr...@qpid.apache.org For additional commands, e-mail: dev-h...@qpid.apache.org
[GitHub] [qpid-dispatch] ChugR commented on a change in pull request #472: Dispatch 1288 1 - policy for outbound connectors
ChugR commented on a change in pull request #472: Dispatch 1288 1 - policy for outbound connectors URL: https://github.com/apache/qpid-dispatch/pull/472#discussion_r267843341 ## File path: src/policy.c ## @@ -404,18 +409,47 @@ bool qd_policy_open_lookup_user( } else { qd_log(policy->log_source, QD_LOG_DEBUG, "Internal: lookup_user: lookup_user"); } +Py_XDECREF(module); } -if (!res) { -if (module) { -Py_XDECREF(module); -} -qd_python_unlock(lock_state); -return false; -} +qd_python_unlock(lock_state); -// if (name_buf[0]) { -// Go get the named settings +qd_log(policy->log_source, + QD_LOG_TRACE, + "[%"PRIu64"]: ALLOW AMQP Open lookup_user: %s, rhost: %s, vhost: %s, connection: %s. Usergroup: '%s'%s", + conn_id, username, hostip, vhost, conn_name, name_buf, (res ? "" : " Internal error.")); +} +return res; +} + + +/** Fetch policy settings for a vhost/group + * A vhost database user group name has been returned by qd_policy_open_lookup_user + * or by some configuration value. Access the vhost database for that group and + * extract the run-time settings. + * @param[in] policy pointer to policy + * @param[in] username authenticated user name (for logging) + * @param[in] hostip numeric host ip address (for logging) + * @param[in] vhost vhost name + * @param[in] conn_name connection name for tracking (for logging) + * @param[in] name_buf group name that holds the settings of interest + * @param[in] conn_id connection id for log tracking (for logging) + * @param[out] settings pointer to settings object to be filled with policy values + **/ +bool qd_policy_open_fetch_settings( +qd_policy_t *policy, +const char *username, +const char *hostip, +const char *vhost, +const char *conn_name, +const char *name_buf, +uint64_tconn_id, +qd_policy_settings_t *settings) +{ +bool res = false; +qd_python_lock_state_t lock_state = qd_python_lock(); +PyObject *module = PyImport_ImportModule("qpid_dispatch_internal.policy.policy_manager"); +if (module) { res = false; Review comment: true. it's a wasted line of code as it gets optimized away. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: dev-unsubscr...@qpid.apache.org For additional commands, e-mail: dev-h...@qpid.apache.org
[GitHub] [qpid-dispatch] ChugR commented on a change in pull request #472: Dispatch 1288 1 - policy for outbound connectors
ChugR commented on a change in pull request #472: Dispatch 1288 1 - policy for outbound connectors URL: https://github.com/apache/qpid-dispatch/pull/472#discussion_r267842419 ## File path: src/policy.c ## @@ -1110,6 +1152,62 @@ void qd_policy_amqp_open(qd_connection_t *qd_conn) { } +void qd_policy_amqp_open_connector(qd_connection_t *qd_conn) { +pn_connection_t *conn = qd_connection_pn(qd_conn); +qd_dispatch_t *qd = qd_server_dispatch(qd_conn->server); +qd_policy_t *policy = qd->policy; +bool connection_allowed = true; + +if (policy->enableVhostPolicy && +(!qd_conn->role || !strcmp(qd_conn->role, "normal") || !strcmp(qd_conn->role, "route-container"))) { +// Open connection or not based on policy. +const char *hostip = qd_connection_remote_ip(qd_conn); +const char *conn_name = qd_connection_name(qd_conn); +uint32_t conn_id = qd_conn->connection_id; + +qd_connector_t *connector = qd_connection_connector(qd_conn); +const char *policy_vhost = qd_connector_policy_vhost(connector); + +if (!qd_conn->policy_settings) { +qd_conn->policy_settings = NEW(qd_policy_settings_t); // TODO: memory pool for settings +ZERO(qd_conn->policy_settings); +} + +if (strlen(policy_vhost) > 0) { +// This connector connection is controlled by policy. +if (qd_policy_open_fetch_settings(policy, qd_conn->user_id, hostip, policy_vhost, conn_name, +POLICY_VHOST_GROUP, conn_id, +qd_conn->policy_settings)) { +// It's too late to apply transport policy settings as the local +// AMQP Open has already been sent. +// TODO: Apply transport max_frame and channel_max to outgoing Review comment: There are duplicate settings for the same AMQP values in connector and in vhostUserGroupSettings. At the point of this comment it is too late to for the vhostUserGroupSettings value to be applied. This sounds like a need to document which settings from the user group get applied. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: dev-unsubscr...@qpid.apache.org For additional commands, e-mail: dev-h...@qpid.apache.org
[GitHub] [qpid-dispatch] ChugR commented on a change in pull request #472: Dispatch 1288 1 - policy for outbound connectors
ChugR commented on a change in pull request #472: Dispatch 1288 1 - policy for outbound connectors URL: https://github.com/apache/qpid-dispatch/pull/472#discussion_r267838761 ## File path: src/policy.c ## @@ -1110,6 +1152,62 @@ void qd_policy_amqp_open(qd_connection_t *qd_conn) { } +void qd_policy_amqp_open_connector(qd_connection_t *qd_conn) { Review comment: The connector connection hasn't really opened yet. In this function policy is approving or denying the connection. Sure, the second Open has arrived but policy may slam the connection closed for some reason. The name is similar to the peer function that handles the remote open for incoming connections. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: dev-unsubscr...@qpid.apache.org For additional commands, e-mail: dev-h...@qpid.apache.org
[GitHub] [qpid-dispatch] ChugR commented on a change in pull request #472: Dispatch 1288 1 - policy for outbound connectors
ChugR commented on a change in pull request #472: Dispatch 1288 1 - policy for outbound connectors URL: https://github.com/apache/qpid-dispatch/pull/472#discussion_r267829682 ## File path: src/policy.c ## @@ -404,18 +409,47 @@ bool qd_policy_open_lookup_user( } else { qd_log(policy->log_source, QD_LOG_DEBUG, "Internal: lookup_user: lookup_user"); } +Py_XDECREF(module); } -if (!res) { -if (module) { -Py_XDECREF(module); -} -qd_python_unlock(lock_state); -return false; -} +qd_python_unlock(lock_state); -// if (name_buf[0]) { -// Go get the named settings +qd_log(policy->log_source, + QD_LOG_TRACE, + "[%"PRIu64"]: ALLOW AMQP Open lookup_user: %s, rhost: %s, vhost: %s, connection: %s. Usergroup: '%s'%s", + conn_id, username, hostip, vhost, conn_name, name_buf, (res ? "" : " Internal error.")); +} +return res; +} + + +/** Fetch policy settings for a vhost/group + * A vhost database user group name has been returned by qd_policy_open_lookup_user + * or by some configuration value. Access the vhost database for that group and + * extract the run-time settings. + * @param[in] policy pointer to policy + * @param[in] username authenticated user name (for logging) + * @param[in] hostip numeric host ip address (for logging) + * @param[in] vhost vhost name + * @param[in] conn_name connection name for tracking (for logging) + * @param[in] name_buf group name that holds the settings of interest + * @param[in] conn_id connection id for log tracking (for logging) + * @param[out] settings pointer to settings object to be filled with policy values + **/ +bool qd_policy_open_fetch_settings( +qd_policy_t *policy, +const char *username, +const char *hostip, +const char *vhost, +const char *conn_name, +const char *name_buf, +uint64_tconn_id, Review comment: Well spotted. These are left over from when there was a log message that used them. This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services - To unsubscribe, e-mail: dev-unsubscr...@qpid.apache.org For additional commands, e-mail: dev-h...@qpid.apache.org