Add Sieve support (RFC 5804) to s_client ("-starttls sieve") #2300
Conversation
apps/s_client.c
Outdated
| { | ||
| int foundit = 0; | ||
| BIO *fbio = BIO_new(BIO_f_buffer()); | ||
| BIO_push(fbio, sbio); |
There was a problem hiding this comment.
blank line after declarations.
apps/s_client.c
Outdated
| if (strstr(mbuf, "\"STARTTLS\"") == mbuf) | ||
| foundit = 1; | ||
| } | ||
| } |
There was a problem hiding this comment.
while should go on the same line as its close-curly.
apps/s_client.c
Outdated
| BIO_free(fbio); | ||
| if (!foundit) | ||
| BIO_printf(bio_err, | ||
| "didn't find STARTTLS in server response," |
apps/s_client.c
Outdated
| * is case-insensitive, make it uppercase | ||
| */ | ||
| if (mbuf_len > 1 && mbuf[0] == '"') { | ||
| for (i = 0; mbuf[i] != '\0'; i++) |
There was a problem hiding this comment.
There's a similar loop in apps/ca.c Create a make_uppercase utility routine in apps/apps.[ch] and use that there and here.
|
@robert-scheck can you rebase now that #2310 went in? thanks! |
|
@richsalz indeed – done. |
apps/s_client.c
Outdated
| */ | ||
| if (mbuf_len > 1 && mbuf[0] == '"') { | ||
| make_uppercase(mbuf); | ||
| if (strstr(mbuf, "\"STARTTLS\"") == mbuf) |
There was a problem hiding this comment.
so this could be strncmp, or do you want it to appear anywhere?
There was a problem hiding this comment.
does it make sense to put that do/while loop into common code for nntp to also use?
There was a problem hiding this comment.
I wanted to check if the string starts with the keyword…shall I do this better using strncmp()? Not sure about the do/while loop, given it is slightly different per protocol (case sensitivity, different loop condition).
There was a problem hiding this comment.
okay, then use strncmp and don't worry about the common code aspect now.
apps/s_client.c
Outdated
| @@ -2190,6 +2193,25 @@ int s_client_main(int argc, char **argv) | |||
| if (strstr(mbuf, "STARTTLS")) | |||
| foundit = 1; | |||
| } while (mbuf_len > 1 && mbuf[0] != '.'); | |||
| case PROTO_SIEVE: | |||
There was a problem hiding this comment.
I don't follow, where is break;? Does it really have to fall through?
There was a problem hiding this comment.
It appears to be totally busted even if I check out. I.e. it's not this page rendering issue.
apps/s_client.c
Outdated
| "Didn't find STARTTLS in server response," | ||
| " trying anyway...\n"); | ||
| BIO_printf(sbio, "STARTTLS\r\n"); | ||
| BIO_read(sbio, sbuf, BUFSIZZ); |
There was a problem hiding this comment.
Question is (and one can probably ask it even in other cases) if it's worth if not paying attention to negative reply after STARTTLS, but at least print it as clue for user. I mean specification allows to reject STARTTLS request, and it's ignored here. And if request was denied and you insist on TLS handshake anyway, it will fail, but user won't be given any clue. In other words question is if it would be appropriate at least recognize negative reply and print it.
There was a problem hiding this comment.
Your raised questions are valid, I just tried to stick with what already exists for other protocols. However personally, I don't have an opinion regarding that either, because I'm more or less just interested in having STARTTLS handled, seeing certificates and handshake/connection details.
There was a problem hiding this comment.
I just tried to stick with what already exists for other protocols.
Well, multiple wrongs don't make it right. If anything one should overhaul rather than keeping on wrong path. Yes, you can say that it was inconsistent of me/us to approve previous request, but we're better off if it stops at some point, so why not now :-)
Basically there are two options, past reply to STARTTLS is parsed that is. a) write something on screen if reply was negative and ignore it; b) respect negative reply and exit. I would vote for a), because when we call something a protocol, we king of make am implicit commitment to follow it. One can argue that not finding STARTTLS in CAPABILITIES can be ignored, because server would still have an option to deny the explicit request, but initiating TLS handshake when server explicitly refused to do so is hardly appropriate.
I'd appreciate if somebody else from the team chimes in with opinion. If we decide in favour of the respecting negative reply in response to STARTTLS, I'd appreciate if you could file a fix for recently committed NNTP. If you decline, it's fine too, it would be something for us to do, along with other flavours.
apps/s_client.c
Outdated
| BIO_printf(sbio, "STARTTLS\r\n"); | ||
| mbuf_len = BIO_read(sbio, mbuf, BUFSIZZ); | ||
| mbuf[mbuf_len] = 0; | ||
| if (mbuf_len < 0) { |
There was a problem hiding this comment.
Do you see the problem? I.e. possibility of mbuf[-1] = 0;?
There was a problem hiding this comment.
Hmm... I'd also expect some compiler to complain about = 0, but they seem to be ok. I mean I'd expect them to insist on = '\0'...
There was a problem hiding this comment.
'\0' gets promoted to (int)0 before assignment anyway, so it would not necessarily fall out of generic type checking code.
|
I suppose it's lesser point waiting out travis... +1 with following reservation: everything has to be squashed. |
Reviewed-by: Andy Polyakov <appro@openssl.org> Reviewed-by: Rich Salz <rsalz@openssl.org> (Merged from #2300)
|
merged. next up, ldap? :) |
Checklist
Description of change
Add Sieve support (RFC 5804) to s_client ("-starttls sieve").