[jira] [Commented] (PROTON-992) Proton's use of Cyrus SASL is not thread-safe.
[ https://issues.apache.org/jira/browse/PROTON-992?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=14902731#comment-14902731 ] michael goulish commented on PROTON-992: oops. this is a duplicate of PROTON-862 > Proton's use of Cyrus SASL is not thread-safe. > -- > > Key: PROTON-992 > URL: https://issues.apache.org/jira/browse/PROTON-992 > Project: Qpid Proton > Issue Type: Bug > Components: proton-c >Affects Versions: 0.10 >Reporter: michael goulish >Assignee: michael goulish >Priority: Critical > > Documentation for the Cyrus SASL library says that the library is believed to > be thread-safe only if the code that uses it meets several requirements. > The requirements are: > * you supply mutex functions (see sasl_set_mutex()) > * you make no libsasl calls until sasl_client/server_init() completes > * no libsasl calls are made after sasl_done() is begun > * when using GSSAPI, you use a thread-safe GSS / Kerberos 5 library. > It says explicitly that that sasl_set* calls are not thread safe, since they > set global state. > The proton library makes calls to sasl_set* functions in : > pni_init_client() > pni_init_server(), and > pni_process_init() > Since those are internal functions, there is no way for code that uses Proton > to lock around those calls. > I think proton needs a new API call to let applications call > sasl_set_mutex(). Or something. > We probably also need other protections to meet the other requirements > specified in the Cyrus documentation (and quoted above). -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Commented] (PROTON-992) Proton's use of Cyrus SASL is not thread-safe.
[ https://issues.apache.org/jira/browse/PROTON-992?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=14875683#comment-14875683 ] Andrew Stitcher commented on PROTON-992: This is a fair point. It's worth noting that for Cyrus SASL there is no intrinsically global state, which makes this whole issue all the more irritating. With some work we could actually modify the Cyrus SASL codebase to not have the global init/fini. Equally we could change the code to use gnu SASL which does this stuff correctly (but would require us to handle the authentication database side of things). Incidentally this bug is a restatement of PROTON-862 > Proton's use of Cyrus SASL is not thread-safe. > -- > > Key: PROTON-992 > URL: https://issues.apache.org/jira/browse/PROTON-992 > Project: Qpid Proton > Issue Type: Bug > Components: proton-c >Affects Versions: 0.10 >Reporter: michael goulish >Assignee: michael goulish >Priority: Critical > > Documentation for the Cyrus SASL library says that the library is believed to > be thread-safe only if the code that uses it meets several requirements. > The requirements are: > * you supply mutex functions (see sasl_set_mutex()) > * you make no libsasl calls until sasl_client/server_init() completes > * no libsasl calls are made after sasl_done() is begun > * when using GSSAPI, you use a thread-safe GSS / Kerberos 5 library. > It says explicitly that that sasl_set* calls are not thread safe, since they > set global state. > The proton library makes calls to sasl_set* functions in : > pni_init_client() > pni_init_server(), and > pni_process_init() > Since those are internal functions, there is no way for code that uses Proton > to lock around those calls. > I think proton needs a new API call to let applications call > sasl_set_mutex(). Or something. > We probably also need other protections to meet the other requirements > specified in the Cyrus documentation (and quoted above). -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Commented] (PROTON-992) Proton's use of Cyrus SASL is not thread-safe.
[ https://issues.apache.org/jira/browse/PROTON-992?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=14875651#comment-14875651 ] Alan Conway commented on PROTON-992: You need more than an atomic counter to implement exactly once - if there are concurrent calls to init you must not only ensure that only one of them executes, but also that the others *wait till it is complete*. You need a lock or the equivalent of pthread_once. In terms of safety, ease of use and avoiding API change I agree this is the way to go. However my concern is that it makes proton harder to port since we need a portable abstraction for "once" and we introduce a dependency on some library for locking. The original design goal of proton was to be a very portable, pure C99 codebase, thread *agnostic* codebase. Pulling in a dependency on pthreads doesn't seem to be in that spirit. E.g. I'm not sure how pthreads will play with the Go binding. That's why the SASL allows you to specify locks externally - so it doesn't force a thread library on you. That's a horrible solution but I'm not sure I like forcing pthreads any better. The "traditional" C solution to this problem is to have sufficient start-up and shut-down functions that must be called in main before and after use or yes, you will get leaks. It's C. Everybody does it - SASL, OpenSSL, everybody. Trying to be more clever than the language we are coding in seems likely to lead to trouble - IMO it already has and this JIRA is it. > Proton's use of Cyrus SASL is not thread-safe. > -- > > Key: PROTON-992 > URL: https://issues.apache.org/jira/browse/PROTON-992 > Project: Qpid Proton > Issue Type: Bug > Components: proton-c >Affects Versions: 0.10 >Reporter: michael goulish >Assignee: michael goulish >Priority: Critical > > Documentation for the Cyrus SASL library says that the library is believed to > be thread-safe only if the code that uses it meets several requirements. > The requirements are: > * you supply mutex functions (see sasl_set_mutex()) > * you make no libsasl calls until sasl_client/server_init() completes > * no libsasl calls are made after sasl_done() is begun > * when using GSSAPI, you use a thread-safe GSS / Kerberos 5 library. > It says explicitly that that sasl_set* calls are not thread safe, since they > set global state. > The proton library makes calls to sasl_set* functions in : > pni_init_client() > pni_init_server(), and > pni_process_init() > Since those are internal functions, there is no way for code that uses Proton > to lock around those calls. > I think proton needs a new API call to let applications call > sasl_set_mutex(). Or something. > We probably also need other protections to meet the other requirements > specified in the Cyrus documentation (and quoted above). -- This message was sent by Atlassian JIRA (v6.3.4#6332)
Re: [jira] [Commented] (PROTON-992) Proton's use of Cyrus SASL is not thread-safe.
Thanks! I wondered about that (briefly) but thought there was nothing to be done. If you have a sketch, I would be happy to see it! - Original Message - [ https://issues.apache.org/jira/browse/PROTON-992?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=14802937#comment-14802937 ] Andrew Stitcher commented on PROTON-992: Upon a few days reflection I've realised that you cannot fix this problem with a global init for proton: This is because there are a couple of parameters of the Cyrus library that *must* be set before calling either sasl_server_init() or sasl_client_init(). These are the configuration file directory and the server name. Currently if you want to customise these you must set them before the first usage of SASL and this works as SASL is initialised lazily. However in the proposed API there is literally no place to set them: Since the usage pattern has to be documented that you must initialise the library before using it the Cyrus Sasl ibrary will be initialised before you are allowed to use the APIs that set the path or name. So I think the only workable solution is to have an atomic count of use of the library and initialise on the going from 0->1 then finalise on going from 1->0. Obviously also keeping track of how many uses the library has as well so that we can finalise in the correct place. An important point here is to make the count atomic so that we can be sure to avoid any re-entrance into the initialisation or finalisation code (this is doable using gcc/clang builtins - we don't really support Cyrus on Win32 so Visual Studio isn't too important, but it has atomic primitives too) I would note that really we should alos be using atomic counts like this in the OpenSSL code too,. > Proton's use of Cyrus SASL is not thread-safe. > -- > > Key: PROTON-992 > URL: https://issues.apache.org/jira/browse/PROTON-992 > Project: Qpid Proton > Issue Type: Bug > Components: proton-c >Affects Versions: 0.10 >Reporter: michael goulish >Assignee: michael goulish >Priority: Critical > > Documentation for the Cyrus SASL library says that the library is believed to > be thread-safe only if the code that uses it meets several requirements. > The requirements are: > * you supply mutex functions (see sasl_set_mutex()) > * you make no libsasl calls until sasl_client/server_init() completes > * no libsasl calls are made after sasl_done() is begun > * when using GSSAPI, you use a thread-safe GSS / Kerberos 5 library. > It says explicitly that that sasl_set* calls are not thread safe, since they > set global state. > The proton library makes calls to sasl_set* functions in : > pni_init_client() > pni_init_server(), and > pni_process_init() > Since those are internal functions, there is no way for code that uses Proton > to lock around those calls. > I think proton needs a new API call to let applications call > sasl_set_mutex(). Or something. > We probably also need other protections to meet the other requirements > specified in the Cyrus documentation (and quoted above). -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Commented] (PROTON-992) Proton's use of Cyrus SASL is not thread-safe.
[ https://issues.apache.org/jira/browse/PROTON-992?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=14802937#comment-14802937 ] Andrew Stitcher commented on PROTON-992: Upon a few days reflection I've realised that you cannot fix this problem with a global init for proton: This is because there are a couple of parameters of the Cyrus library that *must* be set before calling either sasl_server_init() or sasl_client_init(). These are the configuration file directory and the server name. Currently if you want to customise these you must set them before the first usage of SASL and this works as SASL is initialised lazily. However in the proposed API there is literally no place to set them: Since the usage pattern has to be documented that you must initialise the library before using it the Cyrus Sasl ibrary will be initialised before you are allowed to use the APIs that set the path or name. So I think the only workable solution is to have an atomic count of use of the library and initialise on the going from 0->1 then finalise on going from 1->0. Obviously also keeping track of how many uses the library has as well so that we can finalise in the correct place. An important point here is to make the count atomic so that we can be sure to avoid any re-entrance into the initialisation or finalisation code (this is doable using gcc/clang builtins - we don't really support Cyrus on Win32 so Visual Studio isn't too important, but it has atomic primitives too) I would note that really we should alos be using atomic counts like this in the OpenSSL code too,. > Proton's use of Cyrus SASL is not thread-safe. > -- > > Key: PROTON-992 > URL: https://issues.apache.org/jira/browse/PROTON-992 > Project: Qpid Proton > Issue Type: Bug > Components: proton-c >Affects Versions: 0.10 >Reporter: michael goulish >Assignee: michael goulish >Priority: Critical > > Documentation for the Cyrus SASL library says that the library is believed to > be thread-safe only if the code that uses it meets several requirements. > The requirements are: > * you supply mutex functions (see sasl_set_mutex()) > * you make no libsasl calls until sasl_client/server_init() completes > * no libsasl calls are made after sasl_done() is begun > * when using GSSAPI, you use a thread-safe GSS / Kerberos 5 library. > It says explicitly that that sasl_set* calls are not thread safe, since they > set global state. > The proton library makes calls to sasl_set* functions in : > pni_init_client() > pni_init_server(), and > pni_process_init() > Since those are internal functions, there is no way for code that uses Proton > to lock around those calls. > I think proton needs a new API call to let applications call > sasl_set_mutex(). Or something. > We probably also need other protections to meet the other requirements > specified in the Cyrus documentation (and quoted above). -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Commented] (PROTON-992) Proton's use of Cyrus SASL is not thread-safe.
[ https://issues.apache.org/jira/browse/PROTON-992?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=14740985#comment-14740985 ] Alan Conway commented on PROTON-992: Yep, was looking at an older version of the code. My bad, re-reading. > Proton's use of Cyrus SASL is not thread-safe. > -- > > Key: PROTON-992 > URL: https://issues.apache.org/jira/browse/PROTON-992 > Project: Qpid Proton > Issue Type: Bug > Components: proton-c >Affects Versions: 0.10 >Reporter: michael goulish >Assignee: michael goulish >Priority: Critical > > Documentation for the Cyrus SASL library says that the library is believed to > be thread-safe only if the code that uses it meets several requirements. > The requirements are: > * you supply mutex functions (see sasl_set_mutex()) > * you make no libsasl calls until sasl_client/server_init() completes > * no libsasl calls are made after sasl_done() is begun > * when using GSSAPI, you use a thread-safe GSS / Kerberos 5 library. > It says explicitly that that sasl_set* calls are not thread safe, since they > set global state. > The proton library makes calls to sasl_set* functions in : > pni_init_client() > pni_init_server(), and > pni_process_init() > Since those are internal functions, there is no way for code that uses Proton > to lock around those calls. > I think proton needs a new API call to let applications call > sasl_set_mutex(). Or something. > We probably also need other protections to meet the other requirements > specified in the Cyrus documentation (and quoted above). -- This message was sent by Atlassian JIRA (v6.3.4#6332)
[jira] [Commented] (PROTON-992) Proton's use of Cyrus SASL is not thread-safe.
[ https://issues.apache.org/jira/browse/PROTON-992?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel=14739069#comment-14739069 ] Alan Conway commented on PROTON-992: The general proton philosophy on thread safety is to be "thread agnosic". Calls involving values associated with a single engine or reactor *cannot* be made concurrently, but objects belonging to different engines or reactors are independent and can be used concurrently simply because there are no shared variables. Locking or serialization is left to higher layers, which is as it should be. We need to preserve that philosophy. The main problem is that proton tries to hide the sasl_client_init() and sasl_client_done() calls in calls that can be made per-connection. Looking at the cyrus code, these calls they are clearly not intended to be safe to call multiple times or concurrently as they over-write global state. Many C libraries have init/done functions that must be called exactly once per process (from main() in C, from static ctor/dtor in C++ etc.) It is a hateful requirement and proton has done well to avoid it but if we depend on external libraries I fear we can avoid it no more. We need global init/done functions at least for the sasl stuff. In a single threaded reactor we could call them as a convenience in default handlers for reactor start/stop events but code using multiple reactors in multiple thread contexts needs to be able to override such behavior, and real C programmers will prefer to know that all the global init/done functions are called in order under their control in main(). Practical experience with qpid C++ broker suggests that this is enough and after that per-connection serialization is sufficient, qpidd does not set the SASL mutex functions. However the SASL docs say it is *not* sufficient, and maybe qpidd is just lucky or a future version of SASL will break it. We should dig deeper before deciding what further serialization horrors need to be inflicted on the per-connection part of the SASL negotiation. > Proton's use of Cyrus SASL is not thread-safe. > -- > > Key: PROTON-992 > URL: https://issues.apache.org/jira/browse/PROTON-992 > Project: Qpid Proton > Issue Type: Bug > Components: proton-c >Affects Versions: 0.10 >Reporter: michael goulish >Priority: Critical > > Documentation for the Cyrus SASL library says that the library is believed to > be thread-safe only if the code that uses it meets several requirements. > The requirements are: > * you supply mutex functions (see sasl_set_mutex()) > * you make no libsasl calls until sasl_client/server_init() completes > * no libsasl calls are made after sasl_done() is begun > * when using GSSAPI, you use a thread-safe GSS / Kerberos 5 library. > It says explicitly that that sasl_set* calls are not thread safe, since they > set global state. > The proton library makes calls to sasl_set* functions in : > pni_init_client() > pni_init_server(), and > pni_process_init() > Since those are internal functions, there is no way for code that uses Proton > to lock around those calls. > I think proton needs a new API call to let applications call > sasl_set_mutex(). Or something. > We probably also need other protections to meet the other requirements > specified in the Cyrus documentation (and quoted above). -- This message was sent by Atlassian JIRA (v6.3.4#6332)
