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.

Sign up for GitHub

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

Jump to bottom

Add NSS endpoints for ECH interop #25

Merged
merged 3 commits into from
Feb 2, 2021
Merged

Add NSS endpoints for ECH interop #25

merged 3 commits into from
Feb 2, 2021

Conversation

kjacobs-moz
Copy link
Contributor

@kjacobs-moz kjacobs-moz commented Jan 27, 2021 •
edited
Loading

This currently works for ech-accept between NSS and cloudflare-go.

@kjacobs-moz kjacobs-moz force-pushed the add_nss_ech_interop branch 2 times, most recently from 6653be6 to 3d2d33a Compare January 28, 2021 01:47
@cjpatton cjpatton self-requested a review January 28, 2021 15:12
@cjpatton
Copy link
Collaborator

Is this ready to review, @kjacobs-moz?

@kjacobs-moz
Copy link
Contributor Author

Is this ready to review, @kjacobs-moz?

Yes, the NSS change has landed.

cjpatton
cjpatton requested changes Feb 2, 2021
View reviewed changes
Copy link
Collaborator

@cjpatton cjpatton left a comment

Choose a reason for hiding this comment

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

This looks great! Just a few minor nits and one ask (see inline comments).

Also, a question: The ECH draft does not specify how to format secret keys. Would you endorse PKCS#8 as a format for our purposes here? In other words, should we modify util.go to output an ECHConfigs and, separately, an the corresponding PKCS#8-formatted key pair? (See #17.)

impl-endpoints/nss/ech_key_converter.py Show resolved Hide resolved
impl-endpoints/nss/ech_key_converter.py Show resolved Hide resolved
impl-endpoints/nss/ech_key_converter.py Outdated Show resolved Hide resolved
impl-endpoints/nss/run_endpoint.sh Outdated Show resolved Hide resolved
impl-endpoints/nss/ech_key_converter.py Outdated Show resolved Hide resolved
cmd/util/make.go Show resolved Hide resolved
@cjpatton cjpatton mentioned this pull request Feb 2, 2021
Closed
@cjpatton
Copy link
Collaborator

cjpatton commented Feb 2, 2021

For the other direction, I'll need to land a change to selfserv.c to consume the PKCS8-format HPKE keypair. With that change applied, connection succeeds from cloudflare-go to NSS.

FWIW, cloudflare-go client -> nss server works correctly with the existing conversion script.

chris-wood
chris-wood approved these changes Feb 2, 2021
View reviewed changes
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.

LGTM pending @cjpatton's suggestions for the key conversion script.

@cjpatton
Copy link
Collaborator

cjpatton commented Feb 2, 2021

LGTM pending @cjpatton's suggestions for the key conversion script.

I'm happy to merge without the conversion and get back to it later.

@chris-wood
Copy link
Collaborator

Yeah, same. I was referring to the documentation suggestions. But even those I'd be fine without.

@kjacobs-moz
Copy link
Contributor Author

LGTM pending @cjpatton's suggestions for the key conversion script.

I'm happy to merge without the conversion and get back to it later.

Thanks for the reviews.

Unfortunately, I'm not going to have time in the immediate future to rewrite it in Go, but I would definitely support PKCS8-formatting the ECH/HPKE keypair rather than storing the raw private key, which some libraries may not be able to import easily. NSS is one such library, and doing it outside of NSS/selfserv avoids having to write a second ECHConfigs parser at the application level. With that change, we could remove the script entirely, but it might be worth waiting to see if other libraries have an opinion.

Another option is to output two formats from util.go.

cjpatton
cjpatton approved these changes Feb 2, 2021
View reviewed changes
Copy link
Collaborator

@cjpatton cjpatton left a comment

Choose a reason for hiding this comment

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

LGTM. Pleas squash the last commit before merging.

@cjpatton
Copy link
Collaborator

cjpatton commented Feb 2, 2021

LGTM. Pleas squash the last commit before merging.

Oops, scratch that. Commits look good. I'm merging now.

@cjpatton cjpatton merged commit e695b93 into xvzcf:main Feb 2, 2021
@cjpatton cjpatton mentioned this pull request Feb 2, 2021
Open
@kjacobs-moz kjacobs-moz deleted the add_nss_ech_interop branch February 2, 2021 22:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chris-wood chris-wood chris-wood approved these changes

@cjpatton cjpatton cjpatton approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@kjacobs-moz @cjpatton @chris-wood