Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add LDAP support to s_client -starttls #1735

Closed
wants to merge 2 commits into from

Conversation

ChadSikorra
Copy link

This was submitted to the mailing list at one time from Alex Bergmann (Can't seem to find his GitHub account..). See this:

https://groups.google.com/forum/#!topic/mailing.openssl.users/1OOwXp45iIw

Seems like a fairly common thing to want to do according to some Stackoverflow posts:

http://stackoverflow.com/questions/7084482/how-to-save-the-ldap-ssl-certificate-from-openssl

I tested against AD and it worked for me.

@@ -740,7 +740,8 @@ typedef enum PROTOCOL_choice {
PROTO_XMPP_SERVER,
PROTO_CONNECT,
PROTO_IRC,
PROTO_POSTGRES
PROTO_POSTGRES,
PROTO_LDAP,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove that comma, please

@levitte levitte added the branch: master Merge to master branch label Oct 18, 2016
@levitte levitte self-assigned this Oct 18, 2016
@@ -2105,6 +2107,32 @@ int s_client_main(int argc, char **argv)
goto shut;
}
break;
case PROTO_LDAP:
{
char *ldap_tls_genconf = "asn1=SEQUENCE:LDAPMessage\n"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be "static char" array

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

const too?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a "char asdf[]" array is const.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I missed the "array" part, sorry.

"messageID=INTEGER:1\n"
"extendedReq=EXPLICIT:23A,IMPLICIT:0C,FORMAT:ASCII,OCT:1.3.6.1.4.1.1466.20037\n";
long errline;
char *genstr;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if genstr and atyp are moved later, then they can be init'd as part of their declaration.

long errline;
char *genstr;
ASN1_TYPE *atyp = NULL;
CONF *cnf = NCONF_new(NULL);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

on the other hand, what if cnf is null?

@@ -420,7 +420,7 @@ command for more information.
send the protocol-specific message(s) to switch to TLS for communication.
B<protocol> is a keyword for the intended protocol. Currently, the only
supported keywords are "smtp", "pop3", "imap", "ftp", "xmpp", "xmpp-server",
"irc" and "postgres."
"irc", "postgres", and "ldap".
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

punctuation should come inside the quotes. since you're hear, please fix it in 422 and 423.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All docs I've seen have the punctuation outside the quotes. It would be very confusing to see it inside the quotes. It will be like the value to be put on command line includes punctuation.
I'm not a native speaker and I would not argue which way is correct according to english grammar. It will surely be confusing for users though in a technical documentation.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @akostadinov ... having the period inside the quotes may make it confusing to know if it's part of the keyword or not. Having it outside the quotes makes that clear.

@richsalz
Copy link
Contributor

Ping @ChadSikorra any plans to update this?

@akostadinov
Copy link

akostadinov commented Mar 2, 2017

can be closed as #2293 has been merged

@levitte levitte closed this Mar 2, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
branch: master Merge to master branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants