ipa-getkeytab enhancements #202

martbab · 2016-11-01T11:10:22Z

This PR implements '-H' and '-Y' options mentioned in
https://fedorahosted.org/freeipa/ticket/6409 along with the ability to specify
CA cert on the command line (which proved useful during the work on installer
refactoring).

Since my C skills are not at the level I would like them to be it would be nice
if you point out even the tiniest mistakes, risky code or non-idiomatic usage.

Also the test case test_retrieval_using_plain_ldap fails due to unsuccesful
simple bind. I wanted to implement StartTLS for simple binds over ldap://, but
I get the following errors in dirsrv error log:

    [01/Nov/2016:10:44:52.395126000 +0000] connection - conn=883 fd=135
    Incoming BER Element was 3 bytes, max allowable is 209715200 bytes.
    Change the nsslapd-maxbersize attribute in cn=config to increase.

I guess there is something fishy with the way I initialize the StartTLS
session. I would appreciate your help with it.

simo5 · 2016-11-01T14:09:23Z

client/ipa-getkeytab.c

@@ -712,6 +717,8 @@ int main(int argc, const char *argv[])
              _("LDAP DN"), _("DN to bind as if not using kerberos") },
 	    { "bindpw", 'w', POPT_ARG_STRING, &bindpw, 0,
              _("LDAP password"), _("password to use if not using kerberos") },
+	    { "cacert", 'c', POPT_ARG_STRING, &ca_cert_file, 0,
+            _("Path to the IPA CA certificate"), _("IPA CA certificate")},


indentation does not follow the style of the rest of the code here

simo5 · 2016-11-01T16:00:26Z

client/ipa-getkeytab.c

