RFC 8701 implementation by ericcurtin · Pull Request #14560 · openssl/openssl

ericcurtin · 2021-03-15T20:12:36Z

No description provided.

richsalz · 2021-03-15T20:37:57Z

there's some style nits that others will point out, but this is great!

davidben · 2021-03-15T22:43:20Z

This and the enum probably should not be exported. Rather the idea (I wrote the original. :-) ) was that the library would internally call into these as needed as part of serializing the ClientHello, etc.

The whole business with grease_seed is just because there are cases (HelloVerifyRequest, HelloRetryRequest), where variations on a ClientHello are serialized multiple times, and they need to be consistent. (Originally we even just sampled client_random, but switched to a separate seed later since that seemed tidier.)

Ok, thanks so much for keeping an eye on this! I watched one of your talks on youtube so I could gather what your thoughts were on some of this :)

Well, the other half is that you probably need to actually call the function in the library to go send GREASE ciphers, etc. Otherwise there's just a floating unused function in here.

Haha, I can't watch that talk. It's too embarrassing to see myself speak. :-)

Well, the other half is that you probably need to actually call the function in the library to go send GREASE ciphers

Yeah of course, I'm a fan of pushing code early. Nothing more disheartening that writing a load of code a certain way only for someone more knowledgeable to untangle it all :)

Expect to see frequent updates in this PR, I was on vacation the last day or so. St. Patrick's day and all that.

From include/openssl/ssl.h.in -> ssl/ssl_local.h

Declarations at start of scope

Some comments also trying to identify synonyms in bssl as I'm not overly familiar with either codebase and it's my first time looking at bssl.

Other style stuff

ericcurtin · 2021-03-22T22:49:12Z

Rebased, because I think I saw in the other PR's that the current commit id has green CI builds. @davidben @paulidale is this looking ok?

davidben · 2021-03-22T23:10:32Z

                goto err;
            }
+
+            /* s->session->ext is CBB extensions in bssl */


Er, it's not. CBB is BoringSSL's thing for assembling a message to serialize somewhere, while s->session->ext is some data stored in the SSL_SESSION. The point of GREASE is to actually send it over the wire. You also probably don't want to store it in the SSL_SESSION, which gets carried over across resumptions.

Thanks David I'll look into this in more detail!

The session thing you explain makes complete sense, it's something I wasn't confident on when I wrote this. Thanks for noticing my assumption in the comment.

CIs look relevant

openssl-machine · 2021-03-23T23:00:14Z

24 hours has passed since 'approval: done' was set, but as this PR has been updated in that time the label 'approval: ready to merge' is not being automatically set. Please review the updates and set the label manually.

ericcurtin · 2021-03-24T19:19:05Z

I noticed the stalled: awaiting contributor response, don't worry I'll get to this. 🙂 This is very much in my volunteer time in my personal hours. Although I'd like to get this into a TLS 1.3 usage of mine alright at the same time. Spinning a few plates...

openssl-machine · 2021-04-24T00:07:30Z

This PR is waiting for the creator to make requested changes but it has not been updated for 30 days. If you have made changes or commented to the reviewer please make sure you re-request a review (see icon in the 'reviewers' section).

openssl-machine · 2021-05-25T00:06:57Z

This PR is waiting for the creator to make requested changes but it has not been updated for 61 days. If you have made changes or commented to the reviewer please make sure you re-request a review (see icon in the 'reviewers' section).

openssl-machine · 2021-06-25T00:06:54Z

This PR is waiting for the creator to make requested changes but it has not been updated for 92 days. If you have made changes or commented to the reviewer please make sure you re-request a review (see icon in the 'reviewers' section).

openssl-machine · 2021-07-23T00:07:34Z

This PR has been closed. It was waiting for the creator to make requested changes but it has not been updated for 120 days.

ericcurtin mentioned this pull request Mar 15, 2021

Implement RFC 8701 (was draft-ietf-tls-grease) #9660

Closed

davidben reviewed Mar 15, 2021

View reviewed changes

paulidale reviewed Mar 15, 2021

View reviewed changes

Comment thread include/openssl/ssl.h.in Outdated

Comment thread include/openssl/ssl.h.in Outdated

Comment thread ssl/ssl_lib.c Outdated

paulidale reviewed Mar 15, 2021

View reviewed changes

Comment thread ssl/ssl_lib.c Outdated

paulidale reviewed Mar 15, 2021

View reviewed changes

Comment thread ssl/ssl_lib.c Outdated

FdaSilvaYY reviewed Mar 21, 2021

View reviewed changes

Comment thread ssl/ssl_lib.c Outdated

ericcurtin added 6 commits March 22, 2021 22:47

RFC 8701 implementation

d969017

Move GREASE enum, function signature, etc. to private header

6a52fed

From include/openssl/ssl.h.in -> ssl/ssl_local.h

C89-style variables in SSL_get_grease_value

486126d

Declarations at start of scope

Add uint16_t grease_value to extension struct

4261843

Some comments also trying to identify synonyms in bssl as I'm not overly familiar with either codebase and it's my first time looking at bssl.

Added boolean flag grease_seeded

c9185a9

Other style stuff

C++ to C-style comments

e570d31

ericcurtin force-pushed the rfc_8701 branch from a46482a to e570d31 Compare March 22, 2021 22:47

paulidale approved these changes Mar 22, 2021

View reviewed changes

paulidale added approval: done This pull request has the required number of approvals approval: review pending This pull request needs review by a committer branch: master Applies to master branch labels Mar 22, 2021

2 spaces to 4 in SSL_get_grease_value

8cfea49

davidben reviewed Mar 22, 2021

View reviewed changes

paulidale previously approved these changes Mar 23, 2021

View reviewed changes

t8m added waiting-for: contributor response This pull request is awaiting a response by the contributor and removed approval: done This pull request has the required number of approvals approval: review pending This pull request needs review by a committer labels Mar 24, 2021

openssl-machine closed this Jul 23, 2021

mcrmck mentioned this pull request Mar 8, 2026

Implement RFC 8701 GREASE for TLS ClientHello #30303

Closed

Uh oh!

Uh oh!

Conversation

ericcurtin commented Mar 15, 2021

Uh oh!

richsalz commented Mar 15, 2021

Uh oh!

davidben Mar 15, 2021

Choose a reason for hiding this comment

Uh oh!

ericcurtin Mar 18, 2021

Choose a reason for hiding this comment

Uh oh!

davidben Mar 18, 2021

Choose a reason for hiding this comment

Uh oh!

ericcurtin Mar 18, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ericcurtin commented Mar 22, 2021

Uh oh!

davidben Mar 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ericcurtin Mar 22, 2021

Choose a reason for hiding this comment

Uh oh!

ericcurtin Mar 22, 2021

Choose a reason for hiding this comment

Uh oh!

openssl-machine commented Mar 23, 2021

Uh oh!

ericcurtin commented Mar 24, 2021

Uh oh!

openssl-machine commented Apr 24, 2021

Uh oh!

openssl-machine commented May 25, 2021

Uh oh!

openssl-machine commented Jun 25, 2021

Uh oh!

openssl-machine commented Jul 23, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

davidben Mar 22, 2021 •

edited

Loading