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 CSR support for key delivery and proof of possession #527

Merged
merged 6 commits into from Apr 20, 2022

Conversation

haydentherapper
Copy link
Contributor

@haydentherapper haydentherapper commented Apr 15, 2022

Summary

Many ecosystems and libraries have support for generating CSRs, but do
not have easy-to-use utilities for generating encoded public keys. A CSR
provides a simple way to send an encoded public key, along with a proof
of possession since CSRs are self-signed.

Existing clients do not have to change their behavior, as we will
continue to support providing a public key and signed challenge.

Signed-off-by: Hayden Blauzvern hblauzvern@google.com

Ticket Link

Fixes #503

Release Note

Added CSR support in the API for key delivery and proof of possession

@haydentherapper
Copy link
Contributor Author

There are three options for the API changes:

  1. (What's here) Add a field to the HTTP and gRPC API. All other fields must become optional.
  2. Add a oneof to the gRPC layer, combining the PublicKey and Challenge fields into a single message, and adding the CSR as the other oneof field. The benefit is the gRPC API layer is well-defined. The downside is the HTTP layer does not support oneofs and we don't want to break the HTTP API, so we must still do (1) and make those fields optional.
  3. Don't add fields to the API. Support the CSR being set in PublicKey.Content, parsing it server-side. Make the challenge field optional. The downside with this approach is now the public key field supports 3 different encodings.

Currently leaning towards (2), it's a cleaner gRPC API.

Many ecosystems and libraries have support for generating CSRs, but do
not have easy-to-use utilities for generating encoded public keys. A CSR
provides a simple way to send an encoded public key, along with a proof
of possession since CSRs are self-signed.

Existing clients do not have to change their behavior, as we will
continue to support providing a public key and signed challenge.

Signed-off-by: Hayden Blauzvern <hblauzvern@google.com>
Signed-off-by: Hayden Blauzvern <hblauzvern@google.com>
@znewman01
Copy link
Contributor

Looking forward to seeing this come through!

+1 to (2), I think the plan of "make gRPC API nice" is the right one and HTTP will always be a bit hacky regardless of this choice. I think (3) is a bad idea because the number of possible encodings may continue to grow.

This provides a cleaner API.

Signed-off-by: Hayden Blauzvern <hblauzvern@google.com>
Signed-off-by: Hayden Blauzvern <hblauzvern@google.com>
@codecov-commenter
Copy link

codecov-commenter commented Apr 18, 2022

Codecov Report

Merging #527 (3698c11) into main (38798fe) will increase coverage by 1.72%.
The diff coverage is 42.38%.

❗ Current head 3698c11 differs from pull request most recent head fdb5d98. Consider uploading reports for the commit fdb5d98 to get more accurate results

@@            Coverage Diff             @@
##             main     #527      +/-   ##
==========================================
+ Coverage   33.75%   35.47%   +1.72%     
==========================================
  Files          18       18              
  Lines        1357     1415      +58     
==========================================
+ Hits          458      502      +44     
- Misses        836      853      +17     
+ Partials       63       60       -3     
Impacted Files Coverage Δ
pkg/api/client.go 0.00% <ø> (ø)
pkg/api/error.go 100.00% <ø> (ø)
pkg/api/legacy_server.go 0.00% <0.00%> (ø)
pkg/challenges/challenges.go 28.78% <46.15%> (+7.30%) ⬆️
pkg/api/grpc_server.go 52.97% <100.00%> (+7.24%) ⬆️
pkg/ca/fileca/load.go 58.62% <0.00%> (-10.35%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 38798fe...fdb5d98. Read the comment docs.

@haydentherapper haydentherapper marked this pull request as ready for review April 18, 2022 18:11
@haydentherapper
Copy link
Contributor Author

FYI @di

Signed-off-by: Hayden Blauzvern <hblauzvern@google.com>
Signed-off-by: Hayden Blauzvern <hblauzvern@google.com>
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.

Support an API that accepts Certificate Signing Requests
4 participants