Add an early callback by kaduk · Pull Request #2279 · openssl/openssl

kaduk · 2017-01-24T00:38:30Z

Checklist

documentation is added or updated
tests are added or updated
CLA is signed

Description of change

Implement an early callback similar to BoringSSL's select_certificate_cb, allowing for SNI processing and server certificate selection prior to session resumption and version negotiation.

The chosen API (with opaque structure and accessors, rather than public structure) differs from BoringSSL's, in order to remain consistent with most other OpenSSL APIs, and retain the trend of opaque data structures that facilitate ABI stability. This also should make it easier to backport the API to 1.1.0 if desired.

The WIP label will remain until tests are added, but other interesting questions include:

should the raw clienthello bytes be exposed at all?
Is now the time to implement a TLS extension type to index lookup, to avoid having to loop over the preprocessed extensions?
What level of support should SSLv2 clienthellos get from this API? (I am not much of an expert on the SSLv2 wire protocol, so the current support is likely lacking.)
Should we expose the alert to send to the callback, or even allow the callback to terminate the connection at all?

cc @mattcaswell

kaduk · 2017-01-24T02:18:43Z

I guess also:

Should the callback be able to mark an extension as "processed"

I also got one request out of band to more clearly document the callback's responsibility to handle SSLv2 ClientHellos more clearly.

davidben · 2017-01-24T03:40:13Z

I notice this callback is blocking only and does not have a way to pause the state machine like cert_cb does. You could imagine situations where looking up the certificate or other configuration needs to be asynchronous.

For instance, Node.js's SNICallback here is asynchronous. Looks like they currently query SSL_get_servername from cert_cb (which does support this), but this hits the session resumption ordering issues alluded to in your man page. @indutny may be able to speak to Node's needs more.

Is now the time to implement a TLS extension type to index lookup, to avoid having to loop over the preprocessed extensions?

I too twitch when I see such O(N) calls. :-) But I don't think it actually matters for this. There's no reason to ever write code like:

for (uint16_t ext = 0; ext < 0xffff; ext++) {
  SSL_EARLY_get0_ext(ssl, ectx, ext, ...);
  ...
}

The callback would ultimately only query the three or five specific extension types it cares about, so it wouldn't go quadratic.

(Ours just naively walks the serialized extensions block to find the extension.)

What level of support should SSLv2 clienthellos get from this API? (I am not much of an expert on the SSLv2 wire protocol, so the current support is likely lacking.)

One detail to note is the cipher list format is different (2-byte vs 3-byte cipher values), so you'll need to decide whether you convert internally or force the caller to handle both cases. (Our callback acts after V2ClientHellos are converted to SSLv3/TLS ClientHellos, so the callback never sees V2ClientHellos. Though I see OpenSSL 1.1.x uses a different strategy for V2ClientHello now.)

The random is also different, but less likely to trip callers up (or even be queried): in SSLv3/TLS it has fixed size, while in a V2ClientHello, the size is variable.

Should the callback be able to mark an extension as "processed"

Why? What would that do?

kaduk · 2017-01-24T06:25:20Z

Should the callback be able to mark an extension as "processed"

Why? What would that do?

The idea was to prevent the builtin processing of that extension from taking place, though I realize now that I may be misremembering what that field in the structure is used for.

indutny · 2017-01-24T06:29:28Z

I'd like to sincerely ask you to consider doing asynchronous version of this. There is just no functionality that can substitute this, and both Node.js and bud need this to some extent.

davidben · 2017-01-24T06:33:43Z

The idea was to prevent the builtin processing of that extension from taking place, though I realize now that I may be misremembering what that field in the structure is used for.

