Support for macOS #182
Support for macOS #182
Conversation
pkern
force-pushed
the
macos
branch
2 times, most recently
from
e8211a2 to
8173d7b
Compare
Apparently C has deprecated them, while undeprecating them in the newest - not quite available version, in alignment with C++.
Otherwise the include path is not plumbed through for consumption by login.c's compilation process when openssl is in a non-standard path (like with Homebrew on macOS).
Darwin does not define HOST_NAME_MAX and also Linux is setting it to 64 instead of 255 as defined by the minimum of _POSIX_HOST_NAME_MAX. What a mess.
At least on macOS you don't get vsyslog() if you set _POSIX_SOURCE this way.
login/pam.c
Outdated
| #ifndef PAM_GLOME_LINUX | ||
| void pam_syslog(void *pamh, ...) { (void)(pamh); } | ||
| void pam_vsyslog(void *pamh, ...) { (void)(pamh); } | ||
| #endif |
There was a problem hiding this comment.
The logic here could be clearer if the meson config would determine HAVE_PAM_EXT and then we'd
#ifdef HAVE_PAM_EXT
#include <security/pam_ext.h>
#else
void pam_syslog(void *pamh, ...) { (void)(pamh); }
void pam_vsyslog(void *pamh, ...) { (void)(pamh); }
#endifand
#ifdef PLATFORM_DARWIN
#include <security/pam_appl.h>
#endifThere was a problem hiding this comment.
I renamed the define to HAVE_PAM_EXT but made the include conditional on the else branch.
| @@ -29,6 +33,11 @@ | |||
|
|
|||
| #define MODULE_NAME "pam_glome" | |||
|
|
|||
| #ifndef PAM_GLOME_LINUX | |||
| void pam_syslog(void *pamh, ...) { (void)(pamh); } | |||
There was a problem hiding this comment.
Should we implement this in terms of syslog instead?
There was a problem hiding this comment.
With syslog it might end up in a different log file or with different format than the other PAM modules so not sure about that.
There was a problem hiding this comment.
To be fair: I have only made this compile and did not test against PAM's Darwin implementation. I suspect that way lies a lot of pain. My main worry here would be how to do it properly on the other BSD flavors.
For instance pam_krb5 documents these options:
- debug: turns on debugging via syslog(3). Debugging messages are logged with priority LOG_DEBUG.
- debug_sensitive: turns on debugging of sensitive information via syslog(3). Debug messages are logged with priority LOG_DEBUG.
Russ Albery wrote https://github.com/rra/pam-krb5/blob/main/portable/pam_vsyslog.c which I find amusing. Presumably we could handle compatibility the same way...
And there I was thinking that PAM is the proper abstraction between different OSes. ;-)
|
Maybe we should just abandon diff --git a/login/login.c b/login/login.c
index 0f6689b..1f20804 100644
--- a/login/login.c
+++ b/login/login.c
@@ -12,10 +12,6 @@
// See the License for the specific language governing permissions and
// limitations under the License.
-// For vsyslog
-#define _BSD_SOURCE
-#define _DEFAULT_SOURCE
-
#include "login.h"
#include <assert.h>
@@ -211,9 +207,15 @@ void login_syslog(glome_login_config_t* config, pam_handle_t* pamh,
int priority, const char* format, ...) {
UNUSED(pamh);
if (config->options & SYSLOG) {
+ char* buf = calloc(1024, 1);
+ if (!buf) {
+ return;
+ }
+
va_list args;
va_start(args, format);
- vsyslog(LOG_MAKEPRI(LOG_AUTH, priority), format, args);
+ vsnprintf(buf, 1024, format, args);
+ syslog(priority, "%s", buf);
va_end(args);
}
}
diff --git a/meson.build b/meson.build
index 64a51d4..a23b0b0 100644
--- a/meson.build
+++ b/meson.build
@@ -19,9 +19,7 @@ pkg = import('pkgconfig')
sysconfdir = join_paths(get_option('prefix'), get_option('sysconfdir'))
add_project_arguments('-DSYSCONFDIR="' + sysconfdir + '"', language : 'c')
-if host_machine.system() != 'darwin'
- add_project_arguments('-D_POSIX_C_SOURCE=200809L', language : 'c')
-endif
+add_project_arguments('-D_POSIX_C_SOURCE=200809L', language : 'c')
add_project_arguments('-DOPENSSL_API_COMPAT=10100', language : 'c')
openssl_dep = dependency('openssl', version : '>=1.1') |
login/login.c
Outdated
| @@ -308,6 +308,7 @@ int login_prompt(glome_login_config_t* config, pam_handle_t* pamh, | |||
| static char* create_login_message(glome_login_config_t* config, | |||
| pam_handle_t* pamh, const char** error_tag) { | |||
| char* host_id = NULL; | |||
| int max_hostname_len = sysconf(_SC_HOST_NAME_MAX) + 1; | |||
There was a problem hiding this comment.
Should we check return value of sysconf?
There was a problem hiding this comment.
Fair enough, yes. Let's pick _POSIX_HOST_NAME_MAX (given that it's per standard the minimum, even if Linux disagrees) as the fallback value.
| @@ -29,6 +33,11 @@ | |||
|
|
|||
| #define MODULE_NAME "pam_glome" | |||
|
|
|||
| #ifndef PAM_GLOME_LINUX | |||
| void pam_syslog(void *pamh, ...) { (void)(pamh); } | |||
There was a problem hiding this comment.
With syslog it might end up in a different log file or with different format than the other PAM modules so not sure about that.
vsnprintf is part of C99 and thus does not need feature test macros. Co-authored-by: Philipp Kern <pkern@google.com>
Applied, thank you! C99 ftw. |
Otherwise cpplint complains about the hardcoded buffer size.
This implements some source changes to make the code base compatible with macOS. Which is, of course, a little frustrating to fix. I have no idea how compatibility with other awkward OSes (like BSDs) will be after this change.