Backport of ccae4a1582efcad311d095a8e6832b2b67d5ed05 #918
Conversation
| return DTLSv1_2_client_method(); | ||
| else | ||
| return NULL; | ||
| } |
There was a problem hiding this comment.
So it looks like those function also become unused in 1.0.2, but maybe we should keep them around and try to minimize the changes. I'm not sure yet what I want.
There was a problem hiding this comment.
Ok, just for the sake of compatibility. I agree.
We now send the highest supported version by the client, even if the session uses an older version. This fixes 2 problems: - When you try to reuse a session but the other side doesn't reuse it and uses a different protocol version the connection will fail. - When you're trying to reuse a session with an old version you might be stuck trying to reuse the old version while both sides support a newer version
| if (s->new_session || s->session == NULL) | ||
| i = 0; | ||
| else | ||
| i = s->session->session_id_length; |
There was a problem hiding this comment.
I see this in various places, but I have to wonder that new_session is correct or not, both here and in the master branch. At least the ssl_get_new_session() call doesn't seem to set that, nor does SSL_set_session().
There was a problem hiding this comment.
I'll check it, but this code is really a copy-paste from s3_clnt.c.
There was a problem hiding this comment.
It looks like s->new_session here handles the case of handshake right after SSL_renegotiate().
Session resumption involves a version check, so version negotiation must happen first. Currently, the DTLS implementation cannot do session resumption in DTLS 1.0 because the ssl_version check always checks against 1.2. Switching the order also removes the need to fixup ssl_version in DTLS version negotiation.
indutny
force-pushed
the
feature/backport-ccae4a1582efcad311d095a8e6832b2b67d5ed05
branch
from
d5b927c
to
5e4d5ac
Compare
|
@kroeckx pushed new commits. Looking into The concern that I have so far, is that DTLS_ANY_VERSION seems to be working differently for clients in 1.0.2. It looks like it selects either DTLSv1 or DTLSv1_2 during generation of ClientHello. So there is no chance to make tests work as they do in master. Hopefully, my backport attempt will cover some of the cases. Additional concern is that those tests are passing without @davidben 's DTLS-cookie commit. It doesn't break stuff either, so I committed it anyway. |
|
@kroeckx kindly reminding you about this. |
|
@kroeckx kindly reminder, sorry if this is too noisy! |
|
I was just going to look at this again ... |
| */ | ||
| int ssl23_version_supported(const SSL *s, int version) | ||
| { | ||
| if (s->method == SSLv23_method()) { |
There was a problem hiding this comment.
This at least looks wrong. It can be a SSLv23_client_method() too?
There was a problem hiding this comment.
oh gosh, you are right. Fixed. (Added server_method too, just for consistency).
|
So it looks like your changes in s3_srvr.c actually is the DTLS fix. |
Yikes, sorry for pinging you then. I won't bother you with these "pings" anymore, sorry!
They are indeed! |
|
@kroeckx what's the status on this? |
|
@kroeckx I'm really sorry to bother you, but may I ask you to take a look at this? It seems that activity has ceased on this thread 😢 |
| } else if (strcmp(*argv, "-should_negotiate") == 0) { | ||
| if (--argc < 1) | ||
| goto bad; | ||
| should_negotiate = *(++argv); |
There was a problem hiding this comment.
This is adding new features to a stable branch which we cannot do.
There was a problem hiding this comment.
You mean only that one part, right? Not the rest of this which seems like a bugfix (correct me if I'm wrong). And then, does adding this could make it possible to test the bugfix?
There was a problem hiding this comment.
Yes - just this bit. I understand that the test uses this - but I still don't thing we should add this bit to a stable branch.
There was a problem hiding this comment.
Isn't this just a test binary that isn't shipped anywhere? It's not adding a flag to the openssl binary, as far as I can tell.
There was a problem hiding this comment.
Ahhh....oh, yes my mistake. I thought I was looking at an "apps" file here. I withdraw my comment.
|
Hey all... wanted to quickly ping to see what the status of this is. @indutny looks like you may need to rebase |
|
Kindly reminding you about this. |
|
+1. Ship it, @kroeckx :) |
|
Ping @kroeckx |
|
@kroeckx ... still worth doing this? |
richsalz
added
branch: 1.0.2
|
I think it is. If someone else wants to review this, please do so.
|
|
This looks okay to me but I would be happier if @mattcaswell would also take a look. |
|
This change makes me nervous. I'm thinking this is too big to backport. |
|
No further discussion on this, so I'm closing. |
This is a backport of two commits:
Main differences between ccae4a1 and approach here:
1.0.2 has two clients: s23 and s3.
s->methodwas set early onSSL_set_sessioncall to makeSSL_connectcalls3_client_hello, which is capable of generating client hello with session. So the logical step was to introduce session field tos23_client_hello. Additionally, some copy-pasted code was added ins3_clntto handle session resumption and version support.NULL and deprecate
ssl_get_methodinstead of removing it from the structure. Needed for ABI-compatbility.ssl23_version_supportedis slightly different, as there are no method tables in 1.0.2. Hopefully it is ok.Notes
I took some courage to include some other changes from master into ssltest to allow running tests from b7dffce. However
dtlstests are in quite odd shape here, since there are no-dtlsflag (will probably add it later, if the rest looks ok).cc @kroeckx @mattcaswell @davidben PTAL, I will be glad to answer any questions or incorporate any suggested changes. Tests appears to be passing locally.