-
Notifications
You must be signed in to change notification settings - Fork 103
james/dt4a auth #2149
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
james/dt4a auth #2149
Conversation
ee/localserver/server.go
Outdated
// In the future, we will want to make this authenticated; for now, it is not authenticated. | ||
mux.Handle("/zta", ls.requestZtaInfoHandler()) | ||
mux.Handle("/zta", ztaAuthMiddleware.Wrap(ls.requestZtaInfoHandler())) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rename endpoint to dt4a
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm afraid this review will get really gnarly if I rename all the zta references to dt4a, so I want to wait until after this merges
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests look great and really thorough!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, nice work!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice! great docs/comments throughout 🔥
ee/localserver/server.go
Outdated
mux.Handle("/zta", ls.requestZtaInfoHandler()) | ||
|
||
// mux.Handle("/zta", ztaAuthMiddleware.Wrap(ls.requestZtaInfoHandler())) | ||
mux.Handle("/v0/dt4a", ztaAuthMiddleware.Wrap(ls.requestZtaInfoHandler())) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This shouldn't be on /v0
or /v1
unless it's in ecKryptoMiddleware
.
Could be /dt4a
or /dt4a/v0
I guess we could also call it /v3/dt4a
but my hunch is that we're not going to bring more into this api. Or if we do, it might be dt4a/app-list
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe /v3/dt4a
and then /v3/app-list
(if needed) is growing on me 🤷
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yea, it grew on me too, updated to /v3/dt4a
ee/localserver/certs.go
Outdated
"encoding/json" | ||
"fmt" | ||
|
||
"github.com/lestrrat-go/jwx/jwk" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to pull in this library? It feels a little sprawling, and I feel very -_- about third parties these days
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yea, it had quite a few dependencies, I dropped it and now just using std lib. More code to handle key conversions, but once separated, makes core logic code simpler. Updated
a542e4f
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔥
adds chain of trust / NaCl auth middleware to dt4a endpoint
relates to https://github.com/kolide/k2/issues/11530