TLSv1.3: Certificate and CertificateStatus by mattcaswell · Pull Request #2020 · openssl/openssl

mattcaswell · 2016-12-02T15:25:29Z

Checklist

tests are added or updated
CLA is signed

Description of change

This PR builds on #2015 to implement the TLSv1.3 format Certificate message, including the extensions block. It then also uses those extensions to send the CertificateStatus data and drops the old separate CertficateStatus message in TLSv1.3

mattcaswell · 2016-12-02T17:16:31Z

I added a commit to update SSL_trace() for the new format Certificate message.

mattcaswell · 2016-12-08T18:19:40Z

I rebased this onto the latest master (phew that was hard....loads of conflicts). Anyway, this PR no longer depends on anything else so can be reviewed.

mattcaswell · 2016-12-29T13:44:58Z

Rebased this against latest master. Ping?

richsalz · 2016-12-29T13:46:32Z

I will not be able to look at it for a few days, if anyone else wants to jump in.

levitte

You need to do a make update and fix the issues that come up.

mattcaswell · 2016-12-30T09:51:13Z

New version pushed resolving make update issues.

mattcaswell · 2017-01-02T11:14:36Z

Rebased again.

mattcaswell · 2017-01-02T11:20:21Z

Also added a new commit to fix a travis failure.

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

ssl/t1_trce.c

extra blank line :)

richsalz · 2017-01-02T18:29:30Z

ssl/statem/extensions.c

"chain" is the wrong name. It's really numcerts or xcount. And it should be an int not a size_t.

No - it means which certificate this is in the chain as presented by the peer. numcerts or xcount would be wrong. A Certificate message contains multiple certificates, and each one has its own extensions block. Therefore the parse function is called for each one - if there are 3 certs then parse_ctos will be called three times (if the extension exists) with chain set to 0, 1 and 2. It is bounded by 0 <= chain < "number of certificates". Since the number of certificates is a quantity, the correct type is a size_t.

I still disagree. size_t is for the size of something, not an index. int is appropriate. And chain is still a bad name, idx or index or which is better.

Ok, I changed the name to chainidx.

