Move extension data into sub-structs by richsalz · Pull Request #2052 · openssl/openssl

richsalz · 2016-12-08T20:45:19Z

So instead of tlsext_hostname, for example, you have ext.hostname. It ensures that all extension stuff is at least grouped together. And shorter more consistent names for some things. Lists end in plural and have a matching _len element. ALPN and NPN are spelled with four or three letters, not next_proto and next_protos, inconsistently. And so on.

kaduk · 2016-12-08T20:50:43Z

IIRC, some of these fields have different semantics for client vs. server SSL{,_CTX} objects. (tlsext_hostname is one that comes to mind, though there is also a field of that name in SSL_SESSION just to make things extra confusing, so maybe I am misremembering.) Do we want to differentiate the semantics based on field name, and if so is this the right place to make such a change?

richsalz · 2016-12-08T21:02:13Z

I am reluctant to tease apart re-used field names here, as this is strictly just textual renaming.

kaduk · 2016-12-08T21:14:50Z

Sounds good; keeping the scope limited is fine by me.

levitte

Quick review. I will get back to this next week if needed.

levitte · 2016-12-08T21:55:42Z

util/libssl.num

levitte · 2016-12-08T21:55:53Z

util/libssl.num

levitte · 2016-12-08T21:58:53Z

Frankly, I'm hesitant about the renaming of global functions. Our code style guide says this in "Chapter 4: Naming":

Global variables (to be used only if you REALLY need them) need to
have descriptive names, as do global functions. If you have a function
that counts the number of active users, you should call that
count_active_users() or similar, you should NOT call it cntusr().

FdaSilvaYY · 2016-12-08T22:02:10Z

ABI compatibility ?

richsalz · 2016-12-08T22:06:03Z

I guess I got the ABI backwards, make the newname an alias to the old name. Will fix, tnx @FdaSilvaYY.

Nobody calls the TLS extension anything other than ALPN and NPN, @levitte

richsalz · 2016-12-08T22:13:02Z

updated commit pushed.

levitte · 2016-12-08T22:17:57Z

@richsalz, I just realised that a function rename like this is an ABI change (not addition). Therefore, this is 1.2.0 material as it currently stands.

richsalz · 2016-12-08T22:19:19Z

yes, i fixed that.

levitte · 2016-12-08T22:19:28Z

Never mind, now I see that the rename / alias thing has been reversed

ABI/renaming addressed.

levitte · 2016-12-21T23:43:42Z

It would be nice if you rebased onto a fresher master

richsalz · 2016-12-22T02:48:45Z

rebased and push'd

richsalz · 2017-01-02T18:01:57Z

ping @levitte post-Jan 1 :)

Add option ExpectedTmpKeyType to test the temporary key the server sends is of the correct type. Reviewed-by: Kurt Roeckx <[email protected]> Reviewed-by: Matt Caswell <[email protected]> (Merged from #2191)

Reviewed-by: Kurt Roeckx <[email protected]> Reviewed-by: Matt Caswell <[email protected]> (Merged from #2191)

mattcaswell · 2017-01-09T14:01:30Z

ssl/s3_lib.c

Probably should adjust the indentation on this line

yes. and also fixed it in ssl_locl.h

mattcaswell · 2017-01-09T14:06:54Z

ssl/ssl_sess.c

ext_data != NULL

mattcaswell

Yawn! This all looks very straight forward...a big search and replace job mainly! A couple of minor review comments above. Assuming those get fixed: +1

mattcaswell

Oh...I take that back. Just spotted that travis and appveyor are failing for this PR...that needs sorting first.

richsalz · 2017-01-09T17:43:18Z

added commit and rebased/pushed. let's see if travis works now...

richsalz · 2017-01-09T21:07:28Z

ping @mattcaswell -- travis and appveyor passed.

richsalz · 2017-01-10T03:33:19Z

thanks.

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

richsalz self-assigned this Dec 8, 2016

richsalz requested review from levitte and mattcaswell December 8, 2016 20:45

levitte previously requested changes Dec 8, 2016

View reviewed changes

richsalz mentioned this pull request Jan 7, 2017

TLSv1.3: Interoperable 1-RTT handshake #2157

Closed

2 tasks

snhenson added 2 commits January 8, 2017 19:36

Add new ssl_test option.

b93ad05

Add option ExpectedTmpKeyType to test the temporary key the server sends is of the correct type. Reviewed-by: Kurt Roeckx <[email protected]> Reviewed-by: Matt Caswell <[email protected]> (Merged from #2191)

Add server temp key type checks

9c4319b

Reviewed-by: Kurt Roeckx <[email protected]> Reviewed-by: Matt Caswell <[email protected]> (Merged from #2191)

mattcaswell reviewed Jan 9, 2017

View reviewed changes

mattcaswell approved these changes Jan 9, 2017

View reviewed changes

mattcaswell requested changes Jan 9, 2017

View reviewed changes

Rich Salz added 3 commits January 9, 2017 12:34

Move extension data into sub-structs

6854095

Use typedefs for PSK, NPN, ALPN callback functions

ef50dba

Review comments

1feac1f

mattcaswell approved these changes Jan 9, 2017

View reviewed changes

mattcaswell added the approval: done This pull request has the required number of approvals label Jan 9, 2017

richsalz closed this Jan 10, 2017

levitte pushed a commit that referenced this pull request Jan 10, 2017

Move extension data into sub-structs

aff8c12

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

levitte pushed a commit that referenced this pull request Jan 10, 2017

Use typedefs for PSK, NPN, ALPN callback functions

8cbfcc7

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

levitte pushed a commit that referenced this pull request Jan 10, 2017

Review comments

1ed327f

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

richsalz deleted the shorter-field-names branch January 10, 2017 03:34

Uh oh!

Comments

Conversation

richsalz commented Dec 8, 2016

Uh oh!

kaduk commented Dec 8, 2016

Uh oh!

richsalz commented Dec 8, 2016

Uh oh!

kaduk commented Dec 8, 2016

Uh oh!

levitte left a comment

Choose a reason for hiding this comment

Uh oh!

levitte Dec 8, 2016

Choose a reason for hiding this comment

Uh oh!

levitte Dec 8, 2016

Choose a reason for hiding this comment

Uh oh!

levitte commented Dec 8, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

FdaSilvaYY commented Dec 8, 2016

Uh oh!

richsalz commented Dec 8, 2016

Uh oh!

richsalz commented Dec 8, 2016

Uh oh!

levitte commented Dec 8, 2016

Uh oh!

richsalz commented Dec 8, 2016

Uh oh!

levitte commented Dec 8, 2016

Uh oh!

levitte commented Dec 21, 2016

Uh oh!

richsalz commented Dec 22, 2016

Uh oh!

richsalz commented Jan 2, 2017

Uh oh!

mattcaswell Jan 9, 2017

Choose a reason for hiding this comment

Uh oh!

richsalz Jan 9, 2017

Choose a reason for hiding this comment

Uh oh!

mattcaswell Jan 9, 2017

Choose a reason for hiding this comment

Uh oh!

richsalz Jan 9, 2017

Choose a reason for hiding this comment

Uh oh!

mattcaswell left a comment

Choose a reason for hiding this comment

Uh oh!

mattcaswell left a comment

Choose a reason for hiding this comment

Uh oh!

richsalz commented Jan 9, 2017

Uh oh!

richsalz commented Jan 9, 2017

Uh oh!

richsalz commented Jan 10, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

levitte commented Dec 8, 2016 •

edited

Loading