A new PKI View - #262
Open
doanac wants to merge 6 commits into
Open
Conversation
Member
Author
CLI Example: |
Member
Author
Web UI
|
doanac
requested review from
mike-scott and
vkhoroz
and removed request for
mike-scott
September 4, 2026 19:26
doanac
force-pushed
the
security-view
branch
from
September 4, 2026 19:30
f5f3b8f to
aeeeb12
Compare
vkhoroz
reviewed
Sep 5, 2026
vkhoroz
left a comment
vkhoroz
left a comment
Member
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) { |
Member
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.
Comment on lines
+46
to
+54
| 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) |
Member
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] |
Member
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.
Member
Author
There was a problem hiding this comment.
i had incremented on this PR and never fixed this one thing. I consolidated them all in the latest force-push and it makes things a lot simpler
| buf, err := h.fs.Certs.ReadFile(name) | ||
| if err != nil { | ||
| if errors.Is(err, os.ErrNotExist) { | ||
| buf = nil |
Member
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)) | ||
| } | ||
| } | ||
| } |
Member
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.
Comment on lines
+133
to
+136
| 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")) |
Member
There was a problem hiding this comment.
Maybe, use a \t instead of two spaces as an indentation?
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>
Signed-off-by: Andy Doan <doanac@qti.qualcomm.com>
This helps users get information to debug registration and connection issues with devices. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Andy Doan <doanac@qti.qualcomm.com>
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>
doanac
force-pushed
the
security-view
branch
from
September 8, 2026 18:19
aeeeb12 to
349d50f
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.