A new PKI View - #262
Conversation
I've stumbled on this serveral times now working with certs. It gets confusing to look in your certs directory and see root.crt, device-ca.crt and then tls.pem. Granted we also have a cas.pem, but this file contains multiple certs. Signed-off-by: Andy Doan <doanac@qti.qualcomm.com>
This helps users get information to debug registration and ctonnection issues with devices. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Andy Doan <doanac@qti.qualcomm.com>
CLI Example: |
Web UI
|
Signed-off-by: Andy Doan <doanac@qti.qualcomm.com> Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Signed-off-by: Andy Doan <doanac@qti.qualcomm.com>
Covers the "fiocli pki show" command and the "/pki" page added in 9b6e0d87, 32efa5c1, and 6e07ea0a, checking both report the same root CA, device CA, and gateway TLS certificates. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Andy Doan <doanac@qti.qualcomm.com>
f5f3b8f to
aeeeb12
Compare
vkhoroz
left a comment
vkhoroz
left a comment
There was a problem hiding this comment.
Looks good.
My comments are important yet easy to take.
| var EchoError = server.EchoError | ||
|
|
||
| func RegisterHandlers(e *echo.Echo, ca *DeviceCa, storage *storage.Storage, userStorage *users.Storage, a auth.Provider) { | ||
| func RegisterHandlers(e *echo.Echo, ca *DeviceCa, storage *storage.Storage, userStorage *users.Storage, fs *storage.FsHandle, a auth.Provider) { |
There was a problem hiding this comment.
This inclusion breaks our indirection/encapsulation layering.
Those "trivial" certificate files access operations need to be added to the API's storage.Storage, so that the API handlers don't have to access the filesystem directly.
| buf, err := h.fs.Certs.ReadFile(name) | ||
| if err != nil { | ||
| if errors.Is(err, os.ErrNotExist) { | ||
| buf = nil | ||
| } else { | ||
| return EchoError(c, err, http.StatusInternalServerError, "Failed to read certificate: "+name) | ||
| } | ||
| } | ||
| resp[name] = string(buf) |
There was a problem hiding this comment.
This file existence check would look good on the storage/api level to unload the handler.
| // @Summary Get the CA bundle trusted for device mTLS | ||
| // @Produce text/plain | ||
| // @Success 200 | ||
| // @Router /pki/cas.pem [get] |
There was a problem hiding this comment.
Why is this different?
I do get that cas.pem is a list set of certs rather than a single cert, but from the API perspective we're simply returning a map of PKI files, so the unification is as simple as using a plural for /pki/certs endpoint.
| buf, err := h.fs.Certs.ReadFile(name) | ||
| if err != nil { | ||
| if errors.Is(err, os.ErrNotExist) { | ||
| buf = nil |
There was a problem hiding this comment.
I'm not sure one can cast nil to a string at line 54.
| cobra.CheckErr(printCertSummaries(certBytes)) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Once you unify the API, this can be unified too. There is technically no reason why cas.pem is treated specially here.
One tiny problem I already see with this "special" treatment is that:
- when e.g.
tls.crtdoes not exist, we shownot configured; - but when
cas.pemdoes not exist , we show a 404 error.
| fmt.Printf(" Serial: %s\n", formatSerial(cert.SerialNumber.Bytes())) | ||
| fmt.Printf(" Issuer: %s\n", cert.Issuer) | ||
| fmt.Printf(" Subject: %s\n", cert.Subject) | ||
| fmt.Printf(" Expires: %s\n", cert.NotAfter.Format("2006-01-02 15:04:05 MST")) |
There was a problem hiding this comment.
Maybe, use a \t instead of two spaces as an indentation?
| for _, name := range opts.Names { | ||
| if !slices.Contains(allowed, name) { | ||
| msg := "Invalid certificate requested: " + name | ||
| msg := "invalid certificate requested: " + name |
There was a problem hiding this comment.
it looks like these two changes belong to a different commit.
| return nil, fmt.Errorf("unexpected status code: HTTP_%d: %s", resp.StatusCode, string(body)) | ||
| } | ||
| return io.ReadAll(resp.Body) | ||
| } |
There was a problem hiding this comment.
A small change to reqJsonWithHeaders would make it a reqWithHeaders, so that you might enclose 90% of common code between reqJsonWithHeaders and getRaw in one place.
These "small deviations" costs a lot as the product evolves, as it was with getJson/putJson/postJson before the consolidation.
| if err != nil { | ||
| return nil, err | ||
| } | ||
| auth.PassCsrfCookie(ctx, req) |
There was a problem hiding this comment.
IIUC CSRF cookie is only needed for "update" operations... we don't need it for the GET method.
At least getJson doesn't send it and works somehow.
2 participants
As this project matures we are seeing that operators aren't going to spend time directly logged into the server and there's a need to expose more data via UIs. This introduces a small change to expose PKI related information to the user.