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

[DRAFT] RFC: Add certificate compression to EVE API #4552

Open
rucoder opened this issue Jan 27, 2025 · 9 comments
Open

[DRAFT] RFC: Add certificate compression to EVE API #4552

rucoder opened this issue Jan 27, 2025 · 9 comments
Assignees

Comments

@rucoder
Copy link
Contributor

rucoder commented Jan 27, 2025

Problem description

Currently /cert endpoint send all available certificates at once and doesn't use any compression. Certificate chain may be very big and consume a lot of network traffic.

Proposed change

  1. Introduce a POST /api/v2/edgeDevice/certs with following content
// This is a request payload for POST /api/v2/edgeDevice/certs
// The client may request only required certificates
// The list of supported compression methods can be provider
message ZCertRequest {
  repeated ZCertType type = 1; //type of certificate requested
  repeated ZCertCompressionType compression = 2; //compression type requested
}
  1. Introduce URL parameters for GET request as following
  • certs=1,2,..n to indecate requested certificate type. All certificates are requested if not specified
  • compress=1,2,..4 to indicate compression type. No compression is used if not specified
  1. Define possible compression types that are aligned with RFC 8879 for TLS certificate compression
// Possible compression types for certificates that reflects the
// compression algorithms defined in RFC 8879 for TLS certificate compression
enum ZCertCompressionType {
  Z_CERT_COMPRESSION_TYPE_INVALID = 0;
  Z_CERT_COMPRESSION_TYPE_NONE = 1; //no compression
  Z_CERT_COMPRESSION_TYPE_ZLIB = 2; //zlib compression
  Z_CERT_COMPRESSION_TYPE_BROTLI = 3; //brotli compression
  Z_CERT_COMPRESSION_TYPE_ZSTD = 4; //zstd compression
}
  1. Introduce a compression field in ZCertAttr structure
message ZCertAttr {
  bool is_mutable = 1;            //set to false for immutable certificates
  bool is_tpm = 2;                //generated by a TPM
  ZCertCompressionType compression = 3; //compression type used for the certificate
}

Client behavior

  1. POST is the preferable method to request certificates. The client SHOULD use this method first
  2. If POST return "unsupported request" the client falls back to GET request.

Controller behavior

  1. Controller MUST use only compression methods known to client and requested in ZCertRequest structure GET URL parameters
  2. Controller MUST NOT send compressed certificate if GET method is used
  3. Controller MAY prepare compressed certificate in advance to minimize CPU load

TODO

  1. HTTP vs HTTPs protocol for /cert clarification
    2.How do we sign AuthContainer ? With onboarding cert first and then with device cert like any other messages?
@deitch
Copy link
Contributor

deitch commented Jan 27, 2025

Questions:

  1. why use POST instead of GET? Aren't we "getting" a resource (i.e. certs)?
  2. why would we use compression type and cert type in the body? Wouldn't those fit well into the path and header of REST API endpoints? Isn't that what Accept header is for?

@rucoder
Copy link
Contributor Author

rucoder commented Jan 27, 2025

Questions:

1. why use POST instead of GET? Aren't we "getting" a resource (i.e. certs)?

we are getting a lot of resources in API but we need to send a signed request in AuthhContainer to do so. GET request doesn't have body. we could use arguments like /api/v2/edgeDevice/certs?comression=<> but idk whether adding parameters to URL is backword compatible with HTTP servers @eriknordmark ?

2. why would we use compression type and cert type in the body? Wouldn't those fit well into the path and header of REST API endpoints? Isn't that what [Accept](https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Accept) header is for?

No, it is for body encoding. In our case it is a application/x-proto-binary Besides other structures already exist, I just added fields

@deitch
Copy link
Contributor

deitch commented Jan 27, 2025

No, it is for body encoding. In our case it is a application/x-proto-binary

That is kind of what I am asking. Our API is complex for good reasons, includes authcontainer etc. For something like this, does it have to be?

From a different angle. If I didn't know about the EVE API, and someone came to me and said, "I need to expose an API endpoint to send certificates, and there are options to give it compression, how would I do it?" My answer immediately would be:

  • GET /v1/certificates (or some sub path) for all
  • GET /v1/certificates/125 for a specific one
  • GET /v1/certificates?issuer=X,Y to filter
  • use the Accept header to indicate format, like compression

Could be you are saying that this has to be under the EVE API structure for good reasons. OK, then my points aren't relevant. But worth asking.

@rucoder
Copy link
Contributor Author

rucoder commented Jan 27, 2025

  • use the Accept header to indicate format, like compression

cwe cannot use Accept header, we use it to indecate response format which is already application/x-proto-binary and we need to maintain backward compatibility.

I agree that we do not need POST and just encode supported compression type and certificate type in the URL

@naiming-zededa
Copy link
Contributor

naiming-zededa commented Jan 27, 2025

although it happens only once or twice during bootup, the /certs API data.
if it's the real TLS certs compression, that would be a great help to reduce the data over wire, but cloudflare does not support this, even though the RFC itself is by Cloudflare ;-)

@rucoder
Copy link
Contributor Author

rucoder commented Jan 27, 2025

although it happens only once or twice during bootup, the /certs API data. if it's the real TLS certs compression, that would be a great help to reduce the data over wire, but cloudflare does not support this, even though the RFC itself is by Cloudflare ;-)

@naiming-zededa it is not yet TLS compression, it is a similar approach for /cert endpoint which may be called much often if controller rotate certificates. to track TLS compression issue I created another issue #4545

@deitch I updated the proposal

@christoph-zededa
Copy link
Contributor

[controller] DEBUG | 2025-01-27T18:51:43+01:00: /certs req: &{Method:GET URL:/api/v2/edgedevice/certs Proto:HTTP/1.1 ProtoMajor:1 ProtoMinor:1 Header:map[Accept-Encoding:[gzip] Connection:[close] User-Agent:[Go-http-client/1.1] X-Request-Id:[af90c627-c176-4d89-a47c-cae1cffb6416] X-Serial-Number:[31415926]] Body:{} GetBody:<nil> ContentLength:0 TransferEncoding:[] Close:true Host:zedcloud.local.zededa.net Form:map[] PostForm:map[] MultipartForm:<nil> Trailer:map[] RemoteAddr:192.168.169.100:48406 RequestURI:/api/v2/edgedevice/certs TLS:0xc0003b40c0 Cancel:<nil> Response:<nil> Pattern: ctx:0xc0002ec420 pat:<nil> matches:[] otherValues:map[*:]}

This is the request that I see from EVE.
No Accept header, but an Accept-Encoding header that is already set to gzip. So if the response is not encrypted, then the compression should work, shouldn't it?

@rucoder
Copy link
Contributor Author

rucoder commented Jan 28, 2025

@deitch it seems it can be done using other header field Accept-Encoding: gzip, deflate for a particular endpoint so we do not need to do anything. My initial concern was that server side have to do it for all traffic and this is a CPU consuming so if we can confirm this than the whole proposal is not needed

@deitch
Copy link
Contributor

deitch commented Jan 28, 2025

@rucoder to be clear, I wasn't pushing for, "we really have to make this whole thing RESTful no matter what." It was more of a, "we have a lot of complexity in many places, and if this endpoint can be cleaner and more standardized, let's do it; if not, fine, at least we spent 5 mins thinking about it."

But if we can encode the format in the request in a standard way, all the better.

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

No branches or pull requests

6 participants