sorry for the delay, i'm kinda concentrating on another project. and
even some real life. :D
On Wed, Mar 27, 2019 at 10:03:03PM +0100, Oliver Runge wrote:
> - PassCmd is run even if if the Keychain lookup is successful
>
huh? that's not what i asked for.
the idea is to have clear precedence rules: pass_cmd, keychain,
configured password, interactive query. fail when the chosen option
fails.
> +AC_CANONICAL_HOST
> +case "${host_os}" in
>
you actually want AC_CANONICAL_TARGET.
> + AC_SUBST([KEYCHAIN_LIBS], ["-Wl,-framework -Wl,Security"])
>
you can write that as -Wl,-framework,Security afaik.
> +if test "$have_macos_keychain" = yes; then
> + AC_MSG_RESULT([Using macOS Keychain])
> +else
> + AC_MSG_RESULT([Not using macOS Keychain])
> +fi
>
for extra pedantry, you could enclose the whole thing inside a macos
conditional.
from which follows that trying to enable this option outside macos
should be an error ... hmm, actually, maybe just ignore the whole
comment.
> +++ b/src/drv_imap.c
> @@ -52,6 +56,8 @@ typedef struct imap_server_conf {
> + char *pass_keychain_name;
> + char *pass_keychain_account;
>
the variable names should not start with pass_.
> int max_in_progress;
> int cap_mask;
> string_list_t *auth_mechs;
> @@ -1870,6 +1876,28 @@ ensure_password( imap_server_conf_t *srvc )
> {
> char *cmd = srvc->pass_cmd;
>
> +#ifdef HAVE_MACOS_KEYCHAIN
> + if (srvc->pass_keychain_name && srvc->pass_keychain_account) {
> + void *password_data;
> + UInt32 password_length;
> + if (SecKeychainFindGenericPassword(
> + NULL,
> + strlen(srvc->pass_keychain_name),
> srvc->pass_keychain_name,
>
there should be internal spacing in the strlen() (the exception is
operator-like use on string constants).
that's actually a repeating problem.
> + strlen(srvc->pass_keychain_account),
> srvc->pass_keychain_account,
> + &password_length, &password_data,
> + NULL) == noErr) {
> + srvc->pass = nfmalloc((password_length + 1) *
> sizeof(char));
> + strncpy(srvc->pass, password_data,
> (size_t)password_length);
> + srvc->pass[password_length] = '\0';
>
nfstrndup() ...
> + SecKeychainItemFreeContent(NULL, password_data);
> + } else {
>
if you invert the condition, you don't need an else any more.
> + error( "Looking up Keychain item failed: name = %s,
> account = %s\n",
> + srvc->pass_keychain_name,
> srvc->pass_keychain_account );
>
that's not a very good message. it should be more like "lookup of ...
failed: [decoded error goes here]".
> +++ b/src/mbsync.1
> @@ -333,6 +333,16 @@ Prepend \fB+\fR to the command to indicate that it
> produces TTY output
> messier output.
> ..
> .TP
> +\fBKeychainName\fR \fIname\fR
> +Specify a name to look up in the macOS Keychain, requires
> \fBKeychainAccount\fR
>
i kind of have no clue what "name" refers to ...
the line length should be kept below 80 columns.
> +to be set as well. (\fBPassCmd\fR takes precedence, if specified)
> +..
>
i wouldn't document the precedence; it's not done for the other keys,
either. it's important to keep the logic in the code clean, but users
shouldn't actually rely on it - such a config file would be rather ugly
anyway.
but as suggested elsewhere, it probably makes sense to mention the
global keychain as a hint/recommendation.
> +++ b/src/mbsyncrc.sample
> @@ -21,7 +21,7 @@ Pass xxxxxxxx
> #PassCmd "echo -ne 'GET myIsp\\tpassword' | pwmc datafile"
> -# On Mac OS X, run "KeyChain Access" -- File->New Password Item. Fill out
> form using
> +# On macOS, run "KeyChain Access" -- File->New Password Item. Fill out form
> using
>
you shouldn't fix the message's style in the same commit when you don't
actually change the line otherwise.
but i would expect you to actually mention that this method is
discouraged i favor of the new options on macos.
happy zombie jesus holiday!
_______________________________________________
isync-devel mailing list
isync-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/isync-devel
