On Mon, Sep 19, 2022 at 09:54:56PM +1000, Jonathan Matthew wrote:
> This adds client certificate authentication to ypldap(8). libtls makes the
> actual certificate part of this straightforward (I would still like it
> reviewed, though), but there are some LDAP complications.
>
> Depending on your LDAP server and how you connect to it (LDAPS on port 636
> or LDAP+TLS on port 389), a client presenting a certificate might
> automatically be bound as the subject of the certificate, or it might not.
> If it's not, the client can do an LDAP bind operation using the SASL
> EXTERNAL mechanism to bind as the cert subject, and it can optionally specify
> an identity, which means the bind will fail if the cert subject doesn't match
> that identity. If the client didn't present a certificate, the bind will
> also fail (one would hope).
>
> For reference, with Active Directory, SASL EXTERNAL bind is required when
> using LDAP+TLS, but not when using LDAPS, and the client identity can be
> specified in the form of "dn:" followed by the expected cert subject DN.
> OpenLDAP doesn't seem to do automatic bind at all, so SASL EXTERNAL would
> always be required there, and it doesn't appear to support specifying the
> expected identity with the bind.
>
> The diff adds 'certfile' and 'keyfile' config directives for specifying
> the certificate to use, and a 'bindext' directive for enabling SASL EXTERNAL
> bind, optionally including the identity string. SASL EXTERNAL bind doesn't
> get enabled implicitly when you configure a client cert, because ypldap
> can't tell if it's required or supported by the server. It's also not an
> error to enable SASL EXTERNAL bind without a client cert, since you could be
> connecting through stunnel or something.
>
> To configure this in ypldap.conf, you'd do something like this:
>
> directory "ldap.example.com" tls {
> bindext "dn:CN=ypldap,OU,Accounts,DC=example,DC=com"
> certfile "/etc/ssl/ypldap-cert.pem"
> keyfile "/etc/ssl/private/ypldap-key.pem"
>
> ...
> }
>
> ok?
The libtls parts of this are of course fine. I do not have a way to test
this, but your explanations and the code make sense.
ok tb
Two minor comments below.
>
> Index: aldap.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/ypldap/aldap.c,v
> retrieving revision 1.48
> diff -u -p -r1.48 aldap.c
> --- aldap.c 31 Mar 2022 09:06:55 -0000 1.48
> +++ aldap.c 19 Sep 2022 11:47:13 -0000
> @@ -220,6 +220,40 @@ fail:
> }
>
> int
> +aldap_bind_sasl_external(struct aldap *ldap, char *bindid)
> +{
> + struct ber_element *root = NULL, *elm;
> +
> + if ((root = ober_add_sequence(NULL)) == NULL)
> + goto fail;
> +
> + elm = ober_printf_elements(root, "d{tds{ts", ++ldap->msgid,
> + BER_CLASS_APP, LDAP_REQ_BIND, VERSION, "",
> + BER_CLASS_CONTEXT, LDAP_AUTH_SASL, LDAP_SASL_MECH_EXTERNAL);
I think a NULL check for elm would be appropriate: while it's unlikely,
the subsequent ober_add_*() calls might hide an error in
ober_printf_elements().
> + if (bindid == NULL)
> + elm = ober_add_null(elm);
> + else
> + elm = ober_add_string(elm, bindid);
> +
> + if (elm == NULL)
> + goto fail;
> +
> + LDAP_DEBUG("aldap_bind_sasl_external", root);
> +
> + if (aldap_send(ldap, root) == -1) {
> + root = NULL;
> + goto fail;
> + }
> + return (ldap->msgid);
> +fail:
> + if (root != NULL)
Since ber.c r1.32, this NULL check is no longer necessary. Perhaps all
such checks could be garbage collected in another diff.
> + ober_free_elements(root);
> +
> + ldap->err = ALDAP_ERR_OPERATION_FAILED;
> + return (-1);
> +}
> +
> +int
> aldap_unbind(struct aldap *ldap)
> {
> struct ber_element *root = NULL, *elm;
> Index: aldap.h
> ===================================================================
> RCS file: /cvs/src/usr.sbin/ypldap/aldap.h,v
> retrieving revision 1.14
> diff -u -p -r1.14 aldap.h
> --- aldap.h 11 May 2019 17:46:02 -0000 1.14
> +++ aldap.h 19 Sep 2022 11:47:13 -0000
> @@ -32,6 +32,8 @@
> #define LDAP_PAGED_OID "1.2.840.113556.1.4.319"
> #define LDAP_STARTTLS_OID "1.3.6.1.4.1.1466.20037"
>
> +#define LDAP_SASL_MECH_EXTERNAL "EXTERNAL"
> +
> struct aldap {
> #define ALDAP_ERR_SUCCESS 0
> #define ALDAP_ERR_PARSER_ERROR 1
> @@ -137,6 +139,7 @@ enum deref_aliases {
>
> enum authentication_choice {
> LDAP_AUTH_SIMPLE = 0,
> + LDAP_AUTH_SASL = 3,
> };
>
> enum scope {
> @@ -222,6 +225,7 @@ void aldap_freemsg(struct
> aldap_messa
> int aldap_req_starttls(struct aldap *);
>
> int aldap_bind(struct aldap *, char *, char *);
> +int aldap_bind_sasl_external(struct aldap *, char *);
> int aldap_unbind(struct aldap *);
> int aldap_search(struct aldap *, char *, enum scope, char *, char **, int,
> int, int, struct aldap_page_control *);
> int aldap_get_errno(struct aldap *, const char **);
> Index: ldapclient.c
> ===================================================================
> RCS file: /cvs/src/usr.sbin/ypldap/ldapclient.c,v
> retrieving revision 1.45
> diff -u -p -r1.45 ldapclient.c
> --- ldapclient.c 22 Aug 2022 10:10:59 -0000 1.45
> +++ ldapclient.c 19 Sep 2022 11:47:13 -0000
> @@ -635,7 +635,11 @@ client_try_idm(struct env *env, struct i
> int rc;
>
> where = "binding";
> - if (aldap_bind(al, idm->idm_binddn, idm->idm_bindcred) == -1)
> + if (idm->idm_bindext != 0)
> + rc = aldap_bind_sasl_external(al, idm->idm_bindextid);
> + else
> + rc = aldap_bind(al, idm->idm_binddn, idm->idm_bindcred);
> + if (rc == -1)
> goto bad;
>
> where = "parsing";
> Index: parse.y
> ===================================================================
> RCS file: /cvs/src/usr.sbin/ypldap/parse.y,v
> retrieving revision 1.35
> diff -u -p -r1.35 parse.y
> --- parse.y 19 Aug 2022 03:50:32 -0000 1.35
> +++ parse.y 19 Sep 2022 11:47:13 -0000
> @@ -108,7 +108,7 @@ typedef struct {
> %token USER GROUP TO EXPIRE HOME SHELL GECOS UID GID INTERVAL
> %token PASSWD NAME FIXED LIST GROUPNAME GROUPPASSWD GROUPGID MAP
> %token INCLUDE DIRECTORY CLASS PORT ERROR GROUPMEMBERS LDAPS TLS CAFILE
> -%token BIND LOCAL PORTMAP
> +%token BIND LOCAL PORTMAP BINDEXT CERTFILE KEYFILE
> %token <v.string> STRING
> %token <v.number> NUMBER
> %type <v.number> opcode attribute
> @@ -213,6 +213,11 @@ attribute : NAME
> { $$ = 0; }
> ;
>
> diropt : BINDDN STRING {
> + if (idm->idm_bindext != 0) {
> + yyerror("can't specify multiple bind types");
> + free($2);
> + YYERROR;
> + }
> idm->idm_flags |= F_NEEDAUTH;
> if (strlcpy(idm->idm_binddn, $2,
> sizeof(idm->idm_binddn)) >=
> @@ -224,6 +229,11 @@ diropt : BINDDN STRING
> {
> free($2);
> }
> | BINDCRED STRING {
> + if (idm->idm_bindext != 0) {
> + yyerror("can't specify multiple bind types");
> + free($2);
> + YYERROR;
> + }
> idm->idm_flags |= F_NEEDAUTH;
> if (strlcpy(idm->idm_bindcred, $2,
> sizeof(idm->idm_bindcred)) >=
> @@ -234,6 +244,64 @@ diropt : BINDDN STRING
> {
> }
> free($2);
> }
> + | BINDEXT STRING {
> + if (idm->idm_flags & F_NEEDAUTH) {
> + yyerror("can't specify multiple bind types");
> + free($2);
> + YYERROR;
> + }
> + idm->idm_flags |= F_NEEDAUTH;
> + idm->idm_bindext = 1;
> + if (strlcpy(idm->idm_bindextid, $2,
> + sizeof(idm->idm_bindextid)) >=
> + sizeof(idm->idm_bindextid)) {
> + yyerror("directory bindext truncated");
> + free($2);
> + YYERROR;
> + }
> + free($2);
> + }
> + | BINDEXT {
> + if (idm->idm_flags & F_NEEDAUTH) {
> + yyerror("can't specify multiple bind types");
> + YYERROR;
> + }
> + idm->idm_flags |= F_NEEDAUTH;
> + idm->idm_bindext = 1;
> + idm->idm_bindextid[0] = '\0';
> + }
> + | CERTFILE STRING {
> + if (idm->idm_tls_config == NULL) {
> + yyerror("can't set cert file without tls"
> + " enabled");
> + free($2);
> + YYERROR;
> + }
> + if (tls_config_set_cert_file(idm->idm_tls_config, $2)
> + == -1) {
> + yyerror("tls set cert file failed: %s",
> + tls_config_error(
> + idm->idm_tls_config));
> + free($2);
> + YYERROR;
> + }
> + }
> + | KEYFILE STRING {
> + if (idm->idm_tls_config == NULL) {
> + yyerror("can't set key file without tls"
> + " enabled");
> + free($2);
> + YYERROR;
> + }
> + if (tls_config_set_key_file(idm->idm_tls_config, $2)
> + == -1) {
> + yyerror("tls set key file failed: %s",
> + tls_config_error(
> + idm->idm_tls_config));
> + free($2);
> + YYERROR;
> + }
> + }
> | BASEDN STRING {
> if (strlcpy(idm->idm_basedn, $2,
> sizeof(idm->idm_basedn)) >=
> @@ -460,7 +528,9 @@ lookup(char *s)
> { "bind", BIND },
> { "bindcred", BINDCRED },
> { "binddn", BINDDN },
> + { "bindext", BINDEXT },
> { "cafile", CAFILE },
> + { "certfile", CERTFILE },
> { "change", CHANGE },
> { "class", CLASS },
> { "directory", DIRECTORY },
> @@ -479,6 +549,7 @@ lookup(char *s)
> { "home", HOME },
> { "include", INCLUDE },
> { "interval", INTERVAL },
> + { "keyfile", KEYFILE },
> { "ldaps", LDAPS },
> { "list", LIST },
> { "local", LOCAL },
> Index: ypldap.conf.5
> ===================================================================
> RCS file: /cvs/src/usr.sbin/ypldap/ypldap.conf.5,v
> retrieving revision 1.27
> diff -u -p -r1.27 ypldap.conf.5
> --- ypldap.conf.5 22 Aug 2022 07:07:45 -0000 1.27
> +++ ypldap.conf.5 19 Sep 2022 11:47:13 -0000
> @@ -144,6 +144,9 @@ or
> attribute to the LDAP attribute name supplied.
> .It Ic basedn Ar string
> Use the supplied search base as starting point for the directory search.
> +.It Ic certfile Ar string
> +Use the specified client certificate when connecting to the directory.
> +The file must contain a PEM encoded certificate.
> .It Ic groupdn Ar string
> Use the supplied search base as starting point for the directory search for
> groups.
> @@ -152,12 +155,23 @@ If not supplied, the basedn value will b
> Use the supplied credentials for simple authentication against the directory.
> .It Ic binddn Ar string
> Use the supplied Distinguished Name to bind to the directory.
> +.It Ic bindext Oo Ar string Oc
> +Bind to the directory using SASL EXTERNAL, optionally using a supplied
> identity
> +string.
> +When using a TLS client certificate, this allows the client to bind as the
> +subject of the certificate.
> +If an identity string is supplied, usually in the form of a distinguished
> name
> +prefixed with "dn:", the directory will only allow the bind to succeed if it
> +matches the subject of the certificate.
> .It Ic fixed attribute Ar attribute string
> Do not retrieve the specified attribute from LDAP but
> instead set it unconditionally to the supplied value for
> every entry.
> .It Ic group filter Ar string
> Use the supplied LDAP filter to retrieve group entries.
> +.It Ic keyfile Ar string
> +Use the specified private key when connecting to the directory.
> +The file must contain a PEM encoded key.
> .It Xo
> .Ic list Ar name Ic maps to Ar string
> .Xc
> Index: ypldap.h
> ===================================================================
> RCS file: /cvs/src/usr.sbin/ypldap/ypldap.h,v
> retrieving revision 1.22
> diff -u -p -r1.22 ypldap.h
> --- ypldap.h 19 Aug 2022 03:50:32 -0000 1.22
> +++ ypldap.h 19 Sep 2022 11:47:13 -0000
> @@ -101,7 +101,9 @@ struct idm {
> u_int32_t idm_list;
> struct ypldap_addr_list idm_addr;
> in_port_t idm_port;
> + int idm_bindext;
> char idm_binddn[LINE_WIDTH];
> + char idm_bindextid[LINE_WIDTH];
> char idm_bindcred[LINE_WIDTH];
> char idm_basedn[LINE_WIDTH];
> char idm_groupdn[LINE_WIDTH];
>
