PROTON-1865: Improve the split between general SASL code and the specific implementations
Project: http://git-wip-us.apache.org/repos/asf/qpid-proton/repo Commit: http://git-wip-us.apache.org/repos/asf/qpid-proton/commit/885d68ae Tree: http://git-wip-us.apache.org/repos/asf/qpid-proton/tree/885d68ae Diff: http://git-wip-us.apache.org/repos/asf/qpid-proton/diff/885d68ae Branch: refs/heads/go1 Commit: 885d68aeaf522021a35b7b5cecb7c7c53663929b Parents: 21eb6d8 Author: Andrew Stitcher <astitc...@apache.org> Authored: Tue Jun 19 08:53:40 2018 -0400 Committer: Andrew Stitcher <astitc...@apache.org> Committed: Tue Jun 19 08:53:40 2018 -0400 ---------------------------------------------------------------------- c/include/proton/sasl-plugin.h | 1 + c/src/sasl/cyrus_sasl.c | 20 +------------------- c/src/sasl/default_sasl.c | 20 ++++++++------------ c/src/sasl/sasl.c | 28 +++++++++++++++++++++++++--- 4 files changed, 35 insertions(+), 34 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/qpid-proton/blob/885d68ae/c/include/proton/sasl-plugin.h ---------------------------------------------------------------------- diff --git a/c/include/proton/sasl-plugin.h b/c/include/proton/sasl-plugin.h index d420eaf..ae6b0ec 100644 --- a/c/include/proton/sasl-plugin.h +++ b/c/include/proton/sasl-plugin.h @@ -108,6 +108,7 @@ enum pnx_sasl_state { /* APIs used by sasl implementations */ PN_EXTERN void pnx_sasl_logf(pn_transport_t *transport, const char *format, ...); +PN_EXTERN void pnx_sasl_error(pn_transport_t *transport, const char* err, const char* condition_name); PN_EXTERN void *pnx_sasl_get_context(pn_transport_t *transport); PN_EXTERN void pnx_sasl_set_context(pn_transport_t *transport, void *context); http://git-wip-us.apache.org/repos/asf/qpid-proton/blob/885d68ae/c/src/sasl/cyrus_sasl.c ---------------------------------------------------------------------- diff --git a/c/src/sasl/cyrus_sasl.c b/c/src/sasl/cyrus_sasl.c index c589fab..a7387e6 100644 --- a/c/src/sasl/cyrus_sasl.c +++ b/c/src/sasl/cyrus_sasl.c @@ -97,10 +97,7 @@ static inline bool pni_check_result(sasl_conn_t *conn, int r, pn_transport_t *lo if (r==SASL_OK) return true; const char* err = conn ? sasl_errdetail(conn) : sasl_errstring(r, NULL, NULL); - pnx_sasl_logf(logger, "sasl error: %s", err); - pn_condition_t* c = pn_transport_condition(logger); - pn_condition_set_name(c, condition_name); - pn_condition_set_description(c, err); + pnx_sasl_error(logger, err, condition_name); return false; } @@ -120,9 +117,6 @@ static void pni_cyrus_interact(pn_transport_t *transport, sasl_interact_t *inter for (sasl_interact_t *i = interact; i->id!=SASL_CB_LIST_END; i++) { switch (i->id) { case SASL_CB_USER: - i->result = 0; - i->len = 0; - break; case SASL_CB_AUTHNAME: { const char *username = pnx_sasl_get_username(transport); i->result = username; @@ -473,8 +467,6 @@ static void pni_process_server_result(pn_transport_t *transport, int result) const void* value; sasl_getprop(cyrus_conn, SASL_USERNAME, &value); pnx_sasl_succeed_authentication(transport, (const char*) value); - pnx_sasl_logf(transport, "Authenticated user: %s with mechanism %s", - pnx_sasl_get_username(transport), pnx_sasl_get_selected_mechanism(transport)); pnx_sasl_set_desired_state(transport, SASL_POSTED_OUTCOME); break; } @@ -495,16 +487,6 @@ static void pni_process_server_result(pn_transport_t *transport, int result) void cyrus_sasl_process_init(pn_transport_t *transport, const char *mechanism, const pn_bytes_t *recv) { int result = pni_wrap_server_start(transport, mechanism, recv); - if (result==SASL_OK) { - // We need to filter out a supplied mech in in the inclusion list - // as the client could have used a mech that we support, but that - // wasn't on the list we sent. - if (!pnx_sasl_is_included_mech(transport, pn_bytes(strlen(mechanism), mechanism))) { - sasl_conn_t *cyrus_conn = (sasl_conn_t*)pnx_sasl_get_context(transport); - sasl_seterror(cyrus_conn, 0, "Client mechanism not in mechanism inclusion list."); - result = SASL_FAIL; - } - } pni_process_server_result(transport, result); } http://git-wip-us.apache.org/repos/asf/qpid-proton/blob/885d68ae/c/src/sasl/default_sasl.c ---------------------------------------------------------------------- diff --git a/c/src/sasl/default_sasl.c b/c/src/sasl/default_sasl.c index 66dd318..7299318 100644 --- a/c/src/sasl/default_sasl.c +++ b/c/src/sasl/default_sasl.c @@ -93,6 +93,7 @@ void default_sasl_impl_free(pn_transport_t *transport) } // Client handles ANONYMOUS or PLAIN mechanisms if offered +// Offered mechanisms have already been filtered against the "allowed" list bool default_sasl_process_mechanisms(pn_transport_t *transport, const char *mechs) { const char *username = pnx_sasl_get_username(transport); @@ -101,9 +102,8 @@ bool default_sasl_process_mechanisms(pn_transport_t *transport, const char *mech // Check whether offered EXTERNAL, PLAIN or ANONYMOUS // Look for "EXTERNAL" in mechs const char *found = strstr(mechs, EXTERNAL); - // Make sure that string is separated and terminated and allowed - if (found && (found==mechs || found[-1]==' ') && (found[8]==0 || found[8]==' ') && - pnx_sasl_is_included_mech(transport, pn_bytes(8, found))) { + // Make sure that string is separated and terminated + if (found && (found==mechs || found[-1]==' ') && (found[8]==0 || found[8]==' ')) { pnx_sasl_set_selected_mechanism(transport, EXTERNAL); if (username) { size_t size = strlen(username); @@ -127,7 +127,6 @@ bool default_sasl_process_mechanisms(pn_transport_t *transport, const char *mech // Make sure that string is separated and terminated, allowed // and we have a username and password and connection is encrypted or we allow insecure if (found && (found==mechs || found[-1]==' ') && (found[5]==0 || found[5]==' ') && - pnx_sasl_is_included_mech(transport, pn_bytes(5, found)) && (pnx_sasl_is_transport_encrypted(transport) || pnx_sasl_get_allow_insecure_mechs(transport)) && username && password) { pnx_sasl_set_selected_mechanism(transport, PLAIN); @@ -155,8 +154,7 @@ bool default_sasl_process_mechanisms(pn_transport_t *transport, const char *mech // Look for "ANONYMOUS" in mechs found = strstr(mechs, ANONYMOUS); // Make sure that string is separated and terminated and allowed - if (found && (found==mechs || found[-1]==' ') && (found[9]==0 || found[9]==' ') && - pnx_sasl_is_included_mech(transport, pn_bytes(9, found))) { + if (found && (found==mechs || found[-1]==' ') && (found[9]==0 || found[9]==' ')) { pnx_sasl_set_selected_mechanism(transport, ANONYMOUS); if (username) { size_t size = strlen(username); @@ -188,11 +186,11 @@ const char *default_sasl_impl_list_mechs(pn_transport_t *transport) } } +// The selected mechanism has already been verified against the "allowed" list void default_sasl_process_init(pn_transport_t *transport, const char *mechanism, const pn_bytes_t *recv) { - // Check that mechanism is ANONYMOUS and it is allowed - if (strcmp(mechanism, ANONYMOUS)==0 && - pnx_sasl_is_included_mech(transport, pn_bytes(sizeof(ANONYMOUS)-1, ANONYMOUS))) { + // Check that mechanism is ANONYMOUS + if (strcmp(mechanism, ANONYMOUS)==0) { pnx_sasl_succeed_authentication(transport, "anonymous"); pnx_sasl_set_desired_state(transport, SASL_POSTED_OUTCOME); return; @@ -200,9 +198,7 @@ void default_sasl_process_init(pn_transport_t *transport, const char *mechanism, // Or maybe EXTERNAL const char *ext_username = pnx_sasl_get_external_username(transport); - if (strcmp(mechanism, EXTERNAL)==0 && - pnx_sasl_is_included_mech(transport, pn_bytes(sizeof(EXTERNAL)-1, EXTERNAL)) && - ext_username) { + if (strcmp(mechanism, EXTERNAL)==0 && ext_username) { pnx_sasl_succeed_authentication(transport, ext_username); pnx_sasl_set_desired_state(transport, SASL_POSTED_OUTCOME); return; http://git-wip-us.apache.org/repos/asf/qpid-proton/blob/885d68ae/c/src/sasl/sasl.c ---------------------------------------------------------------------- diff --git a/c/src/sasl/sasl.c b/c/src/sasl/sasl.c index f015514..8648afa 100644 --- a/c/src/sasl/sasl.c +++ b/c/src/sasl/sasl.c @@ -48,6 +48,14 @@ void pnx_sasl_logf(pn_transport_t *logger, const char *fmt, ...) va_end(ap); } +void pnx_sasl_error(pn_transport_t *logger, const char* err, const char* condition_name) +{ + pnx_sasl_logf(logger, "sasl error: %s", err); + pn_condition_t* c = pn_transport_condition(logger); + pn_condition_set_name(c, condition_name); + pn_condition_set_description(c, err); +} + void *pnx_sasl_get_context(pn_transport_t *transport) { return transport->sasl ? transport->sasl->impl_context : NULL; @@ -137,6 +145,9 @@ void pnx_sasl_succeed_authentication(pn_transport_t *transport, const char *use transport->sasl->username = username; transport->sasl->outcome = PN_SASL_OK; transport->authenticated = true; + + pnx_sasl_logf(transport, "Authenticated user: %s with mechanism %s", + username, transport->sasl->selected_mechanism); } } @@ -822,7 +833,16 @@ int pn_do_init(pn_transport_t *transport, uint8_t frame_type, uint16_t channel, if (err) return err; sasl->selected_mechanism = pn_strndup(mech.start, mech.size); - pni_sasl_impl_process_init(transport, sasl->selected_mechanism, &recv); + // We need to filter out a supplied mech in in the inclusion list + // as the client could have used a mech that we support, but that + // wasn't on the list we sent. + if (!pni_sasl_included_mech(sasl->included_mechanisms, mech)) { + pnx_sasl_error(transport, "Client mechanism not in mechanism inclusion list.", "amqp:unauthorized-access"); + sasl->outcome = PN_SASL_AUTH; + pnx_sasl_set_desired_state(transport, SASL_POSTED_OUTCOME); + } else { + pni_sasl_impl_process_init(transport, sasl->selected_mechanism, &recv); + } return 0; } @@ -845,7 +865,7 @@ int pn_do_mechanisms(pn_transport_t *transport, uint8_t frame_type, uint16_t cha // Now keep checking for end of array and pull a symbol while(pn_data_next(args)) { pn_bytes_t s = pn_data_get_symbol(args); - if (pnx_sasl_is_included_mech(transport, s)) { + if (pni_sasl_included_mech(sasl->included_mechanisms, s)) { pn_string_addf(mechs, "%*s ", (int)s.size, s.start); } } @@ -860,7 +880,9 @@ int pn_do_mechanisms(pn_transport_t *transport, uint8_t frame_type, uint16_t cha int err = pn_data_scan(args, "D.[s]", &symbol); if (err) return err; - pn_string_setn(mechs, symbol.start, symbol.size); + if (pni_sasl_included_mech(sasl->included_mechanisms, symbol)) { + pn_string_setn(mechs, symbol.start, symbol.size); + } } if (!(pni_sasl_impl_init_client(transport) && --------------------------------------------------------------------- To unsubscribe, e-mail: commits-unsubscr...@qpid.apache.org For additional commands, e-mail: commits-h...@qpid.apache.org
