-
Couldn't load subscription status.
- Fork 1
feat: implement client-integration token exchange #7
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
base: main
Are you sure you want to change the base?
Conversation
This diff implements token exchange for client integrations. Here's the general design we are implementing: 1. organizations maintaining client integrations (e.g., Acme Inc embedding m-lab/ndt7-client-js to provide NDT to its users) register with the system and obtain 1+ API keys 2. Acme Inc provisions a backend `B` that stores its API keys 3. backend `B` receives a query for Acme Inc clients when they initiate running a NDT test 4. backend `B` queries m-lab/token-exchange to exchange one of its API keys with a short-lived JWT 5. backend `B` returns the JWT to the client 6. the client queries m-lab/locate including the JWT 7. m-lab/locate uses the JWT to route the client to the proper NDT server and/or to perform accounting and access control to prevent a single integration from overloading the platform 8. the JWT is included into the URLs to access m-lab/ndt-server where it is again used to perform access control This diff specifically only implements the portion of this design pertaining to m-lab/token-exchange. We take care of not storing the API key in Datastore. We use bcrypt instead to compare it to a known hash.
|
This pull request is not yet read for review. We need to discuss some design aspects a bit. Also, obviously, we cannot merge code that lacks tests. But, I think it makes sense to write tests once we're sorted out the design aspects. (The same argument applies to fixing existing tests: better to iron out the design first.) |
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.
@robertodauria reviewed 6 of 6 files at r1, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @bassosimone)
internal/handler/exchange.go line 3 at r1 (raw file):
package handler // TODO(bassosimone): This should become `autojoin.go` for clarity.
Agreed (same for all the TODOs in this file)
store/integration.go line 125 at r1 (raw file):
// TODO(bassosimone): Implement CRUD operations for integration management. // // IMPORTANT: CreateAPIKey MUST use NameKey(IntegrationAPIKeyKind, keyID, nil) for O(1) lookups.
The majority of the algorithm stays the same if we implement solution n.3 for the API key schema, but this part will need to change.
main.go line 33 at r1 (raw file):
// (`mlab-sandbox`). We specified these flags when `main.go` was meant to only support the // autojoin use case however now we have two use cases. I suspect a single project ID is 100% // fine, but I am missing information about what we're using namespaces for.
Agree: a single project ID seems to be enough. In my mind the namespace differs based on the audience (e.g. platform credentials and client integrations have different schemas).
internal/handler/integration.go line 67 at r1 (raw file):
const ( audience = "integration" expiry = 20 * time.Second
Note: 20s is fine for now, pending better metrics on how long clients actually take to use a given JWT.
store/datastore.go line 3 at r1 (raw file):
package store // TODO(bassosimone): Why is this package public? Who is importing it? Locate
The orgadm tool will eventually use this version of datastore.go (we're in the middle of a transition, which explains why this is public and with no apparent consumers).
store/datastore.go line 48 at r1 (raw file):
// TODO(bassosimone): I wonder whether we should upgrade this implementation // to use use bcrypt for autojoin or whether we don't care.
I wouldn't change it for now — not in this PR anyway.
store/datastore.go line 165 at r1 (raw file):
// - Con: Deletion requires query, requires globally unique keys (vs per-org uniqueness) // // 3. Scoped keys: Encode parent in API key format, e.g., "{orgName}.{keyValue}"
+1 for {orgname}.{keyId} to keep the structure flat and validation/deletion O(1). Update will be more complex, but update isn't a common operation at all.
This diff implements token exchange for client integrations.
Here's the general design we are implementing:
organizations maintaining client integrations (e.g., Acme Inc embedding m-lab/ndt7-client-js to provide NDT to its users) register with the system and obtain 1+ API keys
Acme Inc provisions a backend
Bthat stores its API keysbackend
Breceives a query for Acme Inc clients when they initiate running a NDT testbackend
Bqueries m-lab/token-exchange to exchange one of its API keys with a short-lived JWTbackend
Breturns the JWT to the clientthe client queries m-lab/locate including the JWT
m-lab/locate uses the JWT to route the client to the proper NDT server and/or to perform accounting and access control to prevent a single integration from overloading the platform
the JWT is included into the URLs to access m-lab/ndt-server where it is again used to perform access control
This diff specifically only implements the portion of this design pertaining to m-lab/token-exchange.
We take care of not storing the API key in Datastore. We use bcrypt instead to compare it to a known hash.
This change is