That sounds undesirable. Extensions aren't independent sub-protocols. They're how we add new ClientHello fields. Two different extensions may or may not interact. (Of course, we usually make them not interact because it's simpler that way, but not always. The crypto negotiation extensions used in TLS 1.3 are fairly coupled together.)

If you have two extensions which do interact, having a low-level "prevent built-in processing" knob means you expose implementation details. Perhaps you happen to implement it such that feature X is keyed on extension A rather than B.

It's also not clear to me why you'd want to do that anyway. I can imagine "please disable high-level feature X", but that should be done by having an API to disable said feature, not by shutting off low-level extensions via this callback.

kaduk · 2017-01-24T15:35:16Z

prevent the builtin processing of that extension

That sounds undesirable.

Yeah, I was pretty sure I didn't want it, but figured I could mention it explicitly so we consciously decide to not do it.

richsalz · 2017-01-24T15:37:07Z

We don't want to prevent the builtin processing.
If someone needs to do that, hack the source.

kaduk · 2017-01-24T16:26:54Z

One detail to note is the cipher list format is different (2-byte vs 3-byte cipher values)

#2280 seems relevant

tmshort · 2017-01-31T19:35:39Z

Does anything even send SSLv2-compatible ClientHellos any more?

tmshort · 2017-01-31T19:50:14Z

All session handling is done via the initial_ctx/session_ctx (ugh, two names for the same thing), so, if you want to change how resumed sessions are handled, an API to change the initial_ctx/session_ctx needs to be proposed. Right now, sessions from different virtual servers may share the same resumption database (and ticket crypto). This may be desirable if those virtual servers are on the same machine, but not so much if certificate authentication is being used.

tmshort · 2017-01-31T19:42:30Z

ssl/statem/extensions.c

If this is a number of extension, not sure it needs to be a size_t; the original int is likely sufficient, given the maximum size of a ClientHello.

This has theh type match with clienthello.pre_proc_exts_len, which is what we're going to assign it to, but I agree that int is going to be big enough to hold the values that would be seen.

I prefer not to overload the return type to mean different things, i.e. I'd prefer a simple boolean (int) return type to indicate success or failure, with a separate size_t * to set the length. Incidentally the length should definitely be a size_t and not an int in order to match the types. I have seen far too many subtle bugs introduced by lengths swapping backwards and forwards between int and size_t. They should be consistent throughout. I don't want to have to redo all the size_t work I did in libssl just recently.

Okay, I'll change to a separate output parameter.

tmshort · 2017-01-31T19:53:45Z

doc/man3/SSL_CTX_set_early_cb.pod

Typically, SSL_EARLY would be the first type in a function named as such, so perhaps not include the SSL structure, and "hide" it inside the SSL_EARLY? Maybe not? But the name might need to be changed to make it consistent with other APIs.

I thought about that, but the temptation to make SSL_EARLY just be reusing an existing type (CLIENTHELLO_MSG) was too strong, at least for the first crack. The callback is almost certainly going to need the SSL object itself, though, so it seems reasonable to have it be a top-level API parameter. I guess we could go the other way and attach the CLIENTHELLO_MSG to the SSL before invoking the callback, though.

If the function name is SSL_EARLY_* then the SSL_EARLY argument should be the first IMO

That's a reasonable argument. But maybe we should confirm that we're okay reusing the clienthello_msg structure as-is for the SSL_EARLY type first, since the other option is probably to define an entirely new type that contains a pointer to clienthello_msg and pointer to SSL, which would clean up these function signatures.

davidben · 2017-01-31T20:03:32Z

Does anything even send SSLv2-compatible ClientHellos any more?

We recently added code to get some data on our end. I would love to lose the V2ClientHello code, but I'm not optimistic.

indutny · 2017-01-31T20:27:07Z

Just to re-state my inquiry, it would be great if this could work asynchronously.

kaduk · 2017-01-31T20:31:19Z

Just to re-state my inquiry, it would be great if this could work asynchronously.

Acknowledged. Last week was a bit busy around here...
(Also it looks like it will need a little bit of re-plumbing compared to cert_cb.)

kaduk · 2017-01-31T20:40:15Z

if you want to change how resumed sessions are handled, an API to change the initial_ctx/session_ctx needs to be proposed

I am a little reluctant to add that into this changeset (which is going to be getting bigger once I add the ability for the callback to be async and tests. It seems like it could potentially be done as a follow-up change, if there is even consensus from the team that it should be done.

tmshort · 2017-01-31T20:41:59Z

It's really more of a question, is modifying session resumption part of the intended functionality? If yes, then an API will need to be introduced.

kaduk

I'm working on plumbing support for early_cb into test/handshake_helper.c, and making the callback resumable/async.
I also have some code to make SSL_bytes_to_cipher_list() a public function (with tests), for use by early callbacks, since SSL_CIPHERs will be more useful to callback implementors than the raw bytes.

kaduk · 2017-01-31T20:43:50Z

doc/man3/SSL_CTX_set_early_cb.pod

I thought about that, but the temptation to make SSL_EARLY just be reusing an existing type (CLIENTHELLO_MSG) was too strong, at least for the first crack. The callback is almost certainly going to need the SSL object itself, though, so it seems reasonable to have it be a top-level API parameter. I guess we could go the other way and attach the CLIENTHELLO_MSG to the SSL before invoking the callback, though.

kaduk · 2017-01-31T20:45:02Z

ssl/statem/extensions.c

This has theh type match with clienthello.pre_proc_exts_len, which is what we're going to assign it to, but I agree that int is going to be big enough to hold the values that would be seen.

kaduk · 2017-02-03T05:23:04Z

No async yet (still considering options), but this adds tests and exposes a helper to convert raw cipherlist octets into SSL_CIPHERs, that would probably be useful enough to applications to be worth exposing.
Removing the WIP label to seek more review, though I expect there will be several things that need changing (not least, to be compatible with the various no-foo options; hopefully travis will help with that).

mattcaswell

I get test failures with enable-tls1_3 (probably the use of SSL_OP_NO_TLSv1_2 instead of SSL_set_max_proto_version() noted below)

mattcaswell · 2017-02-03T09:02:26Z

doc/man3/SSL_CTX_set_early_cb.pod

typo: whens

mattcaswell · 2017-02-03T09:03:20Z

doc/man3/SSL_CTX_set_early_cb.pod

If the function name is SSL_EARLY_* then the SSL_EARLY argument should be the first IMO

mattcaswell · 2017-02-03T09:08:55Z

doc/man3/SSL_get_ciphers.pod

Presumably SSL_bytes_to_cipher_list() returns NULL on failure (e.g. a bad length)

Added "NULL is returned on error" above, since it would be weird to have something other than "See DESCRIPTION" here for just one function.

Actually, ended up changing the return convention to use an output pointer, since we want to return SCSVs separately so that the state machine can act on them.

mattcaswell · 2017-02-03T09:11:31Z

doc/man3/SSL_get_ciphers.pod

Yuck. That sounds like a really bad idea. Surely we can fix this?

We should be able to, yes, it just requires a bit of refactoring that I didn't want to do right while in the midst of things.

mattcaswell · 2017-02-03T09:13:00Z

include/openssl/ssl.h

Hmmm.....we should perhaps change the name of clienthello_msg_st. That's going to get really confusing.

Any suggestions? Alternately we can embed clienthello_msg* and SSL* into a new ssl_early_st, as mentioned above. Might be worth the extra type...

Given that you are now storing the clienthello structure in the SSL object, do you still need the SSL_EARLY type at all? Just use the SSL object.

Good point; done.

mattcaswell · 2017-02-03T09:22:32Z

ssl/ssl_lib.c

Why is it necessary to distinguish whether it is v2 here or not? If it is v2 then we use the variable length challenge as the random. If it is longer than SSL3_RANDOM_SIZE then we just use the first SSL3_RANDOM_SIZE bytes. If it is less than SSL3_RANDOM_SIZE then we pad to the left with 0s until it is SSL3_RANDOM_SIZE in length. Either way the resulting length is SSL3_RANDOM_SIZE in all cases.

To be sure that someone who actually is familiar with the v2 protocol (i.e., not me) looks at this logic :)
I'm happy to take your word for it; it didn't really seem worth putting the time in to understand the v2-parsing code given that it's (hopefully) on its way out.

mattcaswell · 2017-02-03T09:22:59Z

ssl/ssl_lib.c

Should we call this legacy_version?

mattcaswell · 2017-02-03T09:32:03Z

ssl/statem/extensions.c

I prefer not to overload the return type to mean different things, i.e. I'd prefer a simple boolean (int) return type to indicate success or failure, with a separate size_t * to set the length. Incidentally the length should definitely be a size_t and not an int in order to match the types. I have seen far too many subtle bugs introduced by lengths swapping backwards and forwards between int and size_t. They should be consistent throughout. I don't want to have to redo all the size_t work I did in libssl just recently.

mattcaswell · 2017-02-03T09:35:02Z

ssl/ssl_lib.c

pre_proc_exts_len is the number of built in extensions plus the number of custom extensions that are defined. It is not the number of extensions that were sent. If we don't recognise them then they don't end up in pre_proc_exts. So this is bounded by the server config not by how many extensions we get sent. That said and index -> type lookup might not be a bad idea....or maybe expose the index????

I'll remove the XXX comment, given this and davidben's comment.

That said and index -> type lookup might not be a bad idea....or maybe expose the index????

Do you mean type->index lookup? I had thought about doing that at some point, but demurred given that (1) there wasn't one already, possibly intentionally, and (2) I didn't want to think about how it would interact with custom extensions right then.

mattcaswell · 2017-02-03T09:45:18Z

test/handshake_helper.c

Use SSL_set_max_proto_version()

Good catch, thanks.

kaduk · 2017-02-06T20:47:24Z

The latest push is just the minor fixes and making SSL_bytes_to_cipher_list() "pure" -- async is still WIP.
But, it seems that the way to do that is to move the last ~half of tls_process_client_hello() (the early callback and later) into a separate function and do all that stuff from the "post_work" portion of the state machine.
(Matt notes that this will require adding a new WORK_MORE_X state.)

kaduk · 2017-02-07T17:43:19Z

And, landed the async functionality.
The sslv2-in-sslv2 test currently fails since we now send an alert and the test is expecting no alert. (This is a side effect of the refactoring, as there's not an easy no-alert error path from the subroutine moved from the "processing" to the "post-processing" stage. Should we just adopt the test to the new behavior?
I also took out the raw clienthello bytes as direct arguments to the early callback, as it got in the way of the refactoring needed for the async behavior, and I was not entirely keen on it in the first place. (This does not preclude us from later adding an accessor to get the raw bytes, of course, it just helps motivate people to use the accessors instead of rolling their own parser.)
@mattcaswell please take another look

kaduk · 2017-02-07T17:46:41Z

test/handshake_helper.c

I guess we could add some calls to the SSL_EARLY_* accessors in here to stop coveralls from complaining.
(And please confirm that I should make the change to putting the SSL_EARLY object first, or make a new type for it to avoid the need to pass the SSL as well, or ...)

As mentioned earlier I think you can get rid of SSL_EARLY altogether and just use SSL

kaduk · 2017-02-07T17:47:36Z

ssl/ssl_lib.c

This arguably ought to be part of the SCSV processing, but I didn't want to change the order of processing more than needed, at first.

Yes - it definitely should - it makes no sense here.

kaduk · 2017-02-07T17:48:25Z

doc/man3/SSL_CTX_set_early_cb.pod

Hmm, this needs to go if we decide to keep the commits that remove this functionality.

kaduk · 2017-02-07T17:49:47Z

ssl/statem/statem_srvr.c

Also need to revert this comment back, if we keep the commits that remove access to the raw ClientHello octets.

kaduk · 2017-02-07T17:50:49Z

ssl/statem/statem_srvr.c

This is related to the test failure I mentioned in a previous comment.

mattcaswell · 2017-02-08T12:36:26Z

doc/man3/SSL_CTX_set_early_cb.pod

Name needs updating to SSL_EARLY_get0_legacy_version()

mattcaswell · 2017-02-08T12:58:53Z

ssl/ssl_lib.c

Mem leak here if one is NULL but the other is not. Perhaps just goto err?

mattcaswell · 2017-02-08T13:23:55Z

ssl/statem/statem_srvr.c

Probably should just ditch the f_err label

done, even though it adds more churn.

mattcaswell · 2017-02-08T13:28:35Z

ssl/ssl_lib.c

Yes - it definitely should - it makes no sense here.

mattcaswell · 2017-02-08T13:30:48Z

test/cipherbytes_test.c

Perhaps this should be !SSL_bytes_to_cipher_list() || sk != NULL

SSL_bytes_to_cipher_list() is expected to fail when a zero-length byte array is provided.
So it's better as SSL_bytes_to_cipher_list() || sk != NULL, I think. (Later modified to also check the SCSVs.)

mattcaswell · 2017-02-08T13:36:12Z

test/cipherbytes_test.c

Add a test with SCSV ciphersuites?

I added the two known SCSVs to test_v3().

mattcaswell · 2017-02-08T13:37:23Z

test/recipes/80-test_cipherbytes.t

mattcaswell · 2017-02-08T13:40:01Z

include/openssl/ssl.h

Given that you are now storing the clienthello structure in the SSL object, do you still need the SSL_EARLY type at all? Just use the SSL object.

mattcaswell · 2017-02-08T13:41:04Z

test/handshake_helper.c

As mentioned earlier I think you can get rid of SSL_EARLY altogether and just use SSL

mattcaswell · 2017-02-08T14:05:27Z

test/ssl-tests/05-sni.conf.in

This test fails with the no-tls1_1 config option

Hmm, and we don't have skip functionality at this fine-grained of a level (right?). Should I move this to a new 05-early.conf.in so that 05-sni.conf.in can be run always?

Take a look at 20-cert-select.conf.in for an example of how to do this type of thing. Some of the other ".in" files do similar things.

Ah, thanks. I got there ... eventually ... I should probably call it quits for the night if I'm making this many brainos.

kaduk · 2017-02-10T00:11:55Z

Decided to not push -f for this round of changes; please let me know if/when I should rebase/squash.
(Not everything that could be squashed is explicitly noted as such in the commit message.)

kaduk · 2017-02-23T14:14:01Z

Hmm, just a doc-nits issue from travis, apparently.

richsalz · 2017-02-23T14:20:17Z

you must fix all doc nits you know :)

levitte

This is for some of the doc-nits issues.

levitte · 2017-02-23T14:22:46Z

doc/man3/SSL_CTX_set_early_cb.pod

+
+=head1 SYNOPSIS
+
+ typedef int (*SSL_early_cb_fn) (SSL *s, int *al, void *arg);


I think that space between ) and ( is the cause of one of the doc-nits issues...

levitte · 2017-02-23T14:23:13Z

doc/man3/SSL_CTX_set_early_cb.pod

+
+=head1 NAME
+
+SSL_CTX_set_early_cb, SSL_early_isv2, SSL_early_get0_legacy_version, SSL_early_get0_random, SSL_early_get0_session, SSL_early_get0_ciphers, SSL_early_get0_compression_methods, SSL_early_get0_ext - callback functions for early server-side ClientHello processing


SSL_early_get0_session should be SSL_early_get0_session_id...

[skip ci]

kaduk · 2017-02-23T16:15:38Z

Thanks for the pointer about the space in ') (', @levitte , that would have been rather nonobvious from the given error message.
I went with the [skip ci] route rather than push -f, since the CI seems to have quite a backlog.

kaduk · 2017-02-23T18:08:20Z

Who should merge it?

richsalz · 2017-02-23T18:09:49Z

Per standard practice, it will usually be one of the reviewers. It must be someone on the team of course :)