I strongly disagree on the type. The range of valid values exactly matches that of a size_t. It cannot be negative and the biggest (theoretical) value is SIZE_MAX - 1. The most common operations that you tend to perform on an index is to compare it to its boundary values...which is a size_t. Without that you have to perform casts to do the comparison. In fact there is scope for overflow errors if you use an int (INT_MAX != SIZE_MAX) and other oddities where the index goes -ve if its an int. A quick google search turned up a number of discussions on this issue (e.g. https://gustedt.wordpress.com/2013/07/15/a-praise-of-size_t-and-other-unsigned-types/).

mattcaswell · 2017-01-05T15:11:33Z

Fixed the blank line issue and rebased this.

richsalz · 2017-01-05T16:23:28Z

ssl/statem/statem_srvr.c

richsalz · 2017-01-05T16:23:50Z

ssl/statem/statem_clnt.c

richsalz · 2017-01-05T16:26:13Z

ssl/statem/statem_lib.c

richsalz · 2017-01-05T16:26:29Z

ssl/statem/statem_lib.c

richsalz · 2017-01-05T16:29:06Z

ssl/statem/statem_srvr.c

indent wrong?

richsalz · 2017-01-05T16:30:19Z

ssl/t1_trce.c

shouldn't this be msginlen?

richsalz · 2017-01-05T16:32:35Z

Okay on size_t for subscript.

The Certificate message in TLS1.3 has an extensions block for each Certificate. Therefore we need to extend tls_construct_extensions() to pass in the certificate we are working on. We also pass in the position in the chain (with 0 being the first certificate).

Continuing from the previous commit we also need to extend the extensions framework to supply the Certificate we just read during parsing.

Also updates TLSProxy to be able to understand the format and parse the contained extensions.

We remove the separate CertificateStatus message for TLSv1.3, and instead send back the response in the appropriate Certificate message extension.

al can be used uninitialised in an error path.

This variable represents the index of the cert within the chain, so give it a name that better represents that.

mattcaswell · 2017-01-06T11:04:51Z

Fixed the various issues above and also rebased to pick up the Appveyor fix from master.

richsalz · 2017-01-06T12:41:09Z

+1. nice work!

mattcaswell · 2017-01-06T15:54:21Z

Pushed. Thanks!

The Certificate message in TLS1.3 has an extensions block for each Certificate. Therefore we need to extend tls_construct_extensions() to pass in the certificate we are working on. We also pass in the position in the chain (with 0 being the first certificate). Reviewed-by: Rich Salz <[email protected]> (Merged from #2020)

Continuing from the previous commit we also need to extend the extensions framework to supply the Certificate we just read during parsing. Reviewed-by: Rich Salz <[email protected]> (Merged from #2020)

Also updates TLSProxy to be able to understand the format and parse the contained extensions. Reviewed-by: Rich Salz <[email protected]> (Merged from #2020)

We remove the separate CertificateStatus message for TLSv1.3, and instead send back the response in the appropriate Certificate message extension. Reviewed-by: Rich Salz <[email protected]> (Merged from #2020)

Reviewed-by: Rich Salz <[email protected]> (Merged from #2020)

al can be used uninitialised in an error path. Reviewed-by: Rich Salz <[email protected]> (Merged from #2020)

Reviewed-by: Rich Salz <[email protected]> (Merged from #2020)

This variable represents the index of the cert within the chain, so give it a name that better represents that. Reviewed-by: Rich Salz <[email protected]> (Merged from #2020)

Reviewed-by: Rich Salz <[email protected]> (Merged from #2020)

mattcaswell force-pushed the tls1_3-certificate-changes branch from 210c4c1 to 7759e04 Compare December 8, 2016 18:18

mattcaswell force-pushed the tls1_3-certificate-changes branch from 7759e04 to 049385a Compare December 29, 2016 13:44

levitte requested changes Dec 29, 2016

View reviewed changes

mattcaswell force-pushed the tls1_3-certificate-changes branch from 049385a to 0a1df2c Compare December 30, 2016 09:50

mattcaswell force-pushed the tls1_3-certificate-changes branch from 0a1df2c to 94b8894 Compare January 2, 2017 11:14

richsalz suggested changes Jan 2, 2017

View reviewed changes

mattcaswell mentioned this pull request Jan 5, 2017

TLSv1.3: Interoperable 1-RTT handshake #2157

Closed

2 tasks

mattcaswell force-pushed the tls1_3-certificate-changes branch from 53adf88 to b435955 Compare January 5, 2017 15:11

richsalz suggested changes Jan 5, 2017

View reviewed changes

mattcaswell added 9 commits January 6, 2017 10:25

Extends extension parsing to take the Certificate

5dcb30e

Continuing from the previous commit we also need to extend the extensions framework to supply the Certificate we just read during parsing.

Create Certificate messages in TLS1.3 format

a2b6360

Also updates TLSProxy to be able to understand the format and parse the contained extensions.

Implement TLSv1.3 style CertificateStatus

28edac9

We remove the separate CertificateStatus message for TLSv1.3, and instead send back the response in the appropriate Certificate message extension.

Update SSL_trace to understand TLSv1.3 Certificates

3271780

Initialise the al variable

fbf8899

al can be used uninitialised in an error path.

Fix a double blank line style issue

d2c4118

Rename the chain variable to chainidx

952fddb

This variable represents the index of the cert within the chain, so give it a name that better represents that.

Fix various style issues following feedback

92020ad

mattcaswell force-pushed the tls1_3-certificate-changes branch from ce2e815 to 92020ad Compare January 6, 2017 11:04

mattcaswell closed this Jan 6, 2017

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

Create Certificate messages in TLS1.3 format

e96e0f8

Also updates TLSProxy to be able to understand the format and parse the contained extensions. Reviewed-by: Rich Salz <[email protected]> (Merged from #2020)

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

Update SSL_trace to understand TLSv1.3 Certificates

ac52c4b

Reviewed-by: Rich Salz <[email protected]> (Merged from #2020)

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

Initialise the al variable

0baed5e

al can be used uninitialised in an error path. Reviewed-by: Rich Salz <[email protected]> (Merged from #2020)

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

Fix a double blank line style issue

3dd826b

Reviewed-by: Rich Salz <[email protected]> (Merged from #2020)

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

Rename the chain variable to chainidx

8521ced

This variable represents the index of the cert within the chain, so give it a name that better represents that. Reviewed-by: Rich Salz <[email protected]> (Merged from #2020)

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

Fix various style issues following feedback

d805a57

Reviewed-by: Rich Salz <[email protected]> (Merged from #2020)

slontis mentioned this pull request Jul 19, 2018

no-asm build fails (due to changes to ec/curve25519.c) #6743

Closed

Uh oh!

Comments

Conversation

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

Uh oh!

Checklist

Description of change

Uh oh!

mattcaswell commented Dec 2, 2016

Uh oh!

mattcaswell commented Dec 8, 2016

Uh oh!

mattcaswell commented Dec 29, 2016

Uh oh!

richsalz commented Dec 29, 2016

Uh oh!

levitte left a comment

Choose a reason for hiding this comment

Uh oh!

mattcaswell commented Dec 30, 2016

Uh oh!

mattcaswell commented Jan 2, 2017

Uh oh!

mattcaswell commented Jan 2, 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!

mattcaswell commented Jan 5, 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!

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!

richsalz commented Jan 5, 2017 via email

Uh oh!

mattcaswell commented Jan 6, 2017

Uh oh!

richsalz commented Jan 6, 2017

Uh oh!

mattcaswell commented Jan 6, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mattcaswell commented Dec 2, 2016 •

edited

Loading