Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add "ech-reject" test case for nss and cloudflare-go #36

Merged
merged 1 commit into from
Feb 13, 2021

Conversation

cjpatton
Copy link
Collaborator

@cjpatton cjpatton commented Feb 11, 2021

Partially addresses #27.

Adds a test case for exercising the ECH rejection path, with initial support for NSS and Cloudflare-Go.

In this test case, the client offers ECH with an invalid config, the server rejects, and the client aborts the connection with "ech_required" alert. They don't attempt to retry the connection with the retry configs.

Thanks to @kjacobs-moz for help getting the NSS client endpoint right!

@cjpatton cjpatton force-pushed the cjpatton/ech-reject branch 2 times, most recently from 766eb56 to 78dee24 Compare February 11, 2021 15:32
@cjpatton cjpatton marked this pull request as ready for review February 11, 2021 15:33
@cjpatton cjpatton requested review from chris-wood and xvzcf February 11, 2021 15:33
@cjpatton cjpatton requested a review from xvzcf February 12, 2021 22:06
Copy link
Collaborator

@chris-wood chris-wood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only one blocking comment (panic vs return sentinel). The rest looks good!

}
} else {
log.Fatalf("unknown role \"%s\"", *role)
log.Fatalf("Unknown role \"%s\"", *role)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes me wonder if we should have a sentinel return value for invalid parameters. @xvzcf, thoughts?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will address this in the follow-up PR.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See #39, though I'm not sure this totally addresses the point!

In this test case, the client offers ECH using an invalid config, the
server rejects, and the client aborts the connection.

This adds support for this test case to the nss and cloudflare-go
endpoints.
@cjpatton cjpatton merged commit 8b6aab5 into main Feb 13, 2021
@cjpatton cjpatton deleted the cjpatton/ech-reject branch February 13, 2021 21:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants