On Fri, Apr 09, 2010 at 06:36:10AM -0500, Mike Abbott wrote:
> Attached please find a patch that adds support to postfix-2.7.0 for RFC
> 4468 - Submission BURL.
> --- postfix-2.7.0/src/global/ehlo_mask.c 2008-01-08 14:36:13.000000000
> -0600
> --- postfix-2.7.0/src/global/ehlo_mask.h 2005-05-17 12:41:50.000000000
> -0500
OK
> --- postfix-2.7.0/src/global/mail_params.h 2010-01-17 14:54:35.000000000
> -0600
> +/* APPLE - burl */
> +#define VAR_IMAP_SUBMIT_CRED_FILE "imap_submit_cred_file"
> +#define DEF_IMAP_SUBMIT_CRED_FILE ""
> +#if defined(USE_SASL_AUTH) && defined(USE_TLS)
> +extern char *var_imap_submit_cred_file;
> +#endif
This should be a lookup table, not a plain file. Barring a compelling
reason to the contrary, fields should be white-space separated. It would
not be too tragic if white-space in passwords were not supported, but
if desired, that field can go last, and the parser can stop looking for
white-space once the password is reached.
The "extern" declaration should probably not be #ifdefed.
> --- postfix-2.7.0/src/smtpd/Makefile.in 2009-11-03 18:32:46.000000000
> -0600
> smtpd_peer.c smtpd_sasl_proto.c smtpd_sasl_glue.c smtpd_proxy.c \
> - smtpd_xforward.c smtpd_dsn_fix.c smtpd_milter.c smtpd_resolve.c
> + smtpd_xforward.c smtpd_dsn_fix.c smtpd_milter.c smtpd_resolve.c \
> + smtpd_imap.c imap-url.c
C-source modules that extend "smtpd" (and only "smtpd", otherwise they
go into "global/") should have names that start with "smtpd_". So this
one should be "smtpd_imap_url.c".
> + smtpd_resolve.h imap-url.h
Likewise "<smtpd_imap_url.h>"
> --- postfix-2.7.0/src/smtpd/imap-url.c 1969-12-31 18:00:00.000000000
> -0600
> +++ postfix-2.7.0+burl/src/smtpd/imap-url.c 2010-03-08 11:05:06.000000000
> -0600
> @@ -0,0 +1,615 @@
> +/*
> + * Copyright (c) 2010 Apple Inc. All rights reserved.
> + *
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions
> + * are met:
> + * 1. Redistributions of source code must retain the above copyright
> + * notice, this list of conditions and the following disclaimer.
> [...]
I am not a lawyer, but it it not obvious that the license terms are
compatible with the Postfix license. You can and should assert Apple
copyright, but I think you need to release the code under the Postfix
(not the proposed 3-clause BSD-like) license. Wietse?
> +#define LOWALPHA "abcdefghijklmnopqrstuvwxyz" /* RFC 1738 */
> +#define HIALPHA "ABCDEFGHIJKLMNOPQRSTUVWXYZ" /* RFC 1738 */
> +#define ALPHA LOWALPHA""HIALPHA /* RFC 1738 */
> +#define DIGIT "0123456789" /* RFC 1738 */
> +#define SAFE "$-_.+" /* RFC 1738 */
> +#define EXTRA "!*'()," /* RFC 1738 */
> +#define UNRESERVED ALPHA""DIGIT""SAFE""EXTRA /* RFC 1738 */
> +#define ESCAPE "%" /* RFC 1738 */
> +#define UCHAR UNRESERVED""ESCAPE /* RFC 1738 */
> +#define ACHAR UCHAR"&=~" /* RFC 2192 */
> +#define BCHAR ACHAR":@/" /* RFC 2192 */
> +#define HEXDIG DIGIT"ABCDEFabcdef" /* RFC 2234 */
> +#define HOST_CHARS ALPHA".:-"DIGIT /* RFC 1738 */
> +#define DATETIME_CHARS DIGIT".+-:TZ" /* RFC 3339 */
This looks ugly, Postfix already has a _UCHAR_ macro that is too similar
to your UCHAR, and runs with a clean environment that specifies LANG=C,
so using <ctype.h> is IMHO a lot cleaner than all these macros that
duplicate ctype.h functionality. One or special lists of metacharacters
would be fine, but this seems excessive.
> +// parse an RFC 2192+4467 URL into its parts
> +void imap_url_parse(const char *url, struct imap_url_parts *parts)
Postfix code must compile with ANSI-C compilers, not just "gcc". So C++
style comments are not acceptable.
This parser should probably be a state-machine that moves forward along
the input string, with state functions that parse an element, and return
the next state (function pointer). The syntax validation of each element
can then co-reside with the parser for that element.
> +bool imap_url_quoted_validate(const char *s)
> +{
> + const unsigned char *cp;
> +
> + if (*s != '"')
> + return FALSE;
> +
> + init_allowed();
> +
> + for (cp = (unsigned char *) s + 1; *cp; cp++) {
> + if (*cp == '\\') {
> + if (!quoted_specials_allowed[cp[1] & 0xff])
> + return FALSE;
> + ++cp;
For example, this only barely works, because "quoted_specials_allowed"
is zero for ASCII NUL ('\0'). Otherwise, a terminal "\" would run past
the end of the input string.
> +bool imap_url_hostport_validate(const char *s)
Postfix already has functions for parsing host names and host:port
syntax. You should try to re-use these.
Even if the overall parsing strategy is not changed, this code is fragile
and needs to be simplified. I am not going to dissect the parser further,
I think it needs a rewrite. If someone sees a way to improve it rather
than rewrite it, I am open to suggestions.
> --- postfix-2.7.0/src/smtpd/smtpd.c 2010-02-13 19:50:21.000000000 -0600
> + in_stream = state->client;
> +#if defined(USE_SASL_AUTH) && defined(USE_TLS)
> + if (burl) {
> + url = argv[1].strval;
> + len = strlen(url);
> + if (len >= 2 && url[0] == '"' && url[len - 1] == '"')
> + url = mystrndup(url + 1, len - 2);
> + in_stream = imap_open(state, url);
The IMAP input stream timeouts should be adjusted to match the SMTP server
timeout, (which may stress dependent, ...). Either pass var_smtpd_timeout
to imap_open, or introduce a new config variable for the imap timeout.
> + url = NULL;
> + }
> + if (in_stream == NULL) {
> + /* must fail the entire transaction */
> + chat_reset(state, var_smtpd_hist_thrsh);
> + mail_reset(state);
> + rcpt_reset(state);
> + return -1;
> + }
Why no response to the client? This puts the SMTP protocol out of
sync, since the client's BURL command gets no response...
Why are we resetting the chat history?
> + case SMTP_ERR_EOF:
> + smtpd_chat_reply(state, "554 4.6.6 EOF from IMAP server");
> + vstream_longjmp(state->client, SMTP_ERR_QUIET);
> + break;
Why is the DSN code 4.X.X when the SMTP reply code is 5XX? Is this a
permanent or a transient error code?
> + case SMTP_ERR_TIME:
> + smtpd_chat_reply(state, "554 4.6.6 Timeout from IMAP server");
> + vstream_longjmp(state->client, SMTP_ERR_QUIET);
> + break;
Ditto.
> +/* APPLE - burl */
> +#if defined(USE_SASL_AUTH) && defined(USE_TLS)
> + imap_read_config();
> +#endif
> }
With a cred table, this will not be necessary, but the table will need
to be opened in the pre-jail code.
> --- postfix-2.7.0/src/smtpd/smtpd_imap.c 1969-12-31 18:00:00.000000000
> -0600
> +static TLS_APPL_STATE *tls_ctx = NULL;
> ...
> +static TLS_SESS_STATE *imap_starttls(VSTREAM *stream,
> + const struct imap_server *is)
The TLS application context for accessing the IMAP servers should be
initialized pre-jail. It may need access to private keys or other
root-only material at some point in the future. At the time that a
particular IMAP server needs to be accessed, you should only be creating
TLS session contexts.
> + tls_ctx = TLS_CLIENT_INIT(&init_props,
> + log_level = 0,
> + verifydepth = DEF_SMTP_TLS_SCERT_VD,
> + /* XXX TLS_MGR_SCACHE_IMAP? */
The verification depth needs to be configurable.
smtpd_imap_tls_... = ...
> + cache_type = TLS_MGR_SCACHE_SMTPD,
The cache type is fine.
> + cert_file = "",
> + key_file = "",
> + dcert_file = "",
> + dkey_file = "",
> + eccert_file = "",
> + eckey_file = "",
> + CAfile = "",
> + CApath = "",
These need to be configurable, and in particular, administrators should
be able to enable certificate checks and non-empty CAfile/CApath. The
former may outside the chroot jail. Hence, pre-jail initialization.
> + fpt_dgst = DEF_SMTP_TLS_FPT_DGST);
This should be configurable or default to "sha1" not the legacy "md5",
since there are no backwards-compatibility issues here.
> + sess_ctx = TLS_CLIENT_START(&start_props,
> + ctx = tls_ctx,
> + stream = stream,
> + log_level = 0,
smtp_imap_tls_loglevel = ...
> + timeout = 30,
smtpd_imap_timeout = ${stress?10:300}
pass SMTP server or appropriate other timeout from caller...
> + tls_level = TLS_LEV_ENCRYPT,
Configurable: smtp_imap_tls_security_level = verify
> + nexthop = "",
> + host = is->hostport,
Use the imap server name in the URL as both hostname and nexthop,
remove the the port from the name, this is needed for certificate
validation.
> + namaddr = is->hostport,
This is fine.
> + serverid = is->hostport,
The serverid is used to identify a remote TLS session cache. With
SMTP servers behind load-balancers, we learn the server identty
from the remote EHLO response that precedes STARTTLS.
So when the IMAP server access method is IMAP + STARTTLS (rather
than "imaps", the server id should include the hostname from
the IMAP server banner:
* OK [CAPABILITY IMAP4 IMAP4rev1 LITERAL+ ID STARTTLS AUTH=LOGIN
AUTH=GSSAPI SASL-IR] imap.example.com Cyrus IMAP <version> server ready
It should include the hostport data, in addition to this, since the
name in the banner is not always reliable, but salting the serverid
with the remote banner name makes TLS session caching much more
effective.
> + protocols = DEF_SMTP_TLS_MAND_PROTO,
smtpd_imap_tls_protocols = $smtpd_tls_mandatory_protocols
> + cipher_grade = DEF_SMTP_TLS_MAND_CIPH,
smtpd_imap_tls_ciphers = $smtp_tls_mandatory_ciphers
> + cipher_exclusions = "SSLv2, aNULL, ADH, eNULL",
None of these are needed by default. To make this user configurable:
smtpd_imap_tls_exclude_ciphers =
> + matchargv = NULL,
The verification policy should be specified in the imap policy
table entry.
> + fpt_dgst = DEF_SMTP_TLS_FPT_DGST);
The digest algorithm should be "sha1" by default, but should
be configurable:
smtpd_imap_tls_fingerprint_digest = sha1
> +VSTREAM *imap_open(SMTPD_STATE *state, const char *url)
> +{
> + fd = inet_connect(hostport, BLOCKING, 30);
The timeout should not be hard-coded:
smtpd_imap_connect_timeout = 30
> + smtp_timeout_setup(stream, 30);
smtpd_imap_timeout = 300
> + sess_ctx = NULL;
This should be in SMTPD_STATE, rather than a static variable...
> + /* negotiate SSL now if applicable (IMAPS) */
> + if (port == 993) {
> + sess_ctx = imap_starttls(stream, is);
> + if (sess_ctx == NULL) {
> + vstream_fpurge(stream, VSTREAM_PURGE_BOTH);
> + vstream_longjmp(stream, -1);
> + }
> + }
Is it illegal to run "imaps" on ports other than "993", I would expect
imaps://host:port/...
to work, so the "imaps" code-path should be independent of the port?
> + /* read server greeting */
> + if (smtp_get(response, stream, 0) != '\n' || VSTRING_LEN(response) < 4 ||
> + strncasecmp(vstring_str(response), "* OK", 4) != 0) {
> + msg_warn("bad greeting from IMAP server %s: %s", is->hostport,
> + vstring_str(response));
> + vstream_longjmp(stream, -1);
> + }
Get the serverid salt here, if we are doing "STARTTLS". Handle certificate
validation policy, by examining the TLS session flag bits to see whether
appropriate security was established. Disconnect gracefully from the
IMAP server if security criteria are not met.
> + /* determine which authentication mechanism to use; prefer PLAIN */
> + plain = FALSE;
> + if (imap_capable_of(stream, is, request, response, "AUTH=PLAIN", FALSE))
> + plain = TRUE;
> + else if (!imap_capable_of(stream, is, request, response,
> + "AUTH=X-PLAIN-SUBMIT", FALSE)) {
> + msg_warn("IMAP server %s supports neither "
> + "AUTH=PLAIN nor AUTH=X-PLAIN-SUBMIT. can't log in.",
> + is->hostport);
> + vstream_longjmp(stream, -1);
> + }
Should perhaps support "external" if IMAP server offered it, and we have
client certs.
--
Viktor.
P.S. Morgan Stanley is looking for a New York City based, Senior Unix
system/email administrator to architect and sustain our perimeter email
environment. If you are interested, please drop me a note.
