On Wed, Sep 9, 2015 at 9:43 PM, Lawrence Teo <l...@openbsd.org> wrote:
> On Wed, Sep 09, 2015 at 11:17:55AM -0500, Brent Cook wrote:
>> On Wed, Sep 9, 2015 at 10:15 AM, Todd C. Miller
>> <todd.mil...@courtesan.com> wrote:
>> > On Wed, 09 Sep 2015 10:02:17 -0400, Lawrence Teo wrote:
>> >> In s_time.c, NO_SHUTDOWN is always defined, so there is no need for a
>> >> bunch of NO_SHUTDOWN #ifdef blocks.
>> >
>> > I'm less sure about this as without calling SSL_shutdown() the
>> > client is not notified. I suppose that's intentional as s_time is
>> > just for timing connections?
>> >
>> > - todd
>> >
>>
>> OK, who has a camera looking over my shoulder? I was just looking at this :)
>
> Oops! Please pay no attention to the drone buzzing behind you. :)
>
>> TBH, I'd rather this were a flag rather than a define. Yes, a knob,
>> but this is a benchmark that really should be able to benchmark a full
>> TLS connection and shutdown to be accurate. The default behavior of
>> faking out the shutdown state machine does make the this run about 25%
>> faster, but I would have never known that if not for playing with the
>> define.
>
> Thank you all for the feedback. I agree that a flag would be preferred
> over recompiling openssl(1).
>
> Here's a diff that adds a flag called -no_shutdown (the underscore is
> there to match the -no_* flags used by other openssl(1) commands).
>
> The diff also changes the behavior of s_time so that it will perform
> a proper full shutdown by default (i.e. if -no_shutdown is not
> specified).
>
> Thoughts?
Nice, I like this better, and the behavior change makes it a little more honest
(but still allows apples-to-apples comparison with OpenSSL's version).
ok bcook@
>
> Index: openssl.1
> ===================================================================
> RCS file: /cvs/src/usr.bin/openssl/openssl.1,v
> retrieving revision 1.19
> diff -u -p -u -p -r1.19 openssl.1
> --- openssl.1 11 Aug 2015 05:01:03 -0000 1.19
> +++ openssl.1 10 Sep 2015 01:51:18 -0000
> @@ -7074,6 +7074,7 @@ unknown cipher suites a client says it s
> .Op Fl key Ar keyfile
> .Op Fl nbio
> .Op Fl new
> +.Op Fl no_shutdown
> .Op Fl reuse
> .Op Fl time Ar seconds
> .Op Fl verify Ar depth
> @@ -7135,6 +7136,10 @@ nor
> .Fl reuse
> are specified,
> they are both on by default and executed in sequence.
> +.It Fl no_shutdown
> +Shutdown the connection without sending a
> +.Dq close notify
> +shutdown alert to the server.
> .It Fl reuse
> Performs the timing test using the same session ID;
> this can be used as a test that session caching is working.
> Index: s_time.c
> ===================================================================
> RCS file: /cvs/src/usr.bin/openssl/s_time.c,v
> retrieving revision 1.9
> diff -u -p -u -p -r1.9 s_time.c
> --- s_time.c 22 Aug 2015 16:36:05 -0000 1.9
> +++ s_time.c 10 Sep 2015 01:58:17 -0000
> @@ -56,8 +56,6 @@
> * [including the GNU Public Licence.]
> */
>
> -#define NO_SHUTDOWN
> -
> /*-----------------------------------------
> s_time - SSL client connection timer program
> Written and donated by Larry Streepy <stre...@healthcare.com>
> @@ -114,6 +112,7 @@ struct {
> char *keyfile;
> int maxtime;
> int nbio;
> + int no_shutdown;
> int perform;
> int verify;
> int verify_depth;
> @@ -184,6 +183,12 @@ struct option s_time_options[] = {
> .value = 1,
> },
> {
> + .name = "no_shutdown",
> + .desc = "Shutdown the connection without notifying the
> server",
> + .type = OPTION_FLAG,
> + .opt.flag = &s_time_config.no_shutdown,
> + },
> + {
> .name = "reuse",
> .desc = "Reuse the same session ID for each connection",
> .type = OPTION_VALUE,
> @@ -221,7 +226,7 @@ s_time_usage(void)
> "usage: s_time "
> "[-bugs] [-CAfile file] [-CApath directory] [-cert file]\n"
> " [-cipher cipherlist] [-connect host:port] [-key keyfile]\n"
> - " [-nbio] [-new] [-reuse] [-time seconds]\n"
> + " [-nbio] [-new] [-no_shutdown] [-reuse] [-time seconds]\n"
> " [-verify depth] [-www page]\n\n");
> options_usage(s_time_options);
> }
> @@ -341,11 +346,11 @@ s_time_main(int argc, char **argv)
> while ((i = SSL_read(scon, buf, sizeof(buf))) > 0)
> bytes_read += i;
> }
> -#ifdef NO_SHUTDOWN
> - SSL_set_shutdown(scon, SSL_SENT_SHUTDOWN |
> SSL_RECEIVED_SHUTDOWN);
> -#else
> - SSL_shutdown(scon);
> -#endif
> + if (s_time_config.no_shutdown)
> + SSL_set_shutdown(scon, SSL_SENT_SHUTDOWN |
> + SSL_RECEIVED_SHUTDOWN);
> + else
> + SSL_shutdown(scon);
> shutdown(SSL_get_fd(scon), SHUT_RDWR);
> close(SSL_get_fd(scon));
>
> @@ -400,11 +405,11 @@ next:
> SSL_write(scon, buf, strlen(buf));
> while (SSL_read(scon, buf, sizeof(buf)) > 0);
> }
> -#ifdef NO_SHUTDOWN
> - SSL_set_shutdown(scon, SSL_SENT_SHUTDOWN | SSL_RECEIVED_SHUTDOWN);
> -#else
> - SSL_shutdown(scon);
> -#endif
> + if (s_time_config.no_shutdown)
> + SSL_set_shutdown(scon, SSL_SENT_SHUTDOWN |
> + SSL_RECEIVED_SHUTDOWN);
> + else
> + SSL_shutdown(scon);
> shutdown(SSL_get_fd(scon), SHUT_RDWR);
> close(SSL_get_fd(scon));
>
> @@ -434,11 +439,11 @@ next:
> while ((i = SSL_read(scon, buf, sizeof(buf))) > 0)
> bytes_read += i;
> }
> -#ifdef NO_SHUTDOWN
> - SSL_set_shutdown(scon, SSL_SENT_SHUTDOWN |
> SSL_RECEIVED_SHUTDOWN);
> -#else
> - SSL_shutdown(scon);
> -#endif
> + if (s_time_config.no_shutdown)
> + SSL_set_shutdown(scon, SSL_SENT_SHUTDOWN |
> + SSL_RECEIVED_SHUTDOWN);
> + else
> + SSL_shutdown(scon);
> shutdown(SSL_get_fd(scon), SHUT_RDWR);
> close(SSL_get_fd(scon));
>
>