+    int port = 0;
+    char *scheme = NULL;
+
+    if (!mech) {


I do not understand why you are using the mech to determine the URI scheme, these are unrelated things.

That is probably some leftover from previous incarnations of the patch. If I manage to make StartTLS work this may actually be irrelevant since we may use ldap://$FQDN and have secure connection.

simo5 · 2016-11-01T16:08:20Z

client/ipa-getkeytab.c

+    else {
+        return 1;
+    }
+    url_len = snprintf(url,0,"%s://%s:%d",scheme,servername,port) +1;


please just use asprintf() in here.

simo5 · 2016-11-01T16:10:01Z

client/ipa-getkeytab.c

+        ret = ipa_server_to_uri(server, sasl_mech, &ldap_uri);
+        if (ret) {
+            fprintf(stderr, _("Incompatible combination of server name and "
+                              "SASL bind mechanism\n"));


This error message looks wrong.
ipa_server_to_uri() currently fails only if the mechanism is not recognized (but that one is checked earlier already), or if string allocation fails.

simo5 · 2016-11-01T16:10:28Z

client/ipa-getkeytab.c

@@ -837,7 +931,7 @@ int main(int argc, const char *argv[])
 		exit(4);
 	}

-	if (NULL == bindpw) {
+	if (NULL == bindpw && strncmp(sasl_mech, LDAP_SASL_GSSAPI, 79) == 0) {


simo5 · 2016-11-01T18:37:53Z

client/ipa-getkeytab.c

+    int rc = LDAP_SUCCESS;
+    char *startswith_ldap = NULL;
+
+    startswith_ldap = strstr("ldap://", ldap_uri);


ldap:// can be only at the start of the string, please use strncmp(ldap_uri, "ldap://", 7)

simo5

Close, but there are minor issues to fix, also I would not couple SAL mech with choice of URI scheme in all cases.

martbab · 2016-11-02T16:01:11Z

Thank you for review @simo5 . I have fixed the issues and reworked the LDAP initialization and binding logic a bit to clean it up. It produced green tests for me. I have also updated the command man page as I missed that during initial work.

martbab · 2016-11-04T08:23:09Z

Bump for review, this is prerequisite to further installer refactoring work.

simo5

Mostly just style nitpicks

simo5 · 2016-11-04T09:31:15Z

client/ipa-getkeytab.c

-	}
-	sprintf(url,"%s://%s:%d",scheme,servername,port);
-	int rc = ldap_initialize(ld, url);
+    url_len = asprintf(&url,"%s://%s:%d",scheme,servername,port);


space after comma

simo5 · 2016-11-04T09:35:05Z

client/ipa-getkeytab.c


-	free(url);
-	return rc;
+    if (url_len == -1){


space after close parens

simo5 · 2016-11-04T09:35:08Z

client/ipa-getkeytab.c

+    int tls_hard = LDAP_OPT_X_TLS_HARD;
+    int tls_demand = LDAP_OPT_X_TLS_DEMAND;
+    char *ldap = "ldap://";
+    char *ldaps = "ldaps://";


there is no need to use an actual variable, beside these would have to be defined as const char as the strings are constant.

Use defines:
#define SCHEMA_LDAP "ldap://"
#define SCHEMA_LDAPS "ldaps://"
Then for lenght you can use sizeof(SCHEMA_LDAP) -1

simo5 · 2016-11-04T09:36:00Z

client/ipa-getkeytab.c

            return ret;
        }
+


don't add empty line

simo5 · 2016-11-04T09:38:08Z

client/ipa-getkeytab.c

@@ -221,21 +274,26 @@ static int ipa_ldap_bind(const char *server_name, krb5_principal bind_princ,
                               &bv, NULL, NULL, NULL);
        if (ret != LDAP_SUCCESS) {
            fprintf(stderr, _("Simple bind failed\n"));
-            goto done;


This looks wrong, if bind fails we should get out

simo5 · 2016-11-04T09:42:53Z

client/ipa-getkeytab.c

+        fprintf(stderr, _("Cannot specify server and LDAP uri "
+                          "simultaneously.\n"));
+		if (!quiet)
+			poptPrintUsage(pc, stderr, 0);


incorrect indentation

simo5 · 2016-11-04T09:45:24Z

client/man/ipa-getkeytab.1

+The LDAP password to use when not binding with Kerberos. \fB\-D\fR and \fB\-w\fR can not be used together with \fB\-Y\fR.
+.TP
+\fB\-\-cacert\fR
+The path to IPA CA certificate used to validate LDAPS/STARTTLS connection.


-> path to 'the' IPA CA certificate
-> connections (plural)

simo5 · 2016-11-04T09:45:44Z

client/man/ipa-getkeytab.1

+Defaults to /etc/ipa/ca.crt
+.TP
+\fB\-H, \-\-ldapuri\fR
+LDAP URI. If ldap:// is specified, STARTTLS session is initiated by default. Can not be used with \fB\-s\fR.


remove 'session'

simo5 · 2016-11-04T09:47:25Z

client/man/ipa-getkeytab.1

+LDAP URI. If ldap:// is specified, STARTTLS session is initiated by default. Can not be used with \fB\-s\fR.
+.TP
+\fB\-Y, \-\-mech\fR
+SASL mechanism to use if fB\-D\fR and \fB\-w\fR are not used. Choose between GSSAPI and EXTERNAL.


-> s/Choose between GSSAPI and/Use either GSSAPI or/

simo5 · 2016-11-04T09:49:11Z

client/ipa-getkeytab.c

-        ssl = LDAP_OPT_X_TLS_HARD;;
-        ret = ldap_set_option(ld, LDAP_OPT_X_TLS, &ssl);
+    }
+    else if (strncmp(ldap_uri, ldaps, strlen(ldaps)) == 0) {


} else if,
don't go to a new line wth the else

martbab · 2016-11-08T07:58:57Z

Another bump for review.

simo5 · 2016-11-08T13:47:59Z

client/ipa-getkeytab.c

-	}
-	sprintf(url,"%s://%s:%d",scheme,servername,port);
-	int rc = ldap_initialize(ld, url);
+    url_len = asprintf(&url,"%s%s:%d", SCHEMA_LDAP, servername, port);


space after comma

simo5

fix the last nit and it's an ACK

martbab · 2016-11-08T14:39:16Z

Done.

get rid of hardcoded CA cert path and allow the caller to use supplied custom paths instead https://fedorahosted.org/freeipa/ticket/6409

ipa-getkeytab command was augmented in a way that allows more flexible selection of bind mechanisms: * -H <LDAP_URI> option was added to specify full LDAP uri. By default the URI will be constructed from retrieved server name as is done now. Specifying this options precludes use of -s. * -Y <EXTERNAL|GSSAPI> specifes SASL bind mechanism if no bind DN was given (which implies simple bind) This allows the command to be used also locally via LDAPI, eliminating the need to provide any credentials at all as root (e.g. in installers) https://fedorahosted.org/freeipa/ticket/6409

The test suite is now leveraging host/service tracker objects as test case fixture, removing much of ad-hoc setup/teardown. https://fedorahosted.org/freeipa/ticket/6409

All new retrieval methods are covered including testing for excluded option combinations. https://fedorahosted.org/freeipa/ticket/6409

martbab · 2016-11-08T16:03:03Z

Fixed upstream
master:
https://fedorahosted.org/freeipa/changeset/294fc3dc5645eeb7942908c3e351c06aa0af329e
https://fedorahosted.org/freeipa/changeset/0c68c27e51c2a30265a760382d7d4fab7d21937b
https://fedorahosted.org/freeipa/changeset/8480d0e3333f6813439e7b3321a0e33ce80d30f1
https://fedorahosted.org/freeipa/changeset/2725e440bf1e4930f9b1d19223424bcb0d4b7066

simo5 reviewed Nov 1, 2016

View reviewed changes

simo5 requested changes Nov 1, 2016

View reviewed changes

simo5 reviewed Nov 1, 2016

View reviewed changes

simo5 requested changes Nov 1, 2016

View reviewed changes

martbab force-pushed the getkeytab-enhancements branch from 10a844a to 3598470 Compare November 2, 2016 15:58

martbab force-pushed the getkeytab-enhancements branch from 3598470 to 35e2e2a Compare November 3, 2016 12:20

simo5 requested changes Nov 4, 2016

View reviewed changes

martbab force-pushed the getkeytab-enhancements branch from 35e2e2a to 17e5c73 Compare November 4, 2016 12:54

simo5 reviewed Nov 8, 2016

View reviewed changes

simo5 approved these changes Nov 8, 2016

View reviewed changes

martbab force-pushed the getkeytab-enhancements branch from 17e5c73 to 05f9275 Compare November 8, 2016 14:39

martbab added the ack Pull Request approved, can be merged label Nov 8, 2016

Martin Babinsky added 4 commits November 8, 2016 17:02

ipa-getkeytab: expose CA cert path as option

63a0c7c

get rid of hardcoded CA cert path and allow the caller to use supplied custom paths instead https://fedorahosted.org/freeipa/ticket/6409

Modernize ipa-getkeytab test suite

f8b6bb3

The test suite is now leveraging host/service tracker objects as test case fixture, removing much of ad-hoc setup/teardown. https://fedorahosted.org/freeipa/ticket/6409

Extend keytab retrieval test suite to cover new options

5dad135

All new retrieval methods are covered including testing for excluded option combinations. https://fedorahosted.org/freeipa/ticket/6409

martbab force-pushed the getkeytab-enhancements branch from 05f9275 to 5dad135 Compare November 8, 2016 16:02

martbab added the pushed Pull Request has already been pushed label Nov 8, 2016

martbab closed this Nov 8, 2016

martbab deleted the getkeytab-enhancements branch December 16, 2016 14:27

ipa-getkeytab enhancements #202

ipa-getkeytab enhancements #202

martbab commented Nov 1, 2016 •

edited

simo5 Nov 1, 2016

simo5 Nov 1, 2016

martbab Nov 2, 2016

simo5 Nov 1, 2016

simo5 Nov 1, 2016

simo5 Nov 1, 2016

simo5 Nov 1, 2016 •

edited

simo5 left a comment

martbab commented Nov 2, 2016

martbab commented Nov 4, 2016

simo5 left a comment

simo5 Nov 4, 2016

simo5 Nov 4, 2016

simo5 Nov 4, 2016

simo5 Nov 4, 2016

simo5 Nov 4, 2016

simo5 Nov 4, 2016

simo5 Nov 4, 2016

simo5 Nov 4, 2016

simo5 Nov 4, 2016

simo5 Nov 4, 2016

martbab commented Nov 8, 2016

simo5 Nov 8, 2016

simo5 left a comment

martbab commented Nov 8, 2016

martbab commented Nov 8, 2016

ipa-getkeytab enhancements #202

ipa-getkeytab enhancements #202

Conversation

martbab commented Nov 1, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simo5 Nov 1, 2016 • edited

Choose a reason for hiding this comment

simo5 left a comment

Choose a reason for hiding this comment

martbab commented Nov 2, 2016

martbab commented Nov 4, 2016

simo5 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martbab commented Nov 8, 2016

Choose a reason for hiding this comment

simo5 left a comment

Choose a reason for hiding this comment

martbab commented Nov 8, 2016

martbab commented Nov 8, 2016

martbab commented Nov 1, 2016 •

edited

simo5 Nov 1, 2016 •

edited