levitte · 2017-02-23T18:23:19Z

I can do it.

Modify the API of tls_collect_extensions() to be able to output the number of extensions that are known (i.e., the length of its 'res' output). This number can never be zero on a successful return due to the builtin extensions list, but use a separate output variable so as to not overload the return value semantics. Having this value easily available will give consumers a way to avoid repeating the calculation. Reviewed-by: Matt Caswell <[email protected]> Reviewed-by: Richard Levitte <[email protected]> (Merged from #2279)

Keep track of the length of the pre_proc_exts array. Reviewed-by: Matt Caswell <[email protected]> Reviewed-by: Richard Levitte <[email protected]> (Merged from #2279)

We'll be adding a field of this type to struct ssl_st in a subsequent commit, and need the type definition to be in scope already. Also move up the RAW_EXTENSION definition that it depends on. Reviewed-by: Matt Caswell <[email protected]> Reviewed-by: Richard Levitte <[email protected]> (Merged from #2279)

Just as we have a table of ssl3_ciphers, add a table of ssl3_scsvs, to contain SSL_CIPHER objects for these non-valid ciphers. This will allow for unified handling of such indicators, especially as we are preparing to pass them around between functions. Since the 'valid' field is not set for the SCSVs, they should not be used for anything requiring a cryptographic cipher (as opposed to something being stuck in a cipher-shaped hole in the TLS wire protocol). Reviewed-by: Matt Caswell <[email protected]> Reviewed-by: Richard Levitte <[email protected]> (Merged from #2279)

Now that we have made SCSVs into more of a first-class object, provide a way for the bytes-to-SSL_CIPHER conversion to actually return them. Add a flag 'all' to ssl_get_cipher_by_char to indicate that we want all the known ciphers, not just the ones valid for encryption. This will, in practice, let the caller retrieve the SCSVs. Reviewed-by: Matt Caswell <[email protected]> Reviewed-by: Richard Levitte <[email protected]> (Merged from #2279)

Move ssl_bytes_to_cipher_list() to ssl_lib.c and create a public wrapper around it. This lets application early callbacks easily get SSL_CIPHER objects from the raw ciphers bytes without having to reimplement the parsing code. In particular, they do not need to know the details of the sslv2 format ClientHello's ciphersuite specifications. Document the new public function, including the arguably buggy behavior of modifying the supplied SSL object. On the face of it, such a function should be able to be pure, just a direct translation of wire octets to internal data structures. Reviewed-by: Matt Caswell <[email protected]> Reviewed-by: Richard Levitte <[email protected]> (Merged from #2279)

Split off the portions that mutate the SSL object into a separate function that the state machine calls, so that the public API can be a pure function. (It still needs the SSL parameter in order to determine what SSL_METHOD's get_cipher_by_char() routine to use, though.) Instead of returning the stack of ciphers (functionality that was not used internally), require using the output parameter, and add a separate output parameter for the SCSVs contained in the supplied octets, if desired. This lets us move to the standard return value convention. Also make both output stacks optional parameters. Reviewed-by: Matt Caswell <[email protected]> Reviewed-by: Richard Levitte <[email protected]> (Merged from #2279)

Reviewed-by: Matt Caswell <[email protected]> Reviewed-by: Richard Levitte <[email protected]> (Merged from #2279)

Add the new enum value and case statements as appropriate. Reviewed-by: Matt Caswell <[email protected]> Reviewed-by: Richard Levitte <[email protected]> (Merged from #2279)

Provide a callback interface that gives the application the ability to adjust the nascent SSL object at the earliest stage of ClientHello processing, immediately after extensions have been collected but before they have been processed. This is akin to BoringSSL's "select_certificate_cb" (though it is not API compatible), and as the name indicates, one major use is to examine the supplied server name indication and select what certificate to present to the client. However, it can also be used to make more sweeping configuration changes to the SSL object according to the selected server identity and configuration. That may include adjusting the permitted TLS versions, swapping out the SSL_CTX object (as is traditionally done in a tlsext_servername_callback), changing the server's cipher list, and more. We also wish to allow an early callback to indicate that it needs to perform additional work asynchronously and resume processing later. To that effect, refactor the second half of tls_process_client_hello() into a subroutine to be called at the post-processing stage (including the early callback itself), to allow the callback to result in remaining in the same work stage for a later call to succeed. This requires allocating for and storing the CLIENTHELLO_MSG in the SSL object to be preserved across such calls, but the storage is reclaimed after ClientHello processing finishes. Information about the CliehtHello is available to the callback by means of accessor functions that can only be used from the early callback. This allows extensions to make use of the existing internal parsing machinery without exposing structure internals (e.g., of PACKET), so that applications do not have to write fragile parsing code. Applications are encouraged to utilize an early callback and not use a servername_callback, in order to avoid unexpected behavior that occurs due to the relative order of processing between things like session resumption and the historical servername callback. Also tidy up nearby style by removing unnecessary braces around one-line conditional bodies. Reviewed-by: Matt Caswell <[email protected]> Reviewed-by: Richard Levitte <[email protected]> (Merged from #2279)

Plumb things through in the same place as the SNI callback, since we recommend that the early callback replace (and supplement) the SNI callback, and add a few test cases. Reviewed-by: Matt Caswell <[email protected]> Reviewed-by: Richard Levitte <[email protected]> (Merged from #2279)

Reviewed-by: Matt Caswell <[email protected]> Reviewed-by: Richard Levitte <[email protected]> (Merged from #2279)

create_ssl_connection() prints out the results if SSL_accept() and/or SSL_connect() fail, but was reusing the client return value when printing about SSL_accept() failures. Reviewed-by: Matt Caswell <[email protected]> Reviewed-by: Richard Levitte <[email protected]> (Merged from #2279)

Certain callback APIs allow the callback to request async processing by trickling a particular error value up the stack to the application as an error return from the handshake function. In those cases, SSL_want() returns a code specific to the type of async processing needed. The create_ssl_connection() helper function for the tests is very helpful for several things, including creating API tests. However, it does not currently let us test the async processing functionality of these callback interfaces, because the special SSL error codes are treated as generic errors and the helper continues to loop until it reaches its maximum iteration count. Add a new parameter, 'want', that indicates an expected/desired special SSL error code, so that the helper will terminate when either side reports that error, giving control back to the calling function and allowing the test to proceed. Reviewed-by: Matt Caswell <[email protected]> Reviewed-by: Richard Levitte <[email protected]> (Merged from #2279)

Make sure that we can stop handshake processing and resume it later. Also check that the cipher list and compression methods are sane. Unfortunately, we don't have the client-side APIs needed to force a specific (known) session ID to be sent in the ClientHello, so that accessor cannot be tested here. Reviewed-by: Matt Caswell <[email protected]> Reviewed-by: Richard Levitte <[email protected]> (Merged from #2279)

levitte · 2017-02-23T19:22:46Z

Merged! fc5ece2 .. 2afaee5

levitte · 2017-02-23T19:23:49Z

(I did some editorial work... the last commits were obviously fixups, so I squashed them with the appropriate original commits)

tmshort reviewed Jan 31, 2017

View reviewed changes

kaduk commented Jan 31, 2017

View reviewed changes

kaduk force-pushed the earlycb branch from 8cdae8c to 670d07c Compare February 3, 2017 05:20

kaduk changed the title ~~WIP: early callback~~ Add an early callback Feb 3, 2017

kaduk force-pushed the earlycb branch from 670d07c to 5d8df66 Compare February 3, 2017 06:17

mattcaswell requested changes Feb 3, 2017

View reviewed changes

kaduk force-pushed the earlycb branch from 5d8df66 to c3ccada Compare February 6, 2017 20:42

kaduk force-pushed the earlycb branch 2 times, most recently from 150ca45 to 05ea38c Compare February 7, 2017 17:39

kaduk commented Feb 7, 2017

View reviewed changes

mattcaswell requested changes Feb 8, 2017

View reviewed changes

kaduk force-pushed the earlycb branch from 058cd48 to 7839ee4 Compare February 22, 2017 16:39

levitte requested changes Feb 23, 2017

View reviewed changes

Fix doc nits

9283c44

[skip ci]

levitte approved these changes Feb 23, 2017

View reviewed changes

levitte added the approval: done This pull request has the required number of approvals label Feb 23, 2017

levitte pushed a commit that referenced this pull request Feb 23, 2017

Tests for SSL_bytes_to_cipher_list()

6e3dac1

Reviewed-by: Matt Caswell <[email protected]> Reviewed-by: Richard Levitte <[email protected]> (Merged from #2279)

levitte pushed a commit that referenced this pull request Feb 23, 2017

Prepare for WORK_MORE_C

ddf9725

Add the new enum value and case statements as appropriate. Reviewed-by: Matt Caswell <[email protected]> Reviewed-by: Richard Levitte <[email protected]> (Merged from #2279)

levitte pushed a commit that referenced this pull request Feb 23, 2017

Adopt test to changed behavior

0f82d2f

Reviewed-by: Matt Caswell <[email protected]> Reviewed-by: Richard Levitte <[email protected]> (Merged from #2279)

levitte closed this Feb 23, 2017

kaduk mentioned this pull request Oct 10, 2017

Style: ssl.h #4466

Closed

1 task


		=head1 SYNOPSIS

		typedef int (SSL_early_cb_fn) (SSL s, int al, void arg);


		=head1 NAME

		SSL_CTX_set_early_cb, SSL_early_isv2, SSL_early_get0_legacy_version, SSL_early_get0_random, SSL_early_get0_session, SSL_early_get0_ciphers, SSL_early_get0_compression_methods, SSL_early_get0_ext - callback functions for early server-side ClientHello processing

Uh oh!

Comments

Conversation

kaduk commented Jan 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Description of change

Uh oh!

kaduk commented Jan 24, 2017

Uh oh!

davidben commented Jan 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kaduk commented Jan 24, 2017

Uh oh!

indutny commented Jan 24, 2017

Uh oh!

davidben commented Jan 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kaduk commented Jan 24, 2017

Uh oh!

richsalz commented Jan 24, 2017

Uh oh!

kaduk commented Jan 24, 2017

Uh oh!

tmshort commented Jan 31, 2017

Uh oh!

tmshort commented Jan 31, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidben commented Jan 31, 2017

Uh oh!

indutny commented Jan 31, 2017

Uh oh!

kaduk commented Jan 31, 2017

Uh oh!

kaduk commented Jan 31, 2017

Uh oh!

tmshort commented Jan 31, 2017

Uh oh!

kaduk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kaduk commented Feb 3, 2017

Uh oh!

mattcaswell left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kaduk commented Jan 24, 2017 •

edited

Loading

davidben commented Jan 24, 2017 •

edited

Loading

davidben commented Jan 24, 2017 •

edited

Loading